Comment 1 Philipe Mulet 2006-05-18 17:56:07 EDT

Please provide steps to reproduce.
Too late for RC5 anyway.

Comment 2 Philipe Mulet 2006-05-18 18:05:20 EDT

Please reopen once steps are available.

Comment 3 Missing name Mising name 2006-05-18 19:07:48 EDT

The problem seems to be caused by Eclipse not gracefully handling an inconsistent type hierarchy.  I can recreate the first internal error by defining the following classes and interfaces in separate files:

public class Child extends Parent<Object> {}
public abstract class Parent<T> extends Grandparent<T> implements IParent {}
public interface IParent<T> extends IGrandparent<T> {}
public abstract class Grandparent<T> implements IGrandparent<T> {}
public interface IGrandparent<T> {}

In this case Eclipse points out the inconsistent type hierarchy with a marker on Parent.java ("The interface IGrandparent cannot be implemented more than once with different arguments: IGrandparent<T> and IGrandparent").  (No such error was given in my original code - the only error was the internal compiler fault).

An error can be seen on "line 0" of Child.java as before:

Internal compiler error
java.lang.NullPointerException
	at org.eclipse.jdt.internal.compiler.lookup.MethodVerifier.computeInheritedMethods(MethodVerifier.java:507)
	:
	
The error in the source can be resolved by modifying the Parent to use type parameters in its interface implementation:

public abstract class Parent<T> extends Grandparent<T> implements IParent<T> {}

Making a similar correction in my project also stops this internal error.  I have been unable to reproduce the second stack trace, however.

Comment 4 Philipe Mulet 2006-05-19 03:15:45 EDT

The two cases are consequence of the same bug. An array of superinterfaces contain null values.

These are likely coming from the collision detected before (cannot implement twice the same interface in different parameterized form).

Comment 5 Philipe Mulet 2006-05-19 04:16:08 EDT

The problem comes from late removal of colliding superinterfaces from the type binding; at a stage where parameterized type instances already got created way before this (and did not get readjusted).

Given this change was made late, and also causes grief to other places, e.g. DOM AST; I believe we should reintroduce them back and improve the verifier to ignore them.

Comment 6 Philipe Mulet 2006-05-19 06:33:34 EDT

One thing to consider is having colliding superinterfaces being persisted in (problem) classfiles. This shouldn't be an issue, since through binary compatibility scenari, the same may occur with valid classfiles (i.e. forgot to recompile some supertype after a subclass change).

Tagging for 3.2.1. Though the situation is not ideal, this is a sensitive change to perform, and we need to check for ripples, even in dependent components (what about refactoring etc...). 
Currently, we have an internal exception when compiling incorrect code, resulting in an extra marker being created. We are not rejecting valid code, but rather are not optimally handling incorrect code. Still we want to address it asap, but I don't think it qualifies as a stop ship issue for 3.2.0.

Comment 7 Philipe Mulet 2006-05-19 06:58:26 EDT

Created attachment 42000 [details]
Proposed patch

The patch is no longer altering the binding superinterfaces as a side effect of the collision check.
It also defers the check until method verification occurs, since this is where the non colliding interface list is needed to avoid complaining unnecessarily about nameclash issues...

Comment 8 Philipe Mulet 2006-05-19 06:58:48 EDT

Kent - what do you think ?

Comment 9 Philipe Mulet 2006-05-19 07:00:15 EDT

Added GenericTypeTest#test0989-0992 + MethodVerifyTest#test088

Comment 10 Philipe Mulet 2006-05-19 07:04:57 EDT

Created attachment 42001 [details]
Better patch

Slight improvement, avoid unnecessary work when compliance < 1.5

Comment 11 Philipe Mulet 2006-05-19 12:17:19 EDT

Reassign to Kent. After discussion, we will make the fix a little bit better to reduce noise in case of collisions.
e.g.
import java.util.*;
public class X extends X2 {}
abstract class X2 extends X3 implements List<String> {}
abstract class X3 implements List<Thread> {}

here we shouldn't report any missing method implementation, until the collisions have been addressed. With RC4, we complain about missing List<Thread> methods, with my patch we complain about missing List<Thread> AND List<String> methods.

Also, once this bug is addressed, we should close bug 138671 (DOM with proper test), and bug 138887. 
Bug 138671 is a regression over 3.1, and we need to double check that JDT/UI is correctly behaving (in case they worked around the regression).
CC'ing Martin about later concern.

Comment 12 Philipe Mulet 2006-05-19 12:18:18 EDT

Because of NPE possibilities, I am wondering about the consequences on DOM clients, like refactoring. We may want to address this for 3.2RC6.

Comment 13 Kent Johnson 2006-05-19 14:26:04 EDT

Created attachment 42060 [details]
Patch

Philippe - please check this patch. It finds all the superinterfaces and skips the ones that have the same erasure.

passes all of your new tests without any secondary errors

Comment 14 Philipe Mulet 2006-05-19 15:53:04 EDT

Patch looks good. Shouldn't you use a tagbit to retain the presence of collisions, and thus avoid expensive work during method verification ?

Comment 15 Philipe Mulet 2006-05-22 10:11:33 EDT

Note: The bit usage would only be a small performance improvement. Nothing critical (ie. could occur post 3.2.0).

Comment 16 Philipe Mulet 2006-05-22 10:25:33 EDT

Martin/Dani - would you vote for fixing this in RC6 ?

Comment 17 Philipe Mulet 2006-05-22 10:30:11 EDT

Dani/Martin - current behavior (NPE) is a regression over 3.1.2.

Comment 18 Philipe Mulet 2006-05-22 10:37:41 EDT

Mike - pls cast your vote for RC6.

Basically, in RC1 another fix did introduce a regression in a negative scenario; resulting in causing a NPE in the compiler. 

With the patch, the behavior is even better than in 3.1, since less secondary errors are being produced (ie. no complaint for unimplemented abstract methods from offending superinterfaces). The fix is safe.

This fix could either go in for 3.2.0 or 3.2.1; candidating for RC6 since the regression got promptly identified, and we have a good fix at hand.
Also this is correcting the DOM AST defficiency (small contract change) introduced at RC1 as well, and documented in bug 138671 (DOM) and bug 138887. 

Comment 19 Martin Aeschlimann 2006-05-22 12:04:49 EDT

Discussed the patch with Philippe, I couldn't prove him wrong (So far! :-)).

+1 for 3.2 RC6 as the current code (introduced in Rc2) would return 'null' elements as super interfaces. According to Philippe, this would also be visible in the DOM AST which would be unexpected and result in NPE's in our code.

I wouldn't mind having a more minimal patch at this stage, e.g. don't try to reduce the number of emmited 'type contains unimplemented methods' errors.

Comment 20 Philipe Mulet 2006-05-22 12:20:47 EDT

Kent - what about only fixing ClassScope for 3.2.0, and the method verifier for 3.2.1 ? Afterall, minimizing secondary errors is deluxe... in 3.1 we had half the secondary issues.

I would second this request for RC6; since this makes the fix much safer/simpler.

Comment 21 Dani Megert 2006-05-22 12:27:28 EDT

>I wouldn't mind having a more minimal patch at this stage,
Same here. Could we see a patch that only contains the minimum to fix the reported NPE? More clean up can be done post 3.2.

Comment 22 Kent Johnson 2006-05-23 09:54:20 EDT

I'll look into the smaller fix, but the patch is not that scary.

90% of the code in findSuperinterfaceCollisions is copied from computeInheritedMethods and over half of the patch is new tests.

Comment 23 Mike Wilson 2006-05-23 13:44:04 EDT

I'll leave it to the rest of you to decide whether a more minimal patch is a better answer at this point. I reviewed the latest patch with Kent, and agree that it looks scarier than it really is.

I'm also a bit worried that, for people who actually hit the problem, the secondary errors that will be produced without the verifier fix will be confusing.

Comment 24 Kent Johnson 2006-05-23 14:02:15 EDT

The smallest possible change is a one line change to ClassScope in this block of code in checkParameterizedSuperTypeCollisions():

nextOtherInterface: for (int j = 0; j < i; j++) {
  ReferenceBinding two = interfaces[j];
  if (two == null) continue nextOtherInterface;
  if (hasErasedCandidatesCollisions(one, two...)) {
    interfaces[i] = null; <<<< should be j instead of i
    count--;
    continue nextInterface;
  }
}

But given bug 138671, we should at least take the full change to ClassScope from the existing patch, which no longer removes the interface.

I would prefer not to remove the changes to the MethodVerifier15, but if we do then 2 of the tests in the patch need to be modified to report 4 & 6 additional secondary errors that make it more difficult to see the root cause of the problem (the duplicate interfaces with different arguments).

The changes to the verifier are mostly copied code from computeInheritedMethods so they are not as scary as they look.

Comment 25 Dani Megert 2006-05-24 03:13:43 EDT

>But given bug 138671, we should at least take the full change to ClassScope
>from the existing patch, which no longer removes the interface.
I'd vote for the minimal patch in ClassScope for 3.2 RC6 and doing the full fix in 3.2.1.

Comment 26 Martin Aeschlimann 2006-05-24 04:32:54 EDT

+1 to take full change to ClassScope from the existing patch and leave the MethodVerifiers as is. 
I also reviewed the full patch and believe that the changes in MethodVerifier15 are all fine, but I wouldn't want to take the risk that interfaces are wrongly eliminated and no 'unimplemented method' error is generated.

Comment 27 Mike Wilson 2006-05-24 09:38:12 EDT

this makes sense to me. +1 to putting in the reduced patch for now, and the rest for 3.2.1

Comment 28 Kent Johnson 2006-05-24 11:21:21 EDT

Released ClassScope only change with additional secondary errors in tests to HEAD for RC6.

Released ClassScope + MethodVerifier changes to 3.2.1 branch.

Closed bug 138671 and bug 138887 since they are fixed with the ClassScope change.

Comment 29 Olivier Thomann 2006-05-26 11:33:43 EDT

Verified in I20060526-0010 for 3.2RC6