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

[Jan Lahoda]

On Fri, 27 Sep 2024 13:41:15 GMT, Maurizio Cimadamore <mcimadamore 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?

Internally, after this patch, the distinction is minimal, and usually checked using `== unknownType`. Whether we can change the external behavior not sure. We can try.

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

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