Avoid -Wshadow warnings for shadowed variables that aren't captured by lambdas with an explicit capture list

[arphaman]

Summary

This patch avoids the -Wshadow warning for variables which shadow variables that aren't captured by lambdas with an explicit capture list. It provides an additional note that points to location of the explicit capture.

I noticed that we have -Wshadow-all as well, but I wasn't sure how to preserve the old behaviour for it (since I don't think I can check if a diagnostic group is enabled), or if the old behaviour should be preserved at all. What do you think?

This patch fixes PR 21426, but I think that this is a partial fix, since that bug doesn't specify that the capture list should be explicit. I plan on working on a similar patch for lambdas with a default capture specifier and implicit variable captures, but I'm not sure it's 100% needed for them. Do you think this kind of change would make sense for implicit captures as well?

[rnk]

I'd like to preserve the old behavior under -Wshadow-all. The last time I came here to try to quiet a shadowing warning on constructor parameters, we went to great lengths to try to satisfy users who really want to warn on any and all kinds of shadowing. I think you can do it by adding a new diagnostic with the same text in a new group, and using the new diagnostic id if appropriate instead of returning like you do now.

[arphaman]

The updated patch preserves the old behaviour in -Wshadow-all

[rnk]

include/clang/Basic/DiagnosticSemaKinds.td

361 ↗

(On Diff #77201)

Use the .Text accessor like we do here to avoid repeating the text:

def warn_objc_pointer_masking : Warning<
  "bitmasking for introspection of Objective-C object pointers is strongly "
  "discouraged">,
  InGroup<ObjCPointerIntrospect>;
def warn_objc_pointer_masking_performSelector : Warning<warn_objc_pointer_masking.Text>,

366 ↗

(On Diff #77201)

Richard seemed to feel it was important to have a -Wshadow-field-in-constructor, so we should probably add a -Wshadow-uncaptured-local and put that in -Wshadow-all.

[arphaman]

The updated patch introduces a new warning group named -Wshadow-uncaptured-local that was suggested by Reid.

[rnk]

lgtm, thanks!

cfe/trunk/include/clang/Basic/DiagnosticGroups.td

 def SemiBeforeMethodBody : DiagGroup<"semicolon-before-method-body">;
 def Sentinel : DiagGroup<"sentinel">;
 def MissingMethodReturnType : DiagGroup<"missing-method-return-type">;
 
 def ShadowFieldInConstructorModified : DiagGroup<"shadow-field-in-constructor-modified">;
 def ShadowFieldInConstructor : DiagGroup<"shadow-field-in-constructor",
                                          [ShadowFieldInConstructorModified]>;
 def ShadowIvar : DiagGroup<"shadow-ivar">;
+def ShadowUncapturedLocal : DiagGroup<"shadow-uncaptured-local">;
 
 // -Wshadow-all is a catch-all for all shadowing. -Wshadow is just the
 // shadowing that we think is unsafe.
 def Shadow : DiagGroup<"shadow", [ShadowFieldInConstructorModified,
                                   ShadowIvar]>;
-def ShadowAll : DiagGroup<"shadow-all", [Shadow, ShadowFieldInConstructor]>;
+def ShadowAll : DiagGroup<"shadow-all", [Shadow, ShadowFieldInConstructor,
+                                         ShadowUncapturedLocal]>;
 
 def Shorten64To32 : DiagGroup<"shorten-64-to-32">;
 def : DiagGroup<"sign-promo">;
 def SignCompare : DiagGroup<"sign-compare">;
 def : DiagGroup<"stack-protector">;
 def : DiagGroup<"switch-default">;
 def : DiagGroup<"synth">;
 def SizeofArrayArgument : DiagGroup<"sizeof-array-argument">;

cfe/trunk/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}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 :
   Warning<"modifying constructor parameter %0 that shadows a "
           "field of %1">,
   InGroup<ShadowFieldInConstructorModified>, DefaultIgnore;
 
def err_access_lambda_capture : Error<
     // The ERRORs represent other special members that aren't constructors, in
     // hopes that someone will bother noticing and reporting if they appear
     "capture of variable '%0' as type %1 calls %select{private|protected}3 "
     "%select{default |copy |move |*ERROR* |*ERROR* |*ERROR* |}2constructor">,
     AccessControl;
   def note_lambda_to_block_conv : Note<
     "implicit capture of lambda object due to conversion to block pointer "
     "here">;
+  def note_var_explicitly_captured_here : Note<"variable %0 is explicitly "
+    "captured here">;
 
   // C++14 lambda init-captures.
   def warn_cxx11_compat_init_capture : Warning<
     "initialized lambda captures are incompatible with C++ standards "
     "before C++14">, InGroup<CXXPre14Compat>, DefaultIgnore;
   def ext_init_capture : ExtWarn<
     "initialized lambda captures are a C++14 extension">, InGroup<CXX14>;
   def err_init_capture_no_expression : Error<

cfe/trunk/lib/Sema/SemaDecl.cpp

 /// Determine what kind of declaration we're shadowing.
 static ShadowedDeclKind computeShadowedDeclKind(const NamedDecl *ShadowedDecl,
                                                 const DeclContext *OldDC) {
   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.
+static SourceLocation getCaptureLocation(const LambdaScopeInfo *LSI,
+                                         const VarDecl *VD) {
+  for (const LambdaScopeInfo::Capture &Capture : LSI->Captures) {
+    if (Capture.isVariableCapture() && Capture.getVariable() == VD)
+      return Capture.getLocation();
+  }
+  return SourceLocation();
+}
+
 /// \brief Diagnose variable or built-in function shadowing.  Implements
 /// -Wshadow.
 ///
 /// This method is called whenever a VarDecl is added to a "useful"
 /// scope.
 ///
 /// \param S the scope in which the shadowing name is being declared
 /// \param R the lookup of the name
if (shadowedVar->isExternC()) {
         if (I->isFileVarDecl()) {
           ShadowedDecl = I;
           break;
         }
     }
 
   DeclContext *OldDC = ShadowedDecl->getDeclContext();
 
+  unsigned WarningDiag = diag::warn_decl_shadow;
+  SourceLocation CaptureLoc;
+  if (isa<VarDecl>(ShadowedDecl) && NewDC && isa<CXXMethodDecl>(NewDC)) {
+    if (const auto *RD = dyn_cast<CXXRecordDecl>(NewDC->getParent())) {
+      // Try to avoid warnings for lambdas with an explicit capture list.
+      if (RD->isLambda() && OldDC->Encloses(NewDC->getLexicalParent()) &&
+          RD->getLambdaCaptureDefault() == LCD_None) {
+        const auto *LSI = cast<LambdaScopeInfo>(getCurFunction());
+        // Warn only when the lambda captures the shadowed decl explicitly.
+        CaptureLoc = getCaptureLocation(LSI, cast<VarDecl>(ShadowedDecl));
+        if (CaptureLoc.isInvalid())
+          WarningDiag = diag::warn_decl_shadow_uncaptured_local;
+      }
+    }
+  }
+
   // Only warn about certain kinds of shadowing for class members.
   if (NewDC && NewDC->isRecord()) {
     // In particular, don't warn about shadowing non-class members.
     if (!OldDC->isRecord())
       return;
 
     // TODO: should we warn about static data members shadowing
     // static data members from base classes?
 
     // TODO: don't diagnose for inaccessible shadowed members.
     // This is hard to do perfectly because we might friend the
     // shadowing context, but that's just a false negative.
   }
 
 
   DeclarationName Name = R.getLookupName();
 
   // Emit warning and note.
   if (getSourceManager().isInSystemMacro(R.getNameLoc()))
     return;
   ShadowedDeclKind Kind = computeShadowedDeclKind(ShadowedDecl, OldDC);
-  Diag(R.getNameLoc(), diag::warn_decl_shadow) << Name << Kind << OldDC;
+  Diag(R.getNameLoc(), WarningDiag) << Name << Kind << OldDC;
+  if (!CaptureLoc.isInvalid())
+    Diag(CaptureLoc, diag::note_var_explicitly_captured_here) << Name;
   Diag(ShadowedDecl->getLocation(), diag::note_previous_declaration);
 }
 
 /// \brief Check -Wshadow without the advantage of a previous lookup.
 void Sema::CheckShadow(Scope *S, VarDecl *D) {
   if (Diags.isIgnored(diag::warn_decl_shadow, D->getLocation()))
     return;
 

cfe/trunk/test/SemaCXX/warn-shadow-in-lambdas.cpp

+// RUN: %clang_cc1 -std=c++14 -verify -fsyntax-only -Wshadow -D AVOID %s
+// RUN: %clang_cc1 -std=c++14 -verify -fsyntax-only -Wshadow -Wshadow-uncaptured-local %s
+// RUN: %clang_cc1 -std=c++14 -verify -fsyntax-only -Wshadow-all %s
+
+void foo(int param) { // expected-note 1+ {{previous declaration is here}}
+  int var = 0; // expected-note 1+ {{previous declaration is here}}
+
+  // Warn for lambdas with a default capture specifier.
+  {
+    auto f1 = [=] { int var = 1; };  // expected-warning {{declaration shadows a local variable}}
+    auto f2 = [&] { int var = 2; };  // expected-warning {{declaration shadows a local variable}}
+    auto f3 = [=] (int param) { ; }; // expected-warning {{declaration shadows a local variable}}
+    auto f4 = [&] (int param) { ; }; // expected-warning {{declaration shadows a local variable}}
+  }
+
+  // Warn normally inside of lambdas.
+  auto l1 = [] { // expected-note {{previous declaration is here}}
+      int x = 1; // expected-note {{previous declaration is here}}
+      { int x = 2; } // expected-warning {{declaration shadows a local variable}}
+  };
+  auto l2 = [] (int x) { // expected-note {{previous declaration is here}}
+    { int x = 1; } // expected-warning {{declaration shadows a local variable}}
+  };
+
+  // Avoid warnings for variables that aren't explicitly captured.
+  {
+#ifdef AVOID
+    auto f1 = [] { int var = 1; }; // no warning
+    auto f2 = [] (int param) { ; }; // no warning
+    auto f3 = [param] () { int var = 1; }; // no warning
+    auto f4 = [var] (int param) { ; }; // no warning
+#else
+    auto f1 = [] { int var = 1; }; // expected-warning {{declaration shadows a local variable}}
+    auto f2 = [] (int param) { ; }; // expected-warning {{declaration shadows a local variable}}
+    auto f3 = [param] () { int var = 1; }; // expected-warning {{declaration shadows a local variable}}
+    auto f4 = [var] (int param) { ; }; // expected-warning {{declaration shadows a local variable}}
+#endif
+  };
+
+  // Warn for variables that are explicitly captured.
+  {
+    auto f1 = [var] () { // expected-note {{variable 'var' is explicitly captured here}}
+      int var = 1; // expected-warning {{declaration shadows a local variable}}
+    };
+    auto f2 = [param]   // expected-note {{variable 'param' is explicitly captured here}}
+     (int param) { ; }; // expected-warning {{declaration shadows a local variable}}
+  }
+
+  // Warn for variables defined in the capture list.
+  auto l3 = [z = var] { // expected-note {{previous declaration is here}}
+#ifdef AVOID
+    int var = 1; // no warning
+#else
+    int var = 1; // expected-warning {{declaration shadows a local variable}}
+#endif
+    { int z = 1; } // expected-warning {{declaration shadows a local variable}}
+  };
+#ifdef AVOID
+  auto l4 = [var = param] (int param) { ; }; // no warning
+#else
+  auto l4 = [var = param] (int param) { ; }; // expected-warning {{declaration shadows a local variable}}
+#endif
+
+  // Make sure that inner lambdas work as well.
+  auto l5 = [var, l1] { // expected-note {{variable 'l1' is explicitly captured here}}
+    auto l1 = [] { // expected-warning {{declaration shadows a local variable}}
+#ifdef AVOID
+      int var = 1; // no warning
+#else
+      int var = 1; // expected-warning {{declaration shadows a local variable}}
+#endif
+    };
+#ifdef AVOID
+    auto f1 = [] { int var = 1; }; // no warning
+#else
+    auto f1 = [] { int var = 1; }; // expected-warning {{declaration shadows a local variable}}
+#endif
+    auto f2 = [=] { int var = 1; }; // expected-warning {{declaration shadows a local variable}}
+    auto f3 = [var] // expected-note {{variable 'var' is explicitly captured here}}
+      { int var = 1; }; // expected-warning {{declaration shadows a local variable}}
+  };
+
+  // Generic lambda arguments should work.
+#ifdef AVOID
+  auto g1 = [](auto param) { ; }; // no warning
+#else
+  auto g1 = [](auto param) { ; }; // expected-warning {{declaration shadows a local variable}}
+#endif
+  auto g2 = [param] // expected-note {{variable 'param' is explicitly captured here}}
+   (auto param) { ; }; // expected-warning {{declaration shadows a local variable}}
+}