[SEMA] Added warn_decl_shadow support for structured bindings

[Vexthil]

Summary

https://bugs.llvm.org/show_bug.cgi?id=40858

CheckShadow is now called for each binding in the structured binding to make sure it does not shadow any other variable in scope. This does use a custom implementation of getShadowedDeclaration though because a BindingDecl is not a VarDecl

Added a few unit tests for this. In theory though all the other shadow unit tests should be duplicated for the structured binding variables too but whether it is probably not worth it as they use common code. The MyTuple and std interface code has been copied from live-bindings-test.cpp

[Vexthil]

I have no idea what this error means or what to do about it i'm afraid but it does not look related to the changes I have made. I also do not have access to a Linux machine easily so reproducing it locally is not going to be easy. If anyone knows this error better and can give some pointers that would be great.

class/union requires a filename
!87 = distinct !DICompositeType(tag: DW_TAG_union_type, name: "kmp_cmplrdata_t", size: 64, elements: !2)

[Vexthil]

Updating clang format

[Vexthil]

Fixed the update by doing a full diff rather than just the additive update

[rsmith]

Thanks!

clang/lib/Sema/SemaDecl.cpp

7571

isa<VarDecl>(ShadowedDecl) || isa<FieldDecl>(ShadowedDecl) can be simplified to isa<VarDecl, FieldDecl>(ShadowedDecl).

7571–7573

I think we should also warn if a BindingDecl shadows another BindingDecl, or if a VarDecl shadows a BindingDecl.

clang/lib/Sema/SemaDeclCXX.cpp

872–874

Should this be an else for the if (!Previous.empty()) below? Do we get two diagnostics for:

int a;
struct X { int n; };
auto [a] = X();

(one for shadowing and one for redefinition)?

clang/test/SemaCXX/warn-shadow.cpp

274

It doesn't seem important to test different kinds of bindings here, since the shadowing check for bindings doesn't depend on how we perform the decomposition. So I'd suggest you simplify this test by using only built-in bindings, eg:

namespace structured_binding_tests {
int x; // expected-note {{previous declaration is here}}
int y; // expected-note {{previous declaration is here}}
struct S { int a, b; };

void test1() {
  const auto [x, y] = S(); // expected-warning 2 {{declaration shadows a variable in namespace 'structured_binding_tests'}}
}

[Vexthil]

Update with changes suggested by @rsmith .

[Vexthil]

Fixing clang format issues. The SemaDecl.cpp has a pile of incorrect clang format issues but i've only updated changes relevant to changes I have made

[rsmith]

Looks good, thanks! Do you need someone to commit this for you?

[rsmith]

Hm, before we go forward with this: can we get a test for the "shadows a structured binding" case, please? You might also need to add a new case to the "declaration shadows ..." warning text.

[Vexthil]

Added unit tests for "shadows a structured binding"

[Vexthil]

.. And yes I will need someone to commit this for me I believe once you are happy with it.

[smeenai]

This is very belated, sorry :)

Our shadowing warnings have special behavior for lambdas and local variables: the warning for lambdas shadowing uncaptured local variables falls under -Wshadow-uncaptured-local, which isn't turned on for -Wshadow (only for -Wshadow-all). However, this doesn't extend to structured bindings. Compare https://godbolt.org/z/nP6b9f8o4, which doesn't fire for -Wshadow, to https://godbolt.org/z/9he8Ezsbb, which is the equivalent with structured bindings and does fire for -Wshadow. This is triggering a whole bunch of new -Wshadow warnings in our codebase, and I'm wondering if we'd consider also separating the lambda + structured binding case into its own warning group, to match the behavior for lambdas + local variables.

clang/docs/ReleaseNotes.rst

 Deprecated Compiler Flags
 -------------------------
 
 - ...
 
 Modified Compiler Flags
 -----------------------
 
-- ...
+- -Wshadow now also checks for shadowed structured bindings
 
 Removed Compiler Flags
 -------------------------
 
 - The clang-cl ``/fallback`` flag, which made clang-cl invoke Microsoft Visual
   C++ on files it couldn't compile itself, has been removed.
 
 New Pragmas in Clang

clang/include/clang/Basic/DiagnosticSemaKinds.td

 
 def warn_decl_shadow :
   Warning<"declaration shadows a %select{"
           "local variable|"
           "variable in %2|"
           "static data member of %2|"
           "field of %2|"
           "typedef in %2|"
-          "type alias in %2}1">,
+          "type alias in %2|"
+          "structured binding}1">,
   InGroup<Shadow>, DefaultIgnore;
 def warn_decl_shadow_uncaptured_local :
   Warning<warn_decl_shadow.Text>,
   InGroup<ShadowUncapturedLocal>, DefaultIgnore;
 def warn_ctor_parm_shadows_field:
   Warning<"constructor parameter %0 shadows the field %1 of %2">,
   InGroup<ShadowFieldInConstructor>, DefaultIgnore;
 def warn_modifying_shadowing_decl :

clang/include/clang/Sema/Sema.h

diagnoseIgnoredQualifiers(unsigned DiagID, unsigned Quals,
                             SourceLocation AtomicQualLoc = SourceLocation(),
                             SourceLocation UnalignedQualLoc = SourceLocation());
 
   static bool adjustContextForLocalExternDecl(DeclContext *&DC);
   void DiagnoseFunctionSpecifiers(const DeclSpec &DS);
   NamedDecl *getShadowedDeclaration(const TypedefNameDecl *D,
                                     const LookupResult &R);
   NamedDecl *getShadowedDeclaration(const VarDecl *D, const LookupResult &R);
+  NamedDecl *getShadowedDeclaration(const BindingDecl *D,
+                                    const LookupResult &R);
   void CheckShadow(NamedDecl *D, NamedDecl *ShadowedDecl,
                    const LookupResult &R);
   void CheckShadow(Scope *S, VarDecl *D);
 
   /// Warn if 'E', which is an expression that is about to be modified, refers
   /// to a shadowing declaration.
   void CheckShadowingDeclModification(Expr *E, SourceLocation Loc);
 

clang/lib/Sema/SemaDecl.cpp

 
 /// Enum describing the %select options in diag::warn_decl_shadow.
 enum ShadowedDeclKind {
   SDK_Local,
   SDK_Global,
   SDK_StaticMember,
   SDK_Field,
   SDK_Typedef,
-  SDK_Using
+  SDK_Using,
+  SDK_StructuredBinding
 };
 
 /// Determine what kind of declaration we're shadowing.
 static ShadowedDeclKind computeShadowedDeclKind(const NamedDecl *ShadowedDecl,
                                                 const DeclContext *OldDC) {
   if (isa<TypeAliasDecl>(ShadowedDecl))
     return SDK_Using;
   else if (isa<TypedefDecl>(ShadowedDecl))
     return SDK_Typedef;
+  else if (isa<BindingDecl>(ShadowedDecl))
+    return SDK_StructuredBinding;
   else if (isa<RecordDecl>(OldDC))
     return isa<FieldDecl>(ShadowedDecl) ? SDK_Field : SDK_StaticMember;
 
   return OldDC->isFileContext() ? SDK_Global : SDK_Local;
 }
 
 /// Return the location of the capture if the given lambda captures the given
 /// variable \p VD, or an invalid source location otherwise.
NamedDecl *Sema::getShadowedDeclaration(const VarDecl *D,
   if (!shouldWarnIfShadowedDecl(Diags, R))
     return nullptr;
 
   // Don't diagnose declarations at file scope.
   if (D->hasGlobalStorage())
     return nullptr;
 
   NamedDecl *ShadowedDecl = R.getFoundDecl();
-  return isa<VarDecl>(ShadowedDecl) || isa<FieldDecl>(ShadowedDecl)
-             ? ShadowedDecl
+  return isa<VarDecl, FieldDecl, BindingDecl>(ShadowedDecl) ? ShadowedDecl
                                                             : nullptr;
 }
 
 /// Return the declaration shadowed by the given typedef \p D, or null
 /// if it doesn't shadow any declaration or shadowing warnings are disabled.
 NamedDecl *Sema::getShadowedDeclaration(const TypedefNameDecl *D,
                                         const LookupResult &R) {
   // Don't warn if typedef declaration is part of a class
   if (D->getDeclContext()->isRecord())
     return nullptr;
 
   if (!shouldWarnIfShadowedDecl(Diags, R))
     return nullptr;
 
   NamedDecl *ShadowedDecl = R.getFoundDecl();
   return isa<TypedefNameDecl>(ShadowedDecl) ? ShadowedDecl : nullptr;
 }
 
+/// Return the declaration shadowed by the given variable \p D, or null
+/// if it doesn't shadow any declaration or shadowing warnings are disabled.
+NamedDecl *Sema::getShadowedDeclaration(const BindingDecl *D,
+                                        const LookupResult &R) {
+  if (!shouldWarnIfShadowedDecl(Diags, R))
+    return nullptr;
+
+  NamedDecl *ShadowedDecl = R.getFoundDecl();
+  return isa<VarDecl, FieldDecl, BindingDecl>(ShadowedDecl) ? ShadowedDecl
+                                                            : nullptr;
+}
+
 /// Diagnose variable or built-in function shadowing.  Implements
 /// -Wshadow.
 ///
 /// This method is called whenever a VarDecl is added to a "useful"
 /// scope.
 ///
 /// \param ShadowedDecl the declaration that is shadowed by the given variable
 /// \param R the lookup of the name

clang/lib/Sema/SemaDeclCXX.cpp

for (auto &B : D.getDecompositionDeclarator().bindings()) {
     // It's not permitted to shadow a template parameter name.
     if (Previous.isSingleResult() &&
         Previous.getFoundDecl()->isTemplateParameter()) {
       DiagnoseTemplateParameterShadow(D.getIdentifierLoc(),
                                       Previous.getFoundDecl());
       Previous.clear();
     }
 
+    auto *BD = BindingDecl::Create(Context, DC, B.NameLoc, B.Name);
+
+    // Find the shadowed declaration before filtering for scope.
+    NamedDecl *ShadowedDecl = D.getCXXScopeSpec().isEmpty()
+                                  ? getShadowedDeclaration(BD, Previous)
+                                  : nullptr;
+
     bool ConsiderLinkage = DC->isFunctionOrMethod() &&
                            DS.getStorageClassSpec() == DeclSpec::SCS_extern;
     FilterLookupForScope(Previous, DC, S, ConsiderLinkage,
                          /*AllowInlineNamespace*/false);
+
     if (!Previous.empty()) {
       auto *Old = Previous.getRepresentativeDecl();
       Diag(B.NameLoc, diag::err_redefinition) << B.Name;
       Diag(Old->getLocation(), diag::note_previous_definition);
+    } else if (ShadowedDecl && !D.isRedeclaration()) {
+      CheckShadow(BD, ShadowedDecl, Previous);
     }
-
-    auto *BD = BindingDecl::Create(Context, DC, B.NameLoc, B.Name);
     PushOnScopeChains(BD, S, true);
     Bindings.push_back(BD);
     ParsingInitForAutoVars.insert(BD);
   }
 
   // There are no prior lookup results for the variable itself, because it
   // is unnamed.
   DeclarationNameInfo NameInfo((IdentifierInfo *)nullptr,

clang/test/SemaCXX/warn-shadow.cpp

-// RUN: %clang_cc1 -verify -fsyntax-only -std=c++11 -Wshadow-all %s
+// RUN: %clang_cc1 -verify -fsyntax-only -std=c++17 -Wshadow-all %s
 
 namespace {
   int i; // expected-note {{previous declaration is here}}
 }
 
 namespace one {
 namespace two {
   int j; // expected-note {{previous declaration is here}}

 enum class X { PR24718 }; // Ok, not shadowing
 
 struct PR24718_1;
 struct PR24718_2 {
   enum {
     PR24718_1 // Does not shadow a type.
   };
 };
+
+namespace structured_binding_tests {
+int x; // expected-note {{previous declaration is here}}
+int y; // expected-note {{previous declaration is here}}
+struct S {
+  int a, b;
+};
+
+void test1() {
+  const auto [x, y] = S(); // expected-warning 2 {{declaration shadows a variable in namespace 'structured_binding_tests'}}
+}
+
+void test2() {
+  int a; // expected-note {{previous declaration is here}}
+  bool b; // expected-note {{previous declaration is here}}
+  {
+    auto [a, b] = S(); // expected-warning 2 {{declaration shadows a local variable}}
+  }
+}
+
+class A
+{
+  int m_a; // expected-note {{previous declaration is here}}
+  int m_b; // expected-note {{previous declaration is here}}
+
+  void test3() {
+    auto [m_a, m_b] = S(); // expected-warning 2 {{declaration shadows a field of 'structured_binding_tests::A'}}
+  }
+};
+
+void test4() {
+  const auto [a, b] = S(); // expected-note 3 {{previous declaration is here}}
+  {
+    int a = 4; // expected-warning {{declaration shadows a structured binding}}
+  }
+  {
+    const auto [a, b] = S(); // expected-warning 2 {{declaration shadows a structured binding}}
+  }
+}
+
+}; // namespace structured_binding_tests
 No newline at end of file