RFR: 8294943: Implement record patterns in enhanced for [v2]

[Jan Lahoda]

On Wed, 26 Oct 2022 13:22:28 GMT, Aggelos Biboudis <abimpoudis at openjdk.org> wrote:

>> This PR enables the ability to use record patterns in the enhanced-for, initializing the record components accordingly: 
>> 
>> 
>> record Complex(double real, double img) {}
>> 
>> List<Complex> list = ...;
>> 
>> for (Complex(var real, var img) : list) {
>>     // can use “real” and “img” directly
>> }
>> 
>> 
>> This PR proposes an implementation for the "Record Patterns in Enhanced For" [subtask](https://bugs.openjdk.org/browse/JDK-8294943) regarding the following [CSR](https://bugs.openjdk.org/browse/JDK-8294944) (note the different JBS entries), summarised by the following:
>> 
>> - It enhances the grammar for the `EnhancedForStatement` to support record patterns too, alongside `LocalVariableDeclarations`.
>> - Any pattern variables introduced by the record pattern in the header of the pattern are definitely matched in the statement block of the enhanced for. 
>> - The record patterns are only permitted when the pattern is exhaustive over the enhanced for's expression.
>> - In the case that the element of the iteration is `null`, the switch raises a `MatchException` wrapping the `NullPointerException`.
>> - The enhanced for, supports record patterns for both arrays and reference types.
>> 
>> Currently, the precise meaning of the enhanced for statement is given by translation into a basic for statement. By introducing record patterns in the pattern header, the new meaning is defined by the new translation which incorporates a switch whose selector expression is the enhanced for's expression, and whose singleton case has the given record pattern as a sole label would be exhaustive. Note, that in cases where the imaginary switch would reach the default clause and end abruptly, the enhanced for each will end abruptly for the same reason.
>> 
>> For more information on the changes please see:
>> 
>> - the JEP: [JEP 432](https://openjdk.org/jeps/432) 
>> - the CSR: [JEP 432 - JDK-8294944](https://bugs.openjdk.org/browse/JDK-8294944) 
>> - the current [specification draft](https://cr.openjdk.java.net/~gbierman/jep432%2b433/jep432+433-20221018/specs/patterns-switch-record-patterns-jls.html#jls-14.14.2) 
>> 
>> Looking forward for your review.
>
> Aggelos Biboudis has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Fix applicability bug

Overall, seems good to me. Some comments/adjustments are inline. Thanks!

src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Analyzer.java line 428:

> 426:         @Override
> 427:         boolean match(JCEnhancedForLoop tree){
> 428:             Assert.check(tree.getDeclarationKind() == EnhancedForLoopTree.DeclarationKind.VARIABLE);

This assert is wrong - at the point the Analyzer is running, the enhanced for loop may use patterns Try, for example:

public class ForEachTest {
    private void test(Iterable<? extends R> l) {
        for (R(Object a) : l) {

        }
    }
    record R(Object a) {}
}


And run:

$ javac --enable-preview -source 20 -XDfind=all /tmp/ForEachTest.java 
Note: /tmp/ForEachTest.java uses preview features of Java SE 20.
Note: Recompile with -Xlint:preview for details.
An exception has occurred in the compiler (20-internal). Please file a bug against the Java compiler via the Java bug reporting page (https://bugreport.java.com) after checking the Bug Database (https://bugs.java.com) for duplicates. Include your program, the following diagnostic, and the parameters passed to the Java compiler in your report. Thank you.
java.lang.AssertionError
        at jdk.compiler/com.sun.tools.javac.util.Assert.error(Assert.java:155)
        at jdk.compiler/com.sun.tools.javac.util.Assert.check(Assert.java:46)
        at jdk.compiler/com.sun.tools.javac.comp.Analyzer$RedundantLocalVarTypeAnalyzerForEach.match(Analyzer.java:428)
...


Suggest to change the code of this method to:

            return tree.getDeclarationKind() == EnhancedForLoopTree.DeclarationKind.VARIABLE &&
                   !isImplicitlyTyped((JCVariableDecl) tree.varOrRecordPattern);


Note the asserts in the other methods here are OK, as those methods should not (AFAIK) be called when this method returns false.

Analyzer tests are under `test/langtools/tools/javac/analyzer`.

src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Attr.java line 1549:

> 1547:                 }
> 1548:             }
> 1549:             if(tree.getDeclarationKind() == EnhancedForLoopTree.DeclarationKind.VARIABLE) {

It should be possible to use `tree.varOrRecordPattern instanceof JCVariableDecl jcVariableDecl` as is done in `Flow`, right? (Dtto on other places.)

Nit: we usually put a space between `if` and `(`.

src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Lower.java line 3463:

> 3461:                                                     make.Ident(index)).setType(elemtype);
> 3462: 
> 3463:             JCBlock body = null;

Offhand, it seems there are unnecessary changes in this file (like this declaration of `JCBlock body`. Please go through the patch, and undo the unnecessary changes. Keeping/adding the `Assert.check` is good, of course.

src/jdk.compiler/share/classes/com/sun/tools/javac/comp/TransPatterns.java line 753:

> 751:                         make.at(tree.pos).VarDef(currentValue, null).setType(selectorType);
> 752: 
> 753:                 List<JCExpression> params = List.of(makeNull(), makeNull());

The specification says the exception should be `new MatchException(new NullPointerException())`, but the nested NPE seems to be missing here.

FWIW, I think it is OK, for now, to create an empty NPE, but we may be able to do something better after PR 9746.

-------------

Changes requested by jlahoda (Reviewer).

PR: https://git.openjdk.org/jdk/pull/10798