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

[Jan Lahoda]

Hi,

I've updated the patch with the Flow changes and with additional 
comments in TransPatterns here:
http://cr.openjdk.java.net/~jlahoda/8231826/webrev.01/
diff from previous:
http://cr.openjdk.java.net/~jlahoda/8231826/webrev.delta.00-01/

An additional patch (that would apply on top of this one) which makes 
all instanceof instances to be modelled as instanceof <pattern>:
http://cr.openjdk.java.net/~jlahoda/8231826/webrev.01.unify.instanceof/

Some more comment inline.

On 10. 10. 19 17:33, Maurizio Cimadamore wrote:
> 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?

Yes, the primary intent is to mark variables that need to be hoisted to 
the parent of the current statement.

> 
> * 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). 

The hoisted vars do not have an initializer (they used to have one, but 
it is both unnecessary and was masking out bugs, so I have removed it). 
But I see I've forgot the initializer code commented out in 
TransPatterns, removed in the updated version to avoid confusion.

> 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?

Yes, I think it is correct.

> 
> 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)

I tried to do these two in the updated patch.

Thanks for the comments!

Jan

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