Bug 331446 - [1.4/1.5] Unexpected ambiguous error for 1.4 project
Summary: [1.4/1.5] Unexpected ambiguous error for 1.4 project
Status: VERIFIED FIXED
Alias: None
Product: JDT
Classification: Eclipse Project
Component: Core (show other bugs)
Version: 3.7   Edit
Hardware: PC Mac OS X - Carbon (unsup.)
: P3 major (vote)
Target Milestone: 3.7 M4   Edit
Assignee: Srikanth Sankaran CLA
QA Contact:
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-11-30 10:48 EST by Thomas Watson CLA
Modified: 2010-12-07 07:20 EST (History)
2 users (show)

See Also:


Attachments
ambiguity test projects (7.25 KB, application/zip)
2010-11-30 10:48 EST, Thomas Watson CLA
no flags Details
Plausible patch - Under test (5.57 KB, patch)
2010-12-01 07:48 EST, Srikanth Sankaran CLA
no flags Details | Diff
Proposed fix + regression tests (11.27 KB, patch)
2010-12-01 09:29 EST, Olivier Thomann CLA
no flags Details | Diff
Proposed patch + tests - Take 2 (12.87 KB, patch)
2010-12-02 00:51 EST, Srikanth Sankaran CLA
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Thomas Watson CLA 2010-11-30 10:48:33 EST
Created attachment 184140 [details]
ambiguity test projects

I20101123-0800

Not reproducible in 3.6.

The attached projects reproduce the ambiguous error for a 1.4 project which references some API from a 1.5 project.
Comment 1 Olivier Thomann CLA 2010-11-30 10:58:58 EST
Srikanth, please investigate.
Comment 2 Thomas Watson CLA 2010-11-30 11:01:00 EST
FYI, Srikanth and Olivier, this error can also be observed from the latest OGGi git repo.
Comment 3 Olivier Thomann CLA 2010-11-30 11:05:13 EST
I released the disabled regression test for this issue:
org.eclipse.jdt.core.tests.compiler.regression.MethodVerifyTest#_test331446
Comment 4 Olivier Thomann CLA 2010-11-30 12:51:24 EST
Since this is causing grief in the OSGi workspace, setting the severity to major.
Comment 5 Srikanth Sankaran CLA 2010-11-30 23:25:50 EST
(In reply to comment #2)
> FYI, Srikanth and Olivier, this error can also be observed from the latest OGGi
> git repo.

Jay, How come we didn't see this in our set up ? Is it because, we have
a snap shot and haven't synchronized with the top of trunk ? In any case
we should upgrade to the latest bits and test for the upcoming milestone
build input. We should also test PDE sources + test projects and JDT/UI
sources + test projects. Testing early would help us to react better.
Thanks for your help with this.
Comment 6 Jay Arthanareeswaran CLA 2010-11-30 23:54:43 EST
(In reply to comment #5)
> Jay, How come we didn't see this in our set up ? Is it because, we have
> a snap shot and haven't synchronized with the top of trunk ? In any case
> we should upgrade to the latest bits and test for the upcoming milestone
> build input. We should also test PDE sources + test projects and JDT/UI
> sources + test projects. Testing early would help us to react better.
> Thanks for your help with this.

The OSGi sources we have was last updated two weeks ago. I will update the clone and see if the errors show up in our set-up.
Comment 7 Srikanth Sankaran CLA 2010-12-01 00:30:45 EST
Reduced test case:


//------------- 1.5 project -----------------
class List<T> {}
public class P15 {
    static <T> void foo(List<T> expected) {}
    public static <T> void foo(T expected) {}
}

//------------- 1.4 project -----------------
public class Client {
    Client(List l) {
        P15.foo(l);
    }
}

If both projects are 1.5+ or if both are 1.4- 
(suitable erasure applied), all is well.
Comment 8 Srikanth Sankaran CLA 2010-12-01 04:37:34 EST
Released several more tests: Some codify the current & right behavior,
some codify the expected, but not current behavior, some codify
current behavior that needs to be double checked.

org.eclipse.jdt.core.tests.compiler.regression.MethodVerifyTest.test1415Mix()
org.eclipse.jdt.core.tests.compiler.regression.MethodVerifyTest.test1415Mix2()

show some interesting behavior, in that the behavior is different from
what is exhibited by javac. I think eclipse behavior is the correct one,
Olivier could you double check this ? (We complain about missing foo(String)
method while javac complains about missing foo(Object).
Comment 9 Srikanth Sankaran CLA 2010-12-01 07:48:42 EST
Created attachment 184250 [details]
Plausible patch - Under test

(In reply to comment #8)

> org.eclipse.jdt.core.tests.compiler.regression.MethodVerifyTest.test1415Mix()
> org.eclipse.jdt.core.tests.compiler.regression.MethodVerifyTest.test1415Mix2()
> 
> show some interesting behavior, in that the behavior is different from
> what is exhibited by javac. I think eclipse behavior is the correct one,
> Olivier could you double check this ? (We complain about missing foo(String)
> method while javac complains about missing foo(Object).

Thinking more about this and testing some more, I am actually inclined
to think that javac is the right one here. A 1.4 type should look for
contract fulfillment only in terms of erased types and should not consider
type parameters, substitutions and such.
Comment 10 Olivier Thomann CLA 2010-12-01 09:29:51 EST
Created attachment 184261 [details]
Proposed fix + regression tests

Same patch with reorganized tests.
Comment 11 Srikanth Sankaran CLA 2010-12-01 09:47:46 EST
(In reply to comment #10)
> Created an attachment (id=184261) [details]
> Proposed fix + regression tests
> 
> Same patch with reorganized tests.

Passes all JDT/Core tests. Will test some more
before releasing.
Comment 12 Olivier Thomann CLA 2010-12-01 12:28:41 EST
I could successfully compile the OSGi workspace.
I also passed the AutomatedSuite of JDT/UI tests and I am running the refactoring tests.
Comment 13 Olivier Thomann CLA 2010-12-01 13:02:34 EST
All refactoring tests passed.
Comment 14 Srikanth Sankaran CLA 2010-12-01 16:18:39 EST
In the original example from comment# 0 as well as in the
reduced example from comment# 7, I could verify that the
problem is gone in the compiler. Reconciler still shows the
issue as equivalent fix has to go in the SourceTypeConvertor.
I'll look into it - Not sure yet if we should spawn a new bug
for this and address it post M4. It is relatively less severe.
Comment 15 Srikanth Sankaran CLA 2010-12-01 18:41:16 EST
The latest patch we have fails, on the workaround resorted to
by bug 324850. Unfortunately the stripped down example projects
from bug 324850 comment 2 lose some nuance that is germane here.

I have added two regressions tests and released them:

org.eclipse.jdt.core.tests.compiler.regression.MethodVerifyTest.test213b() and
org.eclipse.jdt.core.tests.compiler.regression.MethodVerifyTest.test213c()

Both pass on HEAD. The former fails with the patch we have while compiling
alright with javac.

Investigating further...
Comment 16 Srikanth Sankaran CLA 2010-12-01 18:47:12 EST
(In reply to comment #14)
> Reconciler still shows the
> issue as equivalent fix has to go in the SourceTypeConvertor.

Released disabled test 
org.eclipse.jdt.core.tests.model.ReconcilerTests._testGenericAPIUsageFromA14Project9()

to test for this.
Comment 17 Srikanth Sankaran CLA 2010-12-02 00:51:33 EST
Created attachment 184322 [details]
Proposed patch + tests - Take 2

This patch uses the same approach, but limits the application
of erasure to the operation in hand which is to determine
the most specific method.

This addresses the reconciler test issue reported earlier also.
Under test.
Comment 18 Srikanth Sankaran CLA 2010-12-02 01:04:43 EST
(In reply to comment #9)
> Created an attachment (id=184250) [details]
> Plausible patch - Under test
> 
> (In reply to comment #8)
> 
> > org.eclipse.jdt.core.tests.compiler.regression.MethodVerifyTest.test1415Mix()
> > org.eclipse.jdt.core.tests.compiler.regression.MethodVerifyTest.test1415Mix2()
> > 
> > show some interesting behavior, in that the behavior is different from
> > what is exhibited by javac. I think eclipse behavior is the correct one,
> > Olivier could you double check this ? (We complain about missing foo(String)
> > method while javac complains about missing foo(Object).
> 
> Thinking more about this and testing some more, I am actually inclined
> to think that javac is the right one here.

Thinking even more about it and testing even more, I don't know what to
think :)

Olivier, please take a look and let me have your opinion.

In essence, we complain about missing foo(String) while javac complains
about missing foo(String). The fact of the matter is foo(String) *is*
missing.
Comment 19 Srikanth Sankaran CLA 2010-12-02 04:14:30 EST
(In reply to comment #17)
> Created an attachment (id=184322) [details]
> Proposed patch + tests - Take 2

> Under test.

RunAllJDTCoreTests is green.
Comment 20 Jay Arthanareeswaran CLA 2010-12-02 06:58:37 EST
(In reply to comment #17)
> Created an attachment (id=184322) [details]
> Proposed patch + tests - Take 2

I am still having some difficulties in building the OSGi workspace consistently. But I managed to get it build couple of times properly and verified that this patch fixes the 'ambiguous' errors (There are as many as 84 errors without the patch)
Comment 21 Olivier Thomann CLA 2010-12-02 09:18:47 EST
The OSGi workspace compiles fine with this patch.
Comment 22 Thomas Watson CLA 2010-12-02 09:32:47 EST
(In reply to comment #21)
> The OSGi workspace compiles fine with this patch.

I also tried out the patch and confirmed it works for my OSGi workspace.
Comment 23 Srikanth Sankaran CLA 2010-12-03 03:45:33 EST
Passes all JDT/UI tests too. 
Released in HEAD for 3.7 M4
Comment 24 Jay Arthanareeswaran CLA 2010-12-07 07:20:23 EST
Verified for 3.7M4 using build I20101206-1800