Bug 488328

Comment 1 Roberto Lublinerman 2016-02-23 13:00:28 EST

Slightly changing the example to 

public class Test2 {

  static class A<R> {
    class  I {
    }
  }

  public static <R> void m(A<R>.I instance, R generic) {
    System.out.println("called with A<R>.I");
  }

  public static void m(Integer s, Object o) {
    System.out.println("called with integer");
  }

  public static void main(String... args) {
    Long l = new Long(3);
    m(l, l);
  }
}

makes it fail under 4.4.2 as well. Under java 7 this code is correctly rejected in 4.4.2.

Comment 2 Stephan Herrmann 2016-02-23 13:32:25 EST

Thanks.

Here's what ecj -1.7 says:

        m(l, l);
        ^
The method m(Test2.A<R>.I, R) in the type Test2 is not applicable for the arguments (Long, Long)

javac reports similarly (across versions).

ecj -1.8 clearly is the odd one out, since neither of the 'm' methods is applicable.

Comment 3 Roberto Lublinerman 2016-02-23 14:04:53 EST

Yes. 

But in the case of the first example (the one in the original post) m(A.I, Object) is chosen incorrectly with -1.8 in 4.5 triggering a VerifyError.

The right overload m(long, Object) is chose with -1.7.

Comment 4 Stephan Herrmann 2016-02-23 17:32:05 EST

(In reply to Roberto Lublinerman from comment #3)
> Yes. 
> 
> But in the case of the first example (the one in the original post) m(A.I,
> Object) is chosen incorrectly with -1.8 in 4.5 triggering a VerifyError.

That sounds like a variant of the same problem: ecj -1.8 considers m(A.I,Object) as applicable where in fact it is not (this method overload is also the one wrongly chosen in comment 1).

Comment 5 Jesper Moller 2016-03-30 05:47:05 EDT

I'll have a look at this unless you object, Stephen?

Comment 6 Stephan Herrmann 2016-04-11 09:39:05 EDT

(In reply to Jesper Moller from comment #5)
> I'll have a look at this unless you object, Stephen?

No objection :)
Do you have a theory already, where things might be going wrong in ecj?

Comment 7 Jesper Moller 2016-04-11 09:41:58 EDT

Sorry, no, I haven't, and I won't be able to work on it this week.

Comment 8 Eclipse Genie 2018-04-12 04:45:44 EDT

New Gerrit change created: https://git.eclipse.org/r/121048

Comment 9 Manoj N Palat 2018-04-12 04:49:21 EDT

(In reply to comment #8)
> New Gerrit change created: https://git.eclipse.org/r/121048

Rationale: In the third level of reduction, we reach the Constraint Subtyping – sec 18.2.3, item 6, subsection 1 – where T is in an inner class of a parameterized type. Ie .. A1, ..., An be
the type arguments of T. Among the supertypes of S, a corresponding class
or interface type is identified, with type arguments B1, ..., Bn. If no such type
exists, the constraint reduces to false…
We were returning true in this case where the type arguments did not exist.  However, I believe we need to continue further and check whether T is in the SuperTypes of S and return accordingly though this case of zero arguments is not very explicitly stated. The patch pushes the return true if args null further down after the check. With the patch, in this specific case, the compatibility returns false which is the correct behavior.

@Stephan: Can you please review this?

Comment 10 Stephan Herrmann 2018-04-12 16:52:20 EDT

Well spotted, Manoj!

I totally agree, the phrase 
> "If no such type exists, the constraint reduces to false"
should be the first thing to check - before diving into type arguments.

Still, I recommend to check the following: addConstraintsFromTypeParameters() is called from within a while loop. While JLS contains a hint at types nested within a parameterized type, it doesn't explicitly require to check their type arguments, too. Software archeology (via branch sherrmann/NewTypeInference), however, leads me back to this post: http://mail.openjdk.java.net/pipermail/lambda-spec-experts/2013-December/000449.html
where I had asked
> "4) I made experiments with pulling E into the inference"
and Dan responded:
>"Yep, that's the right approach.  The 5.2 bullet is meant to cover this case, too."

I guess it's possible to construct compatible parameterized inner types, with incompatible enclosings. So we need to make sure that after the first iteration of the while() loop, failure to findSuperTypeOriginatingFrom() does not cause a 'false'. Do you concur?

Comment 11 Manoj N Palat 2018-04-13 07:33:18 EDT

(In reply to comment #10)
> Well spotted, Manoj!
> 
> I totally agree, the phrase
> > "If no such type exists, the constraint reduces to false"
> should be the first thing to check - before diving into type arguments.

Thanks Stephan for fast review!

> Still, I recommend to check the following: addConstraintsFromTypeParameters() is
> called from within a while loop. While JLS contains a hint at types nested
> within a parameterized type, it doesn't explicitly require to check their type
I agree - explicit requirement is not stated in JLS bordering a little vague in the case of nested type in a parameterized type.
> arguments, too. Software archeology (via branch sherrmann/NewTypeInference),
> however, leads me back to this post:
> http://mail.openjdk.java.net/pipermail/lambda-spec-experts/2013-December/000449.html
> where I had asked
> > "4) I made experiments with pulling E into the inference"
> and Dan responded:
> >"Yep, that's the right approach.  The 5.2 bullet is meant to cover this case,
> too."
Software archeology - nice term :) and I see that that discussion prompted changes to JLS 18.2.3 to talk about inner types
> 
> I guess it's possible to construct compatible parameterized inner types, with
> incompatible enclosings. So we need to make sure that after the first iteration
> of the while() loop, failure to findSuperTypeOriginatingFrom() does not cause a
> 'false'. Do you concur?
Well, I've been trying to get such a case - However, an additional constraint here is that superCandidate (essentially the enclosing type) needs to be paramaterized as well to get into the loop due to the condition - while (superCandidate != null && superCandidate.kind() == Binding.PARAMETERIZED_TYPE...
Was trying with different variants of the following:

public class X {
   static class A<R> {
     class  I<S> {
    }
  }

   class B extends A<Integer>{
	     class  J<S> extends A<S>.I<Integer> {
	    } 
	  } 
  public static <R> void m(A<Integer>.I<R> instance) {
    System.out.println("called with A<Integer>.I<R>");
  }

  public static void main(String... args) {
    B.J<Integer> l = null;
    m(l);
  }
}
At the first entry to  addConstraintsFromTypeParameters:
subCandidate :  B.J<Integer>
superCandidate: A<Integer>.I<R>

If "extends A<Integer>" is removed, then we have an incompatible type, but then that is a compilation error since we have  the A<S> in J's super class
Though, without the enclosing type parameterization it was possible to create a inner parameterized construct compatible with the corresponding inner one, with the enclosing type parameterization I am yet to get one (may be am missing something here) - and since parameterization of the enclosing type is a necessary condition for the loop entry, the cases without parameterization don't qualify.  So until such a case is found, not sure whether we need to check for the first time entry.

Comment 12 Manoj N Palat 2018-04-23 02:16:19 EDT

(In reply to comment #10)

> I guess it's possible to construct compatible parameterized inner types, with
> incompatible enclosings. So we need to make sure that after the first iteration
> of the while() loop, failure to findSuperTypeOriginatingFrom() does not cause a
> 'false'. Do you concur?
finally, yes :) could get the following test case that exhibit this pattern:
class A1<R> {
	class  I1<S> {}
}
public class Test<R> extends A1<R>{
	 class A2  {
		class I2 extends A1<R>.I1<R> {}
	}
  
  public static <R> void m(A1<R>.I1<R> instance) {
    System.out.println("called with A1<R>.I1<R>.I");
  }

  public static void main(String... args) {
	 Test<Integer>.A2.I2  l =  new Test<Integer>().new A2().new I2();
	 m(l);
  }
}       
javac passes this. Have modified the patch as suggested - patch in gerrit and under test.

Comment 13 Eclipse Genie 2018-04-23 05:02:12 EDT

Gerrit change https://git.eclipse.org/r/121048 was merged to [master].
Commit: http://git.eclipse.org/c/jdt/eclipse.jdt.core.git/commit/?id=747856a76606d57cb67c300aa2c06c7104b1d436

Comment 14 Stephan Herrmann 2018-05-08 08:18:17 EDT

The change as contained in 4.8 M7 looks good to me.

Good job :)

Comment 15 Jay Arthanareeswaran 2018-05-10 12:33:30 EDT

Verified for 4.8 M7 with build I20180509-2000