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

Tue Oct 15 11:59:12 UTC 2019

Hi,
the flow changes look good - I think the TransPattern documentation 
should contain less text and more code snippet examples which show what 
the generated code looks like (as I've tried to do in my email, and as 
you've done for visitTypeTest). In other words, what is missing here is 
"the big picture" which shows what are the main ideas behind the 
translation strategy.

Specific example: decorateExpression:

+ //if there are binding variables defined and used only in this expression,
+ //which are not confined to a specific sub-expression,
+ //a let expression is created which replaces the statement, and
+ //the binding variables are hoisted into this let expression:

This kind of illustrates my point:

* "if there are binding variables defined and used only in this 
expression, which are not confined to a specific sub-expression" is very 
convoluted, and will be almost meaningless when we pick up this code 
again in 6 months
* "a let expression is created which replaces the statement" - 
statement? Probably cut and paste error

Thanks
Maurizio

On 15/10/2019 10:46, Jan Lahoda wrote:
> 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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.java.net/pipermail/compiler-dev/attachments/20191015/ef23cc99/attachment.html>