Community
Participate
Working Groups
The MethodOverrideTest from bug 107110 reveals further problems with IMethodBinding#isSubsignature(..): Cases where method from B is *not* a subsignature of the method from A: 1.: class A<T> { public void o1_xoo2(A<?> s) {} } class B<S> extends A<S> { @Override public void o1_xoo2(A<Object> s) {} } 2.: class A<T> { public void o1_xoo3(A<? extends T> s) {} } class B<S> extends A<S> { @Override public void o1_xoo3(A<? super S> s) {} } 3.: class A<S, T> { public void o2_xoo1(List<? extends T> t) {} } class B<V, W> extends A<W, V> { @Override public void o2_xoo1(List<? extends W> t) {} } 4. class A { public void o3_xoo1(List t) {} } class B extends A { @Override public void o3_xoo1(List<Object> t) {} } 5. class A<T> { public A() {} public void o4_xoo1(T t) {} } class B extends A<List<String>> { @Override public void o4_xoo1(List<?> t) {} } 6. class A<S> { public <X, Y> void tp1_xoo3(X x, Y y) {} } class B extends A<String> { @Override public <W, V> void tp1_xoo3(V x, W y) {} } And a case where the method from B *is* a subsignature of the method from A: class A<S> { public <X, Y> void tp1_foo2(S s, X x, Y y) {} } class B extends A<String> { @Override public void tp1_foo2(String s, Object x, Object y) {} } BTW: The ASTView 1.0.5 also shows isSubsignature in the comparison tray: http://dev.eclipse.org/viewcvs/index.cgi/%7Echeckout%7E/jdt-ui-home/plugins/org.eclipse.jdt.astview/AST-View-Site
Sorry it took so long. With 3.3RC4, what I'm seeing is: - 1, 2, 3, and 4: method from B *is* a subsignature of the method from A. This looks correct to me since in the 4 cases, the erasure of the methods are the same - 5 and 6: method from B is *not* a subsignature of the method from A. This looks correct to me since in both cases, the erasure of the methods are not the same - last case: method from B is *not* a subsignature of the method from A. This looks correct to me since in both cases, the erasure of the methods are not the same Markus, please reopen if you don't agree with this analysis
Here's the latest output: (I20070921-0919) a.) ==================================== testOverride1 ==================================== IMethodBinding.overrides(): false IMethodBinding.isSubsignature(): true Bindings.isSubsignature(): false Override Annotation: false // Overridden: ---------------------------------- public class A<T> { public void o1_xoo2(A<?> s) { } } // Overriding: ---------------------------------- class B<S> extends A<S> { @Override public void o1_xoo2(A<Object> s) { } } b.) ==================================== testOverride1 ==================================== IMethodBinding.overrides(): false IMethodBinding.isSubsignature(): true Bindings.isSubsignature(): false Override Annotation: false // Overridden: ---------------------------------- public class A<T> { public void o1_xoo3(A<? extends T> s) { } } // Overriding: ---------------------------------- class B<S> extends A<S> { @Override public void o1_xoo3(A<? super S> s) { } } c.) ==================================== testOverride2 ==================================== IMethodBinding.overrides(): false IMethodBinding.isSubsignature(): true Bindings.isSubsignature(): false Override Annotation: false // Overridden: ---------------------------------- public class A<S, T> { public void o2_xoo1(List<? extends T> t) { } } // Overriding: ---------------------------------- class B<V, W> extends A<W, V> { @Override public void o2_xoo1(List<? extends W> t) { } } d.) ==================================== testOverride3 ==================================== IMethodBinding.overrides(): false IMethodBinding.isSubsignature(): true Bindings.isSubsignature(): false Override Annotation: false // Overridden: ---------------------------------- public class A { public void o3_xoo1(List t) { } } // Overriding: ---------------------------------- class B extends A { @Override public void o3_xoo1(List<Object> t) { } } e.) ==================================== testOverride4 ==================================== IMethodBinding.overrides(): false IMethodBinding.isSubsignature(): true Bindings.isSubsignature(): false Override Annotation: false // Overridden: ---------------------------------- public class A<T> { public void o4_xoo1(T t) { } } // Overriding: ---------------------------------- class B extends A<List<String>> { @Override public void o4_xoo1(List<?> t) { } } f.) ==================================== testOverrideMethodTypeParams1 ==================================== IMethodBinding.overrides(): false IMethodBinding.isSubsignature(): true Bindings.isSubsignature(): true Override Annotation: false // Overridden: ---------------------------------- public class A<S> { public <X, Y> void tp1_xoo3(X x, Y y) { } } // Overriding: ---------------------------------- class B extends A<String> { @Override public <W, V> void tp1_xoo3(V x, W y) { } } g.) ==================================== testOverrideMethodTypeParams1 ==================================== IMethodBinding.overrides(): true IMethodBinding.isSubsignature(): false Bindings.isSubsignature(): true Override Annotation: true // Overridden: ---------------------------------- public class A<S> { public <X, Y> void tp1_foo2(S s, X x, Y y) { } } // Overriding: ---------------------------------- class B extends A<String> { @Override public void tp1_foo2(String s, Object x, Object y) { } } ---------------------------------------------------- My understanding is that a method overrides when - the subtype's method has a signature that is a subsignature of the supertype's method - and the two methods have compatible return types and throws clauses. a.) - f.) return types and throws clauses are the same: Why are 'overrides' and 'isSubsignature' different? g.) methods are not a subsignature. How can they then override?
To get the output below: Go to MethodOverrideTest, and set DEBUG_SHOWRESULTS to true. bindingOverrides= overridingBinding.getMethodDeclaration().overrides( overriddenBinding.getMethodDeclaration()); bindingIsSubsignature= overridingBinding.isSubsignature(overriddenBinding); override annotation: We test if there is a compilation error on the @Override annotation Bindings.isSubsignature() Our own implementation, not 100% correct
It looks like I was wrong on the definition of the subsignature. From the spec at http://java.sun.com/docs/books/jls/third_edition/html/classes.html#38649: The signature of a method m1 is a subsignature of the signature of a method m2 if either * m2 has the same signature as m1, or * the signature of m1 is the same as the erasure of the signature of m2. IMethodBinding#isSubsignature(IMethodBinding) needs to be fixed.
Created attachment 80030 [details] Proposed fix and regression tests Kent, what do you think of the fix? ( Note that IMethodBinding#isSubsignature(IMethodBinding) is implementing subsignature as defined in http://java.sun.com/docs/books/jls/third_edition/html/classes.html#8.4.2 and IMethodBinding#overrides(IMethodBinding) is implementing overrides as defined in http://java.sun.com/docs/books/jls/third_edition/html/classes.html#8.4.8.1. Would it make sense to have these helpers on MethodVerifier?
(forgot to add Kent on CC list) Kent, please see comment 5.
Created attachment 80133 [details] New proposed fix and same regression tests Fixes a small regression
I would try something like this : private boolean overrides(IMethodBinding overridenMethod, boolean checkVisibility) { try { org.eclipse.jdt.internal.compiler.lookup.MethodBinding overridenBinding = ((MethodBinding) overridenMethod).binding; if (!CharOperation.equals(this.binding.selector, overridenBinding.selector)) return false; if (checkVisibility) { if (this.binding == overridenBinding || overridenBinding.isStatic() || this.binding.isStatic()) return false; if (overridenBinding.isPrivate()) return false; if (overridenBinding.isDefault() && overridenBinding.declaringClass.getPackage() != this.binding.declaringClass.getPackage()) return false; } LookupEnvironment lookupEnvironment = this.resolver.lookupEnvironment(); if (lookupEnvironment == null) return false; overridenBinding = overridenBinding.original(); TypeBinding match = this.binding.declaringClass.findSuperTypeOriginatingFrom(overridenBinding.declaringClass); if (!(match instanceof ReferenceBinding)) return false; if (match == overridenBinding.declaringClass) return lookupEnvironment.methodVerifier().doesMethodOverride(this.binding, overridenBinding); org.eclipse.jdt.internal.compiler.lookup.MethodBinding[] superMethods = ((ReferenceBinding) match).getMethods(overridenBinding.selector); for (int i = 0, length = superMethods.length; i < length; i++) if (superMethods[i].original() == overridenBinding) return lookupEnvironment.methodVerifier().doesMethodOverride(this.binding, superMethods[i]); return false; } catch (AbortCompilation e) { // don't surface internal exception to clients // see https://bugs.eclipse.org/bugs/show_bug.cgi?id=143013 return false; } } We can move these to the verifier if you want.
Created attachment 80344 [details] Kent's fix and more regression tests Thanks Kent. Unfortunately, we are still misbehaving in the case of isSubsignature() with the 2 methods in different hierarchies. I attached a new test (test044()) that is currently failing with this last fix. Otherwise, if this code could be moved to the method verifier, it would be great.
So in this case, the 2 methods are supposed to be "subsignatures" : class X { void foo() {} } class Y { void foo() {} } Are they also subsignatures in this case ? class X<T> { void foo(T t, Comparable<T> c) {} } class Y<U> { void foo(U u, Comparable<U> c) {} } Because if they are, then I think we will have a big problem since U != T.
(In reply to comment #10) > So in this case, the 2 methods are supposed to be "subsignatures" : > > class X { > void foo() {} > } > > class Y { > void foo() {} > } > > Are they also subsignatures in this case ? > > class X<T> { > void foo(T t, Comparable<T> c) {} > } > > class Y<U> { > void foo(U u, Comparable<U> c) {} > } > > Because if they are, then I think we will have a big problem since U != T. > According to http://java.sun.com/docs/books/jls/third_edition/html/classes.html#8.4.2, Y#foo(T, Comparable<T>) is not a subsignature of X#foo(U, Comparable<U>) since the signatures are not equal, nor the signature of Y#foo(T, Comparable<T>) is the same as the erasure of the signature of X#foo(U, Comparable<U>)
Reassigning to Kent for a cleaner fix (to add an internal API on MethodVerifier)
David - I see 4 calls to the verifier from the CompletionEngine. findLocalMethodDeclarations findLocalMethods findLocalMethodsFromFavorites findLocalMethodsOfStaticImports All these methods call MethodVerifier.doesMethodOverride() We're trying to make this method more accurate for the DOM, but the code assists tests fail if enforce the rule that the first method is from a subtype of the second method. Can you tell me what exactly you're looking for ? Do you just want to know if the method's parameters are equal ?
David - please see the questions in comment 13
Created attachment 81493 [details] Proposed patch This patch changes MethodVerifier.doesMethodOverride() to enforce the visibility & inheritance requirements of the DOM. Some users such as Code Assist and APT may need to change their senders to MethodVerifier.isMethodSubsignature()
Walter - please try the patch & see if the APT senders of doesMethodOverride() need to be converted over to isMethodSubsignature().
Created attachment 81523 [details] patch for APT That JDT patch does break org.eclipse.jdt.apt.compiler.tests.ModelUtilTests. Our impl of ExecutableElement.hides(Element) is based on doesMethodOverride(). Changing to isMethodSubsignature() appears to be logically correct, and fixes the test. We also call doesMethodOverride() when we list all visible entities in a type, in our impl of Elements.getAllMembers. We might be able to optimize a little by calling isMethodSubsignature() there, since we're guaranteed to be in the context of a single type, but doesMethodOverride() also seems correct and passes tests. I'll attach the necessary patch to o.e.jdt.compiler.apt. Please either check this patch in at the same time as the JDT patch, or let Olivier or me know so we can check it in, to avoid breaking the APT tests.
Answer to comment 12 Currently doesMethodOverride() is called to know if the same method in a subclass is already proposed as a possible completion or not. Code Assist must not take visibility into account and doesn't need to verify inheritance requirements. With the proposed patch the right method to call seems to be isMethodSubsignature() but the test CompletionTests_1_5#test0291() failed and should not.
Yes David, I saw the failure yesterday after I posted the patch. Will investigate a 'newer' patch.
Created attachment 81635 [details] Proposed patch Fixed the problem with the Code assist test. Integrated APT changes. Walter - when you have time, please try out this patch. thx
Released into HEAD for 3.4M4 Includes changes to APT.ExecutableElementImpl & new tests (35-44) in CompatibilityRulesTests
Verified for 3.4M4 using build I20071210-1800.
