RFR: 8324809: compiler can crash with SOE while proving if two recursive types are disjoint

[Maurizio Cimadamore]

On Sat, 11 May 2024 22:18:12 GMT, Vicente Romero <vromero at openjdk.org> wrote:

> javac is crashing with SOE while compiling code like:
> 
> 
> class Criteria<B extends Builder<? extends Criteria>> {
>     public <D extends Builder<E>, E extends Criteria<D>> D builder() {
>         return (D) new Builder<>();
>     }
> 
> }
> 
> class Builder<C extends Criteria<? extends Builder<C>>> {}
> 
> 
> here while attributing: `return (D)new Builder<>();` the compiler is trying to prove that: `Builder<C>` is castable to `D`, then as the upper bound of `D` is `Builder<E>`, we need to check if `Builder<C>` is castable to `Builder<E>`. Then the next step is to try to check if type variables C and E are disjoint. During this process is when the compiler gets out of resources given that
> 
> E <: Criteria<D>
> D <: Builder<E> here we have E again and we start over in an infinite loop,
> 
> Here the proposal is to detect cycles in the type arguments graph and accept these type of casts if we find a cycle and thus can't prove if two types are disjoint,
> 
> TIA

src/jdk.compiler/share/classes/com/sun/tools/javac/code/Types.java line 4738:

> 4736:         boolean rewriteTypeVars;
> 4737:         // map to avoid visiting same type argument twice, like in Foo<T>.Bar<T>
> 4738:         Map<Type, Type> argMap = new HashMap<>();

Do we need to use both a map and a set, or can we just use the map?

src/jdk.compiler/share/classes/com/sun/tools/javac/code/Types.java line 4784:

> 4782:         @Override
> 4783:         public Type visitTypeVar(TypeVar t, Void s) {
> 4784:             if (seen.add(t)) {

IIRC, this code is called by `Types.isCastable`, in an attempt to do some fancy logic (outside the spec) which allow to determine whether two types are disjoint or not. To do this, the code attempts to rewrite occurrences of type-variables using wildcards. So, in some cases the logic fails with SOE - so that result should lead to the consequence that we don't know whether the types are disjoint or not, but, since we're outside the scope of the spec, we need to treat them as _not_ disjoint (e.g. the cast should be possible and we gave up proving that it should NOT be possible).

So, I'm afraid that `return t` here is going to make this method stricter than intended, which will lead javac to think that the types are in fact disjoint, and that will result in a compiler error. I wonder if we should we return a `?` instead?

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

PR Review Comment: https://git.openjdk.org/jdk/pull/19194#discussion_r1603718186
PR Review Comment: https://git.openjdk.org/jdk/pull/19194#discussion_r1603715888