diff --git a/clang/include/clang/Tooling/Refactoring/Transformer.h b/clang/include/clang/Tooling/Refactoring/Transformer.h --- a/clang/include/clang/Tooling/Refactoring/Transformer.h +++ b/clang/include/clang/Tooling/Refactoring/Transformer.h @@ -251,6 +251,12 @@ std::vector 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 & diff --git a/clang/lib/Tooling/Refactoring/Transformer.cpp b/clang/lib/Tooling/Refactoring/Transformer.cpp --- a/clang/lib/Tooling/Refactoring/Transformer.cpp +++ b/clang/lib/Tooling/Refactoring/Transformer.cpp @@ -150,6 +150,21 @@ 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 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 @@ 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 @@ 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)); diff --git a/clang/unittests/Tooling/TransformerTest.cpp b/clang/unittests/Tooling/TransformerTest.cpp --- a/clang/unittests/Tooling/TransformerTest.cpp +++ b/clang/unittests/Tooling/TransformerTest.cpp @@ -710,6 +710,57 @@ 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