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

[Aggelos Biboudis]

On Mon, 31 Oct 2022 08:52:45 GMT, Jan Lahoda <jlahoda at openjdk.org> wrote:

>> Aggelos Biboudis has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   Fix applicability bug
>
> 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`.

Very good point! Added it as a stand alone test too!

> 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 `(`.

I used what you propose when we need to initialise a variable, but kept the `getDeclarationKind()` method call when a variable is not needed (e.g. in assertions). Testing that way is visually clearer I think.

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

I reverted this but kept the necessary cast.

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

Addressed!

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

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