Summary: | [1.5][compiler] Erroneous Report of an Ambiguous Method | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | [Eclipse Project] JDT | Reporter: | Robert Simmons Jr. <wo8vi45y9w4yv> | ||||||
Component: | Core | Assignee: | Maxime Daniel <maxime_daniel> | ||||||
Status: | VERIFIED FIXED | QA Contact: | |||||||
Severity: | critical | ||||||||
Priority: | P3 | CC: | philippe_mulet | ||||||
Version: | 3.3 | ||||||||
Target Milestone: | 3.3 M4 | ||||||||
Hardware: | Macintosh | ||||||||
OS: | Mac OS X - Carbon (unsup.) | ||||||||
Whiteboard: | |||||||||
Attachments: |
|
Description
Robert Simmons Jr.
2006-10-23 20:28:27 EDT
Addendum: The code this.foo.fetch((Number)305.5f); Should result in an unnecessary cast warning. Reproduced with HEAD. This is not a regression from 3.2.0. Added test cases AmbiguousMethodTest#25 (inactive) and 26. 162065 is at most loosely related to this one. It may depend on 162073 being fixed though. Investigating further. From JLS 3 15.12.2, I infer that, in the context of AmbiguousMethodTest#25, m.foo(1.0f), the type into which we must search foo is J. Since J#foo overrides I#foo, we should consider a single method, that is J#foo, and avoid using mostSpecificMethodBinding. Or else if we want to collect both J#foo and I#foo at that step of the algorithm for other reasons, the semantics of mostSpecificMethodBinding are inappropriate for the post selection. Experimenting with a more selective collection process. Created attachment 52818 [details]
Tentative fix + test cases modifications (also includes patch for 162073)
This patch passes all existing tests by eliminating overridden methods more thoroughly in the case of abstract methods (in Scope#mostSpecificMethodBinding). There remains a few things to do:
- document the fact that Scope#mostSpecificMethodBinding is not an implementation of the algorithm described by JLS3; amongst other differences, it takes more methods in its input, and uses additional exclusion rules to trim it down;
- exhibit a test case that would leverage the code that this patch suppresses, and refine the patch; I suspect that the code the patch suppresses may be useful in some cases (most probably involving binary type bindings), and that the real fix must be more cautious; if I am wrong, at least I need to improve the test coverage...
Further analysis - if (original2 == null || !original.areParameterErasuresEqual(original2) guards the cases when there is no override between the compared methods, as far as parameters are concerned; - only the return type remaines; at very least, original.returnType.isCompatibleWith(original2.returnType), which tests whether the return types are assignment compatible, is not the test to perform; this is because, while returning Float is compatible with returning T extends Number according to the return type compatibility definition, assigning a Float to a T extends Number is not always valid (take T == Integer); if the test is needed, it must be of a different nature; - by the way, this clears isCompatibleWith responsibility in the bug: its current (and correct) semantics are not the ones needed at the calling point; - I believe that the right test is performed during the method verification process for source files, leaving us with methods that are in effect return type compatible; the question now would be to find another path by which we could get there with incompatible return types (again, no current test case gets us there); looking at the source more closely, that would need that I, J and Y (or some of them) be compiled in advance in such a way that the J#foo or Y#foo would not be return type compatible with I#foo; this would demand that the compiler used to do that be incorrect (for the test case at hand); - there may exist other ways to reach that point though, if mostSpecificMethodBinding was to be called in another context; hence we'd better play safe and complete the test by a test on boundaries. Safer patch will test types boundaries compatibility when they are not compatible. Created attachment 53369 [details]
Safer patch (more conservative)
Released for 3.3 M4. Verified for 3.3M4 with I20061211-1119 Changing OS from Mac OS to Mac OS X as per bug 185991 |