[Sealed types] Regression in instanceof check for sealed generic classes

[nlisker]

This one is a bit tricky as there are several cases to look at in 4.32 and 4.33 (release verions).

In the following snippet there are 3 sealed generic superclasses with 2 final subclasses. In case 1, Maybe, one of the subclasses implements an interface; in case 2, SurelyNot, none implement; and in case 3, SurelyYes, both implement. Then each class is checked if it's instanceof that interface:

interface SuperInt {}

abstract sealed class Maybe<N extends Number> {
	final class Maybe1 extends Maybe<Long> {}
	final class Maybe2 extends Maybe<Long> implements SuperInt {}
}


abstract sealed class SurelyNot<N extends Number> {
	final class SurelyNot1 extends SurelyNot<Long> {}
	final class SurelyNot2 extends SurelyNot<Long> {}
}

abstract sealed class SurelyYes<N extends Number> {
	final class SurelyYes1 extends SurelyYes<Long> implements SuperInt {}
	final class SurelyYes2 extends SurelyYes<Long> implements SuperInt {}
}

class Test {

	void testMaybe(Maybe<?> maybe, SurelyNot<?> surelyNot, SurelyYes<?> surelyYes) {
		if (maybe == null || surelyNot == null || surelyYes == null) return;
		if (maybe instanceof SuperInt sup) {}
		if (surelyNot instanceof SuperInt sup) {}
		if (surelyYes instanceof SuperInt sup) {}
	}
}

What I would expect:

• If the compiler is not trying to be smart (and not specified to be), then all 3 should compile.
• If the compiler is smart, then the 1st case will compile and for the other 2 it will tell me that the result is already known - false for the 2nd case and true for the 3rd (unless it's me who is not smart). This can be either a warning or an error from my point of view, but maybe it's specified.

What happens:
In 4.32 everything compiles normally with no warning. This is correct if the compiler doesn't need to be smart.
In 4.33 all 3 give an error: Incompatible conditional operand types Maybe<capture#4-of ?> and SuperInt etc.

Variations

• Removing the null checks slightly changes the analysis since now SurelyYes is like Maybe (null doesn't implement the interface). There is no difference in the behavior.
• Removing the generic parameter from just one of the subclasses:

	final class Maybe2 extends Maybe implements SuperInt {}
	//...
	final class SurelyNot2 extends SurelyNot {}
	// ...
	final class SurelyYes2 extends SurelyYes implements SuperInt {}
}

removes the error from cases 1 and 3, but leaves it on 2.

• Removing sealed from all 3 removes the error in all 3.

[srikanth-sankaran]

javac 23 reports:

X.java:24: error: incompatible types: SurelyNot<CAP#1> cannot be converted to SuperInt
		if (surelyNot instanceof SuperInt sup) {}
		    ^
  where CAP#1 is a fresh type-variable:
    CAP#1 extends Number from capture of ?
1 error

[srikanth-sankaran]

Ah. See #3038

[srikanth-sankaran]

@nlisker - you must have also run into #3122 which I filed when playing with the test case for this ticket - that issue is resolved already while the present ticket is still being worked on for 4.34 M3.

Sealed types implementation in Eclipse compiler is being completely overhauled and many problems are being fixed. I welcome you test recent builds and report defects if any. In any case when M3 arrives. Thanks.

[nlisker]

Possibly I did. If an error disappears after saving/cleaning/rebuilding, I tend to let it off the hook. Sometimes the types of errors that disappear after saving are due to the complexity of the project (with Maven/Gradle) and can't be reproduced with a small snippet, so it's not clear where the fault is.

I keep a minimal Eclipse installation that I update to nightly builds just for testing to see if a problem I hit with my production Eclipse is still viable "in the future".

By the way, as for the compiler's treatment of sealed types, I see that javac reports an error only for the SurelyNot case. It could be beneficial to at least give a warning for the SurelyYes case as well since the check is redundant (a bit like dead code). It could point to a flaw in the user's design, even if JLS does not spec it.

[srikanth-sankaran]

By the way, as for the compiler's treatment of sealed types, I see that javac reports an error only for the SurelyNot case. It could be beneficial to at least give a warning for the SurelyYes case as well since the check is redundant (a bit like dead code). It could point to a flaw in the user's design, even if JLS does not spec it.

See that null is not an instanceof any reference type - so your surelyYes parameter is not sure to be a SuperInt :)

Besides, a programmer way want to coalesce one type to another using instanceof and materialize a binding variable in the process, so I am not sure a warning would not be incorrect or not add noise value

[nlisker]

See that null is not an instanceof any reference type - so your surelyYes parameter is not sure to be a SuperInt :)

This is why I added a return if the instance is null - now there is no choice but to have one of the 2 instances. Specifically I wrote in the "variations" section:

Removing the null checks slightly changes the analysis since now SurelyYes is like Maybe (null doesn't implement the interface). There is no difference in the behavior.

Besides, a programmer way want to coalesce one type to another using instanceof and materialize a binding variable in the process, so I am not sure a warning would not be incorrect or not add noise value

I'm not really sure what this means, sounds like upcasting. However, the benefit of such a warning is not critical (akin to dead code), so if there are reasons not to do it, it's fine to leave it out.

[srikanth-sankaran]

See that null is not an instanceof any reference type - so your surelyYes parameter is not sure to be a SuperInt :)

This is why I added a return if the instance is null - now there is no choice but to have one of the 2 instances. Specifically I wrote in the "variations" section:

Removing the null checks slightly changes the analysis since now SurelyYes is like Maybe (null doesn't implement the interface). There is no difference in the behavior.

Right, I saw those observations when the defect report came in, but it faded from my memory when I wrote the recent comment.

There is no null analysis based flow analysis that is defined in the language that is worth speaking of and while JDT has some infrastructure in place that has various limitations. See #3088 (https://bugs.eclipse.org/bugs/show_bug.cgi?id=538421 and its 28 duplicates(!))

I'm not really sure what this means, sounds like upcasting. However, the benefit of such a warning is not critical (akin to dead code), so if there are reasons not to do it, it's fine to leave it out.

[srikanth-sankaran]

What I would expect:

• If the compiler is not trying to be smart (and not specified to be), then all 3 should compile.
• If the compiler is smart, then the 1st case will compile and for the other 2 it will tell me that the result is already known - false for the 2nd case and true for the 3rd (unless it's me who is not smart). This can be either a warning or an error from my point of view, but maybe it's specified.

See 5.1.6.1 Allowed Narrowing Reference Conversion

...

A class or interface is disjoint from another class or interface if it can be determined
statically that they have no instances in common (other than the null value). The
rules for disjointess are as follows:
• A class named C is disjoint from an interface named I if (i) it is not the case that
C <: I, and (ii) one of the following cases applies:
– C is final.
– C is sealed, and all of the permitted direct subclasses of C are disjoint from I.
– C is freely extensible (§8.1.1.2), and I is sealed, and C is disjoint from all of
the permitted direct subclasses and subinterfaces of I.
• An interface named I is disjoint from a class named C if C is disjoint from I.
• A class named C is disjoint from another class named D if (i) it is not the case
that C <: D, and (ii) it is not the case that D <: C.
• An interface named I is disjoint from another interface named J if (i) it is not
that case that I <: J, and (ii) it is not the case that J <: I, and (iii) one of the
following cases applies:
– I is sealed, and all of the permitted direct subclasses and subinterfaces of I
are disjoint from J.
– J is sealed, and I is disjoint from all the permitted direct subclasses and
subinterfaces of J.

It is in fact in implementing this section of the JLS for #2634 that we "progressed" by fixing #2595 but "regressed" in handling the present test scenario.

However, prior to #2634, we were correct only by accident.

[srikanth-sankaran]

This program compiles with JDK23 - but that looks highly suspicious:

interface SuperInt {}

class Outer<T> {
    abstract sealed class Maybe<N extends Number> {
        final class Maybe1 extends Outer<Test>.Maybe<Long> implements SuperInt {}
    }
}

class Test {

    void testMaybe(Outer<String>.Maybe<?> maybe) {
        if (maybe == null) return;
        if (maybe instanceof SuperInt sup) {}
    }
}

How can there be an instance of Maybe1 that is an instanceof Outer<String>.Maybe<?> while Maybe expressly extends an incompatible parameterized supertype ??

@stephan-herrmann - What is your take ??