[Sema] Improve side effect checking for unused-lambda-capture warning

[malcolm.parsons]

Summary

Don't warn about unused lambda captures that involve copying a
value of a type that cannot be trivially copied and destroyed.

Fixes PR31977

[aaron.ballman]

lib/Sema/SemaLambda.cpp

1458 ↗

(On Diff #89625)

I think this is missing one more case: capturing a volatile variable by copy does have a side effect in that the variable is read when the capture occurs. e.g., this should not diagnose either:

void f(volatile int v) {
  [v]{}();
}

[malcolm.parsons]

I found this FIXME comment in Expr::HasSideEffects():

case LambdaExprClass: {
  const LambdaExpr *LE = cast<LambdaExpr>(this);
  for (LambdaExpr::capture_iterator I = LE->capture_begin(),
                                    E = LE->capture_end(); I != E; ++I)
    if (I->getCaptureKind() == LCK_ByCopy)
      // FIXME: Only has a side-effect if the variable is volatile or if
      // the copy would invoke a non-trivial copy constructor.
      return true;
  return false;
}

It seems a shame not to fix this now, but I don't think I can call Sema code from AST code, and my CaptureHasSideEffects() depends on Sema::getCurrentThisType().

Any ideas?

[malcolm.parsons]

Handle captures of volatile variables.

[aaron.ballman]

LGTM!

In D30327#688254, @malcolm.parsons wrote:

I found this FIXME comment in Expr::HasSideEffects():

case LambdaExprClass: {
  const LambdaExpr *LE = cast<LambdaExpr>(this);
  for (LambdaExpr::capture_iterator I = LE->capture_begin(),
                                    E = LE->capture_end(); I != E; ++I)
    if (I->getCaptureKind() == LCK_ByCopy)
      // FIXME: Only has a side-effect if the variable is volatile or if
      // the copy would invoke a non-trivial copy constructor.
      return true;
  return false;
}

It seems a shame not to fix this now, but I don't think I can call Sema code from AST code, and my CaptureHasSideEffects() depends on Sema::getCurrentThisType().

Any ideas?

Correct, you cannot call Sema code from AST. I don't know of a good way to resolve this off the top of my head given what getCurrentThisType() needs to do. I agree that this would be good to fix as well, but your patch is a good incremental improvement and should't be held up on this.

cfe/trunk/include/clang/Sema/Sema.h

   void ActOnLambdaError(SourceLocation StartLoc, Scope *CurScope,
                         bool IsInstantiation = false);
 
   /// ActOnLambdaExpr - This is called when the body of a lambda expression
   /// was successfully completed.
   ExprResult ActOnLambdaExpr(SourceLocation StartLoc, Stmt *Body,
                              Scope *CurScope);
 
+  /// \brief Does copying/destroying the captured variable have side effects?
+  bool CaptureHasSideEffects(const sema::LambdaScopeInfo::Capture &From);
+
   /// \brief Diagnose if an explicit lambda capture is unused.
   void DiagnoseUnusedLambdaCapture(const sema::LambdaScopeInfo::Capture &From);
 
   /// \brief Complete a lambda-expression having processed and attached the
   /// lambda body.
   ExprResult BuildLambdaExpr(SourceLocation StartLoc, SourceLocation EndLoc,
                              sema::LambdaScopeInfo *LSI);
 

cfe/trunk/lib/Sema/SemaLambda.cpp

mapImplicitCaptureStyle(CapturingScopeInfo::ImplicitCaptureStyle ICS) {
   case CapturingScopeInfo::ImpCap_LambdaByref:
     return LCD_ByRef;
   case CapturingScopeInfo::ImpCap_Block:
     llvm_unreachable("block capture in lambda");
   }
   llvm_unreachable("Unknown implicit capture style");
 }
 
-void Sema::DiagnoseUnusedLambdaCapture(const LambdaScopeInfo::Capture &From) {
+bool Sema::CaptureHasSideEffects(const LambdaScopeInfo::Capture &From) {
   if (!From.isVLATypeCapture()) {
     Expr *Init = From.getInitExpr();
     if (Init && Init->HasSideEffects(Context))
-      return;
+      return true;
   }
 
+  if (!From.isCopyCapture())
+    return false;
+
+  const QualType T = From.isThisCapture()
+                         ? getCurrentThisType()->getPointeeType()
+                         : From.getCaptureType();
+
+  if (T.isVolatileQualified())
+    return true;
+
+  const Type *BaseT = T->getBaseElementTypeUnsafe();
+  if (const CXXRecordDecl *RD = BaseT->getAsCXXRecordDecl())
+    return !RD->isCompleteDefinition() || !RD->hasTrivialCopyConstructor() ||
+           !RD->hasTrivialDestructor();
+
+  return false;
+}
+
+void Sema::DiagnoseUnusedLambdaCapture(const LambdaScopeInfo::Capture &From) {
+  if (CaptureHasSideEffects(From))
+    return;
+
   auto diag = Diag(From.getLocation(), diag::warn_unused_lambda_capture);
   if (From.isThisCapture())
     diag << "'this'";
   else
     diag << From.getVariable();
   diag << From.isNonODRUsed();
 }
 

cfe/trunk/test/SemaCXX/warn-unused-lambda-capture.cpp

-// RUN: %clang_cc1 -fsyntax-only -Wunused-lambda-capture -Wused-but-marked-unused -Wno-uninitialized -verify -std=c++14 %s
+// RUN: %clang_cc1 -fsyntax-only -Wunused-lambda-capture -Wused-but-marked-unused -Wno-uninitialized -verify -std=c++1z %s
 
 class NonTrivialConstructor {
 public:
   NonTrivialConstructor() {}
 };
 
+class NonTrivialCopyConstructor {
+public:
+  NonTrivialCopyConstructor() = default;
+  NonTrivialCopyConstructor(const NonTrivialCopyConstructor &) {}
+};
+
 class NonTrivialDestructor {
 public:
   ~NonTrivialDestructor() {}
 };
 
 class Trivial {
 public:
   Trivial() = default;
void test() {
   auto explicit_initialized_value_trivial_init = [j = Trivial()]{}; // expected-warning{{lambda capture 'j' is not used}}
   auto explicit_initialized_value_non_trivial_init = [j = Trivial(42)]{};
   auto explicit_initialized_value_with_side_effect = [j = side_effect()]{};
 
   auto nested = [&i] {
     auto explicit_by_value_used = [i] { return i + 1; };
     auto explicit_by_value_unused = [i] {}; // expected-warning{{lambda capture 'i' is not used}}
   };
+
+  Trivial trivial;
+  auto explicit_by_value_trivial = [trivial] {}; // expected-warning{{lambda capture 'trivial' is not used}}
+
+  NonTrivialConstructor cons;
+  auto explicit_by_value_non_trivial_constructor = [cons] {}; // expected-warning{{lambda capture 'cons' is not used}}
+
+  NonTrivialCopyConstructor copy_cons;
+  auto explicit_by_value_non_trivial_copy_constructor = [copy_cons] {};
+
+  NonTrivialDestructor dest;
+  auto explicit_by_value_non_trivial_destructor = [dest] {};
+
+  volatile int v;
+  auto explicit_by_value_volatile = [v] {};
 }
 
-class Foo
-{
+class TrivialThis : Trivial {
   void test() {
     auto explicit_this_used = [this] { return i; };
     auto explicit_this_used_void = [this] { (void)this; };
     auto explicit_this_unused = [this] {}; // expected-warning{{lambda capture 'this' is not used}}
+    auto explicit_star_this_used = [*this] { return i; };
+    auto explicit_star_this_used_void = [*this] { (void)this; };
+    auto explicit_star_this_unused = [*this] {}; // expected-warning{{lambda capture 'this' is not used}}
+  }
+  int i;
+};
+
+class NonTrivialConstructorThis : NonTrivialConstructor {
+  void test() {
+    auto explicit_this_used = [this] { return i; };
+    auto explicit_this_used_void = [this] { (void)this; };
+    auto explicit_this_unused = [this] {}; // expected-warning{{lambda capture 'this' is not used}}
+    auto explicit_star_this_used = [*this] { return i; };
+    auto explicit_star_this_used_void = [*this] { (void)this; };
+    auto explicit_star_this_unused = [*this] {}; // expected-warning{{lambda capture 'this' is not used}}
+  }
+  int i;
+};
+
+class NonTrivialCopyConstructorThis : NonTrivialCopyConstructor {
+  void test() {
+    auto explicit_this_used = [this] { return i; };
+    auto explicit_this_used_void = [this] { (void)this; };
+    auto explicit_this_unused = [this] {}; // expected-warning{{lambda capture 'this' is not used}}
+    auto explicit_star_this_used = [*this] { return i; };
+    auto explicit_star_this_used_void = [*this] { (void)this; };
+    auto explicit_star_this_unused = [*this] {};
+  }
+  int i;
+};
+
+class NonTrivialDestructorThis : NonTrivialDestructor {
+  void test() {
+    auto explicit_this_used = [this] { return i; };
+    auto explicit_this_used_void = [this] { (void)this; };
+    auto explicit_this_unused = [this] {}; // expected-warning{{lambda capture 'this' is not used}}
+    auto explicit_star_this_used = [*this] { return i; };
+    auto explicit_star_this_used_void = [*this] { (void)this; };
+    auto explicit_star_this_unused = [*this] {};
   }
   int i;
 };
 
 template <typename T>
 void test_templated() {
   int i = 0;
   const int k = 0;
void test_templated() {
   auto explicit_initialized_value_trivial_init = [j = Trivial()]{}; // expected-warning{{lambda capture 'j' is not used}}
   auto explicit_initialized_value_non_trivial_init = [j = Trivial(42)]{};
   auto explicit_initialized_value_with_side_effect = [j = side_effect()]{};
 
   auto nested = [&i] {
     auto explicit_by_value_used = [i] { return i + 1; };
     auto explicit_by_value_unused = [i] {}; // expected-warning{{lambda capture 'i' is not used}}
   };
+
+  Trivial trivial;
+  auto explicit_by_value_trivial = [trivial] {}; // expected-warning{{lambda capture 'trivial' is not used}}
+
+  NonTrivialConstructor cons;
+  auto explicit_by_value_non_trivial_constructor = [cons] {}; // expected-warning{{lambda capture 'cons' is not used}}
+
+  NonTrivialCopyConstructor copy_cons;
+  auto explicit_by_value_non_trivial_copy_constructor = [copy_cons] {};
+
+  NonTrivialDestructor dest;
+  auto explicit_by_value_non_trivial_destructor = [dest] {};
+
+  volatile int v;
+  auto explicit_by_value_volatile = [v] {};
 }
 
 void test_use_template() {
   test_templated<int>(); // expected-note{{in instantiation of function template specialization 'test_templated<int>' requested here}}
 }