eclipse-jdt / eclipse.jdt.core

Eclipse Public License 2.0
164 stars 130 forks source link

[compiler] Compile error with Eclipse, but not with javac #273

Open jubax opened 2 years ago

jubax commented 2 years ago

The following code compiles with javac, but not with eclipse:

import java.util.ArrayList;
import java.util.List;

public class Instanceof<T> {

    interface A<T> {
    //
    }

    interface B<T> extends A<T> {
    //
    }

    interface Foo<T> {
    A<T> getX();
    }

    public void test() {
    List<? extends A<T>> list = new ArrayList<>();

    // Compile error in the following line: 
    //   "Type capture#1 of ? extends Instanceof.A<T> cannot be safely cast to Instanceof.A<T>
    if (list.get(0) instanceof B<T>) { // <== compile error with Eclipse, but not with javac
        // do something
    }

    // No compile error
    A<T> item = list.get(0);
    if (item instanceof B<T>) {
        // do something
    }

    }
}

Is there a workaround without changing the code?

stephan-herrmann commented 2 years ago

This code uses a feature introduced in our implementation via https://bugs.eclipse.org/bugs/show_bug.cgi?id=562392.

Perhaps @jarthana can provide some insights?

forax commented 1 year ago

One of my student stumble on the same issue yesterday but with an instanceof and a type pattern.

The JLS section 15.20.2 [1] says The RelationalExpression must be downcast compatible with the ReferenceType (5.5 [1]), or a compile-time error occurs.

i.e. expr instanceof B is allowed if (B) expr is allowed.

The problem is that ecj considers that it's an unsafe cast List<? extends A> list = new ArrayList<>(); var v = (B) list.get(0); <-- unsafe cast

But it should not.

[1] https://docs.oracle.com/javase/specs/jls/se19/html/jls-5.html#jls-5.5

jarthana commented 1 year ago

Will consider this for 4.27.

forax commented 1 year ago

Thanks Jay !

jarthana commented 1 year ago

OK, I see the mistake i did when introducing the type parameters in instanceof expresssion: I should have gone with the same error message that we used for unsafeCast rather than introducing a new one. The new one is not configurable and will always result in an error. I will fix this shortly.

@stephan-herrmann I was thinking of picking some of the code from CastExpression for this (around line number 655 inside if (isLegal) { and noticed some null annotation related code. Should we do the same for instanceof expression as well? If it's simply a matter of copy-pasting or just making the code common for reuse, I could include that as well in my patch.

jarthana commented 1 year ago

I am thinking of splitting these two conditions with the latter being used for issuing the unsafe cast warning:

if (!isLegal || (this.bits & ASTNode.UnsafeCast) != 0) {

@stephan-herrmann Stephan, I am a bit baffled by this testcase:

public void foo(A<? extends T> obj) { if (obj instanceof B<T> ) { // } }

Javac issues an error here and with my above fix, we will only get the warning. I am using the existing checkCastTypesCompatibility() for this check. Is this expected that this method returns true for this code?

iloveeclipse commented 1 year ago

I've unset 4.27 milestone because it is too late for 4.27. Please consider to set a new one if you plan to contribute to 4.28.

jarthana commented 1 year ago

Sorry, couldn't complete this in 4.27. Will keep it on my radar for 4.28.