RFR: 8302344: Compiler Implementation for Unnamed patterns and variables (Preview) [v4]

Fri Apr 28 09:46:53 UTC 2023

On Thu, 27 Apr 2023 14:56:53 GMT, Aggelos Biboudis <abimpoudis at openjdk.org> wrote:

>> This PR implements [JEP 443](https://openjdk.org/jeps/443), the preview feature for Unnamed Patterns and Variables in Java.
>> 
>> Draft Spec: https://cr.openjdk.org/~abimpoudis/unnamed/latest/
>
> Aggelos Biboudis has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Update test files with JDK 8307007 bug id

src/jdk.compiler/share/classes/com/sun/tools/javac/code/Source.java line 240:

> 238:         RECORD_PATTERNS(JDK21, Fragments.FeatureDeconstructionPatterns, DiagKind.PLURAL),
> 239:         WARN_ON_ILLEGAL_UTF8(MIN, JDK21),
> 240:         UNNAMED_VARIABLES(JDK21, Fragments.FeatureUnnamedVariables, DiagKind.PLURAL),

While the JEP unifies unnamed variables and unnamed patterns, would it make sense, at the code level, to split between the two?

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

> 1775:                                 log.error(guard.pos(), Errors.GuardHasConstantExpressionFalse);
> 1776:                             }
> 1777:                         } else if (guard != null) {

I guess this is caused by something like:

case _ when ...

If now a guard can appear regardless of how many labels there are, should be refactor this code so that if `guard != null` it is always attributed? And then we do other stuff in case `labels.tail.isEmpty()` ? Note that the code for attributing the guard is slightly different in the two cases in this patch: the original code creates a nested scope - while the new code doesn't.

src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Flow.java line 746:

> 744: 
> 745:         sealed interface PatternDescription {
> 746:             public static PatternDescription from(Types types, Type selectorType, JCPattern pattern, Symtab syms) {

Optional comment: the fact that this factory needs so much compiler state makes me think that perhaps a better place for it would be as an instance method of Flow, rather than a static method in PatternDescription.

src/jdk.compiler/share/classes/com/sun/tools/javac/comp/MemberEnter.java line 295:

> 293:                 : tree.vartype.type;
> 294:         Name name;
> 295:         if (Feature.UNNAMED_VARIABLES.allowedInSource(source) && (tree.mods.flags & UNNAMED) != 0) {

Not 100% sure what this code does. E.g. shouldn't we just set the name of the variable as the name stored in the tree? It seems more economical if we could just set in the parser the name as empty (if preview features are enabled etc.) - and then all these special cases can disappear. After all, you still have the flag (and even then, do we need a flag? Can't we just see whether the name is empty to see that it's unnamed?)

src/jdk.compiler/share/classes/com/sun/tools/javac/parser/JavacParser.java line 826:

> 824:                 //type test pattern:
> 825:                 UnnamedDetails result = identOrFlagUnderscore(mods);
> 826:                 JCVariableDecl var = toP(F.at(result.varPos()).VarDef(mods, result.name(), e, null));

Do you think we can delegate to `variableDeclarator` here?

src/jdk.compiler/share/classes/com/sun/tools/javac/parser/JavacParser.java line 839:

> 837:     }
> 838: 
> 839:     private UnnamedDetails identOrFlagUnderscore(JCModifiers mods) {

I'm not 100% sure this is required - the main use seems to be in `variableDeclarator` - but in that method you already have a token to look at, and a set of modifiers that you can augment - so why not just doing everything in that method (instead of preprocessing it?)

src/jdk.compiler/share/classes/com/sun/tools/javac/parser/JavacParser.java line 3555:

> 3553:      */
> 3554:     JCVariableDecl variableDeclarator(JCModifiers mods, JCExpression type, boolean reqInit, Comment dc, boolean localDecl) {
> 3555:         UnnamedDetails result = identOrFlagUnderscore(mods);

Why do we need to get the "unnamed details" here, instead of letting the code flow to `variableDeclaratorRest` and do it there?

src/jdk.compiler/share/classes/com/sun/tools/javac/parser/JavacParser.java line 3660:

> 3658:         int pos = token.pos;
> 3659:         Name name;
> 3660:         if (allowThisIdent ||

I note that now the error re. underscore on lambda is no longer given even if preview features are disabled. But I guess that's ok. (instead you will probably get an error that unnamed parameters are a preview feature)

test/langtools/tools/javac/TryWithResources/TwrLintUnderscore.java line 22:

> 20:         return;
> 21:     }
> 22: }

Add a newline at the end of this file (also check others)

test/langtools/tools/javac/patterns/Unnamed.java line 148:

> 146:     }
> 147: 
> 148:     // JEP

This comment looks odd - either we also add JEP number, or we can just drop it.

test/langtools/tools/javac/patterns/Unnamed.java line 153:

> 151:     record ColoredPoint(Point p, Color c) { }
> 152: 
> 153:     void jep(ColoredPoint r) {

Same as above for this method name

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

PR Review Comment: https://git.openjdk.org/jdk/pull/13528#discussion_r1180130293
PR Review Comment: https://git.openjdk.org/jdk/pull/13528#discussion_r1180139794
PR Review Comment: https://git.openjdk.org/jdk/pull/13528#discussion_r1180158709
PR Review Comment: https://git.openjdk.org/jdk/pull/13528#discussion_r1180153920
PR Review Comment: https://git.openjdk.org/jdk/pull/13528#discussion_r1180123845
PR Review Comment: https://git.openjdk.org/jdk/pull/13528#discussion_r1180124901
PR Review Comment: https://git.openjdk.org/jdk/pull/13528#discussion_r1180125696
PR Review Comment: https://git.openjdk.org/jdk/pull/13528#discussion_r1180128207
PR Review Comment: https://git.openjdk.org/jdk/pull/13528#discussion_r1180164818
PR Review Comment: https://git.openjdk.org/jdk/pull/13528#discussion_r1180168228
PR Review Comment: https://git.openjdk.org/jdk/pull/13528#discussion_r1180168421