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

[Maurizio Cimadamore]

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.


> 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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.java.net/pipermail/compiler-dev/attachments/20200325/6558eeb3/attachment.htm>