https://github.com/tJener created https://github.com/llvm/llvm-project/pull/171555
When an expression is in a single macro argument but also contains a macro, `getFileRange` would incorrectly reject that expression, concluding that it came from two different macro arguments because they came from two different expansions. We adjust the logic to look at the full path of macro argument expansion locations instead, tracking that if our traversal up the macro expansions continues all the way through macro arguments all the way to the top. This is similar to the technique used by `makeFileCharRange`. We also add some test cases to ensure we don't introduce any false positives. >From 24c2eda0cd24f4b2db070473a998dcfbbc8a77e4 Mon Sep 17 00:00:00 2001 From: Eric Li <[email protected]> Date: Tue, 9 Dec 2025 20:18:18 -0500 Subject: [PATCH] [clang][tooling] Fix `getFileRange` false negative When an expression is in a single macro argument but also contains a macro, `getFileRange` would incorrectly reject that expression, concluding that it came from two different macro arguments because they came from two different expansions. We adjust the logic to look at the full path of macro argument expansion locations instead, tracking that if our traversal up the macro expansions continues all the way through macro arguments all the way to the top. This is similar to the technique used by `makeFileCharRange`. We also add some test cases to ensure we don't introduce any false positives. --- clang/lib/Tooling/Transformer/SourceCode.cpp | 23 +++++---- clang/unittests/Tooling/SourceCodeTest.cpp | 51 +++++++++++++++----- 2 files changed, 51 insertions(+), 23 deletions(-) diff --git a/clang/lib/Tooling/Transformer/SourceCode.cpp b/clang/lib/Tooling/Transformer/SourceCode.cpp index 7adceda05ad4f..0ca5113035f7b 100644 --- a/clang/lib/Tooling/Transformer/SourceCode.cpp +++ b/clang/lib/Tooling/Transformer/SourceCode.cpp @@ -86,32 +86,35 @@ llvm::Error clang::tooling::validateEditRange(const CharSourceRange &Range, return validateRange(Range, SM, /*AllowSystemHeaders=*/false); } -// Returns the location of the top-level macro argument that is the spelling for -// the expansion `Loc` is from. If `Loc` is spelled in the macro definition, -// returns an invalid `SourceLocation`. -static SourceLocation getMacroArgumentSpellingLoc(SourceLocation Loc, - const SourceManager &SM) { +// Returns the full set of expansion locations of `Loc` from bottom to top-most +// macro, if `Loc` is spelled in a macro argument. If `Loc` is spelled in the +// macro definition, returns an empty vector. +static llvm::SmallVector<SourceLocation, 2> +getMacroArgumentExpansionLocs(SourceLocation Loc, const SourceManager &SM) { assert(Loc.isMacroID() && "Location must be in a macro"); + llvm::SmallVector<SourceLocation, 2> ArgLocs; while (Loc.isMacroID()) { const auto &Expansion = SM.getSLocEntry(SM.getFileID(Loc)).getExpansion(); if (Expansion.isMacroArgExpansion()) { // Check the spelling location of the macro arg, in case the arg itself is // in a macro expansion. Loc = Expansion.getSpellingLoc(); + ArgLocs.push_back(Expansion.getExpansionLocStart()); } else { return {}; } } - return Loc; + return ArgLocs; } static bool spelledInMacroDefinition(CharSourceRange Range, const SourceManager &SM) { if (Range.getBegin().isMacroID() && Range.getEnd().isMacroID()) { - // Check whether the range is entirely within a single macro argument. - auto B = getMacroArgumentSpellingLoc(Range.getBegin(), SM); - auto E = getMacroArgumentSpellingLoc(Range.getEnd(), SM); - return B.isInvalid() || B != E; + // Check whether the range is entirely within a single macro argument by + // checking if they are in the same macro argument at every level. + auto B = getMacroArgumentExpansionLocs(Range.getBegin(), SM); + auto E = getMacroArgumentExpansionLocs(Range.getEnd(), SM); + return B.empty() || B != E; } return Range.getBegin().isMacroID() || Range.getEnd().isMacroID(); diff --git a/clang/unittests/Tooling/SourceCodeTest.cpp b/clang/unittests/Tooling/SourceCodeTest.cpp index a998954a6e9ea..d0eba43b0bb1c 100644 --- a/clang/unittests/Tooling/SourceCodeTest.cpp +++ b/clang/unittests/Tooling/SourceCodeTest.cpp @@ -507,17 +507,21 @@ TEST(SourceCodeTest, EditInvolvingExpansionIgnoringExpansionShouldFail) { // If we specify to ignore macro expansions, none of these call expressions // should have an editable range. llvm::Annotations Code(R"cpp( +#define NOOP(x) x #define M1(x) x(1) -#define M2(x, y) x ## y +#define M2(x, y) NOOP(M2_IMPL(x, y)) +#define M2_IMPL(x, y) x ## y #define M3(x) foobar(x) -#define M4(x, y) x y +#define M4(x, y) NOOP(x y) #define M5(x) x +#define M6(...) M4(__VA_ARGS__) int foobar(int); int a = M1(foobar); int b = M2(foo, bar(2)); int c = M3(3); int d = M4(foobar, (4)); int e = M5(foobar) (5); +int f = M6(foobar, (6)); )cpp"); CallsVisitor Visitor; @@ -592,18 +596,39 @@ int c = BAR 3.0; } TEST_P(GetFileRangeForEditTest, EditWholeMacroArgShouldSucceed) { - llvm::Annotations Code(R"cpp( -#define FOO(a) a + 7.0; -int a = FOO($r[[10]]); -)cpp"); + { + llvm::Annotations Code(R"cpp( + #define FOO(a) a + 7.0; + int a = FOO($r[[10]]); + )cpp"); + + IntLitVisitor Visitor; + Visitor.OnIntLit = [&Code](IntegerLiteral *Expr, ASTContext *Context) { + auto Range = CharSourceRange::getTokenRange(Expr->getSourceRange()); + EXPECT_THAT( + getFileRangeForEdit(Range, *Context, GetParam()), + ValueIs(AsRange(Context->getSourceManager(), Code.range("r")))); + }; + Visitor.runOver(Code.code()); + } - IntLitVisitor Visitor; - Visitor.OnIntLit = [&Code](IntegerLiteral *Expr, ASTContext *Context) { - auto Range = CharSourceRange::getTokenRange(Expr->getSourceRange()); - EXPECT_THAT(getFileRangeForEdit(Range, *Context, GetParam()), - ValueIs(AsRange(Context->getSourceManager(), Code.range("r")))); - }; - Visitor.runOver(Code.code()); + { + llvm::Annotations Code(R"cpp( + #define FOO(a) a + 7.0; + #define DO_NOTHING(x) x + int Frob(int x); + int a = FOO($r[[Frob(DO_NOTHING(40))]]); + )cpp"); + + CallsVisitor Visitor; + Visitor.OnCall = [&Code](CallExpr *Expr, ASTContext *Context) { + auto Range = CharSourceRange::getTokenRange(Expr->getSourceRange()); + EXPECT_THAT( + getFileRangeForEdit(Range, *Context, GetParam()), + ValueIs(AsRange(Context->getSourceManager(), Code.range("r")))); + }; + Visitor.runOver(Code.code()); + } } TEST_P(GetFileRangeForEditTest, EditPartialMacroArgShouldSucceed) { _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
