[C++17] Reject shadowing of capture by parameter in lambda

[Rakete1111]

Summary

This change rejects the shadowing of a capture by a parameter in lambdas in C++17.

int main() {
  int a;
  auto f = [a](int a) { return a; };
}

results in:

main.cpp:3:20: error: a lambda parameter cannot shadow an explicitly captured entity
  auto f = [a](int a) { return a; };
                   ^
main.cpp:3:13: note: variable a is explicitly captured here
  auto f = [a](int a) { return a; };
            ^

[erik.pilkington]

Thanks for working on this! Can you update www/cxx_dr_status.html too?

lib/Sema/SemaLambda.cpp

507

We usually back-port defect report resolutions to older standards, in this case maybe we should just warn pre-17 though. (Even though we warn here anyways with -Wshadow in -Wextra)

509

Nit: the colon should have a space on it's left here.

510

You should compare IdentifierInfos here, rather than StringRefs.

[Rakete1111]

lib/Sema/SemaLambda.cpp

510

IdentifierInfo doesn't have a compare equal or operator== function. How do you propose I do that? Do I add one?

[erik.pilkington]

lib/Sema/SemaLambda.cpp

510

Oh sorry, I should have been more clear. An IdentifierInfo pointer should be the same for all instances of that identifier, so I just mean to compare the pointers like: Capture.Id == Param->getIdentifier().

[Rakete1111]

Addressed review comments! :) Thanks

[Rakete1111]

Use correct clang and version spelling.

[lebedev.ri]

include/clang/Sema/Sema.h

5584

Maybe this should be ArrayRef<LambdaIntroducer::LambdaCapture> Captures instead?

[rsmith]

include/clang/Basic/DiagnosticSemaKinds.td

6636–6638

Please just unconditionally reject this. We don't need to support this as an extension (right?) and our policy is that DRs apply retroactively (unless we have some good reason for them to not do so).

www/cxx_dr_status.html

13084

This file is autogenerated, please do not edit it manually.

Instead, you should add a suitable test in test/CXX/drs, with a // dr2211: 8 comment to indicate that this DR is addressed in Clang 8, and rerun the makx_cxx_dr_status script to regenerate this file. (You'll need to download a recent cwg_index.html file; the script uses it as input.)

[rsmith]

For what it's worth, I think the language rule here is wrong, and we should instead be injecting the simple-captures into the lambda's function scope: http://lists.isocpp.org/core/2018/10/5145.php
But this appears to be a correct implementation of the rule as written, so let's go with it for now.

[Rakete1111]

Addresed review comments :)

I updated the dr status file but a lot of unrelated changes made it in. Is this okay?

[rsmith]

In D53595#1273848, @Rakete1111 wrote:

Addresed review comments :)

I updated the dr status file but a lot of unrelated changes made it in. Is this okay?

Please regenerate the file without your change and check that in as a separate commit prior to this.

[rsmith]

Also please mark PR39428 as fixed once this is submitted :)

[Rakete1111]

In D53595#1276330, @rsmith wrote:

In D53595#1273848, @Rakete1111 wrote:

Addresed review comments :)

I updated the dr status file but a lot of unrelated changes made it in. Is this okay?

Please regenerate the file without your change and check that in as a separate commit prior to this.

Why not afterwards? That way the DR is marked as resolved immediately.

[rsmith]

In D53595#1276346, @Rakete1111 wrote:

In D53595#1276330, @rsmith wrote:

In D53595#1273848, @Rakete1111 wrote:

Addresed review comments :)

I updated the dr status file but a lot of unrelated changes made it in. Is this okay?

Please regenerate the file without your change and check that in as a separate commit prior to this.

Why not afterwards? That way the DR is marked as resolved immediately.

That'd mean you need to track down an old version of the cwg_index.html file, though -- or guess what the relevant change to the HTML file would have been. Seems like more work that way. But the order of the two commits doesn't matter to me -- all I'm looking for is that the commit that resolves the DR /only/ changes that entry in cxx_dr_status.html, and the "rebase" on a new version of the issues list is done in a separate commit. (I don't know what you mean by "immediately"; I would expect the two commits to land within minutes of each other.)

[Rakete1111]

Update DR list.

include/clang/Basic/DiagnosticSemaKinds.td

let CategoryName = "Lambda Issue" in {
 
   // C++17 '*this' captures.
   def warn_cxx14_compat_star_this_lambda_capture : Warning<
     "by value capture of '*this' is incompatible with C++ standards before C++17">,
      InGroup<CXXPre17Compat>, DefaultIgnore;
   def ext_star_this_lambda_capture_cxx17 : ExtWarn<
     "capture of '*this' by copy is a C++17 extension">, InGroup<CXX17>;
 
+  // C++17 parameter shadows capture
+  def err_parameter_shadow_capture : Error<
+    "a lambda parameter cannot shadow an explicitly captured entity">;
+  def warn_cxx14_compat_parameter_shadow_capture : ExtWarn<
+    "a lambda parameter that shadows a capture is incompatible with C++ "
+    "standards before C++17">, InGroup<CXXPre17Compat>;
+
   // C++2a [=, this] captures.
   def warn_cxx17_compat_equals_this_lambda_capture : Warning<
     "explicit capture of 'this' with a capture default of '=' is incompatible "
     "with C++ standards before C++2a">, InGroup<CXXPre2aCompat>, DefaultIgnore;
   def ext_equals_this_lambda_capture_cxx2a : ExtWarn<
     "explicit capture of 'this' with a capture default of '=' "
     "is a C++2a extension">, InGroup<CXX2a>;
   def warn_deprecated_this_capture : Warning<

include/clang/Sema/Sema.h

public:
   /// Build the implicit field for an init-capture.
   FieldDecl *buildInitCaptureField(sema::LambdaScopeInfo *LSI, VarDecl *Var);
 
   /// Note that we have finished the explicit captures for the
   /// given lambda.
   void finishLambdaExplicitCaptures(sema::LambdaScopeInfo *LSI);
 
   /// Introduce the lambda parameters into scope.
-  void addLambdaParameters(CXXMethodDecl *CallOperator, Scope *CurScope);
+  void addLambdaParameters(
+      const SmallVectorImpl<LambdaIntroducer::LambdaCapture> &Captures,
+      CXXMethodDecl *CallOperator, Scope *CurScope);
 
   /// Deduce a block or lambda's return type based on the return
   /// statements present in the body.
   void deduceClosureReturnType(sema::CapturingScopeInfo &CSI);
 
   /// ActOnStartOfLambdaDefinition - This is called just before we start
   /// parsing the body of a lambda; it analyzes the explicit captures and
   /// arguments, and sets up various data-structures for the body of the

lib/Sema/SemaLambda.cpp

if (ExplicitResultType) {
     LSI->HasImplicitReturnType = true;
   }
 }
 
 void Sema::finishLambdaExplicitCaptures(LambdaScopeInfo *LSI) {
   LSI->finishedExplicitCaptures();
 }
 
-void Sema::addLambdaParameters(CXXMethodDecl *CallOperator, Scope *CurScope) {
+void Sema::addLambdaParameters(
+    const SmallVectorImpl<LambdaIntroducer::LambdaCapture> &Captures,
+    CXXMethodDecl *CallOperator, Scope *CurScope) {
   // Introduce our parameters into the function scope
   for (unsigned p = 0, NumParams = CallOperator->getNumParams();
        p < NumParams; ++p) {
     ParmVarDecl *Param = CallOperator->getParamDecl(p);
 
     // If this has an identifier, add it to the scope stack.
     if (CurScope && Param->getIdentifier()) {
+      bool Error = false;
+      // Resolution of CWG 2211 in C++17 renders shadowing ill-formed.
+      for (const auto &Capture : Captures) {
+        if (Capture.Id == Param->getIdentifier()) {
+          Error = true;
+          if (getLangOpts().CPlusPlus17)
+            Diag(Param->getLocation(), diag::err_parameter_shadow_capture);
+          else
+            Diag(Param->getLocation(),
+                 diag::warn_cxx14_compat_parameter_shadow_capture);
+          Diag(Capture.Loc, diag::note_var_explicitly_captured_here)
+              << Capture.Id << true;
+        }
+      }
+      if (!Error)
         CheckShadow(CurScope, Param);
 
       PushOnScopeChains(Param, CurScope);
     }
   }
 }
 
 /// If this expression is an enumerator-like expression of some type
 /// T, return the type T; otherwise, return null.
for (auto C = Intro.Captures.begin(), E = Intro.Captures.end(); C != E;
     if (!LSI->Captures.empty())
       LSI->ExplicitCaptureRanges[LSI->Captures.size() - 1] = C->ExplicitRange;
   }
   finishLambdaExplicitCaptures(LSI);
 
   LSI->ContainsUnexpandedParameterPack = ContainsUnexpandedParameterPack;
 
   // Add lambda parameters into scope.
-  addLambdaParameters(Method, CurScope);
+  addLambdaParameters(Intro.Captures, Method, CurScope);
 
   // Enter a new evaluation context to insulate the lambda from any
   // cleanups from the enclosing full-expression.
   PushExpressionEvaluationContext(
       ExpressionEvaluationContext::PotentiallyEvaluated);
 }
 
 void Sema::ActOnLambdaError(SourceLocation StartLoc, Scope *CurScope,

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
+// RUN: %clang_cc1 -std=c++17 -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}}
 
   // Avoid warnings for variables that aren't implicitly captured.
   {
 #ifdef AVOID
     auto f1 = [=] { int var = 1; };  // no warning

 #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}}
+    auto f2 = [param] 
+     (int param) { ; };
+#if __cplusplus < 201703L
+    // expected-note@-3{{variable 'param' is explicitly captured here}}
+    // expected-warning@-3{{a lambda parameter that shadows a capture is incompatible with C++ standards before C++17}}
+#else
+    // expected-error@-5{{a lambda parameter cannot shadow an explicitly captured entity}}
+    // expected-note@-7{{variable 'param' is explicitly captured here}}
+#endif
   }
 
   // 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
   // Generic lambda arguments should work.
 #ifdef AVOID
   auto g1 = [](auto param) { ; }; // no warning
   auto g2 = [=](auto param) { ; }; // no warning
 #else
   auto g1 = [](auto param) { ; }; // expected-warning {{declaration shadows a local variable}}
   auto g2 = [=](auto param) { ; }; // expected-warning {{declaration shadows a local variable}}
 #endif
-  auto g3 = [param] // expected-note {{variable 'param' is explicitly captured here}}
-   (auto param) { ; }; // expected-warning {{declaration shadows a local variable}}
+  auto g3 = [param]
+   (auto param) { ; };
+#if __cplusplus < 201703L
+    // expected-note@-3{{variable 'param' is explicitly captured here}}
+    // expected-warning@-3{{a lambda parameter that shadows a capture is incompatible with C++ standards before C++17}}
+#else
+    // expected-error@-5{{a lambda parameter cannot shadow an explicitly captured entity}}
+    // expected-note@-7{{variable 'param' is explicitly captured here}}
+#endif
+
 }
 
 void avoidWarningWhenRedefining() {
   int a = 1;
   auto l = [b = a] { // expected-note {{previous definition is here}}
     // Don't warn on redefinitions.
     int b = 0; // expected-error {{redefinition of 'b'}}
   };
 }
+
+void parameterShadowCapture() {
+#if __cplusplus >= 201703L
+  int var = 1; // expected-note{{previous declaration is here}}
+  auto f1 = [var](int var) { return var; }; // expected-error{{a lambda parameter cannot shadow an explicitly captured entity}}
+  // expected-note@-1{{variable 'var' is explicitly captured here}}
+
+  auto f2 = [var = 1](int var) { return var; }; // expected-error{{a lambda parameter cannot shadow an explicitly captured entity}}
+  // expected-note@-1{{variable 'var' is explicitly captured here}}
+  // expected-error@-2{{reference to 'var' is ambiguous}}
+  // expected-note@-3{{candidate found by name lookup is 'var'}}
+  // expected-note@-4{{candidate found by name lookup is 'var'}}
+
+  auto f3 = [=](int var) { return var; };
+  // expected-warning@-1{{declaration shadows a local variable}}
+#endif
+}

www/cxx_dr_status.html

<tr id="2210">
     <td>NAD</td>
     <td>Principal/target constructor confusion</td>
     <td class="none" align="center">Unknown</td>
   </tr>
   <tr id="2211">
     <td><a href="http://www.open-std.org/jtc1/sc22/wg21/docs/cwg_defects.html#2211">2211</a></td>
     <td>C++17</td>
     <td>Hiding by lambda captures and parameters</td>
-    <td class="none" align="center">Unknown</td>
+    <td class="full" align="center">clang 8.0</td>
   </tr>
   <tr class="open" id="2212">
     <td><a href="http://www.open-std.org/jtc1/sc22/wg21/docs/cwg_active.html#2212">2212</a></td>
     <td>open</td>
     <td>Typedef changing linkage after use</td>
     <td align="center">Not resolved</td>
   </tr>
   <tr class="open" id="2213">