diff --git a/clang/include/clang/Tooling/Refactoring/Transformer.h b/clang/include/clang/Tooling/Refactoring/Transformer.h --- a/clang/include/clang/Tooling/Refactoring/Transformer.h +++ b/clang/include/clang/Tooling/Refactoring/Transformer.h @@ -160,11 +160,9 @@ void addInclude(RewriteRule &Rule, llvm::StringRef Header, IncludeFormat Format = IncludeFormat::Quoted); -/// Applies the first rule whose pattern matches; other rules are ignored. -/// -/// N.B. All of the rules must use the same kind of matcher (that is, share a -/// base class in the AST hierarchy). However, this constraint is caused by an -/// implementation detail and should be lifted in the future. +/// Applies the first rule whose pattern matches; other rules are ignored. If +/// the matchers are independent then order doesn't matter. In that case, +/// `applyFirst` is simply joining the set of rules into one. // // `applyFirst` is like an `anyOf` matcher with an edit action attached to each // of its cases. Anywhere you'd use `anyOf(m1.bind("id1"), m2.bind("id2"))` and @@ -243,8 +241,19 @@ // 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. +std::vector +buildMatchers(const RewriteRule &Rule); + /// Returns the \c Case of \c Rule that was selected in the match result. /// Assumes a matcher built with \c buildMatcher. const RewriteRule::Case & diff --git a/clang/lib/Tooling/Refactoring/Transformer.cpp b/clang/lib/Tooling/Refactoring/Transformer.cpp --- a/clang/lib/Tooling/Refactoring/Transformer.cpp +++ b/clang/lib/Tooling/Refactoring/Transformer.cpp @@ -19,10 +19,10 @@ #include "llvm/ADT/StringRef.h" #include "llvm/Support/Errc.h" #include "llvm/Support/Error.h" -#include #include #include #include +#include using namespace clang; using namespace tooling; @@ -80,52 +80,23 @@ Case.AddedIncludes.emplace_back(Header.str(), Format); } -// Determines whether A is a base type of B in the class hierarchy, including -// the implicit relationship of Type and QualType. -static bool isBaseOf(ASTNodeKind A, ASTNodeKind B) { - static auto TypeKind = ASTNodeKind::getFromNodeKind(); - static auto QualKind = ASTNodeKind::getFromNodeKind(); - /// Mimic the implicit conversions of Matcher<>. - /// - From Matcher to Matcher - /// - From Matcher to Matcher - return (A.isSame(TypeKind) && B.isSame(QualKind)) || A.isBaseOf(B); -} - -// Try to find a common kind to which all of the rule's matchers can be -// converted. -static ASTNodeKind -findCommonKind(const SmallVectorImpl &Cases) { - assert(!Cases.empty() && "Rule must have at least one case."); - ASTNodeKind JoinKind = Cases[0].Matcher.getSupportedKind(); - // Find a (least) Kind K, for which M.canConvertTo(K) holds, for all matchers - // M in Rules. - for (const auto &Case : Cases) { - auto K = Case.Matcher.getSupportedKind(); - if (isBaseOf(JoinKind, K)) { - JoinKind = K; - continue; - } - if (K.isSame(JoinKind) || isBaseOf(K, JoinKind)) - // JoinKind is already the lowest. - continue; - // K and JoinKind are unrelated -- there is no least common kind. - return ASTNodeKind(); - } - return JoinKind; +// Filters for supported matcher kinds. FIXME: Explicitly list the allowed kinds +// (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(); } // Binds each rule's matcher to a unique (and deterministic) tag based on -// `TagBase`. -static std::vector -taggedMatchers(StringRef TagBase, - const SmallVectorImpl &Cases) { +// `TagBase` and the id paired with the case. +static std::vector taggedMatchers( + StringRef TagBase, + const SmallVectorImpl> &Cases) { std::vector Matchers; Matchers.reserve(Cases.size()); - size_t count = 0; for (const auto &Case : Cases) { - std::string Tag = (TagBase + Twine(count)).str(); - ++count; - auto M = Case.Matcher.tryBind(Tag); + std::string Tag = (TagBase + Twine(Case.first)).str(); + auto M = Case.second.Matcher.tryBind(Tag); assert(M && "RewriteRule matchers should be bindable."); Matchers.push_back(*std::move(M)); } @@ -142,22 +113,37 @@ return R; } -static DynTypedMatcher joinCaseMatchers(const RewriteRule &Rule) { - assert(!Rule.Cases.empty() && "Rule must have at least one case."); - if (Rule.Cases.size() == 1) - return Rule.Cases[0].Matcher; +std::vector +tooling::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, 1>> + Buckets; + const SmallVectorImpl &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]); + } - auto CommonKind = findCommonKind(Rule.Cases); - assert(!CommonKind.isNone() && "Cases must have compatible matchers."); - return DynTypedMatcher::constructVariadic( - DynTypedMatcher::VO_AnyOf, CommonKind, taggedMatchers("Tag", Rule.Cases)); + std::vector Matchers; + for (const auto &Bucket : Buckets) { + DynTypedMatcher M = DynTypedMatcher::constructVariadic( + DynTypedMatcher::VO_AnyOf, Bucket.first, + taggedMatchers("Tag", Bucket.second)); + M.setAllowBind(true); + // `tryBind` is guaranteed to succeed, because `AllowBind` was set to true. + Matchers.push_back(*M.tryBind(RewriteRule::RootID)); + } + return Matchers; } DynTypedMatcher tooling::detail::buildMatcher(const RewriteRule &Rule) { - DynTypedMatcher M = joinCaseMatchers(Rule); - M.setAllowBind(true); - // `tryBind` is guaranteed to succeed, because `AllowBind` was set to true. - return *M.tryBind(RewriteRule::RootID); + std::vector Ms = buildMatchers(Rule); + assert(Ms.size() == 1 && "Cases must have compatible matchers."); + return Ms[0]; } // Finds the case that was "selected" -- that is, whose matcher triggered the @@ -180,7 +166,8 @@ constexpr llvm::StringLiteral RewriteRule::RootID; void Transformer::registerMatchers(MatchFinder *MatchFinder) { - MatchFinder->addDynamicMatcher(tooling::detail::buildMatcher(Rule), this); + for (auto &Matcher : tooling::detail::buildMatchers(Rule)) + MatchFinder->addDynamicMatcher(Matcher, this); } void Transformer::run(const MatchResult &Result) { @@ -222,12 +209,12 @@ for (const auto &I : Case.AddedIncludes) { auto &Header = I.first; switch (I.second) { - case IncludeFormat::Quoted: - AC.addHeader(Header); - break; - case IncludeFormat::Angled: - AC.addHeader((llvm::Twine("<") + Header + ">").str()); - break; + case IncludeFormat::Quoted: + AC.addHeader(Header); + break; + case IncludeFormat::Angled: + AC.addHeader((llvm::Twine("<") + Header + ">").str()); + break; } } 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 @@ -482,65 +482,85 @@ testRule(applyFirst({ruleStrlenSize(), FlagRule}), Input, Expected); } -// Version of ruleStrlenSizeAny that inserts a method with a different name than -// ruleStrlenSize, so we can tell their effect apart. -RewriteRule ruleStrlenSizeDistinct() { - StringRef S; - return makeRule( - callExpr(callee(functionDecl(hasName("strlen"))), - hasArgument(0, cxxMemberCallExpr( - on(expr().bind(S)), - callee(cxxMethodDecl(hasName("c_str")))))), - change(text("DISTINCT"))); -} - TEST_F(TransformerTest, OrderedRuleRelated) { std::string Input = R"cc( - namespace foo { - struct mystring { - char* c_str(); - }; - int f(mystring s) { return strlen(s.c_str()); } - } // namespace foo - int g(string s) { return strlen(s.c_str()); } + void f1(); + void f2(); + void call_f1() { f1(); } + void call_f2() { f2(); } )cc"; std::string Expected = R"cc( - namespace foo { - struct mystring { - char* c_str(); - }; - int f(mystring s) { return DISTINCT; } - } // namespace foo - int g(string s) { return REPLACED; } + void f1(); + void f2(); + void call_f1() { REPLACE_F1; } + void call_f2() { REPLACE_F1_OR_F2; } )cc"; - testRule(applyFirst({ruleStrlenSize(), ruleStrlenSizeDistinct()}), Input, - Expected); + RewriteRule ReplaceF1 = + makeRule(callExpr(callee(functionDecl(hasName("f1")))), + change(text("REPLACE_F1"))); + RewriteRule ReplaceF1OrF2 = + makeRule(callExpr(callee(functionDecl(hasAnyName("f1", "f2")))), + change(text("REPLACE_F1_OR_F2"))); + testRule(applyFirst({ReplaceF1, ReplaceF1OrF2}), Input, Expected); } -// Change the order of the rules to get a different result. +// Change the order of the rules to get a different result. When `ReplaceF1OrF2` +// comes first, it applies for both uses, so `ReplaceF1` never applies. TEST_F(TransformerTest, OrderedRuleRelatedSwapped) { std::string Input = R"cc( - namespace foo { - struct mystring { - char* c_str(); - }; - int f(mystring s) { return strlen(s.c_str()); } - } // namespace foo - int g(string s) { return strlen(s.c_str()); } + void f1(); + void f2(); + void call_f1() { f1(); } + void call_f2() { f2(); } )cc"; std::string Expected = R"cc( - namespace foo { - struct mystring { - char* c_str(); - }; - int f(mystring s) { return DISTINCT; } - } // namespace foo - int g(string s) { return DISTINCT; } + void f1(); + void f2(); + void call_f1() { REPLACE_F1_OR_F2; } + void call_f2() { REPLACE_F1_OR_F2; } + )cc"; + + RewriteRule ReplaceF1 = + makeRule(callExpr(callee(functionDecl(hasName("f1")))), + change(text("REPLACE_F1"))); + RewriteRule ReplaceF1OrF2 = + makeRule(callExpr(callee(functionDecl(hasAnyName("f1", "f2")))), + change(text("REPLACE_F1_OR_F2"))); + testRule(applyFirst({ReplaceF1OrF2, ReplaceF1}), Input, Expected); +} + +// Verify that a set of rules whose matchers have different base kinds works +// properly, including that `applyFirst` produces multiple matchers. We test +// two different kinds of rules: Expr and Decl. We place the Decl rule in the +// middle to test that `buildMatchers` works even when the kinds aren't grouped +// together. +TEST_F(TransformerTest, OrderedRuleMultipleKinds) { + std::string Input = R"cc( + void f1(); + void f2(); + void call_f1() { f1(); } + void call_f2() { f2(); } )cc"; - - testRule(applyFirst({ruleStrlenSizeDistinct(), ruleStrlenSize()}), Input, - Expected); + std::string Expected = R"cc( + void f1(); + void DECL_RULE(); + void call_f1() { REPLACE_F1; } + void call_f2() { REPLACE_F1_OR_F2; } + )cc"; + + RewriteRule ReplaceF1 = + makeRule(callExpr(callee(functionDecl(hasName("f1")))), + change(text("REPLACE_F1"))); + RewriteRule ReplaceF1OrF2 = + makeRule(callExpr(callee(functionDecl(hasAnyName("f1", "f2")))), + change(text("REPLACE_F1_OR_F2"))); + RewriteRule DeclRule = makeRule(functionDecl(hasName("f2")).bind("fun"), + change(name("fun"), text("DECL_RULE"))); + + RewriteRule Rule = applyFirst({ReplaceF1, DeclRule, ReplaceF1OrF2}); + EXPECT_EQ(tooling::detail::buildMatchers(Rule).size(), 2UL); + testRule(Rule, Input, Expected); } // @@ -718,4 +738,18 @@ testRule(ruleStrlenSize(), Input, Input); } + +#if !defined(NDEBUG) && GTEST_HAS_DEATH_TEST +// Verifies that `Type` and `QualType` are not allowed as top-level matchers in +// rules. +TEST(TransformerDeathTest, OrderedRuleTypes) { + RewriteRule QualTypeRule = makeRule(qualType(), change(text("Q"))); + EXPECT_DEATH(tooling::detail::buildMatchers(QualTypeRule), + "Matcher must be.*node matcher"); + + RewriteRule TypeRule = makeRule(arrayType(), change(text("T"))); + EXPECT_DEATH(tooling::detail::buildMatchers(TypeRule), + "Matcher must be.*node matcher"); +} +#endif } // namespace