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

Tue Oct 22 15:27:26 UTC 2019

Hi,

A new draft of the pattern matching for instanceof has been published here:
https://mail.openjdk.java.net/pipermail/amber-dev/2019-October/004962.html

A significant change in this draft is that safe(*) reifiable types are 
allowed in instanceof of. And updated version of the pattern matching 
patch for javac which supports this change is here:
http://cr.openjdk.java.net/~jlahoda/8231826/webrev.03/

A diff from the previous revision:
http://cr.openjdk.java.net/~jlahoda/8231826/webrev.delta.02-03/

(*) reifiable types are allowed if (for "expr instanceof type"), "expr" 
can be cast to "type" without unchecked warnings.

Does this make sense?

Thanks,
     Jan

On 15. 10. 19 15:02, Maurizio Cimadamore wrote:
> Yep - that would be more helpful (at least to me)
> 
> Thanks
> Maurizio
> 
> On 15/10/2019 13:56, Jan Lahoda wrote:
>> Would this be better?
>>
>> Full patch:
>> http://cr.openjdk.java.net/~jlahoda/8231826/webrev.02/
>>
>> Diff from previous:
>> http://cr.openjdk.java.net/~jlahoda/8231826/webrev.delta.01-02/
>>
>> Thanks,
>>     Jan
>>
>> On 15. 10. 19 13:59, Maurizio Cimadamore wrote:
>>> 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