[clang] Correct FixIt ranges for unused capture warnings (#141148)

Fixes #106445 by using the lexer to find the correct range for the
removal FixIts. Previously the ranges that were generated assuming no
unsurprising formatting, which for the most part works. Being correct in
all cases requires using the lexer to find the bounding tokens for the
region to remove.

As part of this it adds Sema::getRangeForNextToken to wrap
Lexer::findNextToken.

Author: ojhunt

clang/docs/ReleaseNotes.rst

@@ -612,6 +612,8 @@ Improvements to Clang's diagnostics
   diagnostic group ``-Wimplicit-int-comparison-on-negation``, grouped under
   ``-Wimplicit-int-conversion``, so user can turn it off independently.
 
+- Improved the FixIts for unused lambda captures.
+
 Improvements to Clang's time-trace
 ----------------------------------
 

clang/include/clang/Sema/Sema.h

@@ -972,6 +972,13 @@ class Sema final : public SemaBase {
   /// Calls \c Lexer::getLocForEndOfToken()
   SourceLocation getLocForEndOfToken(SourceLocation Loc, unsigned Offset = 0);
 
+  /// Calls \c Lexer::findNextToken() to find the next token, and if the
+  /// locations of both ends of the token can be resolved it return that
+  /// range; Otherwise it returns an invalid SourceRange.
+  SourceRange getRangeForNextToken(
+      SourceLocation Loc, bool IncludeMacros, bool IncludeComments,
+      std::optional<tok::TokenKind> ExpectedToken = std::nullopt);
+
   /// Retrieve the module loader associated with the preprocessor.
   ModuleLoader &getModuleLoader() const;
 
@@ -9134,6 +9141,7 @@ class Sema final : public SemaBase {
   /// Diagnose if an explicit lambda capture is unused. Returns true if a
   /// diagnostic is emitted.
   bool DiagnoseUnusedLambdaCapture(SourceRange CaptureRange,
+                                   SourceRange FixItRange,
                                    const sema::Capture &From);
 
   /// Build a FieldDecl suitable to hold the given capture.

clang/lib/Sema/Sema.cpp

@@ -84,6 +84,28 @@ SourceLocation Sema::getLocForEndOfToken(SourceLocation Loc, unsigned Offset) {
   return Lexer::getLocForEndOfToken(Loc, Offset, SourceMgr, LangOpts);
 }
 
+SourceRange
+Sema::getRangeForNextToken(SourceLocation Loc, bool IncludeMacros,
+                           bool IncludeComments,
+                           std::optional<tok::TokenKind> ExpectedToken) {
+  if (!Loc.isValid())
+    return SourceRange();
+  std::optional<Token> NextToken =
+      Lexer::findNextToken(Loc, SourceMgr, LangOpts, IncludeComments);
+  if (!NextToken)
+    return SourceRange();
+  if (ExpectedToken && NextToken->getKind() != *ExpectedToken)
+    return SourceRange();
+  SourceLocation TokenStart = NextToken->getLocation();
+  SourceLocation TokenEnd = NextToken->getLastLoc();
+  if (!TokenStart.isValid() || !TokenEnd.isValid())
+    return SourceRange();
+  if (!IncludeMacros && (TokenStart.isMacroID() || TokenEnd.isMacroID()))
+    return SourceRange();
+
+  return SourceRange(TokenStart, TokenEnd);
+}
+
 ModuleLoader &Sema::getModuleLoader() const { return PP.getModuleLoader(); }
 
 DarwinSDKInfo *

clang/lib/Sema/SemaLambda.cpp

@@ -2022,6 +2022,7 @@ bool Sema::CaptureHasSideEffects(const Capture &From) {
 }
 
 bool Sema::DiagnoseUnusedLambdaCapture(SourceRange CaptureRange,
+                                       SourceRange FixItRange,
                                        const Capture &From) {
   if (CaptureHasSideEffects(From))
     return false;
@@ -2041,7 +2042,12 @@ bool Sema::DiagnoseUnusedLambdaCapture(SourceRange CaptureRange,
   else
     diag << From.getVariable();
   diag << From.isNonODRUsed();
-  diag << FixItHint::CreateRemoval(CaptureRange);
+  // If we were able to resolve the fixit range we'll create a fixit,
+  // otherwise we just use the raw capture range for the diagnostic.
+  if (FixItRange.isValid())
+    diag << FixItHint::CreateRemoval(FixItRange);
+  else
+    diag << CaptureRange;
   return true;
 }
 
@@ -2095,6 +2101,39 @@ FieldDecl *Sema::BuildCaptureField(RecordDecl *RD,
   return Field;
 }
 
+static SourceRange
+ConstructFixItRangeForUnusedCapture(Sema &S, SourceRange CaptureRange,
+                                    SourceLocation PrevCaptureLoc,
+                                    bool CurHasPreviousCapture, bool IsLast) {
+  if (!CaptureRange.isValid())
+    return SourceRange();
+
+  auto GetTrailingEndLocation = [&](SourceLocation StartPoint) {
+    SourceRange NextToken = S.getRangeForNextToken(
+        StartPoint, /*IncludeMacros=*/false, /*IncludeComments=*/true);
+    if (!NextToken.isValid())
+      return SourceLocation();
+    // Return the last location preceding the next token
+    return NextToken.getBegin().getLocWithOffset(-1);
+  };
+
+  if (!CurHasPreviousCapture && !IsLast) {
+    // If there are no captures preceding this capture, remove the
+    // trailing comma and anything up to the next token
+    SourceRange CommaRange =
+        S.getRangeForNextToken(CaptureRange.getEnd(), /*IncludeMacros=*/false,
+                               /*IncludeComments=*/false, tok::comma);
+    SourceLocation FixItEnd = GetTrailingEndLocation(CommaRange.getBegin());
+    return SourceRange(CaptureRange.getBegin(), FixItEnd);
+  }
+
+  // Otherwise, remove the comma since the last used capture, and
+  // anything up to the next token
+  SourceLocation FixItStart = S.getLocForEndOfToken(PrevCaptureLoc);
+  SourceLocation FixItEnd = GetTrailingEndLocation(CaptureRange.getEnd());
+  return SourceRange(FixItStart, FixItEnd);
+}
+
 ExprResult Sema::BuildLambdaExpr(SourceLocation StartLoc, SourceLocation EndLoc,
                                  LambdaScopeInfo *LSI) {
   // Collect information from the lambda scope.
@@ -2162,21 +2201,11 @@ ExprResult Sema::BuildLambdaExpr(SourceLocation StartLoc, SourceLocation EndLoc,
             IsGenericLambda && From.isNonODRUsed() && From.isInitCapture();
         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);
+          SourceRange FixItRange = ConstructFixItRangeForUnusedCapture(
+              *this, CaptureRange, PrevCaptureLoc, CurHasPreviousCapture,
+              IsLast);
+          IsCaptureUsed =
+              !DiagnoseUnusedLambdaCapture(CaptureRange, FixItRange, From);
         }
       }