RFR: 8294942: Compiler implementation for Record Patterns (Second Preview) [v3]

[Vicente Romero]

On Mon, 7 Nov 2022 10:48:50 GMT, Jan Lahoda <jlahoda at openjdk.org> wrote:

>> This is a partial implementation of [JEP 432: Record Patterns (Second Preview)](https://openjdk.org/jeps/432) and [JEP 433: Pattern Matching for switch (Fourth Preview)](https://openjdk.org/jeps/433). Namely, it implements:
>> 
>>  - removal of named record patterns
>>  - (preview) type inference for type test and record patterns
>>  - cleaner switch case specification (e.g. no combination of `null` constants and type test patterns)
>>  - fixing exhaustiveness of certain switches
>> 
>> The patch does not contain support for record patterns in enhanced for statements, that is [part of a separate pull request](https://github.com/openjdk/jdk/pull/10798).
>> 
>> For more information on the changes please see:
>>  - the JEPs: [JEP 432](https://openjdk.org/jeps/432) and [JEP 433](https://openjdk.org/jeps/433)
>>  - the CSRs: [JEP 432 - JDK-8294944](https://bugs.openjdk.org/browse/JDK-8294944) and [JEP 433 - JDK-8294946](https://bugs.openjdk.org/browse/JDK-8294946)
>>  - the current [specification draft](http://cr.openjdk.java.net/~gbierman/jep432%2b433/jep432%2b433-20221028/specs/patterns-switch-record-patterns-jls.html)
>> 
>> Current total specdiff for both this PR and [the enhanced for PR](https://github.com/openjdk/jdk/pull/10798) is [here](http://cr.openjdk.java.net/~jlahoda/8294945/specdiff.preliminary.00/overview-summary.html).
>> 
>> Any feedback is welcome.
>> 
>> Thanks!
>
> Jan Lahoda has updated the pull request incrementally with two additional commits since the last revision:
> 
>  - Reflecting review comment.
>  - Apply suggestions from code review
>    
>    Co-authored-by: Maurizio Cimadamore <54672762+mcimadamore at users.noreply.github.com>

src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Infer.java line 677:

> 675:         Type exprType = c.asUndetVar(expressionTypeCaptured);
> 676: 
> 677:         capturedWildcards.forEach(s -> ((UndetVar) c.asUndetVar(s)).setNormal());

could be a minor issue but still: it doesn't feel correct to set an UndetVar created from a captured type as normal. I think that we should either create a TypeVar with the info from the CapturedType and then create a `normal` UndetVar with that TypeVar or we should create another category inside `enum Kind`:


enum Kind {
    NORMAL,
    CAPTURED,
    THROWS;
}

`MODIFIABLE_CAPTURED` dunno, just my opinion

src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Infer.java line 681:

> 679:         try {
> 680:             //step 2:
> 681:             if (commonSuperWithDiffParameterization(patternType, exprType)) {

I think this is not correct according to the spec. The spec says:

If T' is a parameterization of a generic class G, and there exists a supertype of R<α1, ..., αn> that is also a parameterization of G, let R' be that supertype...

the current code could look for any common supertype even if that common supertype is not G but a super type of G that is also common to R. More than that the current code will look for all common parameterized supertypes and create bonds etc. I think the right thing to do here according to the spec is:

           if (!types.isSameType(types.asSuper(patternType, exprType.tsym), exprType)) {
                return null;
            }

unless I'm missing something here

src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Infer.java line 689:

> 687:             doIncorporation(c, types.noWarnings);
> 688: 
> 689:             while (c.solveBasic(varsToSolve, EnumSet.of(InferenceStep.EQ)).nonEmpty()) {

if `varsToSolve` is equal to the number of variables in the inference context this step is a no-op and I think this should be always the case, so I think that the real resolution is happening below when `instantiatePatternVars` is invoked, unless I'm missing something this while loop could be safely removed

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

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