RFR: JDK-8231826: Implement javac changes for pattern matching for instanceof

[Maurizio Cimadamore]

Hi Jan,
the code looks generally very clean, kudos.

Some general comments:

* looking at the spec, it seems like both "instanceof T" and "instanceof 
T t" are cases of type test patterns. I guess I'm fine with the 
implementation doing what it always did in terms of plain "instanceof 
T", but I'm worried about the intersection between this and e.g. the 
tree API - InstanceofTree::getPattern returns null in cases like 
"instanceof T"; now, I know I know that we're speaking about a JDK 
specific API, but I think this issue reveals some modelling issues in 
the way we treat instanceof, and I'm worried that some of these issues 
might pop up in the future. I'd prefer to either rectify the spec so 
that plain 'instanceof T' is not a pattern matching instanceof, or 
rectify javac so that these tests are internally also represented with 
patterns (at the expense of some extra allocation, perhaps).

* If I'm not mistaken the only use for the "MATCH_BINDING_TO_OUTER" flag 
is to be able to distinguish between regular 'expression-wide' bindings, 
and bindings which 'leaked' outside a statement (e.g. an if statement). 
And the reason you need to distinguish between these is that you don't 
want Check::checkUnique to flag duplicate errors between regular 
'expression-wide' bindings, which are reported elsewhere 
(MatchBindingsComputer). But this is also, more crucially, used in 
TransPattern, where the 'isPreserved' flag is used to control whether a 
variable decl for the binding variable should be 'lifted' to the 
enclosing statement context or not. Is my understanding correct here?

* The idea behind TransPatterns seems to be: when we process a 
statement, or an expression, we attempt to add all declaration for the 
bindings that are used inside the statements/expression upfront. If we 
are processing a statement, then we surround the results in a block; e.g.

if (obj instanceof Foo f) {
   ...
}

becomes

{
    Foo f$;
    if (let Object temp = obj in (obj instanceof Foo && (f$ = (Foo)temp) 
== temp) {
       ...
}

If we are processing an expression, we instead generate a LetExpr, e.g.

boolean b = obj instanceof Foo t && t.equals(foo);

becomes:

boolean b = let Foo f$ = null in ((let Object temp = obj in (obj 
instanceof Foo && (f$ = (Foo)temp) == temp) && f$.equals(foo))

So, sometimes the hoisted var is a real var decl in a block, other times 
is a var decl inside a let expression. In these cases we have to 
generate an initializer, to set the value (which might be used later). 
On top of that, instanceof generates its own letExpr to cache the target 
of the test (to avoid double computation).

It also seems to me that the code handles cases where the binding 
variable is not used, neither hoisted - e.g.

boolean field = obj instanceof Foo t;

In this case we generate a plain instanceof w/o the init (because the 
't' variable hasn't been hoisted anywhere).

Finally, we use the 'isPreserved()' flag to ensure that variables are 
hoisted correctly - for instance, if something is to be preserved (and 
the enclosing context allows for it) we push things in the enclosing 
context instead.

Am I getting the correct picture here?

It would be nice to have more javadoc spread around to help the reader 
understand what's the rationale and show some snippet of generated code.

* Flow, I wonder if, like you had created SnippetAliveAnalyzer, creating 
a SnippetBreakAnalyzer would help you avoid the breaksOut[0] trick (that 
could become a field in the child visitor)


Other than that, it looks very good.

Maurizio

On 10/10/2019 13:12, Jan Lahoda wrote:
> Hi,
>
> As part of the effort to prepare JEP 305: Pattern Matching for 
> instanceof (Preview) for Propose to Target, I would like to ask for a 
> code review for the corresponding javac changes.
>
> The webrev is here:
> http://cr.openjdk.java.net/~jlahoda/8231826/webrev.00/
>
> The patch applies on top of:
> https://mail.openjdk.java.net/pipermail/compiler-dev/2019-October/013727.html 
>
>
> The current spec the patch is striving to implements is here:
> http://cr.openjdk.java.net/~gbierman/jep305/jep305-20190918/specs/patterns-instanceof-jls.html 
>
>
> As far as I know, there is one (significant) open issue in the spec, 
> and that is whether non-reifiable types should be allowed in 
> "instanceof <type-test-pattern>". Currently (AFAIK), the spec does not 
> allow non-reifiable types in the type test pattern in instanceof, and 
> the javac implementation should be consistent with the spec. Should 
> the spec change, the corresponding update to the javac code should 
> have a very limited impact.
>
> I'll be preparing a CSR for this change in the coming days.
>
> The JBS issue for this code change is:
> https://bugs.openjdk.java.net/browse/JDK-8231826
>
> Any feedback is welcome!
>
> Thanks!
>     Jan