[libTooling] Add flag to getRangeForEdit to ignore macro expansions

[li.zhe.hua]

Summary

This commit resolves the FIXME around the behavior of
Lexer::makeFileCharRange that getRangeForEdit inherits around
source locations in macro expansions.

We add a flag to getRangeForEdit that allows a caller to disable the
behavior, and instead uses the spelling location instead, with checks
to ensure that the source locations are not within a macro definition.

[ymandel]

clang/lib/Tooling/Transformer/SourceCode.cpp

90–92

can you comment on the logic here? Why isn't this just "return true", since it's in a macro but not a part of a macro arg, I'd think it must be part of a macro definition.

111–113

LLVM style doesn't use braces for single-line ifs.

[li.zhe.hua]

Address comments

[li.zhe.hua]

clang/lib/Tooling/Transformer/SourceCode.cpp

90–92

Good catch, it should just return true.

clang/include/clang/Tooling/Transformer/SourceCode.h

 }
 
 /// Determines whether \p Range is one that can be edited by a rewrite;
 /// generally, one that starts and ends within a particular file.
 llvm::Error validateEditRange(const CharSourceRange &Range,
                               const SourceManager &SM);
 
 /// Attempts to resolve the given range to one that can be edited by a rewrite;
-/// generally, one that starts and ends within a particular file. It supports a
-/// limited set of cases involving source locations in macro expansions. If a
-/// value is returned, it satisfies \c validateEditRange.
+/// generally, one that starts and ends within a particular file. If a value is
+/// returned, it satisfies \c validateEditRange.
+///
+/// If \c IncludeMacroExpansion is true, a limited set of cases involving source
+/// locations in macro expansions is supported. For example, if we're looking to
+/// rewrite the int literal 3 to 6, and we have the following definition:
+///    #define DO_NOTHING(x) x
+/// then
+///    foo(DO_NOTHING(3))
+/// will be rewritten to
+///    foo(6)
 llvm::Optional<CharSourceRange>
 getRangeForEdit(const CharSourceRange &EditRange, const SourceManager &SM,
-                const LangOptions &LangOpts);
+                const LangOptions &LangOpts, bool IncludeMacroExpansion = true);
 inline llvm::Optional<CharSourceRange>
-getRangeForEdit(const CharSourceRange &EditRange, const ASTContext &Context) {
+getRangeForEdit(const CharSourceRange &EditRange, const ASTContext &Context,
+                bool IncludeMacroExpansion = true) {
   return getRangeForEdit(EditRange, Context.getSourceManager(),
-                         Context.getLangOpts());
+                         Context.getLangOpts(), IncludeMacroExpansion);
 }
 } // namespace tooling
 } // namespace clang
 #endif // LLVM_CLANG_TOOLING_TRANSFORMER_SOURCECODE_H

clang/lib/Tooling/Transformer/SourceCode.cpp

llvm::Error clang::tooling::validateEditRange(const CharSourceRange &Range,
 
   if (BeginInfo.second > EndInfo.second)
     return llvm::make_error<StringError>(
         errc::invalid_argument, "Range's begin is past its end");
 
   return llvm::Error::success();
 }
 
-llvm::Optional<CharSourceRange>
-clang::tooling::getRangeForEdit(const CharSourceRange &EditRange,
-                                const SourceManager &SM,
-                                const LangOptions &LangOpts) {
-  // FIXME: makeFileCharRange() has the disadvantage of stripping off "identity"
-  // macros. For example, if we're looking to rewrite the int literal 3 to 6,
-  // and we have the following definition:
-  //    #define DO_NOTHING(x) x
-  // then
-  //    foo(DO_NOTHING(3))
-  // will be rewritten to
-  //    foo(6)
-  // rather than the arguably better
-  //    foo(DO_NOTHING(6))
-  // Decide whether the current behavior is desirable and modify if not.
-  CharSourceRange Range = Lexer::makeFileCharRange(EditRange, SM, LangOpts);
+static bool SpelledInMacroDefinition(SourceLocation Loc,
+                                     const SourceManager &SM) {
+  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();
+    } else {
+      return true;
+    }
+  }
+  return false;
+}
+
+llvm::Optional<CharSourceRange> clang::tooling::getRangeForEdit(
+    const CharSourceRange &EditRange, const SourceManager &SM,
+    const LangOptions &LangOpts, bool IncludeMacroExpansion) {
+  CharSourceRange Range;
+  if (IncludeMacroExpansion) {
+    Range = Lexer::makeFileCharRange(EditRange, SM, LangOpts);
+  } else {
+    if (SpelledInMacroDefinition(EditRange.getBegin(), SM) ||
+        SpelledInMacroDefinition(EditRange.getEnd(), SM))
+      return std::nullopt;
+
+    auto B = SM.getSpellingLoc(EditRange.getBegin());
+    auto E = SM.getSpellingLoc(EditRange.getEnd());
+    if (EditRange.isTokenRange())
+      E = Lexer::getLocForEndOfToken(E, 0, SM, LangOpts);
+    Range = CharSourceRange::getCharRange(B, E);
+  }
+
   bool IsInvalid = llvm::errorToBool(validateEditRange(Range, SM));
   if (IsInvalid)
     return std::nullopt;
   return Range;
-
 }
 
 static bool startsWithNewline(const SourceManager &SM, const Token &Tok) {
   return isVerticalWhitespace(SM.getCharacterData(Tok.getLocation())[0]);
 }
 
 static bool contains(const std::set<tok::TokenKind> &Terminators,
                      const Token &Tok) {

clang/unittests/Tooling/SourceCodeTest.cpp

TEST(SourceCodeTest, getAssociatedRangeInvalidForPartialExpansions) {
   FailingVarDeclsVisitor Visitor;
   // Should fail because it only includes a part of the expansion.
   std::string Code = R"cpp(
       #define DECL class foo { }; int x
       DECL;)cpp";
   Visitor.runOver(Code);
 }
 
-TEST(SourceCodeTest, EditRangeWithMacroExpansionsShouldSucceed) {
+class GetRangeForEditTest : public testing::TestWithParam<bool> {};
+INSTANTIATE_TEST_SUITE_P(WithAndWithoutExpansions, GetRangeForEditTest,
+                         testing::Bool());
+
+TEST_P(GetRangeForEditTest, EditRangeWithMacroExpansionsShouldSucceed) {
   // The call expression, whose range we are extracting, includes two macro
   // expansions.
   llvm::Annotations Code(R"cpp(
 #define M(a) a * 13
 int foo(int x, int y);
 int a = $r[[foo(M(1), M(2))]];
 )cpp");
 
   CallsVisitor Visitor;
-
   Visitor.OnCall = [&Code](CallExpr *CE, ASTContext *Context) {
     auto Range = CharSourceRange::getTokenRange(CE->getSourceRange());
-    EXPECT_THAT(getRangeForEdit(Range, *Context),
+    EXPECT_THAT(getRangeForEdit(Range, *Context, GetParam()),
                 ValueIs(AsRange(Context->getSourceManager(), Code.range("r"))));
   };
   Visitor.runOver(Code.code());
 }
 
 TEST(SourceCodeTest, EditWholeMacroExpansionShouldSucceed) {
   llvm::Annotations Code(R"cpp(
 #define FOO 10
 int a = $r[[FOO]];
 )cpp");
 
   IntLitVisitor Visitor;
   Visitor.OnIntLit = [&Code](IntegerLiteral *Expr, ASTContext *Context) {
     auto Range = CharSourceRange::getTokenRange(Expr->getSourceRange());
     EXPECT_THAT(getRangeForEdit(Range, *Context),
                 ValueIs(AsRange(Context->getSourceManager(), Code.range("r"))));
   };
   Visitor.runOver(Code.code());
 }
 
-TEST(SourceCodeTest, EditPartialMacroExpansionShouldFail) {
+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 M1(x) x(1)
+#define M2(x, y) x ## y
+#define M3(x) foobar(x)
+int foobar(int);
+int a = M1(foobar);
+int b = M2(foo, bar(2));
+int c = M3(3);
+)cpp");
+
+  CallsVisitor Visitor;
+  Visitor.OnCall = [](CallExpr *CE, ASTContext *Context) {
+    auto Range = CharSourceRange::getTokenRange(CE->getSourceRange());
+    EXPECT_FALSE(
+        getRangeForEdit(Range, *Context, /*IncludeMacroExpansion=*/false));
+  };
+  Visitor.runOver(Code.code());
+}
+
+TEST_P(GetRangeForEditTest, EditPartialMacroExpansionShouldFail) {
   std::string Code = R"cpp(
 #define BAR 10+
 int c = BAR 3.0;
 )cpp";
 
   IntLitVisitor Visitor;
   Visitor.OnIntLit = [](IntegerLiteral *Expr, ASTContext *Context) {
     auto Range = CharSourceRange::getTokenRange(Expr->getSourceRange());
-    EXPECT_FALSE(getRangeForEdit(Range, *Context));
+    EXPECT_FALSE(getRangeForEdit(Range, *Context, GetParam()));
   };
   Visitor.runOver(Code);
 }
 
-TEST(SourceCodeTest, EditWholeMacroArgShouldSucceed) {
+TEST_P(GetRangeForEditTest, EditWholeMacroArgShouldSucceed) {
   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(getRangeForEdit(Range, *Context),
+    EXPECT_THAT(getRangeForEdit(Range, *Context, GetParam()),
                 ValueIs(AsRange(Context->getSourceManager(), Code.range("r"))));
   };
   Visitor.runOver(Code.code());
 }
 
-TEST(SourceCodeTest, EditPartialMacroArgShouldSucceed) {
+TEST_P(GetRangeForEditTest, EditPartialMacroArgShouldSucceed) {
   llvm::Annotations Code(R"cpp(
 #define FOO(a) a + 7.0;
 int a = FOO($r[[10]] + 10.0);
 )cpp");
 
   IntLitVisitor Visitor;
   Visitor.OnIntLit = [&Code](IntegerLiteral *Expr, ASTContext *Context) {
     auto Range = CharSourceRange::getTokenRange(Expr->getSourceRange());
-    EXPECT_THAT(getRangeForEdit(Range, *Context),
+    EXPECT_THAT(getRangeForEdit(Range, *Context, GetParam()),
                 ValueIs(AsRange(Context->getSourceManager(), Code.range("r"))));
   };
   Visitor.runOver(Code.code());
 }
 
 TEST(SourceCodeTest, EditRangeWithMacroExpansionsIsValid) {
   // The call expression, whose range we are extracting, includes two macro
   // expansions.
   llvm::StringRef Code = R"cpp(
 #define M(a) a * 13
 int foo(int x, int y);
 int a = foo(M(1), M(2));
 )cpp";
 
   CallsVisitor Visitor;
-
   Visitor.OnCall = [](CallExpr *CE, ASTContext *Context) {
     auto Range = CharSourceRange::getTokenRange(CE->getSourceRange());
     EXPECT_THAT_ERROR(validateEditRange(Range, Context->getSourceManager()),
                       Succeeded());
   };
   Visitor.runOver(Code);
 }