[Sema] Add fixit for unused lambda captures

This diff adds a fixit to suggest removing unused lambda captures 
in the appropriate diagnostic.

Patch by Andrew Comminos!

Test plan: make check-all

Differential revision: https://reviews.llvm.org/D48845


git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@337148 91177308-0d34-0410-b5e6-96231b3b80d8

6 files changed, 158 insertions, 12 deletions

diff --git a/include/clang/Sema/DeclSpec.h b/include/clang/Sema/DeclSpec.h
index 02afb093fd..40e5269c15 100644
--- a/include/clang/Sema/DeclSpec.h
+++ b/include/clang/Sema/DeclSpec.h
@@ -2540,12 +2540,16 @@ struct LambdaIntroducer {
     LambdaCaptureInitKind InitKind;
     ExprResult Init;
     ParsedType InitCaptureType;
+    SourceRange ExplicitRange;
+
     LambdaCapture(LambdaCaptureKind Kind, SourceLocation Loc,
                   IdentifierInfo *Id, SourceLocation EllipsisLoc,
                   LambdaCaptureInitKind InitKind, ExprResult Init,
-                  ParsedType InitCaptureType)
+                  ParsedType InitCaptureType,
+                  SourceRange ExplicitRange)
         : Kind(Kind), Loc(Loc), Id(Id), EllipsisLoc(EllipsisLoc),
-          InitKind(InitKind), Init(Init), InitCaptureType(InitCaptureType) {}
+          InitKind(InitKind), Init(Init), InitCaptureType(InitCaptureType),
+          ExplicitRange(ExplicitRange) {}
   };
 
   SourceRange Range;
@@ -2563,9 +2567,10 @@ struct LambdaIntroducer {
                   SourceLocation EllipsisLoc,
                   LambdaCaptureInitKind InitKind,
                   ExprResult Init, 
-                  ParsedType InitCaptureType) {
+                  ParsedType InitCaptureType,
+                  SourceRange ExplicitRange) {
     Captures.push_back(LambdaCapture(Kind, Loc, Id, EllipsisLoc, InitKind, Init,
-                                     InitCaptureType));
+                                     InitCaptureType, ExplicitRange));
   }
 };
 
diff --git a/include/clang/Sema/ScopeInfo.h b/include/clang/Sema/ScopeInfo.h
index fa1b1037a2..1074074c60 100644
--- a/include/clang/Sema/ScopeInfo.h
+++ b/include/clang/Sema/ScopeInfo.h
@@ -829,6 +829,9 @@ public:
   ///  if the enclosing full-expression is instantiation dependent).
   llvm::SmallSet<Expr *, 8> NonODRUsedCapturingExprs;
 
+  /// A map of explicit capture indices to their introducer source ranges.
+  llvm::DenseMap<unsigned, SourceRange> ExplicitCaptureRanges;
+
   /// Contains all of the variables defined in this lambda that shadow variables
   /// that were defined in parent contexts. Used to avoid warnings when the
   /// shadowed variables are uncaptured by this lambda.
diff --git a/include/clang/Sema/Sema.h b/include/clang/Sema/Sema.h
index ec60bb0bd6..2425dff005 100644
--- a/include/clang/Sema/Sema.h
+++ b/include/clang/Sema/Sema.h
@@ -5602,8 +5602,10 @@ public:
   /// Does copying/destroying the captured variable have side effects?
   bool CaptureHasSideEffects(const sema::Capture &From);
 
-  /// Diagnose if an explicit lambda capture is unused.
-  void DiagnoseUnusedLambdaCapture(const sema::Capture &From);
+  /// Diagnose if an explicit lambda capture is unused. Returns true if a
+  /// diagnostic is emitted.
+  bool DiagnoseUnusedLambdaCapture(SourceRange CaptureRange,
+                                   const sema::Capture &From);
 
   /// Complete a lambda-expression having processed and attached the
   /// lambda body.
diff --git a/lib/Parse/ParseExprCXX.cpp b/lib/Parse/ParseExprCXX.cpp
index 647069760d..26e7599951 100644
--- a/lib/Parse/ParseExprCXX.cpp
+++ b/lib/Parse/ParseExprCXX.cpp
@@ -808,6 +808,7 @@ Optional<unsigned> Parser::ParseLambdaIntroducer(LambdaIntroducer &Intro,
     IdentifierInfo *Id = nullptr;
     SourceLocation EllipsisLoc;
     ExprResult Init;
+    SourceLocation LocStart = Tok.getLocation();
 
     if (Tok.is(tok::star)) {
       Loc = ConsumeToken(); 
@@ -981,8 +982,11 @@ Optional<unsigned> Parser::ParseLambdaIntroducer(LambdaIntroducer &Intro,
           Loc, Kind == LCK_ByRef, Id, InitKind, InitExpr);
       Init = InitExpr;
     }
+
+    SourceLocation LocEnd = PrevTokLocation;
+
     Intro.addCapture(Kind, Loc, Id, EllipsisLoc, InitKind, Init,
-                     InitCaptureType);
+                     InitCaptureType, SourceRange(LocStart, LocEnd));
   }
 
   T.consumeClose();
diff --git a/lib/Sema/SemaLambda.cpp b/lib/Sema/SemaLambda.cpp
index 9e53beb449..0b768cc9fa 100644
--- a/lib/Sema/SemaLambda.cpp
+++ b/lib/Sema/SemaLambda.cpp
@@ -993,6 +993,8 @@ void Sema::ActOnStartOfLambdaDefinition(LambdaIntroducer &Intro,
       CheckCXXThisCapture(C->Loc, /*Explicit=*/true, /*BuildAndDiagnose*/ true,
                           /*FunctionScopeIndexToStopAtPtr*/ nullptr,
                           C->Kind == LCK_StarThis);
+      if (!LSI->Captures.empty())
+        LSI->ExplicitCaptureRanges[LSI->Captures.size() - 1] = C->ExplicitRange;
       continue;
     }
 
@@ -1139,6 +1141,8 @@ void Sema::ActOnStartOfLambdaDefinition(LambdaIntroducer &Intro,
                                                    TryCapture_ExplicitByVal;
       tryCaptureVariable(Var, C->Loc, Kind, EllipsisLoc);
     }
+    if (!LSI->Captures.empty())
+      LSI->ExplicitCaptureRanges[LSI->Captures.size() - 1] = C->ExplicitRange;
   }
   finishLambdaExplicitCaptures(LSI);
 
@@ -1478,12 +1482,13 @@ bool Sema::CaptureHasSideEffects(const Capture &From) {
   return false;
 }
 
-void Sema::DiagnoseUnusedLambdaCapture(const Capture &From) {
+bool Sema::DiagnoseUnusedLambdaCapture(SourceRange CaptureRange,
+                                       const Capture &From) {
   if (CaptureHasSideEffects(From))
-    return;
+    return false;
 
   if (From.isVLATypeCapture())
-    return;
+    return false;
 
   auto diag = Diag(From.getLocation(), diag::warn_unused_lambda_capture);
   if (From.isThisCapture())
@@ -1491,6 +1496,8 @@ void Sema::DiagnoseUnusedLambdaCapture(const Capture &From) {
   else
     diag << From.getVariable();
   diag << From.isNonODRUsed();
+  diag << FixItHint::CreateRemoval(CaptureRange);
+  return true;
 }
 
 ExprResult Sema::BuildLambdaExpr(SourceLocation StartLoc, SourceLocation EndLoc,
@@ -1532,18 +1539,49 @@ ExprResult Sema::BuildLambdaExpr(SourceLocation StartLoc, SourceLocation EndLoc,
 
     // Translate captures.
     auto CurField = Class->field_begin();
+    // True if the current capture has a used capture or default before it.
+    bool CurHasPreviousCapture = CaptureDefault != LCD_None;
+    SourceLocation PrevCaptureLoc = CurHasPreviousCapture ?
+        CaptureDefaultLoc : IntroducerRange.getBegin();
+
     for (unsigned I = 0, N = LSI->Captures.size(); I != N; ++I, ++CurField) {
       const Capture &From = LSI->Captures[I];
+
       assert(!From.isBlockCapture() && "Cannot capture __block variables");
       bool IsImplicit = I >= LSI->NumExplicitCaptures;
 
+      // Use source ranges of explicit captures for fixits where available.
+      SourceRange CaptureRange = LSI->ExplicitCaptureRanges[I];
+
       // Warn about unused explicit captures.
+      bool IsCaptureUsed = true;
       if (!CurContext->isDependentContext() && !IsImplicit && !From.isODRUsed()) {
         // Initialized captures that are non-ODR used may not be eliminated.
         bool NonODRUsedInitCapture =
             IsGenericLambda && From.isNonODRUsed() && From.getInitExpr();
-        if (!NonODRUsedInitCapture)
-          DiagnoseUnusedLambdaCapture(From);
+        if (!NonODRUsedInitCapture) {
+          bool IsLast = (I + 1) == LSI->NumExplicitCaptures;
+					SourceRange FixItRange;
+          if (CaptureRange.isValid()) {
+            if (!CurHasPreviousCapture && !IsLast) {
+              // If there are no captures preceding this capture, remove the
+              // following comma.
+              FixItRange = SourceRange(CaptureRange.getBegin(),
+                                       getLocForEndOfToken(CaptureRange.getEnd()));
+            } else {
+              // Otherwise, remove the comma since the last used capture.
+              FixItRange = SourceRange(getLocForEndOfToken(PrevCaptureLoc),
+                                       CaptureRange.getEnd());
+            }
+          }
+
+          IsCaptureUsed = !DiagnoseUnusedLambdaCapture(FixItRange, From);
+        }
+      }
+
+      if (CaptureRange.isValid()) {
+        CurHasPreviousCapture |= IsCaptureUsed;
+        PrevCaptureLoc = CaptureRange.getEnd();
       }
 
       // Handle 'this' capture.
diff --git a/test/FixIt/fixit-unused-lambda-capture.cpp b/test/FixIt/fixit-unused-lambda-capture.cpp
new file mode 100644
index 0000000000..c3120093ed
--- /dev/null
+++ b/test/FixIt/fixit-unused-lambda-capture.cpp
@@ -0,0 +1,94 @@
+// RUN: cp %s %t
+// RUN: %clang_cc1 -x c++ -fsyntax-only -Wunused-lambda-capture -Wno-unused-value -std=c++1z -fixit %t
+// RUN: grep -v CHECK %t | FileCheck %s
+
+void test() {
+  int i = 0;
+  int j = 0;
+  int k = 0;
+  int c = 10;
+  int a[c];
+
+  [i,j] { return i; };
+  // CHECK: [i] { return i; };
+  [i,j] { return j; };
+  // CHECK: [j] { return j; };
+  [&i,j] { return j; };
+  // CHECK: [j] { return j; };
+  [j,&i] { return j; };
+  // CHECK: [j] { return j; };
+  [i,j,k] {};
+  // CHECK: [] {};
+  [i,j,k] { return i + j; };
+  // CHECK: [i,j] { return i + j; };
+  [i,j,k] { return j + k; };
+  // CHECK: [j,k] { return j + k; };
+  [i,j,k] { return i + k; };
+  // CHECK: [i,k] { return i + k; };
+  [i,j,k] { return i + j + k; };
+  // CHECK: [i,j,k] { return i + j + k; };
+  [&,i] { return k; };
+  // CHECK: [&] { return k; };
+  [=,&i] { return k; };
+  // CHECK: [=] { return k; };
+  [=,&i,&j] { return j; };
+  // CHECK: [=,&j] { return j; };
+  [=,&i,&j] { return i; };
+  // CHECK: [=,&i] { return i; };
+  [z = i] {};
+  // CHECK: [] {};
+  [i,z = i] { return z; };
+  // CHECK: [z = i] { return z; };
+  [z = i,i] { return z; };
+  // CHECK: [z = i] { return z; };
+  [&a] {};
+  // CHECK: [] {};
+  [i,&a] { return i; };
+  // CHECK: [i] { return i; };
+  [&a,i] { return i; };
+  // CHECK: [i] { return i; };
+
+  #define I_MACRO() i
+  #define I_REF_MACRO() &i
+  [I_MACRO()] {};
+  // CHECK: [] {};
+  [I_MACRO(),j] { return j; };
+  // CHECK: [j] { return j; };
+  [j,I_MACRO()] { return j; };
+  // CHECK: [j] { return j; };
+  [I_REF_MACRO(),j] { return j; };
+  // CHECK: [j] { return j; };
+  [j,I_REF_MACRO()] { return j; };
+  // CHECK: [j] { return j; };
+
+  int n = 0;
+  [z = (n = i),j] {};
+  // CHECK: [z = (n = i)] {};
+  [j,z = (n = i)] {};
+  // CHECK: [z = (n = i)] {};
+}
+
+class ThisTest {
+  void test() {
+    int i = 0;
+
+    [this] {};
+    // CHECK: [] {};
+    [i,this] { return i; };
+    // CHECK: [i] { return i; };
+    [this,i] { return i; };
+    // CHECK: [i] { return i; };
+    [*this] {};
+    // CHECK: [] {};
+    [*this,i] { return i; };
+    // CHECK: [i] { return i; };
+    [i,*this] { return i; };
+    // CHECK: [i] { return i; };
+    [*this] { return this; };
+    // CHECK: [*this] { return this; };
+    [*this,i] { return this; };
+    // CHECK: [*this] { return this; };
+    [i,*this] { return this; };
+    // CHECK: [*this] { return this; };
+  }
+};