[libTooling] Transformer: refine `SourceLocation` specified as anchor of changes.

Summary: Every change triggered by a rewrite rule is anchored at a particular
location in the source code.  This patch refines how that location is chosen and
defines it as an explicit function so it can be shared by other Transformer
implementations.

This patch was inspired by a bug found by a clang tidy, wherein two changes were
anchored at the same location (the expansion loc of the macro) resulting in the
discarding of the second change.

Reviewers: gribozavr

Subscribers: cfe-commits

Tags: #clang

Differential Revision: https://reviews.llvm.org/D66652

llvm-svn: 373093

clang/include/clang/Tooling/Refactoring/Transformer.h

@@ -251,6 +251,12 @@ ast_matchers::internal::DynTypedMatcher buildMatcher(const RewriteRule &Rule);
 std::vector<ast_matchers::internal::DynTypedMatcher>
 buildMatchers(const RewriteRule &Rule);
 
+/// Gets the beginning location of the source matched by a rewrite rule. If the
+/// match occurs within a macro expansion, returns the beginning of the
+/// expansion point. `Result` must come from the matching of a rewrite rule.
+SourceLocation
+getRuleMatchLoc(const ast_matchers::MatchFinder::MatchResult &Result);
+
 /// Returns the \c Case of \c Rule that was selected in the match result.
 /// Assumes a matcher built with \c buildMatcher.
 const RewriteRule::Case &

clang/lib/Tooling/Refactoring/Transformer.cpp

@@ -150,6 +150,21 @@ DynTypedMatcher tooling::detail::buildMatcher(const RewriteRule &Rule) {
   return Ms[0];
 }
 
+SourceLocation tooling::detail::getRuleMatchLoc(const MatchResult &Result) {
+  auto &NodesMap = Result.Nodes.getMap();
+  auto Root = NodesMap.find(RewriteRule::RootID);
+  assert(Root != NodesMap.end() && "Transformation failed: missing root node.");
+  llvm::Optional<CharSourceRange> RootRange = getRangeForEdit(
+      CharSourceRange::getTokenRange(Root->second.getSourceRange()),
+      *Result.Context);
+  if (RootRange)
+    return RootRange->getBegin();
+  // The match doesn't have a coherent range, so fall back to the expansion
+  // location as the "beginning" of the match.
+  return Result.SourceManager->getExpansionLoc(
+      Root->second.getSourceRange().getBegin());
+}
+
 // Finds the case that was "selected" -- that is, whose matcher triggered the
 // `MatchResult`.
 const RewriteRule::Case &
@@ -178,14 +193,6 @@ void Transformer::run(const MatchResult &Result) {
   if (Result.Context->getDiagnostics().hasErrorOccurred())
     return;
 
-  // Verify the existence and validity of the AST node that roots this rule.
-  auto &NodesMap = Result.Nodes.getMap();
-  auto Root = NodesMap.find(RewriteRule::RootID);
-  assert(Root != NodesMap.end() && "Transformation failed: missing root node.");
-  SourceLocation RootLoc = Result.SourceManager->getExpansionLoc(
-      Root->second.getSourceRange().getBegin());
-  assert(RootLoc.isValid() && "Invalid location for Root node of match.");
-
   RewriteRule::Case Case = tooling::detail::findSelectedCase(Result, Rule);
   auto Transformations = tooling::detail::translateEdits(Result, Case.Edits);
   if (!Transformations) {
@@ -195,14 +202,16 @@ void Transformer::run(const MatchResult &Result) {
 
   if (Transformations->empty()) {
     // No rewrite applied (but no error encountered either).
-    RootLoc.print(llvm::errs() << "note: skipping match at loc ",
-                  *Result.SourceManager);
+    detail::getRuleMatchLoc(Result).print(
+        llvm::errs() << "note: skipping match at loc ", *Result.SourceManager);
     llvm::errs() << "\n";
     return;
   }
 
-  // Record the results in the AtomicChange.
-  AtomicChange AC(*Result.SourceManager, RootLoc);
+  // Record the results in the AtomicChange, anchored at the location of the
+  // first change.
+  AtomicChange AC(*Result.SourceManager,
+                  (*Transformations)[0].Range.getBegin());
   for (const auto &T : *Transformations) {
     if (auto Err = AC.replace(*Result.SourceManager, T.Range, T.Replacement)) {
       Consumer(std::move(Err));

clang/unittests/Tooling/TransformerTest.cpp

@@ -710,6 +710,57 @@ TEST_F(TransformerTest, IdentityMacro) {
   testRule(ruleStrlenSize(), Input, Expected);
 }
 
+// Tests that two changes in a single macro expansion do not lead to conflicts
+// in applying the changes.
+TEST_F(TransformerTest, TwoChangesInOneMacroExpansion) {
+  std::string Input = R"cc(
+#define PLUS(a,b) (a) + (b)
+    int f() { return PLUS(3, 4); }
+  )cc";
+  std::string Expected = R"cc(
+#define PLUS(a,b) (a) + (b)
+    int f() { return PLUS(LIT, LIT); }
+  )cc";
+
+  testRule(makeRule(integerLiteral(), change(text("LIT"))), Input, Expected);
+}
+
+// Tests case where the rule's match spans both source from the macro and its
+// arg, with the begin location (the "anchor") being the arg.
+TEST_F(TransformerTest, MatchSpansMacroTextButChangeDoesNot) {
+  std::string Input = R"cc(
+#define PLUS_ONE(a) a + 1
+    int f() { return PLUS_ONE(3); }
+  )cc";
+  std::string Expected = R"cc(
+#define PLUS_ONE(a) a + 1
+    int f() { return PLUS_ONE(LIT); }
+  )cc";
+
+  StringRef E = "expr";
+  testRule(makeRule(binaryOperator(hasLHS(expr().bind(E))),
+                    change(node(E), text("LIT"))),
+           Input, Expected);
+}
+
+// Tests case where the rule's match spans both source from the macro and its
+// arg, with the begin location (the "anchor") being inside the macro.
+TEST_F(TransformerTest, MatchSpansMacroTextButChangeDoesNotAnchoredInMacro) {
+  std::string Input = R"cc(
+#define PLUS_ONE(a) 1 + a
+    int f() { return PLUS_ONE(3); }
+  )cc";
+  std::string Expected = R"cc(
+#define PLUS_ONE(a) 1 + a
+    int f() { return PLUS_ONE(LIT); }
+  )cc";
+
+  StringRef E = "expr";
+  testRule(makeRule(binaryOperator(hasRHS(expr().bind(E))),
+                    change(node(E), text("LIT"))),
+           Input, Expected);
+}
+
 // No rewrite is applied when the changed text does not encompass the entirety
 // of the expanded text. That is, the edit would have to be applied to the
 // macro's definition to succeed and editing the expansion point would not