Bug 105808 - [1.5][dom] MethodBinding#overrides(..) should not consider return types
Summary: [1.5][dom] MethodBinding#overrides(..) should not consider return types
Status: VERIFIED FIXED
Alias: None
Product: JDT
Classification: Eclipse Project
Component: Core (show other bugs)
Version: 3.1   Edit
Hardware: PC Windows XP
: P3 normal (vote)
Target Milestone: 3.1.2   Edit
Assignee: Kent Johnson CLA
QA Contact:
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2005-08-02 11:40 EDT by Markus Keller CLA
Modified: 2006-01-10 09:11 EST (History)
4 users (show)

See Also:


Attachments
Regression test (1.72 KB, patch)
2005-08-26 05:05 EDT, Jerome Lanneluc CLA
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Markus Keller CLA 2005-08-02 11:40:39 EDT
I20050726-1222 (already a problem in 3.1)

IMethodBinding#overrides(..) should not consider return types. According to JLS3
§8.4.8.1, only method signatures are considered for the override check.

class Top {
    Integer m() {
        return 1;
    }
}

class Sub extends Top {
    @Override
    Object m() { // overrides Top#m(), in spite of the compile error
        return null;
    }
}
Comment 1 Jerome Lanneluc CLA 2005-08-26 05:03:30 EDT
MethodVerifier#doesMethodOverride(...) takes the return types into account. Need
to check with Philippe (when he's back) if this is intended.
Comment 2 Jerome Lanneluc CLA 2005-08-26 05:05:57 EDT
Created attachment 26522 [details]
Regression test
Comment 3 Jerome Lanneluc CLA 2005-09-08 10:16:43 EDT
Kent, could you please move this bug back to me once you've provided a method
that doesn't consider return types ?
Comment 4 Kent Johnson CLA 2005-09-08 11:33:38 EDT
do you need this in 3.1.1 or just 3.2?
Comment 5 Dirk Baeumer CLA 2005-09-08 12:12:34 EDT
Not needed for 3.1.1.
Comment 6 Kent Johnson CLA 2005-10-11 14:04:59 EDT
I just tried this testcase with javac and I got the following errors:

X.java:9: m() in Sub cannot override m() in Top; attempting to use incompa
tible return type
found   : java.lang.Object
required: java.lang.Integer
    Object m() { // overrides Top#m(), in spite of the compile error
           ^

X.java:8: method does not override a method from its superclass
    @Override
     ^


So can someone tell me why we should ignore return types ?
Comment 7 Kent Johnson CLA 2005-10-11 14:21:00 EDT
*** Bug 108782 has been marked as a duplicate of this bug. ***
Comment 8 Kent Johnson CLA 2005-10-11 14:22:41 EDT
The case from bug 108782:

class Sup2<E> {
	E foo(E e) { // m2
		return null;
	}
}

class Sub2<T> extends Sup2<T> {
	@Override
	Object foo(T arg0) { // m1
		return null;
	}
}


Also related to bug 108780, where the case is:

class Sup1<E> {
	E foo(E e) { // m2
		return null;
	}
}

class Sub1<T> extends Sup1<T> {
	@Override
	T foo(Object arg0) { // m1
		return null;
	}
}
Comment 9 Jerome Lanneluc CLA 2005-10-11 17:10:41 EDT
(In reply to comment #6)
> I just tried this testcase with javac and I got the following errors:
> 
> X.java:9: m() in Sub cannot override m() in Top; attempting to use incompa
> tible return type
> found   : java.lang.Object
> required: java.lang.Integer
>     Object m() { // overrides Top#m(), in spite of the compile error
>            ^
> 
> X.java:8: method does not override a method from its superclass
>     @Override
>      ^
> 
> 
> So can someone tell me why we should ignore return types ?

javac checks if the m() in Sub overrides m() in Top AND if the return types are
compatible (as we do in the compiler).

Here we just ask you to extract the overrides() part.
Comment 10 Markus Keller CLA 2005-10-12 04:15:55 EDT
Kent, to give you some context: JDT/UI currently uses their own overrides tests
for the override indicator and for quick fixes. We'd like to switch to the
JDT/Core API, but we can only do so if we don't lose functionality.

IMethodBinding#overrides(..) should adhere to the spec in JLS3, which does not
consider return types. Since Sub#m() overrides Top#m(), we e.g. want to show an
override indicator. That the compiler issues error messages is irrelevant for us
in this case.
Comment 11 Kent Johnson CLA 2005-10-12 10:27:28 EDT
So let me get this straight...

You want a method #overrides() that answers true when the compiler generates 
an error saying it doesn't?

Does that make sense to anyone?
Comment 12 Jerome Lanneluc CLA 2005-10-12 10:41:29 EDT
I interpret the compiler message as follows: 
It says that Sub#m() DOES override Top#m() BUT its return type is incompatible,
so this is not allowed.

Are YOU saying that the compiler doesn't follow JLS3 §8.4.8.1 ? ;-)
Comment 13 Markus Keller CLA 2005-10-12 14:08:37 EDT
(In reply to comment #11)
> You want a method #overrides() that answers true when the compiler generates 
> an error saying it doesn't?

What compiler message are you talking about? The eclipse compiler issues two errors:
1. "The return type is incompatible with Top.m()"
     -> Does not exclude in any way that Sub#m() overrides Top#m().
2. "The method m() of type Sub must override a superclass method"
     -> This is wrong and the subject of bug 108782.

The error messages that javac generates are of minor importance for me ...
Comment 14 Kent Johnson CLA 2005-10-12 17:09:37 EDT
Ok, we'll change the existing method #doesMethodOverride in the MethodVerifier.

I'm also changing the behaviour of the override error, we will no longer 
report it if the methods are equal, but their return types are not.
Comment 15 Kent Johnson CLA 2005-10-13 12:55:36 EDT
Changed the existing method MethodVerifier#doesMethodOverride()

Also see bug 108782 for the override error issue.

Released into HEAD and the 3.1.2 branch
Comment 16 Jerome Lanneluc CLA 2005-10-14 04:48:13 EDT
Released regression test from comment 2 (renamed to test032()) in both HEAD and
R3_1_maintenance branch.
Comment 17 Frederic Fusier CLA 2006-01-10 07:25:59 EST
Verified for 3.1.2 using build M20060109-1200.
Comment 18 Frederic Fusier CLA 2006-01-10 09:11:45 EST
Verified for 3.2 M4 using build I20051215-1506.