[libTooling] Fix Transformer to work with ambient traversal kinds

[ymandel]

Summary

RewriteRule's applyFirst was brittle with respect to the default setting of the TraversalKind (revealed by D72534). This patch builds awareness of traversal kinds directly into rewrite rules so that they are insensitive to any changes in defaults.

[steveire]

It might make sense for now (in order to unblock you) to make the Transformer library explicitly require the AsIs mode. I am not so familiar with the transformer library, but I think you can do that by adding traverse(AsIs, ...) in Transformer::registerMatchers.

The real fix is probably in adjusting transformer::detail::buildMatchers. I can look into that in a few hours.

[ymandel]

In D80606#2057136, @steveire wrote:

It might make sense for now (in order to unblock you) to make the Transformer library explicitly require the AsIs mode. I am not so familiar with the transformer library, but I think you can do that by adding traverse(AsIs, ...) in Transformer::registerMatchers.

The real fix is probably in adjusting transformer::detail::buildMatchers. I can look into that in a few hours.

Thanks for the suggestion - I agree that changing registerMatchers would be a short term fix, but I think its better to just fix buildMatchers directly. I'm working on a patch now, so need to look into it (though thanks for the offer!)

[ymandel]

adds a fix to transformer

[gribozavr2]

clang/lib/Tooling/Transformer/RewriteRule.cpp

154 ↗

(On Diff #266729)

I don't see any callers using the new argument. Do we need this flexibility?

clang/unittests/Tooling/TransformerTest.cpp

574

a matcher => a top-level matcher

575

I'd say "when the AST traversal skips implicit nodes".

576

ti => to

AsIs) => AsIs

[ymandel]

updated per comments and after rebasing

[ymandel]

typo fix

[ymandel]

more typo fixes

[ymandel]

clang/lib/Tooling/Transformer/RewriteRule.cpp

154 ↗

(On Diff #266729)

No. Abortive effort to support pulling it from the context. However, turned out the context is not available when this is called. Moreover, I've decided it's the wrong UX altogether. Matchers are not polymorphic (usually) -- they only make sense with respect to a single TK, whether explicit or implicit. So, my goal is that RewriteRules will specify how "open" matchers are interpreted -- that is, the matchers TK will be set at rule creation time rather than at rule execution time (by pulling from the context). I don't achieve that goal in this revision because I'm only updating applyFirst, but I should follow up with changes makeRule and the corresponding docs to make this clear.`

That said, I chose TK_IgnoreUnlessSpelledInSource as the default setting for "open" matchers to be consistent with the consensus that its worth trying this out as a default.

clang/unittests/Tooling/TransformerTest.cpp

TEST_F(TransformerTest, OrderedRuleMultipleKinds) {
   RewriteRule DeclRule = makeRule(functionDecl(hasName("f2")).bind("fun"),
                                   changeTo(name("fun"), cat("DECL_RULE")));
 
   RewriteRule Rule = applyFirst({ReplaceF1, DeclRule, ReplaceF1OrF2});
   EXPECT_EQ(transformer::detail::buildMatchers(Rule).size(), 2UL);
   testRule(Rule, Input, Expected);
 }
 
+// Demonstrates bug in applyFirst caused by
+// https://reviews.llvm.org/D72534. This passes before and fails after.
+TEST_F(TransformerTest, OrderedRuleUnrelatedIgnored) {
+  std::string Input = R"cc(
+    void f1();
+    int f2();
+    void call_f1() { f1(); }
+    float call_f2() { return f2(); }
+  )cc";
+  std::string Expected = R"cc(
+    void f1();
+    int f2();
+    void call_f1() { REPLACE_F1; }
+    float call_f2() { return REPLACE_F2; }
+  )cc";
+
+  RewriteRule ReplaceF1 =
+      makeRule(callExpr(callee(functionDecl(hasName("f1")))),
+               changeTo(cat("REPLACE_F1")));
+  RewriteRule ReplaceF2 = makeRule(castExpr(hasSourceExpression(callExpr())),
+                                   changeTo(cat("REPLACE_F2")));
+  testRule(applyFirst({ReplaceF1, ReplaceF2}), Input, Expected);
+}
+
+// BAD FIX: This attempt by the API client to fix the problem fails.
+TEST_F(TransformerTest, OrderedRuleUnrelatedIgnoredBadFix) {
+  std::string Input = R"cc(
+    void f1();
+    int f2();
+    void call_f1() { f1(); }
+    float call_f2() { return f2(); }
+  )cc";
+  std::string Expected = R"cc(
+    void f1();
+    int f2();
+    void call_f1() { REPLACE_F1; }
+    float call_f2() { return REPLACE_F2; }
+  )cc";
+
+  RewriteRule ReplaceF1 =
+      makeRule(callExpr(callee(functionDecl(hasName("f1")))),
+               changeTo(cat("REPLACE_F1")));
+  RewriteRule ReplaceF2 = makeRule(
+      traverse(clang::TK_AsIs, castExpr(hasSourceExpression(callExpr()))),
+      changeTo(cat("REPLACE_F2")));
+  testRule(applyFirst({ReplaceF1, ReplaceF2}), Input, Expected);
+}
+
 //
 // Negative tests (where we expect no transformation to occur).
 //
 
 // Tests for a conflict in edits from a single match for a rule.
 TEST_F(TransformerTest, TextGeneratorFailure) {
   std::string Input = "int conflictOneRule() { return 3 + 7; }";
   // Try to change the whole binary-operator expression AND one its operands: