diff --git a/clang/include/clang/Tooling/Transformer/RewriteRule.h b/clang/include/clang/Tooling/Transformer/RewriteRule.h --- a/clang/include/clang/Tooling/Transformer/RewriteRule.h +++ b/clang/include/clang/Tooling/Transformer/RewriteRule.h @@ -273,11 +273,13 @@ /// 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 buildMatchers(const RewriteRule &Rule); diff --git a/clang/lib/Tooling/Transformer/RewriteRule.cpp b/clang/lib/Tooling/Transformer/RewriteRule.cpp --- a/clang/lib/Tooling/Transformer/RewriteRule.cpp +++ b/clang/lib/Tooling/Transformer/RewriteRule.cpp @@ -116,10 +116,13 @@ #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 taggedMatchers( StringRef TagBase, - const SmallVectorImpl> &Cases) { + const SmallVectorImpl> &Cases, + ast_type_traits::TraversalKind DefaultTraversalKind) { std::vector Matchers; Matchers.reserve(Cases.size()); for (const auto &Case : Cases) { @@ -127,8 +130,10 @@ // 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; } @@ -158,14 +163,21 @@ 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 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; } 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 @@ -571,6 +571,59 @@ 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). //