RFR: 8339296: Record deconstruction pattern in switch fails to compile

[Maurizio Cimadamore]

On Fri, 13 Sep 2024 09:23:12 GMT, Jan Lahoda <jlahoda at openjdk.org> wrote:

> Consider code like this:
> 
>     int nestedSwitchesInArgumentPosition(Object o1) {
>         return id(switch (o1) {
>             case R(var o2) -> switch (o2) {
>                 case R(String s) -> s;
>                 default -> "n";
>             };
>             default -> "";
>         });
>     }
> 
>     int id(String s) {
>         return s.length();
>     }
> 
>     int id(int i) {
>         return i;
>     }
> 
> 
> Compiling this fails with a `StackOverflowError`, because:
>  - there are speculative attribution happening for the switch expressions,
>  - "completes normally" is computed during these speculative attribution, which have parts of the AST unfilled - specifically the nested `case R(String s)`
>  - so, `Attr.PostAttrAnalyzer` fills in the missing types. In particular, the `String s` binding will get the `Symtab.unknownType`.
>  - `Flow.makePatternDescription` will eventually ask `Types.isSubtype(..., unknownType)`. This is guaranteed to fail with the `StackOverflowError`, as:
>  - `unknownType.isPartial()` returns `true`, so `Types.isSubtype(t, s)` (`s` is the `unknownType`) calls `Types.isSuperType(s, t)`, and `Types.isSuperType(s, t)` does not contain any special handling for the `unknownType`, so it will delegate to `Types.isSubtype(t, s)`, leading to an infinite recursion.
> 
> It may be possible to side-step the issue by not computing the completes normally property during speculative attribution, or move that computation outside of `Attr`. It may be good to do, for performance reasons.
> 
> But, that does not seem to solve the underlying issue with `unknownType`. Many methods in `Types` misbehave in weird ways when the `unknownType` is passed to them.
> 
> The proposal herein is to attempt to resolve that. In particular, the `UnknownType` is proposed to extend the `ErrorType`, dropping the (internal) `UNKNOWN` type tag. The uses of the `UNKNOWN` type tag appear to be equivalent to handling of the `ERROR` type tag anyway. And the special handling of the `unknownType` appear to usually use ` == syms.unknownType`:
> https://github.com/openjdk/jdk/blob/0c36177fead8b64a4cee9da3c895e3799f8ba231/src/jdk.compiler/share/classes/com/sun/tools/javac/code/Types.java#L904
> 
> After this change, the `unknownType` should behave as an erroneous type, unless specifically requested otherwise.
> 
> The intent is that the public API behavior for the `unknownType` should remain the same.

src/jdk.compiler/share/classes/com/sun/tools/javac/code/Symtab.java line 475:

> 473: 
> 474:         unknownSymbol = new ClassSymbol(PUBLIC|STATIC|ACYCLIC, names.fromString("<any?>"), null, rootPackage);
> 475:         // Create the unknown type

More general question: is the distinction between unknown and erroneous even relevant? It seems that an unknown type is just an erroneous type whose "class" happens to be the unknown symbol class. 

Can we just say:


        unknownSymbol = new ClassSymbol(PUBLIC|STATIC|ACYCLIC, names.fromString("<any?>"), null, rootPackage);
        unknownType = unknownSymbol.type = new ErrorType(unknownSymbol);


Is the only difference what the public `getTypeKind` will report? Do we care if there's a difference there?

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

PR Review Comment: https://git.openjdk.org/jdk/pull/20990#discussion_r1778652300