[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

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

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/include/clang/Tooling/Transformer/RewriteRule.h

 // public and well-supported and move them out of `detail`.
 namespace detail {
 /// Builds a single matcher for the rule, covering all of the rule's cases.
 /// Only supports Rules whose cases' matchers share the same base "kind"
 /// (`Stmt`, `Decl`, etc.)  Deprecated: use `buildMatchers` instead, which
 /// supports mixing matchers of different kinds.
 ast_matchers::internal::DynTypedMatcher buildMatcher(const RewriteRule &Rule);
 
-/// 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.
+/// Builds a set of matchers that cover the rule.
+///
+/// One matcher is built 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);

clang/lib/Tooling/Transformer/RewriteRule.cpp

 // (all node matcher types except for `QualType` and `Type`), rather than just
 // banning `QualType` and `Type`.
 static bool hasValidKind(const DynTypedMatcher &M) {
   return !M.canConvertTo<QualType>();
 }
 #endif
 
 // Binds each rule's matcher to a unique (and deterministic) tag based on
-// `TagBase` and the id paired with the case.
+// `TagBase` and the id paired with the case. All of the returned matchers have
+// their traversal kind explicitly set, either based on a pre-set kind or to the
+// provided `DefaultTraversalKind`.
 static std::vector<DynTypedMatcher> taggedMatchers(
     StringRef TagBase,
-    const SmallVectorImpl<std::pair<size_t, RewriteRule::Case>> &Cases) {
+    const SmallVectorImpl<std::pair<size_t, RewriteRule::Case>> &Cases,
+    ast_type_traits::TraversalKind DefaultTraversalKind) {
   std::vector<DynTypedMatcher> Matchers;
   Matchers.reserve(Cases.size());
   for (const auto &Case : Cases) {
     std::string Tag = (TagBase + Twine(Case.first)).str();
     // HACK: Many matchers are not bindable, so ensure that tryBind will work.
     DynTypedMatcher BoundMatcher(Case.second.Matcher);
     BoundMatcher.setAllowBind(true);
-    auto M = BoundMatcher.tryBind(Tag);
-    Matchers.push_back(*std::move(M));
+    auto M = *BoundMatcher.tryBind(Tag);
+    Matchers.push_back(!M.getTraversalKind()
+                           ? M.withTraversalKind(DefaultTraversalKind)
+                           : std::move(M));
   }
   return Matchers;
 }
 
 // Simply gathers the contents of the various rules into a single rule. The
 // actual work to combine these into an ordered choice is deferred to matcher
 // registration.
 RewriteRule transformer::applyFirst(ArrayRef<RewriteRule> Rules) {
   RewriteRule R;
   for (auto &Rule : Rules)
     R.Cases.append(Rule.Cases.begin(), Rule.Cases.end());
   return R;
 }
 
 std::vector<DynTypedMatcher>
 transformer::detail::buildMatchers(const RewriteRule &Rule) {
   // Map the cases into buckets of matchers -- one for each "root" AST kind,
   // which guarantees that they can be combined in a single anyOf matcher. Each
   // case is paired with an identifying number that is converted to a string id
   // in `taggedMatchers`.
   std::map<ASTNodeKind, SmallVector<std::pair<size_t, RewriteRule::Case>, 1>>
       Buckets;
   const SmallVectorImpl<RewriteRule::Case> &Cases = Rule.Cases;
   for (int I = 0, N = Cases.size(); I < N; ++I) {
     assert(hasValidKind(Cases[I].Matcher) &&
            "Matcher must be non-(Qual)Type node matcher");
     Buckets[Cases[I].Matcher.getSupportedKind()].emplace_back(I, Cases[I]);
   }
 
+  // Each anyOf explicitly controls the traversal kind. The anyOf itself is set
+  // to `TK_AsIs` to ensure no nodes are skipped, thereby deferring to the kind
+  // of the branches. Then, each branch is either left as is, if the kind is
+  // already set, or explicitly set to `TK_IgnoreUnlessSpelledInSource`. We
+  // choose this setting, because we think it is the one most friendly to
+  // beginners, who are (largely) the target audience of Transformer.
   std::vector<DynTypedMatcher> Matchers;
   for (const auto &Bucket : Buckets) {
     DynTypedMatcher M = DynTypedMatcher::constructVariadic(
         DynTypedMatcher::VO_AnyOf, Bucket.first,
-        taggedMatchers("Tag", Bucket.second));
+        taggedMatchers("Tag", Bucket.second, TK_IgnoreUnlessSpelledInSource));
     M.setAllowBind(true);
     // `tryBind` is guaranteed to succeed, because `AllowBind` was set to true.
-    Matchers.push_back(*M.tryBind(RewriteRule::RootID));
+    Matchers.push_back(
+        M.tryBind(RewriteRule::RootID)->withTraversalKind(TK_AsIs));
   }
   return Matchers;
 }
 
 DynTypedMatcher transformer::detail::buildMatcher(const RewriteRule &Rule) {
   std::vector<DynTypedMatcher> Ms = buildMatchers(Rule);
   assert(Ms.size() == 1 && "Cases must have compatible matchers.");
   return Ms[0];

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);
 }
 
+// Verifies that a rule with a top-level matcher for an implicit node (like
+// `implicitCastExpr`) does not change the code, when the AST traversal skips
+// implicit nodes. In this test, only the rule with the explicit-node matcher
+// will fire.
+TEST_F(TransformerTest, OrderedRuleImplicitIgnored) {
+  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 f2(); }
+  )cc";
+
+  RewriteRule ReplaceF1 =
+      makeRule(callExpr(callee(functionDecl(hasName("f1")))),
+               changeTo(cat("REPLACE_F1")));
+  RewriteRule ReplaceF2 =
+      makeRule(implicitCastExpr(hasSourceExpression(callExpr())),
+               changeTo(cat("REPLACE_F2")));
+  testRule(applyFirst({ReplaceF1, ReplaceF2}), Input, Expected);
+}
+
+// Verifies that explicitly setting the traversal kind fixes the problem in the
+// previous test.
+TEST_F(TransformerTest, OrderedRuleImplicitMatched) {
+  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(
+      traverse(clang::TK_AsIs, callExpr(callee(functionDecl(hasName("f1"))))),
+      changeTo(cat("REPLACE_F1")));
+  RewriteRule ReplaceF2 =
+      makeRule(traverse(clang::TK_AsIs,
+                        implicitCastExpr(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: