[clang][tooling] Fix getFileRange false negative (llvm#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.

Author: tJener

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();

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) {