RFR: 8320220: Compilation of cyclic hierarchy causes infinite recursion

[Archie Cobbs]

On Mon, 24 Feb 2025 15:21:24 GMT, Vicente Romero <vromero at openjdk.org> wrote:

>> This input currently causes an infinite loop:
>> 
>> interface A extends B, C {}
>> interface B extends A {}
>> interface C extends A {}
>> 
>> However, less complicated cycles are handled properly.
>> 
>> When a cycle is found, we currently:
>> (a) Emit a warning; and
>> (b) Set the symbol's type to the error type.
>> 
>> These two steps are done in `Check.noteCyclic()`.
>> 
>> Step (b) is what normally prevents the infinite loop from happening later in the compilation. But we only do this for the first class in the loop, presumably because it would be too verbose to do (a) for every class in the loop. But that means we're also only doing (b) for the first class in the loop.
>> 
>> In more complicated scenarios like the bug example, that means some classes in the cycle can escape without (b) being applied. But this is incorrect (or, at least, weirdly indeterminate) because a loop is a loop no matter which class you start with.
>> 
>> So the solution is to continue to do (a) only to the first class in the cycle but do (b) for every class in the cycle.
>
> src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Check.java line 2380:
> 
>> 2378:                 seenClasses.stream()
>> 2379:                   .filter(s -> !s.type.isErroneous())
>> 2380:                   .filter(ClassSymbol.class::isInstance)
> 
> wouldn't a ClassSymbol method be better here like ClassSymbol::isSubClass?

> wouldn't a ClassSymbol method be better here like ClassSymbol::isSubClass?
> ...
> this method will invoke Class::isInstance again, probably better to just do (c -> (ClassSymbol)c)?

My goal was to replicate the existing logic, but do it for every class in `seenClasses` instead of just the current one. But I was not able to prove to myself that every symbol in `seenClasses` is in fact a `ClassSymbol`, although that may actually be true in practice. So that's why the code is being careful not to blind casting everything in `seenClasses` to `ClassSymbol`.

If you're confident that every symbol in `seenClasses` is always a `ClassSymbol` then we can simplify this... but if so, why wasn't `seenClasses` declared as a `Set<ClassSymbol>` instead of a `Set<Symbol>` in the first place? I'm wondering there is some weird (probably invalid) input that could cause that assumption to be violated.

Thanks for taking a look.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/23704#discussion_r1967927130