[PATCH] D65877: [libTooling] In Transformer, generalize `applyFirst` to admit rules with incompatible matchers.

[Dmitri Gribenko via Phabricator via cfe-commits]

gribozavr added inline comments.


================
Comment at: clang/lib/Tooling/Refactoring/Transformer.cpp:85
 // the implicit relationship of Type and QualType.
 static bool isBaseOf(ASTNodeKind A, ASTNodeKind B) {
   static auto TypeKind = ASTNodeKind::getFromNodeKind<Type>();
----------------
I'd find it more readable to s/A/MaybeBase/ and s/B/MaybeDerived/.


================
Comment at: clang/lib/Tooling/Refactoring/Transformer.cpp:90
   /// - From Matcher<Type> to Matcher<QualType>
   /// - From Matcher<Base> to Matcher<Derived>
   return (A.isSame(TypeKind) && B.isSame(QualKind)) || A.isBaseOf(B);
----------------
I don't understand how `Matcher<Base>` implicitly converts to `Matcher<Derived>` -- is the order correct?


================
Comment at: clang/lib/Tooling/Refactoring/Transformer.cpp:91
   /// - From Matcher<Base> to Matcher<Derived>
   return (A.isSame(TypeKind) && B.isSame(QualKind)) || A.isBaseOf(B);
 }
----------------
`A.isSame(TypeKind) && B.isSame(QualKind)` -- is the order correct here?

QualType is a more general kind than Type. That's why Type implicitly converts to QualType. Therefore, QualType is the "base", and Type is "derived". Therefore:

`A.isSame(QualKind) && B.isSame(TypeKind)`

AKA

`MaybeBase.isSame(QualKind) && MaybeDerived.isSame(TypeKind)`


================
Comment at: clang/lib/Tooling/Refactoring/Transformer.cpp:92
   return (A.isSame(TypeKind) && B.isSame(QualKind)) || A.isBaseOf(B);
 }
 
----------------
WDYT about moving this API into a static method on `ASTNodeKind`, and then calling it from `DynTypedMatcher::canConvertTo`, deduplicating the logic?


================
Comment at: clang/lib/Tooling/Refactoring/Transformer.cpp:95
+namespace {
+struct KindBucket {
+  // Common kind which all of the bucket's matchers can support. Specifically, a
----------------
"CompatibleCasesBucket" ?


================
Comment at: clang/lib/Tooling/Refactoring/Transformer.cpp:108
+// means that the type of the case's matcher is a sub/super class of all other
+// cases in that bucket.
+static void insertCase(std::vector<KindBucket> &Buckets, size_t CaseId,
----------------
"sub/super class" part does not seem correct to me -- it must be one or the other (if it can be either, we would only have one bucket for everything).

WDYT about

"Adds the new case to the bucket that contains compatible cases."

I don't think we need to explain the algorithm in the contract.


================
Comment at: clang/lib/Tooling/Refactoring/Transformer.cpp:110
+static void insertCase(std::vector<KindBucket> &Buckets, size_t CaseId,
+                       RewriteRule::Case Case) {
+  ASTNodeKind CaseKind = Case.Matcher.getSupportedKind();
----------------
I would find it more readable to s/CaseId/NewCaseId/ and s/Case/NewCase/. Up to you.


================
Comment at: clang/lib/Tooling/Refactoring/Transformer.cpp:112
+  ASTNodeKind CaseKind = Case.Matcher.getSupportedKind();
+  for (int I = Buckets.size() - 1; I >= 0; --I) {
+    KindBucket &Bucket = Buckets[I];
----------------
I don't understand why it is going backwards.


================
Comment at: clang/lib/Tooling/Refactoring/Transformer.cpp:115
+    if (isBaseOf(Bucket.Kind, CaseKind)) {
+      // Case belongs in this bucket and bucket's new kind is `CaseKind`.
+      Bucket.Kind = CaseKind;
----------------
The comment merely repeats the code -- consider removing it.


================
Comment at: clang/lib/Tooling/Refactoring/Transformer.cpp:116
+      // Case belongs in this bucket and bucket's new kind is `CaseKind`.
+      Bucket.Kind = CaseKind;
+      Bucket.Cases.emplace_back(CaseId, std::move(Case));
----------------
If `isBaseOf(Bucket.Kind, CaseKind)` is true, then `Bucket.Kind` is the more general type. So I don't understand why we are changing the bucket kind to the more specific one, `CaseKind`.


================
Comment at: clang/lib/Tooling/Refactoring/Transformer.cpp:129
+  Buckets.back().Cases.emplace_back(CaseId, std::move(Case));
 }
 
----------------
Would it be easier to implement this algorithm using `ASTNodeKind::getMostDerivedCommonAncestor` instead of `isBaseOf`?

Go over all buckets, for each bucket check if common ancestor between the bucket kind and the new case kind is not none. If it is not none, insert the new case into that bucket, and update bucket's kind to the new common ancestor kind.

You could also go as far as setting the bucket's kind to always the the root of the corresponding kind hierarchy (TemplateArgument, TemplateName, Decl, Stmt etc.), since given enough diverse cases, the bucket kind will converge to the root kind.


================
Comment at: clang/unittests/Tooling/TransformerTest.cpp:520
            Expected);
 }
 
----------------
If this test and the test below are testing rule ordering only, I find them to have too much unrelated detail. We could do something simpler and more self-contained like:

input:
```
void f1();
void f2();
void call_f1() { f1(); }
void call_f2() { f2(); }
```

```
replaceF1 = makeRule(
  callExpr(callee(functionDecl(hasName("f1"))),
  change(text("REPLACE_F1"))
)

replaceF1orF2 = makeRule(
  callExpr(callee(functionDecl(hasAnyName("f1", "f2"))),
  change(text("REPLACE_F1_OR_F2"))
)
```

```
applyFirst({replaceF1, replaceF1orF2})
=> check that the output is =>
void f1();
void f2();
void call_f1() { REPLACE_F1; }
void call_f2() { REPLACE_F1_OR_F2; }
```

```
applyFirst({replaceF1orF2, replaceF1})
=> check that the output is =>
void f1();
void f2();
void call_f1() { REPLACE_F1_OR_F2; }
void call_f2() { REPLACE_F1_OR_F2; }
```

Up to you.


================
Comment at: clang/unittests/Tooling/TransformerTest.cpp:582
+  EXPECT_EQ(tooling::detail::buildMatchers(Rule).size(), 2UL);
+  testRule(Rule, Input, Expected);
+}
----------------
Ditto, I think we can make a simpler and more self-contained test:

input:
```
void f1();
void f2();
void call_f1() { f1(); }
```

```
stmtRule = makeRule(
  callExpr(callee(functionDecl(hasName("f1"))),
  change(text("STMT_RULE")))

declRule = makeRule(
  functionDecl(hasName("f2")).bind("fun"),
  change(name("fun"), text("DECL_RULE")))

rule = applyFirst({stmtRule, declRule})
```

expected output:
```
void f1();
void DECL_RULE();
void call_f1() { STMT_RULE(); }
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65877