RFR: JDK-8240998: Implement javac changes for deconstruction patterns.

[Jan Lahoda]

On 25. 03. 20 20:25, Maurizio Cimadamore wrote:
> 
> On 25/03/2020 08:54, Jan Lahoda wrote:
>> Hi Maurizio,
>>
>> Thanks for the comments!
>>
>> An updated version of the patch is here:
>> http://cr.openjdk.java.net/~jlahoda/8240998/webrev.02/
>> diff to previous patch:
>> http://cr.openjdk.java.net/~jlahoda/8240998/webrev.delta.01.02/
> 
> Looks good - minor nit, in the comments in TransPattern:
> 
> //PATTn for record component COMPn of type Tn;
> + //PATTn is a type test pattern or a deconstruction pattern:
> + //=>
> + //(let Tn $c$COMPn = ((T) N$temp).COMPn(); <PATTn extractor>)
> + //or
> + //(let Tn $c$COMPn = ((T) N$temp).COMPn(); $c$COMPn != null && 
> <nested-extractor>)
> + //or
> + //(let Tn $c$COMPn = ((T) N$temp).COMPn(); $c$COMPn instanceof T' && 
> <nested-extractor>)
> 
> 
> Shouldn't we use either <PATTn extractor> or <nested-extractor> in all 
> of them, consistently? That's what the code appears to be doing.

Thanks, I've updated the comments (+one golden output).

Full Webrev:
https://cr.openjdk.java.net/~jlahoda/8240998/webrev.03/

Delta from previous:
https://cr.openjdk.java.net/~jlahoda/8240998/webrev.delta.02.03/

Does this look better?

Thanks,
     Jan

> 
> 
>> On 23. 03. 20 16:52, Maurizio Cimadamore wrote:
>>> Hi Jan,
>>> the code looks generally good. I have some high-level questions:
>>>
>>> * it would be useful to see what kind of diagnostics are generated - 
>>> I see some weird ones in the golden files (e.g. 
>>> "DeconstructionPatternErrors.out" refer to "compiler.misc.type.none")
>>
>> Diag samples:
>> http://cr.openjdk.java.net/~jlahoda/8240998/diags-examples.html
>>
>> Output of DeconstructionPatternErrors:
>> http://cr.openjdk.java.net/~jlahoda/8240998/DeconstructionPatternErrors.txt 
>>
> Thanks! Look good.
>>
>> Regarding the compiler.misc.type.none (which maps to "<none>") - this 
>> is in a case where there are too many nested patterns in the 
>> deconstruction pattern, and the trailing nested pattern(s) is/are 
>> "var" pattern(s). So this is some kind of indeterminable type, and 
>> "<none>" does not seem too off.
> 
> I'd suggest to use the tree printing machinery we have for deferred type 
> arguments in the rich diagnostic formatter - that would give you better 
> results, I believe.
> 
> Not sure if I'd prefer to break down this:
> 
> "deconstruction patterns can only be applied to records, String is not a 
> record"
> 
> into:
> 
> "invalid deconstruction pattern"
> <source>
> "String is not a record"
> 
> Maurizio
> 
>>
>>>
>>> * this might be a spec issue - there seems to be no support for 
>>> varargs extraction - e.g. if a record is varargs:
>>>
>>> record Foo(int... is) { }
>>>
>>> shouldn't I be able to do this:
>>>
>>> if (o instanceof Foo(int i1, int i2, int i3)) { ... }
>>
>> I believe the spec does not currently allow this, although I assume 
>> there will be discussions about features like this in the future.
> Ok
>>
>>>
>>>
>>> Some more detailed comments below:
>>>
>>> * Attr.java - verifyCastable seems very similar to 
>>> Check.checkCastable, except for non-reifiable types - should we try 
>>> to unify a bit?
>>>
>>> * Attr.java - not sure I get the logic by which target type is 
>>> propagated or not in visitDeconstructionPattern. Seems to me that the 
>>> only case where you need propagation is when you have a nested 
>>> component of the kind "var x" - in which case the type is determined 
>>> by the expected record component type. But this:
>>>
>>> + boolean nestedIsValidPattern = 
>>> !nestedPatterns.head.hasTag(BINDINGPATTERN) ||
>>> + ((JCBindingPattern) nestedPatterns.head).vartype == null;
>>>
>>> Seems to point to a slightly different direction  - e.g. target type 
>>> is propagated even if the nested is a deconstruction pattern, but 
>>> then I don't see any use of resultInfo inside this visitor?
>>
>> Probably a remainder of any patterns. I've cleaned this up.
>>
>>>
>>> * parser changes - looks good :-)
>>>
>>> * TransPattern - as usual, some comments illustrating basic desugared 
>>> shapes would be nice
>>
>> Done.
>>
>> Thanks,
>>     Jan
>>
>>>
>>> Maurizio
>>>
>>> On 19/03/2020 17:25, Jan Lahoda wrote:
>>>> Hi,
>>>>
>>>> Turned out the patch had two bugs - one related to the owners of the 
>>>> temporary variables created, which then broke type annotations; and 
>>>> another that broken de-duplication. I am deeply sorry for that. An 
>>>> updated webrev is here:
>>>> http://cr.openjdk.java.net/~jlahoda/8240998/webrev.01/
>>>>
>>>> A delta from previous round:
>>>> http://cr.openjdk.java.net/~jlahoda/8240998/webrev.delta.00.01/
>>>>
>>>> I am sorry for any inconvenience.
>>>>
>>>> Jan
>>>>
>>>> On 18. 03. 20 9:55, Jan Lahoda wrote:
>>>>> Hi,
>>>>>
>>>>> I would like to ask for a review for a patch that implements the 
>>>>> deconstruction patterns, as described in JEP 375:
>>>>> https://bugs.openjdk.java.net/browse/JDK-8235186
>>>>>
>>>>> The current specification draft is here:
>>>>> http://cr.openjdk.java.net/~gbierman/jep375/jep375-20200316/specs/patterns-instanceof-jls.html 
>>>>>
>>>>>
>>>>> For this phase, the proposal is for javac to desugar the 
>>>>> deconstruction patterns using record accessors.
>>>>>
>>>>> The CSR for this change is being written here:
>>>>> https://bugs.openjdk.java.net/browse/JDK-8240999
>>>>>
>>>>> The proposed patch:
>>>>> http://cr.openjdk.java.net/~jlahoda/8240998/webrev.00
>>>>>
>>>>> JBS: https://bugs.openjdk.java.net/browse/JDK-8240998
>>>>>
>>>>> Any feedback is welcome!
>>>>>
>>>>> Thanks,
>>>>>      Jan