[clang] ce5780b - [libTooling] Fix Transformer to work with ambient traversal kinds.
Yitzhak Mandelbaum via cfe-commits
cfe-commits at lists.llvm.org
Thu May 28 08:42:48 PDT 2020
Author: Yitzhak Mandelbaum
Date: 2020-05-28T11:42:07-04:00
New Revision: ce5780b88c6e2f3303afd266e5e29c1badd9eb3b
URL: https://github.com/llvm/llvm-project/commit/ce5780b88c6e2f3303afd266e5e29c1badd9eb3b
DIFF: https://github.com/llvm/llvm-project/commit/ce5780b88c6e2f3303afd266e5e29c1badd9eb3b.diff
LOG: [libTooling] Fix Transformer to work with ambient traversal kinds.
Summary:
`RewriteRule`'s `applyFirst` was brittle with respect to the default setting of the
`TraversalKind`. This patch builds awareness of traversal kinds directly into
rewrite rules so that they are insensitive to any changes in defaults.
Reviewers: steveire, gribozavr
Subscribers: hokein, cfe-commits
Tags: #clang
Differential Revision: https://reviews.llvm.org/D80606
Added:
Modified:
clang/include/clang/Tooling/Transformer/RewriteRule.h
clang/lib/Tooling/Transformer/RewriteRule.cpp
clang/unittests/Tooling/TransformerTest.cpp
Removed:
################################################################################
diff --git a/clang/include/clang/Tooling/Transformer/RewriteRule.h b/clang/include/clang/Tooling/Transformer/RewriteRule.h
index 4a5e5556cdff..0a961ccc475d 100644
--- a/clang/include/clang/Tooling/Transformer/RewriteRule.h
+++ b/clang/include/clang/Tooling/Transformer/RewriteRule.h
@@ -273,11 +273,13 @@ namespace detail {
/// supports mixing matchers of
diff erent 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);
diff --git a/clang/lib/Tooling/Transformer/RewriteRule.cpp b/clang/lib/Tooling/Transformer/RewriteRule.cpp
index 968f7fa6cd32..ddce6ce59185 100644
--- a/clang/lib/Tooling/Transformer/RewriteRule.cpp
+++ b/clang/lib/Tooling/Transformer/RewriteRule.cpp
@@ -116,10 +116,13 @@ static bool hasValidKind(const DynTypedMatcher &M) {
#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) {
@@ -127,8 +130,10 @@ static std::vector<DynTypedMatcher> taggedMatchers(
// 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 @@ transformer::detail::buildMatchers(const RewriteRule &Rule) {
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;
}
diff --git a/clang/unittests/Tooling/TransformerTest.cpp b/clang/unittests/Tooling/TransformerTest.cpp
index c8c6db059fed..d19f747a69b5 100644
--- a/clang/unittests/Tooling/TransformerTest.cpp
+++ b/clang/unittests/Tooling/TransformerTest.cpp
@@ -571,6 +571,59 @@ TEST_F(TransformerTest, OrderedRuleMultipleKinds) {
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).
//
More information about the cfe-commits
mailing list