Some Eclipse Foundation services are deprecated, or will be soon. Please ensure you've read this important communication.
Bug 322001 - [1.5][compiler] Name Clash error occurs
Summary: [1.5][compiler] Name Clash error occurs
Status: VERIFIED FIXED
Alias: None
Product: JDT
Classification: Eclipse Project
Component: Core (show other bugs)
Version: 3.6   Edit
Hardware: PC Windows XP
: P3 normal (vote)
Target Milestone: 3.6.1   Edit
Assignee: Srikanth Sankaran CLA
QA Contact:
URL:
Whiteboard:
Keywords:
: 321548 (view as bug list)
Depends on:
Blocks:
 
Reported: 2010-08-06 11:10 EDT by akujtan CLA
Modified: 2010-09-14 10:42 EDT (History)
7 users (show)

See Also:
frederic_fusier: review+
satyam.kandula: review+


Attachments
Patch under consideration (7.91 KB, patch)
2010-08-10 03:08 EDT, Srikanth Sankaran CLA
no flags Details | Diff
Revised patch (10.32 KB, patch)
2010-08-10 08:53 EDT, 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 akujtan CLA 2010-08-06 11:10:19 EDT
Build Identifier: 20100617-1415

When I switched from Galileo to Helios this code started throwing the error,
"Name clash: The method apply(F) of type Function<F,T> has the same erasure as apply(T) of type Predicate<T> but does not override it"
Whereas it would compile fine in previous versions.

Code:
class Bar {}
class Foo {}

interface Function<F, T> {
    T apply(F f);
}
interface Predicate<T> {
    boolean apply(T t);
}

public class Concrete implements Predicate<Foo>, Function<Bar, Boolean> {
    @Override
    public Boolean apply(Bar two) {
        return null;
    }

    @Override
    public boolean apply(Foo foo) {
        return false;
    }
}


Reproducible: Always

Steps to Reproduce:
Try to compile the code.
Comment 1 Walter Harley CLA 2010-08-06 19:36:05 EDT
Although this compiles successfully with javac 1.6.0_21, I am not sure javac is correct; I think this actually should be an error.  The relevant section of the Java Language Spec is 8.4.2:

"It is a compile-time error to declare two methods with override-equivalent signatures (defined below) in a class.

Two methods have the same signature if they have the same name and argument types.

Two method or constructor declarations M and N have the same argument types if all of the following conditions hold:

    * They have the same number of formal parameters (possibly zero)
    * They have the same number of type parameters (possibly zero)
    * Let <A1,...,An> be the formal type parameters of M and let <B1,...,Bn> be the formal type parameters of N. After renaming each occurrence of a Bi in N's type to Ai the bounds of corresponding type variables and the argument types of M and N are the same."

The methods both have the same name; they both have one formal parameter; and neither has a type parameter (although the classes that declare the methods do have type parameters, that is not part of the definition).  Importantly, the return type is also not part of the definition.  

We can simplify your example to make Function have only one type argument, and hard-code its return type to be Boolean.  javac will still compile without error.  However, if we then change the return type to be boolean, with a lower-case 'b', javac reports a name clash.

So javac is behaving differently depending on the return type alone; and that is a violation of the JLS, as I read it.  I suspect that Galileo and javac are both in error, and Helios is correct.

Disclaimer: I am not a language lawyer.
Comment 2 Srikanth Sankaran CLA 2010-08-08 23:13:36 EDT
(In reply to comment #1)

> The methods both have the same name; they both have one formal parameter; and
> neither has a type parameter (although the classes that declare the methods do
> have type parameters, that is not part of the definition).  Importantly, the
> return type is also not part of the definition. 

When you say "The methods both have ... " are you referring to Concrete's
apply methods ? They are not clashing here since their signature is
altogether different.

I do think the code is valid and eclipse is behavior is not.

This bug is very likely a duplicate of bug# 321548 rather than
bug #317719 where javac was at fault.

I will confirm after more analysis.
Comment 3 Walter Harley CLA 2010-08-09 00:03:57 EDT
(In reply to comment #2)
> When you say "The methods both have ... " are you referring to Concrete's
> apply methods ? They are not clashing here since their signature is
> altogether different.

Yes, those are the methods I mean.  But I am not sure I agree with your assertion.

First, we need to ignore the return type.  Nothing in the JLS says that the return type is part of the signature.  So, replace the example with one where the return types are the same.

I think if you do that in bug 321548, then javac will also report a name clash.  So that says that javac is (incorrectly, I think) paying attention to return type.

That said, it does make sense that the compiler _should_ be able to tell these apart, given the substitutions of actual types for type variables in the class definition.  But I don't see where in the language spec that takes effect.  I am not by any means sophisticated when it comes to the JLS and generics, so maybe I am just misunderstanding.  But I do think we should base our investigations on an example that uses the same return type, because at least that part of the spec seems pretty clear.
Comment 4 Srikanth Sankaran CLA 2010-08-09 01:09:50 EDT
(In reply to comment #3)
> (In reply to comment #2)
> > When you say "The methods both have ... " are you referring to Concrete's
> > apply methods ? They are not clashing here since their signature is
> > altogether different.
> 
> Yes, those are the methods I mean.  But I am not sure I agree with your
> assertion.

These methods are not clashing at all as their signatures are blatantly
different (even ignoring the return types as you rightly point out below
we should). No substitution is needed to establish this fact as there are
no type variables in the picture in the signature of these two methods.
They use concrete types in their signatures.

> First, we need to ignore the return type.  Nothing in the JLS says that the
> return type is part of the signature.  So, replace the example with one where
> the return types are the same.

Yes.

> I think if you do that in bug 321548, then javac will also report a name clash.
>  So that says that javac is (incorrectly, I think) paying attention to return
> type.

Yes, javac would report an error if you change the int to void consistently
in the example in bug# 321548, AND that would be the right behavior.

In order for virtual method dispatch to work correctly, the compiler sometimes
has to generate an internal bridge method.

For example given:

public interface Interface1<T> {
    public void hello(T greeting);
}

public interface Interface2<T> {
    public int hello(T greeting);
}

public class ErasureTest implements Interface1<String>, Interface2<Double> {
    public void hello(String greeting) {
    }

    public int hello(Double greeting) {
        return 0;
    }
}

ErasureTest's method hello(String greeting) overrides Interface1's
void hello(T greeting);

So if you have a call that looks like:

        Interface1<String>  i = new ErasureTest();
    	i.hello("Blah");

However this will not happen unless the compiler generates a
suitable bridge method. At the JVM level method dispatch very
much involves return type ("method descriptor" which is used
to symbolically reference methods to be invoked is a combination
of name,  parameter count and types and return type all erased)
So the above call will be translated to

        11: invokeinterface #5,  2            // InterfaceMethod Interface1.hell
o:(Ljava/lang/Object;)V

The method descriptor used in invocation here is

      hello:(Ljava/lang/Object;)V

So this byte code instruction says invoke the interface method
named hello which takes an java.lang.Object as argument and
returns void.

This call cannot be bound to ErasureTest's method 
void hello(String greeting) whose descriptor is
   
     hello:(Ljava/lang/String;)V

So in order for polymorphism to work expected in the presence
of generics, the compiler will silently a synthesize bridge
method in ErasureTest of the form 

    public void hello(Object o) { // syntnetic method
        hello(String o);  // invoke Erasure's test void hello(String) method
    }

Now returning to your example where the return types are forced to
be the same (say void), the compiler is in an unhappy of situation
of having to synthesize two bridge methods with the same descriptor
which should bridge the calls to two different destination - an
impossible task and hence a valid complain about name clash.

Bottom line is in a class, no two methods cannot have same descriptor:
if either because the user attempted to supply two such methods
(may be in the same class, or by inheritance + supply a new method)
or because the situation got entered into by the compiler's need
to generate a bridge method collided with what the programmer has
provided, a name clash error will result.

JLS3 discusses this in 15.12.4.5 "Create Frame, Synchronize, Transfer Control"
in the discussion box. (search for "bridge", in my pdf copy it is on page
478)

Also worth noting is that the text of the compiler's complaint
does not say anything about these two methods.

(Multiple markers at this line
	- Name clash: The method hello(T) of type Interface1<T> has the same erasure as hello(T) of type Interface2<T> but does not 
	 override it
	- Name clash: The method hello(T) of type Interface2<T> has the same erasure as hello(T) of type Interface1<T> but does not 
	 override it
)

In summary, after preliminary analysis, it does point to an eclipse
issue. I'll confirm after detailed analysis.

HTH.
Comment 5 Srikanth Sankaran CLA 2010-08-09 01:18:34 EDT
(In reply to comment #4)

I pressed the commit button too fast, some clarifications are
in this update:

> So if you have a call that looks like:
> 
>         Interface1<String>  i = new ErasureTest();
>         i.hello("Blah");

I was going to say, this call is supposed to be dispatched to
ErasureTest's hello(String) and that 

> However this will not happen unless the compiler generates a
> suitable bridge method.

[...]

> So in order for polymorphism to work expected in the presence
> of generics, the compiler will silently a synthesize bridge
> method in ErasureTest of the form 
> 
>     public void hello(Object o) { // syntnetic method
>         hello(String o);  // invoke Erasure's test void hello(String) method
>     }

The method should look like:

     public void hello(Object o) { // syntnetic method
         hello((String) o);  // invoke Erasure's test void hello(String) method
     }
Comment 6 Srikanth Sankaran CLA 2010-08-10 00:45:11 EDT
Targetting 3.6.1 since this is a regression with
respect to 3.5.
Comment 7 Srikanth Sankaran CLA 2010-08-10 00:48:37 EDT
*** Bug 321548 has been marked as a duplicate of this bug. ***
Comment 8 Michael Allman CLA 2010-08-10 01:42:19 EDT
(In reply to comment #6)
> Targetting 3.6.1 since this is a regression with
> respect to 3.5.

Thanks for targetting 3.6.1.  I have a codebase that won't build in Helios because of this bug.
Comment 9 Srikanth Sankaran CLA 2010-08-10 03:08:31 EDT
Created attachment 176204 [details]
Patch under consideration
Comment 10 Srikanth Sankaran CLA 2010-08-10 04:39:05 EDT
All tests pass. Frederic, Satyam, please review, TIA.
Comment 11 Srikanth Sankaran CLA 2010-08-10 04:40:00 EDT
Satyam, please review for 3.6.1. TIA.
Comment 12 Srikanth Sankaran CLA 2010-08-10 04:44:53 EDT
This problem has been dormant for sometime and
came to the fore due to the change done in
bug# 289247

To be clear, this is not a regression caused by
the fix for bug# 289247.

Basically, while verifying that a type satisfies
the contract it is obligated to satisfy given its
supertypes, we should not be looking for name clashes
between methods from supertypes that have been
overridden in the current type (and so are hidden in
the current class and so cannot contribute to a clash).

See also https://bugs.eclipse.org/bugs/show_bug.cgi?id=293615
for a very similar, but not identical problem.
Comment 13 Srikanth Sankaran CLA 2010-08-10 05:00:08 EDT
Comment on attachment 176204 [details]
Patch under consideration

Grrrr. This patch breaks some cases and will have
to be reworked.
Comment 14 Srikanth Sankaran CLA 2010-08-10 08:53:02 EDT
Created attachment 176228 [details]
Revised patch

The older patch attached to comment# 9 passes all
the tests. However while experimenting somewhat with
the bits I discovered some issues:

 (1)  The original fix for bug# 83162 is not proper.
      It did make the problem go away as a side effect
      of the purported fix, but it was not really
      addressing the root cause of the problem reported
      in that bug which is that it is impossible for
      one bridge method to bridge to two targets.

 (2)  There was also no regression test for the bug 83162
      that directly captured the scenario in bug 83162 comment 0
      There were several related variants, but none that mapped
      directly to the reported sample.
     (This has now been added as a part of the current fix)

    - Part of the current fix involved massaging some of the
      same code involved in (1) above, that removed the indirect
      fix to bug 83162 that the code referred to in (1) was
      affording.

The current patch includes proper fix for bug 83162 also.

Essentially, a name clash (methods having the same erasure, without
being override-equivalent) can arise because

    - Two programmer supplied methods in a class clash
    - A programmer supplied method clashes with an inherited method.
    - A synthetic bridge method clashes with a programmer supplied method
    - A synthetic method clashes with another synthetic method.
    - Two inherited methods clash. 

You can also have a clash between a synthetic method and an inherited
method, but at this time, both javac (5,6,7) and eclipse don't handle
this case well:

Try running the following program with both javac and eclipse:

class Base  {
	boolean equalTo(Object other) {return false;}
}

interface EqualityComparable<T> {
	boolean equalTo(T other);
}

public class SomeClass extends Base implements EqualityComparable<String> {
	public boolean equalTo(String other) {
		return true;
	}
	public static void main(String args[]) {
		new SomeClass().equalTo(args);
	}
}

I don't think the CCE is justified in either case and I believe
both compilers are buggy.
Comment 15 Srikanth Sankaran CLA 2010-08-10 08:57:14 EDT
Passes all tests, Frederic, please review for 3.6.1 - TIA.
Comment 16 Srikanth Sankaran CLA 2010-08-10 08:57:50 EDT
Satyam, please review, TIA.
Comment 17 Srikanth Sankaran CLA 2010-08-10 08:58:58 EDT
Ayush, please review, TIA.
Comment 18 Frederic Fusier CLA 2010-08-10 11:24:23 EDT
Patch looks good to me
Comment 19 Satyam Kandula CLA 2010-08-11 02:53:04 EDT
Patch looks good for me.
Comment 20 Srikanth Sankaran CLA 2010-08-11 03:07:50 EDT
Released in HEAD for 3.7 M2
            3.6 maintenance stream for 3.6.1
Comment 21 Srikanth Sankaran CLA 2010-08-16 00:08:43 EDT
(In reply to comment #14)

[...]

> You can also have a clash between a synthetic method and an inherited
> method, but at this time, both javac (5,6,7) and eclipse don't handle
> this case well:
> 
> Try running the following program with both javac and eclipse:

[...]

> I don't think the CCE is justified in either case and I believe
> both compilers are buggy.

Raised bug 322740 for follow up.
Comment 22 Frederic Fusier CLA 2010-08-26 10:52:55 EDT
Verified for 3.6.1 using build M20100825-0800.
Comment 23 Olivier Thomann CLA 2010-09-14 10:42:24 EDT
Verified for 3.7M2 using I20100914-0100