[clang-tidy] TransformerClangTidyCheck: change choice of location for diagnostic message.

[ymandel]

Summary

This patch changes the location specified to`ClangTidyCheck::diag()`.
Currently, the beginning of the matched range is used. This patch uses the
beginning of the first fix's range. This change both simplifies the code and
(hopefully) gives a more intuitive result: the reported location aligns with
the fix(es) provided, rather than the (arbitrary) range of the rule's match.

N.B. this patch will break the line offset numbers in lit tests if the first fix
is not at the beginning of the match.

[gribozavr]

clang-tools-extra/unittests/clang-tidy/TransformerClangTidyCheckTest.cpp

106

Sorry, I don't quite understand the comment -- the test has two replacements (both arguments to the macro are replaced), but the comment says that we only expect one.

[ymandel]

fixed comment.

[ymandel]

clang-tools-extra/unittests/clang-tidy/TransformerClangTidyCheckTest.cpp

106

Right, that comment needs to be updated. It's leftover from while the fix wasn't done yet...

[gribozavr]

I see, it all makes sense now.

clang-tools-extra/clang-tidy/utils/TransformerClangTidyCheck.cpp

for (auto &Matcher : tooling::detail::buildMatchers(*Rule))
       Finder->addDynamicMatcher(Matcher, this);
 }
 
 void TransformerClangTidyCheck::check(
     const ast_matchers::MatchFinder::MatchResult &Result) {
   if (Result.Context->getDiagnostics().hasErrorOccurred())
     return;
 
-  // Verify the existence and validity of the AST node that roots this rule.
-  const ast_matchers::BoundNodes::IDToNodeMap &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.");
-
   assert(Rule && "check() should not fire if Rule is None");
   RewriteRule::Case Case = tooling::detail::findSelectedCase(Result, *Rule);
   Expected<SmallVector<tooling::detail::Transformation, 1>> Transformations =
       tooling::detail::translateEdits(Result, Case.Edits);
   if (!Transformations) {
     llvm::errs() << "Rewrite failed: "
                  << llvm::toString(Transformations.takeError()) << "\n";
     return;
   }
 
   // No rewrite applied, but no error encountered either.
   if (Transformations->empty())
     return;
 
   Expected<std::string> Explanation = Case.Explanation(Result);
   if (!Explanation) {
     llvm::errs() << "Error in explanation: "
                  << llvm::toString(Explanation.takeError()) << "\n";
     return;
   }
-  DiagnosticBuilder Diag = diag(RootLoc, *Explanation);
-  for (const auto &T : *Transformations) {
+
+  // Associate the diagnostic with the location of the first change.
+  DiagnosticBuilder Diag =
+      diag((*Transformations)[0].Range.getBegin(), *Explanation);
+  for (const auto &T : *Transformations)
     Diag << FixItHint::CreateReplacement(T.Range, T.Replacement);
-  }
 
   for (const auto &I : Case.AddedIncludes) {
     auto &Header = I.first;
     if (Optional<FixItHint> Fix = Inserter->CreateIncludeInsertion(
             Result.SourceManager->getMainFileID(), Header,
             /*IsAngled=*/I.second == tooling::IncludeFormat::Angled)) {
       Diag << *Fix;
     }
   }
 }
 
 } // namespace utils
 } // namespace tidy
 } // namespace clang

clang-tools-extra/unittests/clang-tidy/TransformerClangTidyCheckTest.cpp

 #include "clang/Tooling/Refactoring/Stencil.h"
 #include "clang/Tooling/Refactoring/Transformer.h"
 #include "gtest/gtest.h"
 
 namespace clang {
 namespace tidy {
 namespace utils {
 namespace {
+using namespace ::clang::ast_matchers;
+
 using tooling::change;
 using tooling::IncludeFormat;
+using tooling::node;
 using tooling::RewriteRule;
+using tooling::statement;
 using tooling::text;
 using tooling::stencil::cat;
 
 // Invert the code of an if-statement, while maintaining its semantics.
 RewriteRule invertIf() {
-  using namespace ::clang::ast_matchers;
-  using tooling::node;
-  using tooling::statement;
-
   StringRef C = "C", T = "T", E = "E";
   RewriteRule Rule = tooling::makeRule(
       ifStmt(hasCondition(expr().bind(C)), hasThen(stmt().bind(T)),
              hasElse(stmt().bind(E))),
       change(
           statement(RewriteRule::RootID),
           cat("if(!(", node(C), ")) ", statement(E), " else ", statement(T))),
       text("negate condition and reverse `then` and `else` branches"));
const std::string Expected = R"(
     void log(const char* msg);
     void foo() {
       if(!(10 > 1.0)) log("ok"); else log("oh no!");
     }
   )";
   EXPECT_EQ(Expected, test::runCheckOnCode<IfInverterCheck>(Input));
 }
 
+class IntLitCheck : public TransformerClangTidyCheck {
+public:
+  IntLitCheck(StringRef Name, ClangTidyContext *Context)
+      : TransformerClangTidyCheck(tooling::makeRule(integerLiteral(),
+                                                    change(text("LIT")),
+                                                    text("no message")),
+                                  Name, Context) {}
+};
+
+// Tests that two changes in a single macro expansion do not lead to conflicts
+// in applying the changes.
+TEST(TransformerClangTidyCheckTest, TwoChangesInOneMacroExpansion) {
+  const std::string Input = R"cc(
+#define PLUS(a,b) (a) + (b)
+    int f() { return PLUS(3, 4); }
+  )cc";
+  const std::string Expected = R"cc(
+#define PLUS(a,b) (a) + (b)
+    int f() { return PLUS(LIT, LIT); }
+  )cc";
+
+  EXPECT_EQ(Expected, test::runCheckOnCode<IntLitCheck>(Input));
+}
+
+class BinOpCheck : public TransformerClangTidyCheck {
+public:
+  BinOpCheck(StringRef Name, ClangTidyContext *Context)
+      : TransformerClangTidyCheck(
+            tooling::makeRule(
+                binaryOperator(hasOperatorName("+"), hasRHS(expr().bind("r"))),
+                change(node("r"), text("RIGHT")), text("no message")),
+            Name, Context) {}
+};
+
+// Tests case where the rule's match spans both source from the macro and its
+// argument, while the change spans only the argument AND there are two such
+// matches.  Here, we expect a conflict between the two matches and the second
+// to be ignored.
+TEST(TransformerClangTidyCheckTest, TwoMatchesInMacroExpansion) {
+  const std::string Input = R"cc(
+#define M(a,b) (1 + a) * (1 + b)
+    int f() { return M(3, 4); }
+  )cc";
+  const std::string Expected = R"cc(
+#define M(a,b) (1 + a) * (1 + b)
+    int f() { return M(RIGHT, RIGHT); }
+  )cc";
+
+  EXPECT_EQ(Expected, test::runCheckOnCode<BinOpCheck>(Input));
+}
+
 // A trivial rewrite-rule generator that requires Objective-C code.
 Optional<RewriteRule> needsObjC(const LangOptions &LangOpts,
                                 const ClangTidyCheck::OptionsView &Options) {
   if (!LangOpts.ObjC)
     return None;
   return tooling::makeRule(clang::ast_matchers::functionDecl(),
                            change(cat("void changed() {}")), text("no message"));
 }