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

[Maurizio Cimadamore]

Looks good to me

Maurizio

On 27/03/2020 10:35, Jan Lahoda wrote:
> 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