[libTooling] In Transformer, generalize applyFirst to admit rules with incompatible matchers.

Summary:
This patch removes an (artificial) limitation of `applyFirst`, which requires
that all of the rules' matchers can be grouped together in a single `anyOf()`.
This change generalizes the code to group the matchers into separate `anyOf`s
based on compatibility. Correspondingly, `buildMatcher` is changed to
`buildMatchers`, to allow for returning a set of matchers rather than just one.

Reviewers: gribozavr

Subscribers: cfe-commits

Tags: #clang

Differential Revision: https://reviews.llvm.org/D65877

llvm-svn: 368681

Author: ymand

clang/include/clang/Tooling/Refactoring/Transformer.h

@@ -160,11 +160,9 @@ inline RewriteRule makeRule(ast_matchers::internal::DynTypedMatcher M,
 void addInclude(RewriteRule &Rule, llvm::StringRef Header,
                 IncludeFormat Format = IncludeFormat::Quoted);
 
-/// Applies the first rule whose pattern matches; other rules are ignored.
-///
-/// N.B. All of the rules must use the same kind of matcher (that is, share a
-/// base class in the AST hierarchy).  However, this constraint is caused by an
-/// implementation detail and should be lifted in the future.
+/// Applies the first rule whose pattern matches; other rules are ignored.  If
+/// the matchers are independent then order doesn't matter. In that case,
+/// `applyFirst` is simply joining the set of rules into one.
 //
 // `applyFirst` is like an `anyOf` matcher with an edit action attached to each
 // of its cases. Anywhere you'd use `anyOf(m1.bind("id1"), m2.bind("id2"))` and
@@ -243,8 +241,19 @@ inline ASTEdit remove(RangeSelector S) {
 // public and well-supported and move them out of `detail`.
 namespace detail {
 /// Builds a single matcher for the rule, covering all of the rule's cases.
+/// Only supports Rules whose cases' matchers share the same base "kind"
+/// (`Stmt`, `Decl`, etc.)  Deprecated: use `buildMatchers` instead, which
+/// supports mixing matchers of different kinds.
 ast_matchers::internal::DynTypedMatcher buildMatcher(const RewriteRule &Rule);
 
+/// Builds a set of matchers that cover the rule (one for each distinct node
+/// matcher base kind: Stmt, Decl, etc.). Node-matchers for `QualType` and
+/// `Type` are not permitted, since such nodes carry no source location
+/// information and are therefore not relevant for rewriting. If any such
+/// matchers are included, will return an empty vector.
+std::vector<ast_matchers::internal::DynTypedMatcher>
+buildMatchers(const RewriteRule &Rule);
+
 /// Returns the \c Case of \c Rule that was selected in the match result.
 /// Assumes a matcher built with \c buildMatcher.
 const RewriteRule::Case &

clang/lib/Tooling/Refactoring/Transformer.cpp

@@ -19,10 +19,10 @@
 #include "llvm/ADT/StringRef.h"
 #include "llvm/Support/Errc.h"
 #include "llvm/Support/Error.h"
-#include <deque>
 #include <string>
 #include <utility>
 #include <vector>
+#include <map>
 
 using namespace clang;
 using namespace tooling;
@@ -80,52 +80,23 @@ void tooling::addInclude(RewriteRule &Rule, StringRef Header,
     Case.AddedIncludes.emplace_back(Header.str(), Format);
 }
 
-// Determines whether A is a base type of B in the class hierarchy, including
-// the implicit relationship of Type and QualType.
-static bool isBaseOf(ASTNodeKind A, ASTNodeKind B) {
-  static auto TypeKind = ASTNodeKind::getFromNodeKind<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;
+// Filters for supported matcher kinds. FIXME: Explicitly list the allowed kinds
+// (all node matcher types except for `QualType` and `Type`), rather than just
+// banning `QualType` and `Type`.
+static bool hasValidKind(const DynTypedMatcher &M) {
+  return !M.canConvertTo<QualType>();
 }
 
 // Binds each rule's matcher to a unique (and deterministic) tag based on
-// `TagBase`.
-static std::vector<DynTypedMatcher>
-taggedMatchers(StringRef TagBase,
-               const SmallVectorImpl<RewriteRule::Case> &Cases) {
+// `TagBase` and the id paired with the case.
+static std::vector<DynTypedMatcher> taggedMatchers(
+    StringRef TagBase,
+    const SmallVectorImpl<std::pair<size_t, RewriteRule::Case>> &Cases) {
   std::vector<DynTypedMatcher> Matchers;
   Matchers.reserve(Cases.size());
-  size_t count = 0;
   for (const auto &Case : Cases) {
-    std::string Tag = (TagBase + Twine(count)).str();
-    ++count;
-    auto M = Case.Matcher.tryBind(Tag);
+    std::string Tag = (TagBase + Twine(Case.first)).str();
+    auto M = Case.second.Matcher.tryBind(Tag);
     assert(M && "RewriteRule matchers should be bindable.");
     Matchers.push_back(*std::move(M));
   }
@@ -142,22 +113,37 @@ RewriteRule tooling::applyFirst(ArrayRef<RewriteRule> Rules) {
   return R;
 }
 
-static DynTypedMatcher joinCaseMatchers(const RewriteRule &Rule) {
-  assert(!Rule.Cases.empty() && "Rule must have at least one case.");
-  if (Rule.Cases.size() == 1)
-    return Rule.Cases[0].Matcher;
+std::vector<DynTypedMatcher>
+tooling::detail::buildMatchers(const RewriteRule &Rule) {
+  // Map the cases into buckets of matchers -- one for each "root" AST kind,
+  // which guarantees that they can be combined in a single anyOf matcher. Each
+  // case is paired with an identifying number that is converted to a string id
+  // in `taggedMatchers`.
+  std::map<ASTNodeKind, SmallVector<std::pair<size_t, RewriteRule::Case>, 1>>
+      Buckets;
+  const SmallVectorImpl<RewriteRule::Case> &Cases = Rule.Cases;
+  for (int I = 0, N = Cases.size(); I < N; ++I) {
+    assert(hasValidKind(Cases[I].Matcher) &&
+           "Matcher must be non-(Qual)Type node matcher");
+    Buckets[Cases[I].Matcher.getSupportedKind()].emplace_back(I, Cases[I]);
+  }
 
-  auto CommonKind = findCommonKind(Rule.Cases);
-  assert(!CommonKind.isNone() && "Cases must have compatible matchers.");
-  return DynTypedMatcher::constructVariadic(
-      DynTypedMatcher::VO_AnyOf, CommonKind, taggedMatchers("Tag", Rule.Cases));
+  std::vector<DynTypedMatcher> Matchers;
+  for (const auto &Bucket : Buckets) {
+    DynTypedMatcher M = DynTypedMatcher::constructVariadic(
+        DynTypedMatcher::VO_AnyOf, Bucket.first,
+        taggedMatchers("Tag", Bucket.second));
+    M.setAllowBind(true);
+    // `tryBind` is guaranteed to succeed, because `AllowBind` was set to true.
+    Matchers.push_back(*M.tryBind(RewriteRule::RootID));
+  }
+  return Matchers;
 }
 
 DynTypedMatcher tooling::detail::buildMatcher(const RewriteRule &Rule) {
-  DynTypedMatcher M = joinCaseMatchers(Rule);
-  M.setAllowBind(true);
-  // `tryBind` is guaranteed to succeed, because `AllowBind` was set to true.
-  return *M.tryBind(RewriteRule::RootID);
+  std::vector<DynTypedMatcher> Ms = buildMatchers(Rule);
+  assert(Ms.size() == 1 && "Cases must have compatible matchers.");
+  return Ms[0];
 }
 
 // Finds the case that was "selected" -- that is, whose matcher triggered the
@@ -180,7 +166,8 @@ tooling::detail::findSelectedCase(const MatchResult &Result,
 constexpr llvm::StringLiteral RewriteRule::RootID;
 
 void Transformer::registerMatchers(MatchFinder *MatchFinder) {
-  MatchFinder->addDynamicMatcher(tooling::detail::buildMatcher(Rule), this);
+  for (auto &Matcher : tooling::detail::buildMatchers(Rule))
+    MatchFinder->addDynamicMatcher(Matcher, this);
 }
 
 void Transformer::run(const MatchResult &Result) {
@@ -222,12 +209,12 @@ void Transformer::run(const MatchResult &Result) {
   for (const auto &I : Case.AddedIncludes) {
     auto &Header = I.first;
     switch (I.second) {
-      case IncludeFormat::Quoted:
-        AC.addHeader(Header);
-        break;
-      case IncludeFormat::Angled:
-        AC.addHeader((llvm::Twine("<") + Header + ">").str());
-        break;
+    case IncludeFormat::Quoted:
+      AC.addHeader(Header);
+      break;
+    case IncludeFormat::Angled:
+      AC.addHeader((llvm::Twine("<") + Header + ">").str());
+      break;
     }
   }
 

clang/unittests/Tooling/TransformerTest.cpp

@@ -482,65 +482,85 @@ TEST_F(TransformerTest, OrderedRuleUnrelated) {
   testRule(applyFirst({ruleStrlenSize(), FlagRule}), Input, Expected);
 }
 
-// Version of ruleStrlenSizeAny that inserts a method with a different name than
-// ruleStrlenSize, so we can tell their effect apart.
-RewriteRule ruleStrlenSizeDistinct() {
-  StringRef S;
-  return makeRule(
-      callExpr(callee(functionDecl(hasName("strlen"))),
-               hasArgument(0, cxxMemberCallExpr(
-                                  on(expr().bind(S)),
-                                  callee(cxxMethodDecl(hasName("c_str")))))),
-      change(text("DISTINCT")));
-}
-
 TEST_F(TransformerTest, OrderedRuleRelated) {
   std::string Input = R"cc(
-    namespace foo {
-    struct mystring {
-      char* c_str();
-    };
-    int f(mystring s) { return strlen(s.c_str()); }
-    }  // namespace foo
-    int g(string s) { return strlen(s.c_str()); }
+    void f1();
+    void f2();
+    void call_f1() { f1(); }
+    void call_f2() { f2(); }
   )cc";
   std::string Expected = R"cc(
-    namespace foo {
-    struct mystring {
-      char* c_str();
-    };
-    int f(mystring s) { return DISTINCT; }
-    }  // namespace foo
-    int g(string s) { return REPLACED; }
+    void f1();
+    void f2();
+    void call_f1() { REPLACE_F1; }
+    void call_f2() { REPLACE_F1_OR_F2; }
   )cc";
 
-  testRule(applyFirst({ruleStrlenSize(), ruleStrlenSizeDistinct()}), Input,
-           Expected);
+  RewriteRule ReplaceF1 =
+      makeRule(callExpr(callee(functionDecl(hasName("f1")))),
+               change(text("REPLACE_F1")));
+  RewriteRule ReplaceF1OrF2 =
+      makeRule(callExpr(callee(functionDecl(hasAnyName("f1", "f2")))),
+               change(text("REPLACE_F1_OR_F2")));
+  testRule(applyFirst({ReplaceF1, ReplaceF1OrF2}), Input, Expected);
 }
 
-// Change the order of the rules to get a different result.
+// Change the order of the rules to get a different result. When `ReplaceF1OrF2`
+// comes first, it applies for both uses, so `ReplaceF1` never applies.
 TEST_F(TransformerTest, OrderedRuleRelatedSwapped) {
   std::string Input = R"cc(
-    namespace foo {
-    struct mystring {
-      char* c_str();
-    };
-    int f(mystring s) { return strlen(s.c_str()); }
-    }  // namespace foo
-    int g(string s) { return strlen(s.c_str()); }
+    void f1();
+    void f2();
+    void call_f1() { f1(); }
+    void call_f2() { f2(); }
   )cc";
   std::string Expected = R"cc(
-    namespace foo {
-    struct mystring {
-      char* c_str();
-    };
-    int f(mystring s) { return DISTINCT; }
-    }  // namespace foo
-    int g(string s) { return DISTINCT; }
+    void f1();
+    void f2();
+    void call_f1() { REPLACE_F1_OR_F2; }
+    void call_f2() { REPLACE_F1_OR_F2; }
+  )cc";
+
+  RewriteRule ReplaceF1 =
+      makeRule(callExpr(callee(functionDecl(hasName("f1")))),
+               change(text("REPLACE_F1")));
+  RewriteRule ReplaceF1OrF2 =
+      makeRule(callExpr(callee(functionDecl(hasAnyName("f1", "f2")))),
+               change(text("REPLACE_F1_OR_F2")));
+  testRule(applyFirst({ReplaceF1OrF2, ReplaceF1}), Input, Expected);
+}
+
+// Verify that a set of rules whose matchers have different base kinds works
+// properly, including that `applyFirst` produces multiple matchers.  We test
+// two different kinds of rules: Expr and Decl. We place the Decl rule in the
+// middle to test that `buildMatchers` works even when the kinds aren't grouped
+// together.
+TEST_F(TransformerTest, OrderedRuleMultipleKinds) {
+  std::string Input = R"cc(
+    void f1();
+    void f2();
+    void call_f1() { f1(); }
+    void call_f2() { f2(); }
   )cc";
-
-  testRule(applyFirst({ruleStrlenSizeDistinct(), ruleStrlenSize()}), Input,
-           Expected);
+  std::string Expected = R"cc(
+    void f1();
+    void DECL_RULE();
+    void call_f1() { REPLACE_F1; }
+    void call_f2() { REPLACE_F1_OR_F2; }
+  )cc";
+
+  RewriteRule ReplaceF1 =
+      makeRule(callExpr(callee(functionDecl(hasName("f1")))),
+               change(text("REPLACE_F1")));
+  RewriteRule ReplaceF1OrF2 =
+      makeRule(callExpr(callee(functionDecl(hasAnyName("f1", "f2")))),
+               change(text("REPLACE_F1_OR_F2")));
+  RewriteRule DeclRule = makeRule(functionDecl(hasName("f2")).bind("fun"),
+                                  change(name("fun"), text("DECL_RULE")));
+
+  RewriteRule Rule = applyFirst({ReplaceF1, DeclRule, ReplaceF1OrF2});
+  EXPECT_EQ(tooling::detail::buildMatchers(Rule).size(), 2UL);
+  testRule(Rule, Input, Expected);
 }
 
 //
@@ -718,4 +738,18 @@ TEST_F(TransformerTest, NoPartialRewriteOfMacroExpansionForMacroArgs) {
 
   testRule(ruleStrlenSize(), Input, Input);
 }
+
+#if !defined(NDEBUG) && GTEST_HAS_DEATH_TEST
+// Verifies that `Type` and `QualType` are not allowed as top-level matchers in
+// rules.
+TEST(TransformerDeathTest, OrderedRuleTypes) {
+  RewriteRule QualTypeRule = makeRule(qualType(), change(text("Q")));
+  EXPECT_DEATH(tooling::detail::buildMatchers(QualTypeRule),
+               "Matcher must be.*node matcher");
+
+  RewriteRule TypeRule = makeRule(arrayType(), change(text("T")));
+  EXPECT_DEATH(tooling::detail::buildMatchers(TypeRule),
+               "Matcher must be.*node matcher");
+}
+#endif
 } // namespace