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

[Jan Lahoda]

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/

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

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.

> 
> * 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.

> 
> 
> 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