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.

The approach seems good. I left some modelling questions. As you notice, I'm skeptical that just handling unknown types better is going to be the last time we see issues like this. E.g. the switch code in `Attr` seems to be doing way too much when we're in speculative mode.

src/jdk.compiler/share/classes/com/sun/tools/javac/code/Type.java line 2424:

> 2422:         }
> 2423: 
> 2424:         @Override @DefinedBy(Api.LANGUAGE_MODEL)

Modelling-wise, I'm not sure. It almost seems as if `UnknownType` should be subclassed by `ErrorType` and not the other way around. Is there a specific reason you went for this?

test/langtools/tools/javac/types/UnknownTypeTest.java line 46:

> 44: import java.util.Objects;
> 45: 
> 46: public class UnknownTypeTest extends TypeHarness {

Nice test!

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

PR Review: https://git.openjdk.org/jdk/pull/20990#pullrequestreview-2333548497
PR Review Comment: https://git.openjdk.org/jdk/pull/20990#discussion_r1778507224
PR Review Comment: https://git.openjdk.org/jdk/pull/20990#discussion_r1778505062