[PATCH] D24639: [Sema] Warn when returning a lambda that captures a local variable by reference

[Richard Smith via cfe-commits]

rsmith added a comment.

> But not here, because we would have to verify that the pointer in lam wasn't mutated in a previous call of the lambda:


Isn't that guaranteed, because the lambda is not marked `mutable`?


================
Comment at: lib/Sema/SemaChecking.cpp:6594
@@ -6590,1 +6593,3 @@
+    if (LambdaDecl->isLambda())
+      stackE = EvalVal(RetValExp, refVars, /*ParentDecl=*/nullptr);
   }
----------------
I don't think it makes sense to use `EvalVal` for this. `EvalVal` is intended to handle the case of a glvalue that is being returned from the function, which isn't quite the same thing you want to check here -- I think you would be better off instead directly checking the captures of the lambda from here (`LambdaDecl` can give you a list) rather than calling this.

It also seems like the `RetValExp` doesn't actually matter at all here in most cases. If the closure type captures a local by reference, we should warn, regardless of the initialization expression, and likewise if it's not `mutable` and captures a local by address through an init-capture. For `mutable` lambdas with init-captures that capture by address, perhaps we could do a quick localized walk here on the `RetValExp` to see if it's returned directly.

================
Comment at: lib/Sema/SemaChecking.cpp:6845
@@ -6832,3 +6844,3 @@
                            const Decl *ParentDecl) {
-  do {
+  for (;;) {
     // We should only be called for evaluating non-pointer expressions, or
----------------
We prefer `while (true)` over `for (;;)` (but I agree the existing `do ... while (true);` is terrible).

================
Comment at: lib/Sema/SemaChecking.cpp:6886-6887
@@ -6873,3 +6885,4 @@
         if (V->hasLocalStorage()) {
-          if (!V->getType()->isReferenceType())
+          auto *RD = V->getType()->getAsCXXRecordDecl();
+          if (!V->getType()->isReferenceType() && !(RD && RD->isLambda()))
             return DR;
----------------
This is the kind of problem I was worried about. Given:

  auto &f() {
    auto lam = [] {};
    return lam;
  }

... we'll currently diagnose that we're returning the address of a local, from here. But with this change applied, we lose that diagnostic because `EvalVal` can't tell the difference between this case (returning by reference) and returning the lambda by value.

================
Comment at: test/SemaCXX/cxx1y-generic-lambdas-capturing.cpp:185
@@ -184,3 +184,3 @@
           f(y, d);
-          f(z, d);
+          f(z, d); // expected-warning {{address of stack memory associated with local variable 'z' returned}}
           decltype(a) A = a;
----------------
This diagnostic looks like it'll be confusing. Can you add a note pointing at the `return` statement in question (we're warning about the one on line 179, I assume)?

================
Comment at: test/SemaCXX/cxx1y-init-captures.cpp:123-124
@@ -122,3 +122,4 @@
       };
-      return M;
+      // FIXME: We shouldn't emit this warning here.
+      return M; // expected-warning{{stack memory}}
     };
----------------
Do you know why this happens? It looks like `EvalVal` should bail out on the `DeclRefExpr` in the init-capture, because it `refersToEnclosingVariableOrCapture`.

================
Comment at: test/SemaCXX/return-lambda-stack-addr.cpp:68-72
@@ +67,7 @@
+
+auto ref_ref() {
+  int x;
+  int &y = x;              // expected-note{{binding reference variable 'y' here}}
+  return [&] { (void)y; }; // expected-warning{{address of stack memory associated with local variable 'x' returned}}
+}
+
----------------
I'd like to see another testcase for the case when a local reference is safely captured by reference:

  auto ref_ref_ok(int &r) {
    int &y = r;
    return [&] { return y; }; // no warning
  }


https://reviews.llvm.org/D24639