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

[ymandel]

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.

[ymandel]

comment tweaks.

[gribozavr]

clang/lib/Tooling/Refactoring/Transformer.cpp

197 ↗

(On Diff #216838)

This comment was moved into the function and now looks out of place here.

[ymandel]

comments tweaks.

[ymandel]

I'm having second thoughts about this -- I prefer the approach I ended up taking in https://reviews.llvm.org/D66676, which is subtly different.

However, getRuleMatchLoc() will be useful a different purpose: when only reporting a diagnostic, with no corresponding changes. So, I plan to rework this into two revisions: one to match https://reviews.llvm.org/D66676 (and keep the tests esssentially as they are) and one to add getRuleMatchLoc for future use. I can also make both changes in the same revision, if you prefer.

clang/lib/Tooling/Refactoring/Transformer.cpp

197 ↗

(On Diff #216838)

removed in both places. Even in the function, it didn't add anything meaningful to the assert() itself.

[gribozavr]

So, I plan to rework this into two revisions: one to match https://reviews.llvm.org/D66676 (and keep the tests esssentially as they are) and one to add getRuleMatchLoc for future use.

That SGTM.

I don't have an opinion about whether they should be separate revisions or not.

[ymandel]

reworked to follow same scheme as clang-tidy version, per discussion.

[ymandel]

In D66652#1654585, @gribozavr wrote:

So, I plan to rework this into two revisions: one to match https://reviews.llvm.org/D66676 (and keep the tests esssentially as they are) and one to add getRuleMatchLoc for future use.

That SGTM.

I don't have an opinion about whether they should be separate revisions or not.

Reworked, per our discussion. I kept it in one revision, because I ended up useing the new function in this code (for error reporting).

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

 /// Builds a set of matchers that cover the rule (one for each distinct node
 /// matcher base kind: Stmt, Decl, etc.). Node-matchers for `QualType` and
 /// `Type` are not permitted, since such nodes carry no source location
 /// information and are therefore not relevant for rewriting. If any such
 /// matchers are included, will return an empty vector.
 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 &
 findSelectedCase(const ast_matchers::MatchFinder::MatchResult &Result,
                  const RewriteRule &Rule);
 
 /// A source "transformation," represented by a character range in the source to
 /// be replaced and a corresponding replacement string.

cfe/trunk/lib/Tooling/Refactoring/Transformer.cpp

 }
 
 DynTypedMatcher tooling::detail::buildMatcher(const RewriteRule &Rule) {
   std::vector<DynTypedMatcher> Ms = buildMatchers(Rule);
   assert(Ms.size() == 1 && "Cases must have compatible matchers.");
   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 &
 tooling::detail::findSelectedCase(const MatchResult &Result,
                                   const RewriteRule &Rule) {
   if (Rule.Cases.size() == 1)
     return Rule.Cases[0];
 
void Transformer::registerMatchers(MatchFinder *MatchFinder) {
   for (auto &Matcher : tooling::detail::buildMatchers(Rule))
     MatchFinder->addDynamicMatcher(Matcher, this);
 }
 
 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) {
     Consumer(Transformations.takeError());
     return;
   }
 
   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));
       return;
     }
   }
 
   for (const auto &I : Case.AddedIncludes) {

cfe/trunk/unittests/Tooling/TransformerTest.cpp

#define ID(e) e
   std::string Expected = R"cc(
 #define ID(e) e
     int f(string s) { return REPLACED; }
   )cc";
 
   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
 // suffice.
 TEST_F(TransformerTest, NoPartialRewriteOMacroExpansion) {
   std::string Input = R"cc(
 #define ZERO_PLUS 0 + 3
     int f(string s) { return ZERO_PLUS; })cc";