[PATCH] D61335: [LibTooling] Add support to Transformer for composing rules as an ordered choice.

[Yitzhak Mandelbaum via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rC361037: [LibTooling] Add support to Transformer for 
composing rules as an ordered… (authored by ymandel, committed by ).

Changed prior to commit:
  https://reviews.llvm.org/D61335?vs=200042&id=200044#toc

Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D61335/new/

https://reviews.llvm.org/D61335

Files:
  include/clang/Tooling/Refactoring/Transformer.h
  lib/Tooling/Refactoring/Transformer.cpp
  unittests/Tooling/TransformerTest.cpp

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,
+  c

[PATCH] D61335: [LibTooling] Add support to Transformer for composing rules as an ordered choice.

[Yitzhak Mandelbaum via Phabricator via cfe-commits]

ymandel updated this revision to Diff 200042.
ymandel marked an inline comment as done.
ymandel added a comment.

Synced to HEAD in preparation for committing.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D61335/new/

https://reviews.llvm.org/D61335

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
@@ -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).
 //
Index: clang/lib/Tooling/Refactoring/Transformer.cpp
===
--- clang/lib/Tooling/Refactoring/Transformer.cpp
+++ clang/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 : Edit

[PATCH] D61335: [LibTooling] Add support to Transformer for composing rules as an ordered choice.

[Yitzhak Mandelbaum via Phabricator via cfe-commits]

ymandel updated this revision to Diff 200026.
ymandel marked 2 inline comments as done.
ymandel added a comment.

updated comments; moved some decls.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D61335/new/

https://reviews.llvm.org/D61335

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
@@ -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).
 //
Index: clang/lib/Tooling/Refactoring/Transformer.cpp
===
--- clang/lib/Tooling/Refactoring/Transformer.cpp
+++ clang/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) {
@@ -1

[PATCH] D61335: [LibTooling] Add support to Transformer for composing rules as an ordered choice.

[Yitzhak Mandelbaum via Phabricator via cfe-commits]

ymandel marked 2 inline comments as done.
ymandel added inline comments.



Comment at: clang/include/clang/Tooling/Refactoring/Transformer.h:278
+/// Builds the matcher needed for registration.
+ast_matchers::internal::DynTypedMatcher buildMatcher(const RewriteRule &Rule);
+

ilya-biryukov wrote:
> ymandel wrote:
> > ilya-biryukov wrote:
> > > Can it be declared in `.cpp` file instead? Or is it used in `clang-tidy` 
> > > integration? 
> > `buildMatcher` and `findSelectedCase` will be used in the clang-tidy 
> > integration and in the apply-rule-to-single-node function that I'm planning.
> I'd say this makes these functions a public interface of rewrite rule, albeit 
> it's an "advanced" use-case.
> It's probably ok to keep them in `detail` namespace for now, but would be 
> nice to come up with a proper public functions that allow us to implement 
> those use-cases.
> (Or declare these function public and well-supported and move them out of 
> `detail` at some point)
Agreed. I noted this explicitly with a FIXME, reworded the comments to 
explicitly associate these with `RewriteRule` and moved them to immediately 
after the other `RewriteRule` functions.  `Transformer` is now the last 
declaration in the file (and should probably be split into its own header at 
this point, being just one interpreter of `RewriteRule` among many).


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D61335/new/

https://reviews.llvm.org/D61335



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D61335: [LibTooling] Add support to Transformer for composing rules as an ordered choice.

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added inline comments.



Comment at: clang/include/clang/Tooling/Refactoring/Transformer.h:278
+/// Builds the matcher needed for registration.
+ast_matchers::internal::DynTypedMatcher buildMatcher(const RewriteRule &Rule);
+

ymandel wrote:
> ilya-biryukov wrote:
> > Can it be declared in `.cpp` file instead? Or is it used in `clang-tidy` 
> > integration? 
> `buildMatcher` and `findSelectedCase` will be used in the clang-tidy 
> integration and in the apply-rule-to-single-node function that I'm planning.
I'd say this makes these functions a public interface of rewrite rule, albeit 
it's an "advanced" use-case.
It's probably ok to keep them in `detail` namespace for now, but would be nice 
to come up with a proper public functions that allow us to implement those 
use-cases.
(Or declare these function public and well-supported and move them out of 
`detail` at some point)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D61335/new/

https://reviews.llvm.org/D61335



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D61335: [LibTooling] Add support to Transformer for composing rules as an ordered choice.

[Yitzhak Mandelbaum via Phabricator via cfe-commits]

ymandel marked 5 inline comments as done.
ymandel added a comment.

Thanks!!




Comment at: clang/include/clang/Tooling/Refactoring/Transformer.h:278
+/// Builds the matcher needed for registration.
+ast_matchers::internal::DynTypedMatcher buildMatcher(const RewriteRule &Rule);
+

ilya-biryukov wrote:
> Can it be declared in `.cpp` file instead? Or is it used in `clang-tidy` 
> integration? 
`buildMatcher` and `findSelectedCase` will be used in the clang-tidy 
integration and in the apply-rule-to-single-node function that I'm planning.



Comment at: clang/include/clang/Tooling/Refactoring/Transformer.h:282
+const RewriteRule::Case &
+findSelectedCase(const ast_matchers::MatchFinder::MatchResult &Result,
+ const RewriteRule &Rule);

ilya-biryukov wrote:
> Same here, so far this looks like an implementation detail of `Transformer`, 
> why not put it into `.cpp` file?
see above. I think these also make sense as part of the RewriteRule 
abstraction. that is, you can think of these as methods of RewriteRule and they 
make sense even without thinking about transformer.  That said, if you think 
there's a way to make this clearer, i"m happy to adjust the comments/name, etc.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D61335/new/

https://reviews.llvm.org/D61335



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D61335: [LibTooling] Add support to Transformer for composing rules as an ordered choice.

[Yitzhak Mandelbaum via Phabricator via cfe-commits]

ymandel updated this revision to Diff 199869.
ymandel added a comment.

adjust some API comments.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D61335/new/

https://reviews.llvm.org/D61335

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
@@ -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).
 //
Index: clang/lib/Tooling/Refactoring/Transformer.cpp
===
--- clang/lib/Tooling/Refactoring/Transformer.cpp
+++ clang/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;
 }
 
-R

[PATCH] D61335: [LibTooling] Add support to Transformer for composing rules as an ordered choice.

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added a comment.

LG, just a few NITs wrt to exposing implementation details in the header.




Comment at: clang/include/clang/Tooling/Refactoring/Transformer.h:161
+// components are gathered as a `Case` and rules are defined as an ordered list
+// of cases.
 //

NIT: maybe clarify what "ordered" means? E.g. "The first rule that matches gets 
applied and the rest are ignored"



Comment at: clang/include/clang/Tooling/Refactoring/Transformer.h:278
+/// Builds the matcher needed for registration.
+ast_matchers::internal::DynTypedMatcher buildMatcher(const RewriteRule &Rule);
+

Can it be declared in `.cpp` file instead? Or is it used in `clang-tidy` 
integration? 



Comment at: clang/include/clang/Tooling/Refactoring/Transformer.h:282
+const RewriteRule::Case &
+findSelectedCase(const ast_matchers::MatchFinder::MatchResult &Result,
+ const RewriteRule &Rule);

Same here, so far this looks like an implementation detail of `Transformer`, 
why not put it into `.cpp` file?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D61335/new/

https://reviews.llvm.org/D61335



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D61335: [LibTooling] Add support to Transformer for composing rules as an ordered choice.

[Yitzhak Mandelbaum via Phabricator via cfe-commits]

ymandel updated this revision to Diff 199476.
ymandel marked 18 inline comments as done.
ymandel added a comment.
Herald added a subscriber: jfb.

responded to comments.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D61335/new/

https://reviews.llvm.org/D61335

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
@@ -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).
 //
Index: clang/lib/Tooling/Refactoring/Transformer.cpp
===
--- clang/lib/Tooling/Refactoring/Transformer.cpp
+++ clang/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 &E

[PATCH] D61335: [LibTooling] Add support to Transformer for composing rules as an ordered choice.

[Yitzhak Mandelbaum via Phabricator via cfe-commits]

ymandel added inline comments.



Comment at: clang/include/clang/Tooling/Refactoring/Transformer.h:196
+/// Joins multiple rules into a single rule that applies the first rule in
+/// `Rules` whose pattern matches a given node. All of the rules must use the
+/// same kind of matcher (that is, share a base class in the AST hierarchy).

ilya-biryukov wrote:
> ymandel wrote:
> > ilya-biryukov wrote:
> > > > All of the rules must use the same kind of matcher
> > > 
> > > Could you elaborate why we want this restriction?
> > > Why they can't be of different kinds? 
> > > 
> > > It feels ok to have transformations to completely unrelated nodes and 
> > > still apply the first one.
> > > 
> > Agreed. The problem is purely from the implementation perspective. Since 
> > anyOf() enforces this restriction, I need to either
> > 1. pass it on to the user
> > 2. infer the base kind of each matcher and place them in the appropriate 
> > bucket.
> > 
> > I figured for a first pass, we'd go with 1. if you disagree, I can add the 
> > logic to bucket them and thereby support arbitrary combinations. FWIW, I 
> > think it's a desirable feature, so its not wasted work. its just a matter 
> > of splitting up the work.
> Ah, ok, so this is the restriction of `anyOf`.
indeed -- and all the other variadoc operations.



Comment at: clang/lib/Tooling/Refactoring/Transformer.cpp:250
+  assert(!CommonKind.isNone() && "Cases must have compatible matchers.");
+  return DynTypedMatcher::constructVariadic(
+  DynTypedMatcher::VO_AnyOf, CommonKind, taggedMatchers("Tag", 
Rule.Cases));

ilya-biryukov wrote:
> ymandel wrote:
> > ilya-biryukov wrote:
> > > I wonder if there a better way to construct an `anyOf` matcher that can 
> > > tell which case matched...
> > > It looks to be the reason for a more complicated interface on the 
> > > transformer side, but I'm not sure there's a way out of it.
> > > 
> > > Any ideas?
> > I don't quite follow. Which interface complication are you referring to?  
> > FWIW, the reason the code here doesn't just use the anyOf() matches is that 
> > it doesn't take a vector -- it only has a variadic form.
> E.g. the restriction that the matchers should have a common kind seems to 
> come from the fact that we need to later find out which case matched.
> 
> It this a limitation of the AST matchers design? E.g. I can't match both a 
> type and an expression and bind different subnodes in each submatcher, right?
> E.g. the restriction that the matchers should have a common kind seems to 
> come from the fact that we need to later find out which case matched.
>
> It this a limitation of the AST matchers design? E.g. I can't match both a 
> type and an expression and bind different subnodes in each submatcher, right?

Yes, as far as I can tell, but I don't think its connected to the binding -- 
every `DynTypedMatcher` needs to report what kind it supports.  IIRC, this is 
connected to the desire to specialize matchers to types to provide some static 
checking for matchers.  Since there is no universal/root AST type, there's no 
root kind, and we're stuck w/ this restriction.  If we were willing for the 
`Matcher` class to diverge from the AST, we could add a "universal" node kind, 
but that's another discussion...

In practice, this is mitigated in matchers by forcing the user to call a 
different `addMatcher` for each kind. But, that won't work for us since we want 
to (ultimately) support rules of different (base) kinds.



Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D61335/new/

https://reviews.llvm.org/D61335



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D61335: [LibTooling] Add support to Transformer for composing rules as an ordered choice.

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added a comment.

The implementation LG, thanks! Just a few NITs and comments about the public 
interface and the comments




Comment at: clang/include/clang/Tooling/Refactoring/Transformer.h:196
+/// Joins multiple rules into a single rule that applies the first rule in
+/// `Rules` whose pattern matches a given node. All of the rules must use the
+/// same kind of matcher (that is, share a base class in the AST hierarchy).

ymandel wrote:
> ilya-biryukov wrote:
> > > All of the rules must use the same kind of matcher
> > 
> > Could you elaborate why we want this restriction?
> > Why they can't be of different kinds? 
> > 
> > It feels ok to have transformations to completely unrelated nodes and still 
> > apply the first one.
> > 
> Agreed. The problem is purely from the implementation perspective. Since 
> anyOf() enforces this restriction, I need to either
> 1. pass it on to the user
> 2. infer the base kind of each matcher and place them in the appropriate 
> bucket.
> 
> I figured for a first pass, we'd go with 1. if you disagree, I can add the 
> logic to bucket them and thereby support arbitrary combinations. FWIW, I 
> think it's a desirable feature, so its not wasted work. its just a matter of 
> splitting up the work.
Ah, ok, so this is the restriction of `anyOf`.



Comment at: clang/include/clang/Tooling/Refactoring/Transformer.h:232
+// `makeOrderedRule({makeRule(m1, action1), makeRule(m2, action2), ...});`
+RewriteRule makeOrderedRule(ArrayRef Rules);
+

ymandel wrote:
> ilya-biryukov wrote:
> > Any ideas for a better name? `pickFirst`  or `applyFirst`?
> Yes, those are both better. Went w/ `applyFirst`.  Also considered 
> `applyFirstMatch` but not sure that buys much clarity
`applyFirst` looks good, thanks!



Comment at: clang/include/clang/Tooling/Refactoring/Transformer.h:240
 
+/// The following three functions function are a low-level part of the API,
+/// provided to support interpretation of a \c RewriteRule in a tool, like \c

ymandel wrote:
> ilya-biryukov wrote:
> > Could they be made private? If not, why?
> I tried to clarify this. But, I'm also fine splitting these three into a 
> separate header file or moving them to the bottom of this one.  They're only 
> exposed at all because TransformerTidy lives in a different directory and 
> namespace.  WDYT?
I see. Maybe additionally put them into `namespace detail`?
A good way to make sure that any use is a red herring. (Otherwise clients would 
need to read the code to realize it's an internal function.



Comment at: clang/include/clang/Tooling/Refactoring/Transformer.h:195
 
+/// Joins multiple rules into a single rule that applies the first rule in
+/// `Rules` whose pattern matches a given node.

NIT: Maybe shorten a bit? Something like

```
Applies the first rule whose pattern matches, other rules are ignored
```


The current version has a bit too many details, so it's hard to grasp on a 
first read.



Comment at: clang/include/clang/Tooling/Refactoring/Transformer.h:215
+//
+// Ordered rules allow us to specify each independently:
+// ```

NIT: s/Ordered rules allow us/applyFirst allows to... ?

With a new name for the function, "ordered" rules can be confusing



Comment at: clang/include/clang/Tooling/Refactoring/Transformer.h:231
+// ```
+// More generally, anywhere you'd use anyOf(m1.bind("m1"), m2.bind("m2")) and
+// then dispatch on those tags in your code to decide what to do, we'll lift

Maybe start with this? It's a great analogy. Something like this at the start 
of the comment would be great:
```
`applyFirst` is similar to `anyOf` matcher with a replace action attached to 
each of its cases...
```



Comment at: clang/lib/Tooling/Refactoring/Transformer.cpp:239
+  for (auto &Rule : Rules) {
+R.Cases.append(Rule.Cases.begin(), Rule.Cases.end());
+  }

NIT: remove redundant `{}`



Comment at: clang/lib/Tooling/Refactoring/Transformer.cpp:262
+
+// Finds the rule that was "selected" -- that is, whose matcher triggered the
+// `MatchResult`.

NIT:s/Finds the rule/Finds the case
Would help avoid potential confusion...



Comment at: clang/lib/Tooling/Refactoring/Transformer.cpp:250
+  assert(!CommonKind.isNone() && "Cases must have compatible matchers.");
+  return DynTypedMatcher::constructVariadic(
+  DynTypedMatcher::VO_AnyOf, CommonKind, taggedMatchers("Tag", 
Rule.Cases));

ymandel wrote:
> ilya-biryukov wrote:
> > I wonder if there a better way to construct an `anyOf` matcher that can 
> > tell which case matched...
> > It looks to be the reason for a more complicated interface on the 
> > transformer side, but I'm not sure there's a way out of it.
> > 
> > Any ideas?
> I don't quite follow. Which interfac

[PATCH] D61335: [LibTooling] Add support to Transformer for composing rules as an ordered choice.

[Yitzhak Mandelbaum via Phabricator via cfe-commits]

ymandel marked 9 inline comments as done and an inline comment as not done.
ymandel added a comment.

Thanks for the review. PTAL.




Comment at: clang/include/clang/Tooling/Refactoring/Transformer.h:196
+/// Joins multiple rules into a single rule that applies the first rule in
+/// `Rules` whose pattern matches a given node. All of the rules must use the
+/// same kind of matcher (that is, share a base class in the AST hierarchy).

ilya-biryukov wrote:
> > All of the rules must use the same kind of matcher
> 
> Could you elaborate why we want this restriction?
> Why they can't be of different kinds? 
> 
> It feels ok to have transformations to completely unrelated nodes and still 
> apply the first one.
> 
Agreed. The problem is purely from the implementation perspective. Since 
anyOf() enforces this restriction, I need to either
1. pass it on to the user
2. infer the base kind of each matcher and place them in the appropriate bucket.

I figured for a first pass, we'd go with 1. if you disagree, I can add the 
logic to bucket them and thereby support arbitrary combinations. FWIW, I think 
it's a desirable feature, so its not wasted work. its just a matter of 
splitting up the work.



Comment at: clang/include/clang/Tooling/Refactoring/Transformer.h:224
+//
+// RewriteRule R = makeOrderedRule({makeRule(two_calls, two_calls_action)},
+// {makeRule(left_call, left_call_action)},

ilya-biryukov wrote:
> Why would we need braces around each call? Aren't they a rewrite rule in 
> themselves?
typo. thanks!



Comment at: clang/include/clang/Tooling/Refactoring/Transformer.h:232
+// `makeOrderedRule({makeRule(m1, action1), makeRule(m2, action2), ...});`
+RewriteRule makeOrderedRule(ArrayRef Rules);
+

ilya-biryukov wrote:
> Any ideas for a better name? `pickFirst`  or `applyFirst`?
Yes, those are both better. Went w/ `applyFirst`.  Also considered 
`applyFirstMatch` but not sure that buys much clarity



Comment at: clang/include/clang/Tooling/Refactoring/Transformer.h:240
 
+/// The following three functions function are a low-level part of the API,
+/// provided to support interpretation of a \c RewriteRule in a tool, like \c

ilya-biryukov wrote:
> Could they be made private? If not, why?
I tried to clarify this. But, I'm also fine splitting these three into a 
separate header file or moving them to the bottom of this one.  They're only 
exposed at all because TransformerTidy lives in a different directory and 
namespace.  WDYT?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D61335/new/

https://reviews.llvm.org/D61335



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D61335: [LibTooling] Add support to Transformer for composing rules as an ordered choice.

[Yitzhak Mandelbaum via Phabricator via cfe-commits]

ymandel updated this revision to Diff 199465.
ymandel edited the summary of this revision.
ymandel added a comment.

Response to comments.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D61335/new/

https://reviews.llvm.org/D61335

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
@@ -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).
 //
Index: clang/lib/Tooling/Refactoring/Transformer.cpp
===
--- clang/lib/Tooling/Refactoring/Transformer.cpp
+++ clang/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;
@@ -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 

[PATCH] D61335: [LibTooling] Add support to Transformer for composing rules as an ordered choice.

[Yitzhak Mandelbaum via Phabricator via cfe-commits]

ymandel marked an inline comment as done.
ymandel added a comment.

Agreed on all the comments -- just one question:




Comment at: clang/lib/Tooling/Refactoring/Transformer.cpp:250
+  assert(!CommonKind.isNone() && "Cases must have compatible matchers.");
+  return DynTypedMatcher::constructVariadic(
+  DynTypedMatcher::VO_AnyOf, CommonKind, taggedMatchers("Tag", 
Rule.Cases));

ilya-biryukov wrote:
> I wonder if there a better way to construct an `anyOf` matcher that can tell 
> which case matched...
> It looks to be the reason for a more complicated interface on the transformer 
> side, but I'm not sure there's a way out of it.
> 
> Any ideas?
I don't quite follow. Which interface complication are you referring to?  FWIW, 
the reason the code here doesn't just use the anyOf() matches is that it 
doesn't take a vector -- it only has a variadic form.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D61335/new/

https://reviews.llvm.org/D61335



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D61335: [LibTooling] Add support to Transformer for composing rules as an ordered choice.

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added inline comments.



Comment at: clang/include/clang/Tooling/Refactoring/Transformer.h:196
+/// Joins multiple rules into a single rule that applies the first rule in
+/// `Rules` whose pattern matches a given node. All of the rules must use the
+/// same kind of matcher (that is, share a base class in the AST hierarchy).

> All of the rules must use the same kind of matcher

Could you elaborate why we want this restriction?
Why they can't be of different kinds? 

It feels ok to have transformations to completely unrelated nodes and still 
apply the first one.




Comment at: clang/include/clang/Tooling/Refactoring/Transformer.h:224
+//
+// RewriteRule R = makeOrderedRule({makeRule(two_calls, two_calls_action)},
+// {makeRule(left_call, left_call_action)},

Why would we need braces around each call? Aren't they a rewrite rule in 
themselves?



Comment at: clang/include/clang/Tooling/Refactoring/Transformer.h:232
+// `makeOrderedRule({makeRule(m1, action1), makeRule(m2, action2), ...});`
+RewriteRule makeOrderedRule(ArrayRef Rules);
+

Any ideas for a better name? `pickFirst`  or `applyFirst`?



Comment at: clang/include/clang/Tooling/Refactoring/Transformer.h:240
 
+/// The following three functions function are a low-level part of the API,
+/// provided to support interpretation of a \c RewriteRule in a tool, like \c

Could they be made private? If not, why?



Comment at: clang/include/clang/Tooling/Refactoring/Transformer.h:247
+
+/// Returns the \c Action of \c Rule that was selected in the match result.
+const RewriteRule::Case &

s/Action/Case? Or am I missing something?



Comment at: clang/lib/Tooling/Refactoring/Transformer.cpp:182
+// Determines whether A is higher than B in the class hierarchy.
+static bool isHigher(ASTNodeKind A, ASTNodeKind B) {
+  static auto QualKind = ASTNodeKind::getFromNodeKind();

NIT: maybe name it `isBaseOf`? When talking about class hierarchies, using 
'base' and 'derived' is a common convention.



Comment at: clang/lib/Tooling/Refactoring/Transformer.cpp:250
+  assert(!CommonKind.isNone() && "Cases must have compatible matchers.");
+  return DynTypedMatcher::constructVariadic(
+  DynTypedMatcher::VO_AnyOf, CommonKind, taggedMatchers("Tag", 
Rule.Cases));

I wonder if there a better way to construct an `anyOf` matcher that can tell 
which case matched...
It looks to be the reason for a more complicated interface on the transformer 
side, but I'm not sure there's a way out of it.

Any ideas?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D61335/new/

https://reviews.llvm.org/D61335



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D61335: [LibTooling] Add support to Transformer for composing rules as an ordered choice.

[Yitzhak Mandelbaum via Phabricator via cfe-commits]

ymandel added a comment.

In D61335#1495324 , @ilya-biryukov 
wrote:

> Sorry for the delay.
>  I personally like the `RewriteRule::Action` best. Allows to use a rather 
> common term, while still avoiding any possible confusion.


I tried going with `RewriteRule::Action` (it was my preference as well), but 
found that it didn't work well once I tried folding the composite rules into 
RewriteRule.  The problem is that the Action structure forces you to eagerly 
join the matchers (which is what my first diff did). But then it doesn't work 
to join multiple rules that themselves were built with makeOrderedRule. 
Previously, this was impossible because they had different types, but now a 
user could conceivably do this.  So, I changed the implementation build the 
joined matcher later.  RewriteRule now just stores all of the (sub)rules in a 
single list, and `makeOrderedRule` is a trivial merge.  The real work is done 
at `registerMatchers()`-time where we build the joined matcher for the whole 
rule. But, this means keeping the whole (sub)rule around, not just the Action 
part.

Alternatives I considered:

1. We could use the Action structure inside Transformer -- instead of storing a 
RewriteRule, we could simply store a single matcher and vector of Action. In 
the constructor, we would call `buildMatcher(Rule)` and then copy only the 
action parts from the rules.  However, I don't think this added complexity will 
gain us anything other than a structure which more faithfully reflects the 
control flow -- the matchers in each case are "dead" once we build the joined 
matcher, and the Action approach would reflect that.

2. We could add more structure to Case, like:

  struct RewriteRule {
struct Action {
  SmallVector Edits;
  TextGenerator Explanation;
};
struct Case {
  ast_matchers::internal::DynTypedMatcher Matcher;
  Action A;
};
std::vector Cases;
  };


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D61335/new/

https://reviews.llvm.org/D61335



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D61335: [LibTooling] Add support to Transformer for composing rules as an ordered choice.

[Yitzhak Mandelbaum via Phabricator via cfe-commits]

ymandel updated this revision to Diff 198811.
ymandel added a comment.

Folded CompositeRewriteRule into RewriteRule and added examples to comments.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D61335/new/

https://reviews.llvm.org/D61335

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
@@ -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(makeOrderedRule({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(makeOrderedRule({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(makeOrderedRule({ruleStrlenSizeDistinct(), ruleStrlenSize()}), 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
@@ -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;
@@ -171,18 +172,112 @@
   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 higher than B in the class hierarchy.
+static bool isHigher(ASTNodeK

[PATCH] D61335: [LibTooling] Add support to Transformer for composing rules as an ordered choice.

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added a comment.

Sorry for the delay.
I personally like the `RewriteRule::Action` best. Allows to use a rather common 
term, while still avoiding any possible confusion.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D61335/new/

https://reviews.llvm.org/D61335



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D61335: [LibTooling] Add support to Transformer for composing rules as an ordered choice.

[Yitzhak Mandelbaum via Phabricator via cfe-commits]

ymandel added a comment.

Gentle ping...


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D61335/new/

https://reviews.llvm.org/D61335



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D61335: [LibTooling] Add support to Transformer for composing rules as an ordered choice.

[Yitzhak Mandelbaum via Phabricator via cfe-commits]

ymandel added a comment.

In D61335#1489680 , @ilya-biryukov 
wrote:

> > As for naming, agreed, but does that concern drop away once we have only a 
> > single RewriteRule definition?
>
> Sure, that won't be an issue.
>
> The use-cases make sense, thanks for the examples. Could you add one or two 
> to the code? Would probably make sense to express them in code as matchers, 
> rather than explaining what we're trying to do in a natural language.


Sounds good, will do.

> Let's proceed with making `RewriteRule` the vocabulary type that we use for 
> transformations like this if we both agree that's a good idea.
>  There are obviously multiple ways to tackle this, which one you had in mind?

Something like this, although if you have a better name than `RewriteAction` 
I'm open to alternatives. e.g. `RewriteCase`?

  struct RewriteAction {
SmallVector Edits;
TextGenerator Explanation;
  };
  
  struct RewriteRule {
ast_matchers::internal::DynTypedMatcher Matcher;
std::vector Actions;
static constexpr llvm::StringLiteral RootId = "___root___";
  };

Or, nest the definition:

  struct RewriteRule {
struct Action {
  SmallVector Edits;
  TextGenerator Explanation;
};
  
ast_matchers::internal::DynTypedMatcher Matcher;
std::vector Actions;
  
static constexpr llvm::StringLiteral RootId = "___root___";
  };


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D61335/new/

https://reviews.llvm.org/D61335



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D61335: [LibTooling] Add support to Transformer for composing rules as an ordered choice.

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added a comment.

> As for naming, agreed, but does that concern drop away once we have only a 
> single RewriteRule definition?

Sure, that won't be an issue.

The use-cases make sense, thanks for the examples. Could you add one or two to 
the code? Would probably make sense to express them in code as matchers, rather 
than explaining what we're trying to do in a natural language.

Let's proceed with making `RewriteRule` the vocabulary type that we use for 
transformations like this if we both agree that's a good idea.
There are obviously multiple ways to tackle this, which one you had in mind?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D61335/new/

https://reviews.llvm.org/D61335



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D61335: [LibTooling] Add support to Transformer for composing rules as an ordered choice.

[Yitzhak Mandelbaum via Phabricator via cfe-commits]

ymandel added a comment.

In D61335#1488212 , @ilya-biryukov 
wrote:

> - Could you provide a few real-world motivating examples?  Especially 
> interested in cases that were complicated before and are easy to do now?


Sure, below are some examples based on actual tidies I've written (although not 
released).  Let me know whether you think any of these should be expressed in 
the comments:

EXAMPLE 1
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`).

Let’s assume this comes up in testing. We have three cases to consider:

  EXPECT_EQ(a.serialize(), b.serialize());
  EXPECT_EQ(a, b.serialize());
  EXPECT_EQ(a.serialize(), b);

With ordered choice, you can encode the above directly into matchers and then 
compose them into an ordered choice rule.  Without it, you’d have to write 
these as

  EXPECT_EQ(a.serialize(), b.serialize());
  EXPECT_EQ(a, b.serialize());  // where a != a’.serialize()
  EXPECT_EQ(a.serialize(), b);  // where b != b’.serialize()

where the constraints in the comments are explicitly in the matchers.

EXAMPLE 2
Consider patterns of the form

  absl::WrapUnique(e.release())

where `e` is an expression of type `std::unique_ptr<...>`.  This pattern was 
necessary in gcc for certain circumstances relating to upcasting the value type 
of the `unique_ptr`. It is not necessary in clang, and so should be removed.

In some cases, it should be rewritten to `e` and sometimes to `std::move(e)`, 
depending on the value category of `e` and the context (the expression of a 
return statement or not).  That is, there are two cases:
`absl::WrapUnique(e.release())` rewrites to `e`, if `e` is an r/x-value OR the 
call appears as a return expression,
`absl::WrapUnique(e.release())` rewrites to `std::move(e)`.
With ordered choice, the second case is written plainly (that is, just 
translate that expression to a matcher). Without it, you need to further 
express: unless `e` is an r/x-value OR the call appears as a return expression.

EXAMPLE 3
More generally, anywhere you’d use `anyOf(m1.bind(“m1”), m2.bind(“m2”))` and 
then dispatch on those tags in your code to decide what to do, we’ll lift that 
behavior to the rule level, so you can write 
makeOrderedRule({

  makeRule(m1, action1), makeRule(m2, action2), …

});

> - Do we expect most the rules to be written using the new API or is this 
> something that's gonna be used in `5-10%` of the cases?

I'd expect higher than 10% but not the majority of cases.  However, given that 
this semantics exactly matches that of `anyOf`, it may well be more common than 
I expect.

> - Could we consider expressing the `CompositeRewriteRule` as a `RewriteRule` 
> itself? That might require extending the `RewriteRule` abstraction, but the 
> mental model that makes sense to me is: a rewrite rule matches some AST nodes 
> and applies replacements to them. A `CompositeRewriteRule` seems to match the 
> same model, despite the fact it contains other rules. Would be nice if we 
> could make this fact internal to the implementation.
> 
>   Also a bit uneasy about the naming. `Composite` suggests the individual 
> rules compose one after another, while in practice only one alternative is 
> chosen.

Great points!  I'm fine with collapsing the types into one. As you can see from 
line 260 in Transformer.h, we're already folding the single rule into the 
composite rule.  As for naming, agreed, but does that concern drop away once we 
have only a single RewriteRule definition?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D61335/new/

https://reviews.llvm.org/D61335



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D61335: [LibTooling] Add support to Transformer for composing rules as an ordered choice.

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added a comment.

- Could you provide a few real-world motivating examples?  Especially 
interested in cases that were complicated before and are easy to do now?
- Do we expect most the rules to be written using the new API or is this 
something that's gonna be used in `5-10%` of the cases?
- Could we consider expressing the `CompositeRewriteRule` as a `RewriteRule` 
itself? That might require extending the `RewriteRule` abstraction, but the 
mental model that makes sense to me is: a rewrite rule matches some AST nodes 
and applies replacements to them. A `CompositeRewriteRule` seems to match the 
same model, despite the fact it contains other rules. Would be nice if we could 
make this fact internal to the implementation.

Also a bit uneasy about the naming. `Composite` suggests the individual rules 
compose one after another, while in practice only one alternative is chosen.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D61335/new/

https://reviews.llvm.org/D61335



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D61335: [LibTooling] Add support to Transformer for composing rules as an ordered choice.

[Yitzhak Mandelbaum via Phabricator via cfe-commits]

ymandel created this revision.
ymandel added a reviewer: ilya-biryukov.
Herald added a project: clang.

This revision adds a new kind of rewrite rule, `CompositeRewriteRule`, which
composes multiple subrules into a new rule that allows ordered-choice among its
subrules. 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.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D61335

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
@@ -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));
@@ -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(makeOrderedRule({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(makeOrderedRule({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(makeOrderedRule({ruleStrlenSizeDistinct(), ruleStrlenSize()}), 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
@@ -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;
@@ -171,7 +172,7 @@
   return Transformations;
 }
 
-RewriteRule tooling::makeRule(ast_matchers::internal::DynTypedMatcher M,
+RewriteRule tooling::makeRule(DynTypedMatcher M,
   SmallVector Edits) {
   M.setAllowBind(true);
   // `tryBind` is guaranteed to succeed, because `AllowBind` was set to true.
@@ -181,6 +182,80 @@
 
 constexpr llvm::StringLiteral RewriteRule::RootId;
 
+// Determines whether A is higher than B in the class hierarchy.
+static bool isHigher(ASTNo