Index: include/clang/Tooling/Refactoring/Transformer.h =================================================================== --- include/clang/Tooling/Refactoring/Transformer.h +++ include/clang/Tooling/Refactoring/Transformer.h @@ -145,8 +145,8 @@ /// Description of a source-code transformation. // -// A *rewrite rule* describes a transformation of source code. It has the -// following components: +// A *rewrite rule* describes a transformation of source code. A simple rule +// contains each of the following components: // // * Matcher: the pattern term, expressed as clang matchers (with Transformer // extensions). @@ -156,30 +156,31 @@ // * Explanation: explanation of the rewrite. This will be displayed to the // user, where possible; for example, in clang-tidy diagnostics. // -// Rules have an additional, implicit, component: the parameters. These are -// portions of the pattern which are left unspecified, yet named so that we can -// reference them in the replacement term. The structure of parameters can be -// partially or even fully specified, in which case they serve just to identify -// matched nodes for later reference rather than abstract over portions of the -// AST. However, in all cases, we refer to named portions of the pattern as -// parameters. +// However, rules can also consist of (sub)rules, where the first that matches +// is applied and the rest are ignored. So, the above components are gathered +// as a `Case` and a rule is a list of cases. +// +// Rule cases have an additional, implicit, component: the parameters. These are +// portions of the pattern which are left unspecified, yet bound in the pattern +// so that we can reference them in the edits. // -// The \c Transformer class should be used to apply the rewrite rule and obtain -// the corresponding replacements. +// The \c Transformer class can be used to apply the rewrite rule and obtain the +// corresponding replacements. struct RewriteRule { - // `Matcher` describes the context of this rule. It should always be bound to - // at least `RootId`. - ast_matchers::internal::DynTypedMatcher Matcher; - SmallVector Edits; - TextGenerator Explanation; + struct Case { + ast_matchers::internal::DynTypedMatcher Matcher; + SmallVector Edits; + TextGenerator Explanation; + }; + // We expect RewriteRules will most commonly include only one case. + SmallVector Cases; // Id used as the default target of each match. The node described by the // matcher is should always be bound to this id. static constexpr llvm::StringLiteral RootId = "___root___"; }; -/// Convenience function for constructing a \c RewriteRule. Takes care of -/// binding the matcher to RootId. +/// Convenience function for constructing a simple \c RewriteRule. RewriteRule makeRule(ast_matchers::internal::DynTypedMatcher M, SmallVector Edits); @@ -191,12 +192,73 @@ return makeRule(std::move(M), std::move(Edits)); } +/// 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. +// +// `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 +// that behavior to the rule level. So, you can write `applyFirst({makeRule(m1, +// action1), makeRule(m2, action2), ...});` +// +// For example, consider a type `T` with a deterministic serialization function, +// `serialize()`. For performance reasons, we would like to make it +// non-deterministic. Therefore, we want to drop the expectation that +// `a.serialize() = b.serialize() iff a = b` (although we'll maintain +// `deserialize(a.serialize()) = a`). +// +// We have three cases to consider (for some equality function, `eq`): +// ``` +// eq(a.serialize(), b.serialize()) --> eq(a,b) +// eq(a, b.serialize()) --> eq(deserialize(a), b) +// eq(a.serialize(), b) --> eq(a, deserialize(b)) +// ``` +// +// `applyFirst` allows us to specify each independently: +// ``` +// auto eq_fun = functionDecl(...); +// auto method_call = cxxMemberCallExpr(...); +// +// auto two_calls = callExpr(callee(eq_fun), hasArgument(0, method_call), +// hasArgument(1, method_call)); +// auto left_call = +// callExpr(callee(eq_fun), callExpr(hasArgument(0, method_call))); +// auto right_call = +// callExpr(callee(eq_fun), callExpr(hasArgument(1, method_call))); +// +// RewriteRule R = applyFirst({makeRule(two_calls, two_calls_action), +// makeRule(left_call, left_call_action), +// makeRule(right_call, right_call_action)}); +// ``` +RewriteRule applyFirst(ArrayRef Rules); + // Define this overload of `change` here because RewriteRule::RootId is not in // scope at the declaration point above. template ASTEdit change(TextGenerator Replacement) { return change(RewriteRule::RootId, NodePart::Node, std::move(Replacement)); } +/// The following three functions are a low-level part of the RewriteRule +/// API. We expose them for use in implementing the fixtures that interpret +/// RewriteRule, like Transformer and TransfomerTidy, or for more advanced +/// users. +// +// FIXME: These functions are really public, if advanced, elements of the +// RewriteRule API. Recast them as such. Or, just declare these functions +// 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. +ast_matchers::internal::DynTypedMatcher buildMatcher(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 & +findSelectedCase(const ast_matchers::MatchFinder::MatchResult &Result, + const RewriteRule &Rule); + /// A source "transformation," represented by a character range in the source to /// be replaced and a corresponding replacement string. struct Transformation { @@ -206,9 +268,7 @@ /// Attempts to translate `Edits`, which are in terms of AST nodes bound in the /// match `Result`, into Transformations, which are in terms of the source code -/// text. This function is a low-level part of the API, provided to support -/// interpretation of a \c RewriteRule in a tool, like \c Transformer, rather -/// than direct use by end users. +/// text. /// /// Returns an empty vector if any of the edits apply to portions of the source /// that are ineligible for rewriting (certain interactions with macros, for @@ -220,6 +280,7 @@ Expected> translateEdits(const ast_matchers::MatchFinder::MatchResult &Result, llvm::ArrayRef Edits); +} // namespace detail /// Handles the matcher and callback registration for a single rewrite rule, as /// defined by the arguments of the constructor. Index: lib/Tooling/Refactoring/Transformer.cpp =================================================================== --- lib/Tooling/Refactoring/Transformer.cpp +++ lib/Tooling/Refactoring/Transformer.cpp @@ -28,6 +28,7 @@ using namespace tooling; using ast_matchers::MatchFinder; +using ast_matchers::internal::DynTypedMatcher; using ast_type_traits::ASTNodeKind; using ast_type_traits::DynTypedNode; using llvm::Error; @@ -144,9 +145,9 @@ llvm_unreachable("Unexpected case in NodePart type."); } -Expected> -tooling::translateEdits(const MatchResult &Result, - llvm::ArrayRef Edits) { +Expected> +tooling::detail::translateEdits(const MatchResult &Result, + llvm::ArrayRef Edits) { SmallVector Transformations; auto &NodesMap = Result.Nodes.getMap(); for (const auto &Edit : Edits) { @@ -171,18 +172,113 @@ return Transformations; } -RewriteRule tooling::makeRule(ast_matchers::internal::DynTypedMatcher M, +RewriteRule tooling::makeRule(DynTypedMatcher M, SmallVector Edits) { + return RewriteRule{ + {RewriteRule::Case{std::move(M), std::move(Edits), nullptr}}}; +} + +// 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; +} + +// Binds each rule's matcher to a unique (and deterministic) tag based on +// `TagBase`. +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); + assert(M && "RewriteRule matchers should be bindable."); + Matchers.push_back(*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 tooling::applyFirst(ArrayRef Rules) { + RewriteRule R; + for (auto &Rule : Rules) + R.Cases.append(Rule.Cases.begin(), Rule.Cases.end()); + 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; + + auto CommonKind = findCommonKind(Rule.Cases); + assert(!CommonKind.isNone() && "Cases must have compatible matchers."); + return DynTypedMatcher::constructVariadic( + DynTypedMatcher::VO_AnyOf, CommonKind, taggedMatchers("Tag", Rule.Cases)); +} + +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 RewriteRule{*M.tryBind(RewriteRule::RootId), std::move(Edits), - nullptr}; + return *M.tryBind(RewriteRule::RootId); +} + +// Finds the case that was "selected" -- that is, whose matcher triggered the +// `MatchResult`. +const RewriteRule::Case & +tooling::detail::findSelectedCase(const MatchResult &Result, + const RewriteRule &Rule) { + if (Rule.Cases.size() == 1) + return Rule.Cases[0]; + + auto &NodesMap = Result.Nodes.getMap(); + for (size_t i = 0, N = Rule.Cases.size(); i < N; ++i) { + std::string Tag = ("Tag" + Twine(i)).str(); + if (NodesMap.find(Tag) != NodesMap.end()) + return Rule.Cases[i]; + } + llvm_unreachable("No tag found for this rule."); } constexpr llvm::StringLiteral RewriteRule::RootId; void Transformer::registerMatchers(MatchFinder *MatchFinder) { - MatchFinder->addDynamicMatcher(Rule.Matcher, this); + MatchFinder->addDynamicMatcher(tooling::detail::buildMatcher(Rule), this); } void Transformer::run(const MatchResult &Result) { @@ -197,7 +293,8 @@ Root->second.getSourceRange().getBegin()); assert(RootLoc.isValid() && "Invalid location for Root node of match."); - auto Transformations = translateEdits(Result, Rule.Edits); + auto Transformations = tooling::detail::translateEdits( + Result, tooling::detail::findSelectedCase(Result, Rule).Edits); if (!Transformations) { Consumer(Transformations.takeError()); return; Index: unittests/Tooling/TransformerTest.cpp =================================================================== --- unittests/Tooling/TransformerTest.cpp +++ unittests/Tooling/TransformerTest.cpp @@ -116,7 +116,8 @@ }; } - void testRule(RewriteRule Rule, StringRef Input, StringRef Expected) { + template + void testRule(R Rule, StringRef Input, StringRef Expected) { Transformer T(std::move(Rule), consumer()); T.registerMatchers(&MatchFinder); compareSnippets(Expected, rewrite(Input)); @@ -147,7 +148,7 @@ .bind(StringExpr)), callee(cxxMethodDecl(hasName("c_str")))))), change("REPLACED")); - R.Explanation = text("Use size() method directly on string."); + R.Cases[0].Explanation = text("Use size() method directly on string."); return R; } @@ -375,6 +376,92 @@ Input, Expected); } +TEST_F(TransformerTest, OrderedRuleUnrelated) { + StringRef Flag = "flag"; + RewriteRule FlagRule = makeRule( + cxxMemberCallExpr(on(expr(hasType(cxxRecordDecl( + hasName("proto::ProtoCommandLineFlag")))) + .bind(Flag)), + unless(callee(cxxMethodDecl(hasName("GetProto"))))), + change(Flag, "PROTO")); + + std::string Input = R"cc( + proto::ProtoCommandLineFlag flag; + int x = flag.foo(); + int y = flag.GetProto().foo(); + int f(string s) { return strlen(s.c_str()); } + )cc"; + std::string Expected = R"cc( + proto::ProtoCommandLineFlag flag; + int x = PROTO.foo(); + int y = flag.GetProto().foo(); + int f(string s) { return REPLACED; } + )cc"; + + 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("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()); } + )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; } + )cc"; + + testRule(applyFirst({ruleStrlenSize(), ruleStrlenSizeDistinct()}), Input, + Expected); +} + +// Change the order of the rules to get a different result. +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()); } + )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; } + )cc"; + + testRule(applyFirst({ruleStrlenSizeDistinct(), ruleStrlenSize()}), Input, + Expected); +} + // // Negative tests (where we expect no transformation to occur). //