Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 7 commits into from
Jun 4, 2025
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
0db205b
[clang] Correct FixIt ranges for unused capture warnings
ojhunt May 22, 2025
2e67f4c
Fixing macros and adding tests
ojhunt Jun 3, 2025
00f1e7d
Extract the fixit range construction into a separate function
ojhunt Jun 3, 2025
357995f
Add release note
ojhunt Jun 3, 2025
e721495
Merge remote-tracking branch 'origin/llvm.org/main' into users/ojhunt…
ojhunt Jun 3, 2025
2f769f0
Correct corentin's nits :D
ojhunt Jun 3, 2025
1e1872f
Merge remote-tracking branch 'origin/llvm.org/main' into users/ojhunt…
ojhunt Jun 4, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions clang/docs/ReleaseNotes.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
----------------------------------

Expand Down
8 changes: 8 additions & 0 deletions clang/include/clang/Sema/Sema.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down Expand Up @@ -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.
Expand Down
22 changes: 22 additions & 0 deletions clang/lib/Sema/Sema.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we use findLocationAfterToken here instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm using this one function for two purposes:

  • Finding the (presumed) comma and stepping over it
  • Finding the start of the next token

It seemed like a reasonable approach, and given we're producing a fixit diagnostic I don't think the perf matters.

Basically what I'm trying to do is say given an unused capture:

auto foo = [a, unused,           b] ... // just a pile of white space for illustrative purposes
auto bar = [a, unused, /* ... */ b] ...
auto wibble = [a,  /* ... */  unused, b] ...
auto thingy = [a,  unused /* ... */, b] ...

produce fixits that result in

auto foo = [a, b] ... // just a pile of white space for illustrative purposes
auto bar = [a, /* ... */ b] ...
auto wibble = [a, b] ...
auto thingy = [a, b] ...

and for that what we want to do is find the end of the comma, and the start of the next token, simply using the "findNextToken" to just get those tokens seems reasonable.

Of course as we discussed in discord, that function doesn't handle macros, and handling this correctly for macros would imply the Sema "wrapper" excludes macro wrappers (so would not just be a wrapper) or every user has to check for macros.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cor3ntin ping for your copious spare time :D

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();
Comment on lines +101 to +102
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we check TokenEnd.isMacroID() ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup! But that kind of results in this no longer being just a wrapper so I'm not sure if that's "good" - I might make it a flag?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cor3ntin thoughts? basically should the Lexer function take a "allow tokens that intersect macros", should the Sema wrapper, or should - because of use case - the Sema wrapper always exclude macros? I don't particularly like the last option as it means the Sema and Lexer versions of the same function name behave differently

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cor3ntin I've added a "include macros" parameter and removed the default parameters because I think that given the lack of existing usages there's no real reason for the default args, and they present a hazard.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks reasonable

if (!IncludeMacros && (TokenStart.isMacroID() || TokenEnd.isMacroID()))
return SourceRange();

return SourceRange(TokenStart, TokenEnd);
}

ModuleLoader &Sema::getModuleLoader() const { return PP.getModuleLoader(); }

DarwinSDKInfo *
Expand Down
61 changes: 45 additions & 16 deletions clang/lib/Sema/SemaLambda.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;
}

Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -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);
}
}

Expand Down
Loading
Loading