r361037 - [LibTooling] Add support to Transformer for composing rules as an ordered choice.
Yitzhak Mandelbaum via cfe-commits
cfe-commits at lists.llvm.org
Fri May 17 07:23:33 PDT 2019
Author: ymandel
Date: Fri May 17 07:23:33 2019
New Revision: 361037
URL: http://llvm.org/viewvc/llvm-project?rev=361037&view=rev
Log:
[LibTooling] Add support to Transformer for composing rules as an ordered choice.
This revision updates `RewriteRule` to support multiple subrules that are
interpreted as an ordered-choice (apply the first one that matches). With this
feature, users can write the rules that appear later in the list of subrules
knowing that previous rules' patterns *have not matched*, freeing them from
reasoning about those cases in the current pattern.
Reviewers: ilya-biryukov
Subscribers: cfe-commits
Tags: #clang
Differential Revision: https://reviews.llvm.org/D61335
Modified:
cfe/trunk/include/clang/Tooling/Refactoring/Transformer.h
cfe/trunk/lib/Tooling/Refactoring/Transformer.cpp
cfe/trunk/unittests/Tooling/TransformerTest.cpp
Modified: cfe/trunk/include/clang/Tooling/Refactoring/Transformer.h
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Tooling/Refactoring/Transformer.h?rev=361037&r1=361036&r2=361037&view=diff
==============================================================================
--- cfe/trunk/include/clang/Tooling/Refactoring/Transformer.h (original)
+++ cfe/trunk/include/clang/Tooling/Refactoring/Transformer.h Fri May 17 07:23:33 2019
@@ -145,8 +145,8 @@ template <typename T> ASTEdit change(std
/// 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 @@ template <typename T> ASTEdit change(std
// * 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<ASTEdit, 1> Edits;
- TextGenerator Explanation;
+ struct Case {
+ ast_matchers::internal::DynTypedMatcher Matcher;
+ SmallVector<ASTEdit, 1> Edits;
+ TextGenerator Explanation;
+ };
+ // We expect RewriteRules will most commonly include only one case.
+ SmallVector<Case, 1> 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<ASTEdit, 1> Edits);
@@ -191,12 +192,73 @@ inline RewriteRule makeRule(ast_matchers
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<RewriteRule> Rules);
+
// Define this overload of `change` here because RewriteRule::RootId is not in
// scope at the declaration point above.
template <typename T> ASTEdit change(TextGenerator Replacement) {
return change<T>(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 @@ struct Transformation {
/// 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 @@ struct Transformation {
Expected<SmallVector<Transformation, 1>>
translateEdits(const ast_matchers::MatchFinder::MatchResult &Result,
llvm::ArrayRef<ASTEdit> Edits);
+} // namespace detail
/// Handles the matcher and callback registration for a single rewrite rule, as
/// defined by the arguments of the constructor.
Modified: cfe/trunk/lib/Tooling/Refactoring/Transformer.cpp
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Tooling/Refactoring/Transformer.cpp?rev=361037&r1=361036&r2=361037&view=diff
==============================================================================
--- cfe/trunk/lib/Tooling/Refactoring/Transformer.cpp (original)
+++ cfe/trunk/lib/Tooling/Refactoring/Transformer.cpp Fri May 17 07:23:33 2019
@@ -28,6 +28,7 @@ using namespace clang;
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 @@ getTargetRange(StringRef Target, const D
llvm_unreachable("Unexpected case in NodePart type.");
}
-Expected<SmallVector<Transformation, 1>>
-tooling::translateEdits(const MatchResult &Result,
- llvm::ArrayRef<ASTEdit> Edits) {
+Expected<SmallVector<tooling::detail::Transformation, 1>>
+tooling::detail::translateEdits(const MatchResult &Result,
+ llvm::ArrayRef<ASTEdit> Edits) {
SmallVector<Transformation, 1> Transformations;
auto &NodesMap = Result.Nodes.getMap();
for (const auto &Edit : Edits) {
@@ -171,18 +172,113 @@ tooling::translateEdits(const MatchResul
return Transformations;
}
-RewriteRule tooling::makeRule(ast_matchers::internal::DynTypedMatcher M,
+RewriteRule tooling::makeRule(DynTypedMatcher M,
SmallVector<ASTEdit, 1> 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<Type>();
+ static auto QualKind = ASTNodeKind::getFromNodeKind<QualType>();
+ /// Mimic the implicit conversions of Matcher<>.
+ /// - From Matcher<Type> to Matcher<QualType>
+ /// - From Matcher<Base> to Matcher<Derived>
+ 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;
+ }
+ 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<DynTypedMatcher>
+taggedMatchers(StringRef TagBase,
+ const SmallVectorImpl<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);
+ 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<RewriteRule> 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 @@ void Transformer::run(const MatchResult
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;
Modified: cfe/trunk/unittests/Tooling/TransformerTest.cpp
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/unittests/Tooling/TransformerTest.cpp?rev=361037&r1=361036&r2=361037&view=diff
==============================================================================
--- cfe/trunk/unittests/Tooling/TransformerTest.cpp (original)
+++ cfe/trunk/unittests/Tooling/TransformerTest.cpp Fri May 17 07:23:33 2019
@@ -116,7 +116,8 @@ protected:
};
}
- void testRule(RewriteRule Rule, StringRef Input, StringRef Expected) {
+ template <typename R>
+ 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 @@ static RewriteRule ruleStrlenSize() {
.bind(StringExpr)),
callee(cxxMethodDecl(hasName("c_str")))))),
change<clang::Expr>("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 @@ TEST_F(TransformerTest, MultiChange) {
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<clang::Expr>(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<clang::Expr>("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).
//
More information about the cfe-commits
mailing list