[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 <https://reviews.llvm.org/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