Some Eclipse Foundation services are deprecated, or will be soon. Please ensure you've read this important communication.
Bug 340506 - [compiler] Eclipse can compile an ambiguous method invocation that the javac compiler cannot
Summary: [compiler] Eclipse can compile an ambiguous method invocation that the javac ...
Status: VERIFIED FIXED
Alias: None
Product: JDT
Classification: Eclipse Project
Component: Core (show other bugs)
Version: 3.7   Edit
Hardware: PC Windows 7
: P3 normal with 2 votes (vote)
Target Milestone: 4.9 M2   Edit
Assignee: Stephan Herrmann CLA
QA Contact:
URL: http://stackoverflow.com/questions/53...
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-03-20 06:15 EDT by Lukas Eder CLA
Modified: 2018-07-31 15:23 EDT (History)
9 users (show)

See Also:


Attachments
A test file containing an ambiguous method invocation (1.01 KB, text/plain)
2011-03-20 06:17 EDT, Lukas Eder CLA
no flags Details
test & possible fix (10.73 KB, patch)
2012-05-13 20:04 EDT, Stephan Herrmann CLA
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Lukas Eder CLA 2011-03-20 06:15:10 EDT
Build Identifier: 20100917-0705

This is a very tricky issue related to choosing the most specific method on method invocation, when generic type arguments are involved.

Essentially, when overloading methods like this:

    public <T> void setValue(Parameter<T> parameter, T value) {
    }

    public <T> void setValue(Parameter<T> parameter, Field<T> value) {
    }

And calling those methods like this:

    Parameter<Object> p = null;
    Field<Object> f = null;
    setValue(p, f);

We *should* get a compilation error telling us that the method invocation is ambiguous.

Read my full report (and the accepted answer) on stackoverflow:
http://stackoverflow.com/questions/5361513/reference-is-ambiguous-with-generics

Reproducible: Always

Steps to Reproduce:
1. Compile this class with Eclipse, works
-----------------------------------------
public class Test {
    public <T> void setValue(Parameter<T> parameter, T value) {
        System.out.println("Object");
    }

    public <T> void setValue(Parameter<T> parameter, Field<T> value) {
        System.out.println("Field");
    }

    public static void main(String[] args) {
        new Test().test();
    }

    private void test() {
        Parameter<String> p1 = p1();
        Field<String> f1 = f1();
        setValue(p1, f1);

        Parameter<Object> p2 = p2();
        Field<Object> f2 = f2();
        setValue(p2, f2);
    }

    private Field<String> f1() {
        Field<String> f1 = null;
        return f1;
    }

    private Parameter<String> p1() {
        Parameter<String> p1 = null;
        return p1;
    }

    private Parameter<Object> p2() {
        Parameter<Object> p2 = null;
        return p2;
    }

    private Field<Object> f2() {
        Field<Object> f2 = null;
        return f2;
    }
}

interface Field<T> {}
interface Parameter <T> {}


2. Compile that class with javac, doesn't work
----------------------------------------------
Comment 1 Lukas Eder CLA 2011-03-20 06:17:58 EDT
Created attachment 191573 [details]
A test file containing an ambiguous method invocation
Comment 2 Lukas Eder CLA 2011-03-30 13:47:53 EDT
Even worse, I can pass null as the second argument of the overloaded setValue() method. I.e:

  setValue(p1, null);
  setValue(p2, null);

Here, clearly, there should be ambiguity indicated by the compiler
Comment 3 Srikanth Sankaran CLA 2011-03-31 03:43:39 EDT
Eclipse behavior hasn't changed here since at least 3.4.
I agree the difference in behavior needs to be looked into
and understood and fixed if needed. Unfortunately, I don't
expect to get to this any time soon.

For the time being retargetting this to 3.7.
Comment 4 Olivier Thomann CLA 2011-04-21 14:04:08 EDT
If we want to address it for 3.7, it has to be done for RC1.
Comment 5 Srikanth Sankaran CLA 2011-05-04 01:07:38 EDT
>(In reply to comment #3)
> Eclipse behavior hasn't changed here since at least 3.4.
> I agree the difference in behavior needs to be looked into
> and understood and fixed if needed. Unfortunately, I don't
> expect to get to this any time soon.
> 
> For the time being retargetting this to 3.7.

Didn't have time to look into this yet as most of
the cycles in the last few months went into Java 7
work. Postponed to 3.8
Comment 6 Mathias Arens CLA 2011-08-18 03:24:00 EDT
There is a similar issue reported in bug 333011.
Comment 7 Michael OBrien CLA 2011-10-26 10:27:07 EDT
>Check the update in JDK 1.6.0_25 which fixes an issue with erasure of generics that may be related
http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=6302954
Comment 8 Stephan Herrmann CLA 2012-04-15 10:33:32 EDT
The accepted answer at the reference stackoverflow concluded the second method is *not* more specific than the first because Field<V> <: V does not hold, mentioning that 
 "V is not Object in this analysis; the analysis considers all possible values of V."

However, in JLS 15.12.2.5 I find the subtype requirement to be
  Tj <: Sj
where Sj is a type *after* type argument inference:
  let A1 ... Ap be the type arguments inferred (§15.12.2.7) for this invocation under the initial constraints Ti << Ui (1 ≤ i ≤ n), 
  and let Si = Ui[R1=A1,...,Rp=Ap] (1 ≤ i ≤ n).

So my first judgement is: the invocation where V = Object must indeed be considered, so JDT would we correct, and javac wrong.

From the accepted answer at stackoverflow:
> This appears to be a significant bug in Eclipse. The spec quite clearly 
> indicates that the type variables are not substituted in this step.

I don't see this in the spec. 
By contrast I see
  let Si = Ui[R1=A1,...,Rp=Ap] (1 ≤ i ≤ n)

It seems the JLS itself is inconsistent between the "informal intuition" (speaking of *any invocation*) and the formal details (saying *this invocation*).

(In reply to comment #7)
> >Check the update in JDK 1.6.0_25 which fixes an issue with erasure of generics that may be related
> http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=6302954

I don't see a connection as the Sun/Oracle bug talks about finding a substitution for a type parameter, not about overloading.


From these initial findings I lean towards closing as INVALID.

Any comments?
Comment 9 Srikanth Sankaran CLA 2012-05-09 05:41:41 EDT
Thanks for the analysis Stephan.
Comment 10 Zhong Yu CLA 2012-05-10 00:23:31 EDT
> let A1 ... Ap be the type arguments inferred (§15.12.2.7) 
> for this invocation
> under the initial constraints Ti << Ui (1 ≤ i ≤ n), 

Here Ti and Ui are method parameter types, not invocation argument types.

The meaning of "for this invocation" is unclear; if we interpret it as that the initial constraints Ei << Ui should also be included, where Ei is the type of invocation argument ei, then we need to solve

    Ei << Ui
    Ti << Ui

which will usually yield two conflicting results for each type variable Rl.

Suppose the inference process favors results from Ei<<Ui, then the mentioning of Ti<<Ui is apparently pointless in the first place.


Next, the spec requires

    For all j from 1 to n, Tj <: Sj. 

for this example

    <V> void setValue(Parameter<V> parameter, Field<V> value) 
    <R> void setValue(Parameter<R> parameter, R value) 

if we infer, from Ei<<Ui, that R=Object, we need to check

    Parameter<V>   <:   Parameter<Object>
    Field<V>       <:   Object

the 1st subtype relation is false, therefore "more specific" is still false.
Comment 11 Stephan Herrmann CLA 2012-05-13 11:25:50 EDT
Zhong Yu,

thanks for your comment, this is much more substantial than what was discussed so far.

(In reply to comment #10)
> > let A1 ... Ap be the type arguments inferred (§15.12.2.7) 
> > for this invocation
> > under the initial constraints Ti << Ui (1 ≤ i ≤ n), 
> 
> Here Ti and Ui are method parameter types, not invocation argument types.
> 
> The meaning of "for this invocation" is unclear; ...

I'd actually say it a bit stronger: this sentence looks plain inconsistent to me. §15.12.2.7 speaks about "Inferring Type Arguments Based on Actual Arguments " but in 15.12.2.5 that rule seems to be abused to infer type arguments based on another method *declaration* and its formal parameters. If the mentioning of "Ti << Ui" is correct then saying "invocation" is wrong.

I admit I believed the "invocation" part more than the "Ti << Ui" part, which may be different from what the JLS authors intended.

> if we interpret it as that the
> initial constraints Ei << Ui should also be included, where Ei is the type of
> invocation argument ei, then we need to solve
> 
>     Ei << Ui
>     Ti << Ui
> 
> which will usually yield two conflicting results for each type variable Rl...

I agree, considering both these constraints gets us nowhere.
 
> Next, the spec requires
> 
>     For all j from 1 to n, Tj <: Sj. 
> 
> for this example
> 
>     <V> void setValue(Parameter<V> parameter, Field<V> value) 
>     <R> void setValue(Parameter<R> parameter, R value) 
> 
> if we infer, from Ei<<Ui, that R=Object, we need to check
> 
>     Parameter<V>   <:   Parameter<Object>
>     Field<V>       <:   Object
> 
> the 1st subtype relation is false, therefore "more specific" is still false.

I see, comparing two signatures where one has substituted type parameters while the other has not, will not yield the expected compatibility.

Looking at the code the JDT compiler essentially compares these:
    void setValue(Parameter<Object> parameter, Field<Object> value)
    void setValue(Parameter<Object> parameter, Object value)
i.e., we work on the basis of substitutions for both methods.

I've a simple patch under test that changes the JDT compiler to comparing the method declarations disregarding the current invocation. The naive way of fixing this, however, breaks the fixes for bug 216686 and bug 99106

I'm willing to reopen the bug if we agree to realize the semantics based on the constraints "Ti << Ui" while deliberately ignoring the mentioning of an invocation and how 15.12.2.7 is inconsistently used without any actual arguments.

However, if we change the compiler ...
(a) we need to find how to do this without breaking bug 216686 and bug 99106
(b) we will reject more programs where intuition would see no problem
Comment 12 Stephan Herrmann CLA 2012-05-13 12:00:33 EDT
(In reply to comment #11)
> I've a simple patch under test that changes the JDT compiler to comparing the
> method declarations disregarding the current invocation. The naive way of
> fixing this, however, breaks the fixes for bug 216686 and bug 99106

The change regarding bug 99106 seems acceptable: first we report a name clash, and afterwards it is disputable if those erroneous methods should be considered as overloads, or whether secondary errors should be suppressed.
Comment 13 Stephan Herrmann CLA 2012-05-13 20:04:24 EDT
Created attachment 215541 [details]
test & possible fix

*Assuming* ...
 "... we agree to realize the semantics based on the
  constraints "Ti << Ui" while deliberately ignoring the mentioning of an
  invocation and how 15.12.2.7 is inconsistently used without any actual
  arguments"

... then this patch seems to fix the issue.

Note, however, that there is a very fine line between fixing this issue and breaking bug 216686. The patch distinguishes both situations be looking for the presence of a wildcard, if one is found the substituted parameters must be used, otherwise go back to the originals.
This distinction is more based on experimentation than on findings from the JLS and needs further validation.

Unfortunately this part of the implementation is structured differently than the JLS (see comment above Scope#mostSpecificMethodBinding).
Comment 14 Stephan Herrmann CLA 2012-05-13 20:07:58 EDT
Reopening to allow for consideration of comment 10 ff.
Comment 15 Zhong Yu CLA 2012-05-13 22:58:51 EDT
Hi Stephan, my understanding is that if wildcards are involved, capture conversion should be applied first to remove the wildcards. 

15.12.2.7:

(if A<<F)

  If F has the form G<...> ... then if A has a supertype of the form G<...>...

now if A has wildcards, to find super types of A:

4.10.2 :

  The direct supertypes of the type C<R1,...,Rn> , where at least one of the Ri, 1in, is a wildcard type argument, are the direct supertypes of C<X1,...,Xn>, where C<X1,...,Xn> is the result of applying capture conversion (§5.1.10) to C<R1,...,Rn>.


For example 

    ArrayList<? super A>  <<  List<T>

apply capture conversion to  ArrayList<? super A>, we get
    ArrayList<x>, where A <: x <: Object
it has a super type List<x>, therefore

    T = x

inference succeeds.


It is odd if the inference result contains a free/unknown type variable. Usually A in A<<F does not contain wildcards, because the type of a method argument has undergone capture conversion first even before 15.12 starts. Unfortunately I cannot back that up with words in the spec, however I believe it is intended that, the type of any expression must undergo capture conversion before evaluating its enclosing expression.

    List<?> list = ...;
    foo(list); // the type of `list` here is List<x> where x<:Object

it makes sense because `list` indeed is a List<x> for some concrete but unknown type x (x would be knowable if there's no erasure); narrowing down the type of `list` from List<?> to List<x> increases type information.

With the same principle, I think 15.12.2.5 would be better if it says

  If the second method is generic ... inferred under the initial constraints 
T'i << Ui, where T'i is the result of applying capture conversion on Ti, ...

although in its current form it happens to work too, thank to 4.10.2.
Comment 16 Stephan Herrmann CLA 2015-06-04 09:58:55 EDT
Update: at 1.8 ecj also reports an error:

----------
1. ERROR in /tmp/Test.java (at line 21)
        setValue(p2, f2);
        ^^^^^^^^
The method setValue(Parameter<Object>, Object) is ambiguous for the type Test
----------

This can be taken as a hint that the same was also intended for 1.7-.
Comment 17 Stephan Herrmann CLA 2018-06-28 08:48:54 EDT
If this bug indeed only occurs at 1.7- I'm leaning towards closing WORKSFORME or such.

I'm just planning to add some tests before closing.
Comment 18 Eclipse Genie CLA 2018-06-29 17:39:48 EDT
New Gerrit change created: https://git.eclipse.org/r/125278
Comment 20 Stephan Herrmann CLA 2018-06-30 05:38:20 EDT
(In reply to Eclipse Genie from comment #19)
> Gerrit change https://git.eclipse.org/r/125278 was merged to [master].
> Commit:
> http://git.eclipse.org/c/jdt/eclipse.jdt.core.git/commit/
> ?id=355afcea9e91a57a8d79f549075bd38d3046710d

New test released for 4.9 M2.

At 1.8+ the compiler correctly reports ambiguity.

Since type inference in 1.7- is a completely different story, I am not planning to 'downport' this behavior, given this bug has been quiet for some years.
Comment 21 Manoj N Palat CLA 2018-07-31 15:23:56 EDT
Verified with Eclipse 4.9 m2 Build id: I20180731-0800