This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by ymandel on Aug 7 2019, 8:01 AM.

Details

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 anyOfs
based on compatibility. Correspondingly, buildMatcher is changed to
buildMatchers, to allow for returning a set of matchers rather than just one.

Event Timeline

ymandel created this revision.Aug 7 2019, 8:01 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 7 2019, 8:01 AM
ymandel updated this revision to Diff 213932.Aug 7 2019, 9:51 AM

fix comment.

gribozavr added inline comments.Aug 9 2019, 5:43 AM
clang/lib/Tooling/Refactoring/Transformer.cpp
82–83

I don't understand how Matcher<Base> implicitly converts to Matcher<Derived> -- is the order correct?

82–83

I'd find it more readable to s/A/MaybeBase/ and s/B/MaybeDerived/.

82–83

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)

82–83

WDYT about moving this API into a static method on ASTNodeKind, and then calling it from DynTypedMatcher::canConvertTo, deduplicating the logic?

83

"CompatibleCasesBucket" ?

88

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.

96

"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.

98

I would find it more readable to s/CaseId/NewCaseId/ and s/Case/NewCase/. Up to you.

100

I don't understand why it is going backwards.

103

The comment merely repeats the code -- consider removing it.

104

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.

clang/unittests/Tooling/TransformerTest.cpp
506

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.

599

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(); }
ymandel updated this revision to Diff 214442.Aug 9 2019, 1:30 PM
ymandel marked 14 inline comments as done.

Rewrote the code and tests.

ymandel marked 2 inline comments as done.Aug 9 2019, 1:32 PM

Thanks for your detailed and helpful feedback.

clang/lib/Tooling/Refactoring/Transformer.cpp
88

Based on our discussion, I've drastically simplified the code. As per our discussion: since each matcher must be a node matcher, we only have the root AST types to consider. Therefore, there's no need for comparison of relative levels in the kind hierarchy -- we simply map each matcher to a root kind and then put it in that bucket. This lets us remove lots of custom code.

clang/unittests/Tooling/TransformerTest.cpp
506

Much nicer. Thanks!

599

I kept a little of the complexity of the old test -- I still have a third rule with the same kind as the first to make sure rules are bucketed together even when separated by different rule.

gribozavr accepted this revision.Aug 12 2019, 1:16 AM

Nice simplification, thanks!

clang/lib/Tooling/Refactoring/Transformer.cpp
122

unordered_map?

This revision is now accepted and ready to land.Aug 12 2019, 1:16 AM
ymandel marked 2 inline comments as done.Aug 12 2019, 10:44 AM

I was going to add a test for Type/QualType and realized that they don't carry any source location info. Therefore, I don't think they belong as top-level matchers for rewrite rules. Instead, users should use typeLoc(loc(<type matcher here>). Therefore, the logic of getKind can be eliminated in favor of using K.getSupportedKind() directly. However, that means we'll need to keep them out of the collection of matchers that we're processing.

For that, I propose either we ban them in buildMatchers() or we detect them and wrap them in typeLoc(loc(<type matcher here>). I'm fine with either one, but prefer the latter. WDYT?

clang/lib/Tooling/Refactoring/Transformer.cpp
122

fails because ASTNodeKind doesn't have a hash function. Which is odd since there's an obvious one we could define, but doesn't seem worth doing just for here.

I was going to add a test for Type/QualType and realized that they don't carry any source location info. Therefore, I don't think they belong as top-level matchers for rewrite rules.

Agreed.

Instead, users should use typeLoc(loc(<type matcher here>). Therefore, the logic of getKind can be eliminated in favor of using K.getSupportedKind() directly. However, that means we'll need to keep them out of the collection of matchers that we're processing.

For that, I propose either we ban them in buildMatchers() or we detect them and wrap them in typeLoc(loc(<type matcher here>). I'm fine with either one, but prefer the latter. WDYT?

I'd prefer we ban them completely, and let the user wrap them manually if they need to. While some users would appreciate the ergonomics, I think AST matchers are complicated even without us adding more implicit behavior on top.

ymandel updated this revision to Diff 214679.Aug 12 2019, 11:56 AM

Bans qualtype and type and adds corresponding comments and test.

ymandel marked an inline comment as done.Aug 12 2019, 11:58 AM

I'd prefer we ban them completely, and let the user wrap them manually if they need to. While some users would appreciate the ergonomics, I think AST matchers are complicated even without us adding more implicit behavior on top.

Done. I came to the same conclusion once I started implementing. Less (magic) is more, in this case.

gribozavr accepted this revision.Aug 12 2019, 12:46 PM
gribozavr added inline comments.
clang/lib/Tooling/Refactoring/Transformer.cpp
127

Returning an empty vector is a valid API choice, but why not assert instead? If an empty vector is returned, unsupported matchers are silently ignored instead of being diagnosed.

ymandel updated this revision to Diff 214700.Aug 12 2019, 1:32 PM

Changed early return to assertion; updated test.

ymandel marked 2 inline comments as done.Aug 12 2019, 1:36 PM
ymandel added inline comments.
clang/lib/Tooling/Refactoring/Transformer.cpp
127

Good point. I generally dislike assertions, but at this point the caller is passing a malformed-rule and it is not the place for returning errors. If we plug this support into a UI, we can add filters on the front end that fail nicely and return error messages rather than die.

This revision was automatically updated to reflect the committed changes.
ymandel marked an inline comment as done.
Herald added a project: Restricted Project. · View Herald TranscriptAug 13 2019, 5:31 AM