[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

 Modified Compiler Flags
 -----------------------
 
-- ...
+- -Wshadow now also checks for shadowed structured bindings
 
 Removed Compiler Flags
 -------------------------

clang/include/clang/Basic/DiagnosticSemaKinds.td

           "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>,

clang/include/clang/Sema/Sema.h

   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);

clang/lib/Sema/SemaDecl.cpp

   SDK_StaticMember,
   SDK_Field,
   SDK_Typedef,
-  SDK_Using
+  SDK_Using,
+  SDK_StructuredBinding
 };
 
 /// Determine what kind of declaration we're shadowing.

     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 nullptr;
 
   NamedDecl *ShadowedDecl = R.getFoundDecl();
-  return isa<VarDecl>(ShadowedDecl) || isa<FieldDecl>(ShadowedDecl)
-             ? ShadowedDecl
-             : nullptr;
+  return isa<VarDecl, FieldDecl, BindingDecl>(ShadowedDecl) ? ShadowedDecl
+                                                            : nullptr;
 }
 
 /// Return the declaration shadowed by the given typedef \p D, or null

   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.
 ///

clang/lib/Sema/SemaDeclCXX.cpp

       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);

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}}

     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