ymandel created this revision. ymandel added a reviewer: gribozavr. Herald added a project: clang.
This patch removes an (artificial) limitation of `applyFirst`, which requires that all of the rules' matchers can be grouped together in a single `anyOf()`. This change generalizes the code to group the matchers into separate `anyOf`s based on compatibility. Correspondingly, `buildMatcher` is changed to `buildMatchers`, to allow for returning a set of matchers rather than just one. Repository: rG LLVM Github Monorepo https://reviews.llvm.org/D65877 Files: clang/include/clang/Tooling/Refactoring/Transformer.h clang/lib/Tooling/Refactoring/Transformer.cpp clang/unittests/Tooling/TransformerTest.cpp
Index: clang/unittests/Tooling/TransformerTest.cpp =================================================================== --- clang/unittests/Tooling/TransformerTest.cpp +++ clang/unittests/Tooling/TransformerTest.cpp @@ -482,16 +482,17 @@ testRule(applyFirst({ruleStrlenSize(), FlagRule}), Input, Expected); } -// Version of ruleStrlenSizeAny that inserts a method with a different name than +// Version of ruleStrlenSize that doesn't check the type of the targeted +// object. Note that we insert a method with a different name than // ruleStrlenSize, so we can tell their effect apart. -RewriteRule ruleStrlenSizeDistinct() { +RewriteRule ruleStrlenSizeAny() { StringRef S; return makeRule( callExpr(callee(functionDecl(hasName("strlen"))), hasArgument(0, cxxMemberCallExpr( on(expr().bind(S)), callee(cxxMethodDecl(hasName("c_str")))))), - change(text("DISTINCT"))); + change(text("ANY"))); } TEST_F(TransformerTest, OrderedRuleRelated) { @@ -509,16 +510,18 @@ struct mystring { char* c_str(); }; - int f(mystring s) { return DISTINCT; } + int f(mystring s) { return ANY; } } // namespace foo int g(string s) { return REPLACED; } )cc"; - testRule(applyFirst({ruleStrlenSize(), ruleStrlenSizeDistinct()}), Input, + testRule(applyFirst({ruleStrlenSize(), ruleStrlenSizeAny()}), 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 +// `ruleStrlenSizeAny` comes first, it applies for both uses, so +// `ruleStrlenSize` never applies. TEST_F(TransformerTest, OrderedRuleRelatedSwapped) { std::string Input = R"cc( namespace foo { @@ -534,15 +537,51 @@ struct mystring { char* c_str(); }; - int f(mystring s) { return DISTINCT; } + int f(mystring s) { return ANY; } } // namespace foo - int g(string s) { return DISTINCT; } + int g(string s) { return ANY; } )cc"; - testRule(applyFirst({ruleStrlenSizeDistinct(), ruleStrlenSize()}), Input, + testRule(applyFirst({ruleStrlenSizeAny(), ruleStrlenSize()}), Input, Expected); } +// Verify that a set of rules whose matchers have different base kinds works +// properly, including that `applyFirst` produces multiple matchers. +TEST_F(TransformerTest, OrderedRuleMultipleKinds) { + RewriteRule DeclRule = makeRule(functionDecl(hasName("bad")).bind("fun"), + change(name("fun"), text("good"))); + // 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. + RewriteRule Rule = + applyFirst({ruleStrlenSize(), DeclRule, ruleStrlenSizeAny()}); + + 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()); } + int bad(int x); + )cc"; + std::string Expected = R"cc( + namespace foo { + struct mystring { + char* c_str(); + }; + int f(mystring s) { return ANY; } + } // namespace foo + int g(string s) { return REPLACED; } + int good(int x); + )cc"; + + EXPECT_EQ(tooling::detail::buildMatchers(Rule).size(), 2UL); + testRule(Rule, Input, Expected); +} + // // Negative tests (where we expect no transformation to occur). // Index: clang/lib/Tooling/Refactoring/Transformer.cpp =================================================================== --- clang/lib/Tooling/Refactoring/Transformer.cpp +++ clang/lib/Tooling/Refactoring/Transformer.cpp @@ -91,41 +91,53 @@ 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<RewriteRule::Case> &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; +namespace { +struct KindBucket { + // Common kind which all of the bucket's matchers can support. Specifically, a + // (least) Kind K, for which M.canConvertTo(K) holds, for all matchers M in + // `Cases`. + ASTNodeKind Kind; + // Each case is paired with an id that is used to bind it in the matcher. + SmallVector<std::pair<size_t, RewriteRule::Case>, 1> Cases; +}; +} // namespace + +// Finds a bucket that is compatible with the given case and adds the case to +// that bucket; otherwise, creates a new bucket for the case. Compatibility +// means that the type of the case's matcher is a sub/super class of all other +// cases in that bucket. +static void insertCase(std::vector<KindBucket> &Buckets, size_t CaseId, + RewriteRule::Case Case) { + ASTNodeKind CaseKind = Case.Matcher.getSupportedKind(); + for (int I = Buckets.size() - 1; I >= 0; --I) { + KindBucket &Bucket = Buckets[I]; + if (isBaseOf(Bucket.Kind, CaseKind)) { + // Case belongs in this bucket and bucket's new kind is `CaseKind`. + Bucket.Kind = CaseKind; + Bucket.Cases.emplace_back(CaseId, std::move(Case)); + return; + } + if (CaseKind.isSame(Bucket.Kind) || isBaseOf(CaseKind, Bucket.Kind)) { + // Case belongs in this bucket; no update to bucket needed. + Bucket.Cases.emplace_back(CaseId, std::move(Case)); + return; } - 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; + } + // Couldn't find a compatible bucket. Start a new one. + Buckets.push_back(KindBucket{CaseKind, {}}); + Buckets.back().Cases.emplace_back(CaseId, std::move(Case)); } // Binds each rule's matcher to a unique (and deterministic) tag based on -// `TagBase`. -static std::vector<DynTypedMatcher> -taggedMatchers(StringRef TagBase, - const SmallVectorImpl<RewriteRule::Case> &Cases) { +// `TagBase` and the id paired with the case. +static std::vector<DynTypedMatcher> taggedMatchers( + StringRef TagBase, + const SmallVectorImpl<std::pair<size_t, RewriteRule::Case>> &Cases) { std::vector<DynTypedMatcher> 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 +154,29 @@ 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<DynTypedMatcher> +tooling::detail::buildMatchers(const RewriteRule &Rule) { + // Sort the cases into buckets of compatible matchers. + std::vector<KindBucket> Buckets; + for (int I = 0, N = Rule.Cases.size(); I < N; ++I) + insertCase(Buckets, I, Rule.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<DynTypedMatcher> Matchers; + for (const auto &Bucket : Buckets) { + DynTypedMatcher M = DynTypedMatcher::constructVariadic( + DynTypedMatcher::VO_AnyOf, Bucket.Kind, + taggedMatchers("Tag", Bucket.Cases)); + 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<DynTypedMatcher> 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 +199,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 +242,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; } } Index: clang/include/clang/Tooling/Refactoring/Transformer.h =================================================================== --- clang/include/clang/Tooling/Refactoring/Transformer.h +++ clang/include/clang/Tooling/Refactoring/Transformer.h @@ -160,12 +160,10 @@ void addInclude(RewriteRule &Rule, llvm::StringRef Header, IncludeFormat Format = IncludeFormat::Quoted); -/// Applies the first rule whose pattern matches; other rules are ignored. +/// 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. /// -/// 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. -// // `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 // then dispatch on those ids in your code for control flow, `applyFirst` lifts @@ -243,8 +241,16 @@ // 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 all 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 matcher +/// base kind: Stmt, Decl, etc.) +std::vector<ast_matchers::internal::DynTypedMatcher> +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 &
_______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits