[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

   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">;
+
   // C++2a [=, this] captures.
   def warn_cxx17_compat_equals_this_lambda_capture : Warning<
     "explicit capture of 'this' with a capture default of '=' is incompatible "

include/clang/Sema/Sema.h

   void finishLambdaExplicitCaptures(sema::LambdaScopeInfo *LSI);
 
   /// Introduce the lambda parameters into scope.
-  void addLambdaParameters(CXXMethodDecl *CallOperator, Scope *CurScope);
+  void addLambdaParameters(
+      ArrayRef<LambdaIntroducer::LambdaCapture> Captures,
+      CXXMethodDecl *CallOperator, Scope *CurScope);
 
   /// Deduce a block or lambda's return type based on the return
   /// statements present in the body.

lib/Sema/SemaLambda.cpp

   LSI->finishedExplicitCaptures();
 }
 
-void Sema::addLambdaParameters(CXXMethodDecl *CallOperator, Scope *CurScope) {
+void Sema::addLambdaParameters(
+    ArrayRef<LambdaIntroducer::LambdaCapture> Captures,
+    CXXMethodDecl *CallOperator, Scope *CurScope) {
   // Introduce our parameters into the function scope
   for (unsigned p = 0, NumParams = CallOperator->getNumParams();
        p < NumParams; ++p) {

 
     // If this has an identifier, add it to the scope stack.
     if (CurScope && Param->getIdentifier()) {
-      CheckShadow(CurScope, Param);
+      bool Error = false;
+      // Resolution of CWG 2211 in C++17 renders shadowing ill-formed, but we
+      // retroactively apply it.
+      for (const auto &Capture : Captures) {
+        if (Capture.Id == Param->getIdentifier()) {
+          Error = true;
+          Diag(Param->getLocation(), diag::err_parameter_shadow_capture);
+          Diag(Capture.Loc, diag::note_var_explicitly_captured_here)
+              << Capture.Id << true;
+        }
+      }
+      if (!Error)
+        CheckShadow(CurScope, Param);
 
       PushOnScopeChains(Param, CurScope);
     }

   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.

test/CXX/drs/dr22xx.cpp

   const volatile unsigned i3 : 1;
 };
 }
+
+#if __cplusplus >= 201103L
+namespace dr2211 { // dr2211: 8
+void f() {
+  int a;
+  auto f = [a](int a) { (void)a; }; // expected-error {{a lambda parameter cannot shadow an explicitly captured entity}}
+  // expected-note@-1{{variable 'a' is explicitly captured here}}
+  auto g = [=](int a) { (void)a; };
+}
+}
+#endif

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

       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}}
+     (int param) { ; }; // expected-error {{a lambda parameter cannot shadow an explicitly captured entity}}
   }
 
   // Warn for variables defined in the capture list.

   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 param) { ; }; // expected-error {{a lambda parameter cannot shadow an explicitly captured entity}}
 }
 
 void avoidWarningWhenRedefining() {

www/cxx_dr_status.html

     <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="svn" align="center">SVN</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>