Community
Participate
Working Groups
Created attachment 113880 [details] proposed fix Build ID: M20080911-1700 One interface: public interface I1 { Object m(); } and two classes: public class C1 { public abstract Object m(); } public class C2 extends C1 implements I1 { } The compiler reports one error in C1 (OK) and the following in C2: The type C2 must implement the inherited abstract method C1.m() Reading the code in MethodVerifier#checkInheritedMethods it looks very much like this is not exactly intended: C1.m() should _not_ be reported against C2, because this is already reported against C1. Instead, I1.m() should be used in the message. The use of a constant array index 0 is inherited all the way from MethodVerifier.java version 1.1, at what time reporting happened _after_ the loop. Version 1.30 extended the loop to include reporting but did not update the indices. Attaching a micro patch.
Created attachment 113997 [details] better patch The previous patch caused some regressions in MethodVerifierTests. By reversing the loop more interesting methods are found first. All compiler and model tests are unaffected by the updated patch. (I also missed a third occurrence of [0] in the first patch).
This is a case when we should consider NOT reporting a secondary error at all. Since C1.m() is an abstract method in a NON-abstract class, it won't matter if the user implements m() in C2 since he must do something about the error in C1 no matter what. C1 must be made abstract or C1.m() must be implemented instead of being tagged as abstract. Thoughts? Do either of you think that an error on C2 is useful ?
(In reply to comment #2) > This is a case when we should consider NOT reporting a secondary error at all. > > Since C1.m() is an abstract method in a NON-abstract class, it won't matter if > the user implements m() in C2 since he must do something about the error in C1 > no matter what. > > C1 must be made abstract or C1.m() must be implemented instead of being tagged > as abstract. > > > Thoughts? Do either of you think that an error on C2 is useful ? You might want to think of a scenario where the primary error is corrected by removing C1.m(). (note that C1 has no obligation to provide a method m()). In that case the user _might_ be mildly surprised to find a new error popping up in C2, because C2's obligation to have an implementation for I1.m() is not new at that time, and C2 has never taken any (effective) measures to do so. With my patch that "new" error would be reported already in the first place. However, I _could_ also by your argument ;-)
>You might want to think of a scenario where the primary error is corrected by >removing C1.m(). >(note that C1 has no obligation to provide a method m()). But this issue is only a result of C1 supplying an incorrect abstract m(). If C1 implemented m() then no error would be reported against C2. I'm just not convinced that 2 errors (1 on C1 & 1 on C2) is worthwhile when the user should fix the error on C1 first. I accept that can cause an error on C2 to show up, but I'm fine with that case.
If C1 and C2 are in different files, then I would expect both to be flagged. The only case where I could imagine C2 to no longer carry an error is if we'd be resilient for C1.m() and drop its abstract modifier. But this would be problematic, since then we'd have to handle its lack of a body. I think I am fine with the current behavior; since not only the method on C1 is tagged explicitly as abstract (i.e. someone typed it in, so it was intended) and the body is a semicolon, which again is an indication of the method being intended to be abstract. Assuming the class is missing an abstract modifier is my inclination (like user is adding the abstract method, and didn't have time yet to fix the class definition). So I do not really see a problem with our present behavior, assuming the method on C1 is really intended to be abstract in the end. BTW - Kent: can you check the outcome on the problem type we issue ? Are we tagging it as 'abstract' or if not, is this causing some verify error ?
(In reply to comment #5) > I think I am fine with the current behavior; "current behavior" as in "the number of errors reported" or in "the exact error messages provided"? Here is a scenario that really looks confusing to me: (1) Start with C1 and C2 only (no "implements" declaration in C2): -> Only one error reported (against C1). (2) Add "implements I1" to C2 and see this additional error: "The type C2 must implement the inherited abstract method C1.m()" So what I was trying with my patch is: - establish consistency that abstract method inherited from non-abstract class is never reported as a secondary error - if an abstract method from an interface is unimplemented always report mentioning the interface never the buggy superclass. From reading the code, this seems to be the original intention any way ;-)
And I agree that your patch succeeds, but... 1. this is not a common case (non-abstract superclass with abstract method) 2. the current code assumes the first inherited abstract method is worth reporting in the error against C2 and except for this case, that's still true 3. even if we release your patch, I still think a user should be directed to the more important error on C1... but I guess we still need some error on C2 to ensure its a problem type.
> Are we tagging it as 'abstract' or if not, is this causing some verify error? Tried: public class X { public static void main(String[] s) { new X().m(); } public abstract Object m(); } X.m() is NOT abstract in the problem .class file: public class X extends java.lang.Object{ public X(); public static void main(java.lang.String[]); public java.lang.Object m(); } But we fail as expected: Exception in thread "main" java.lang.Error: Unresolved compilation problem: The abstract method m in type X can only be defined by an abstract class
So we have 2 choices : Make the .class file for the problem type C1 tag the method m() as abstract (which it currently does not) and also release Stephan's patch to report a valid error on C2. OR We do not correct the .class file for C1 (method m is non-abstract) & then no error appears on C2 whenever its incrementally compiled. So we should NOT report the error against C2 in a full build & instead tag C1.m() as non-abstract in the SourceTypeBinding. Make sense ? Do you have a preference ? My vote is to remove the abstract modifier on the method C1.m() when we detect that C1 is not abstract. No .class file change is needed & no error will be reported against C2 since it appears that I.m() is implemented by C1. If the user fixes the error on C1 by making C1 abstract, then an error will show up on C2.
Created attachment 114598 [details] Proposed patch This change makes the method C1.m() non-abstract for the rest of the build so no error is reported against C2
No votes for 1 of the 2 choices in comment 9 ?
(In reply to comment #11) > No votes for 1 of the 2 choices in comment 9 ? Well, if you ask me: for the user both choices seem consistent. Choice 2 reduces potential noise in the problems view, which is good. Also consistency between incremental and full build is of course great ;-) Other than these, I still see my comment 3 valid. So actually: I'm undecided :) One CAVEAT: your patch from comment 10 conflicts with your patch for bug 249140 (just change I.m() in this bug to return String). The latter patch depends on still finding the method abstract ... Also, if MethodVerifier#checkInheritedMethods remains unchanged, I would suggest a comment explaining why all methods are investigated, and still an error is reported constantly against methods[0] (what's the assumption about the contents at position 0?) Mh, I made a quick experiment removing this check: if (mustImplementAbstractMethod(methods[i].declaringClass)) { and I couldn't find any behavioral difference (with your patch from comment 10 in place). Would this check become obsolete by the patch? Note that this is just a quick guess.
With this case : interface I { String m(); } class C1 { abstract Object m(); } class C2 extends C1 implements I { } And the patch from comment 10 & bug 249140 I get 2 errors that seem fine to me: The abstract method m in type C1 can only be defined by an abstract class The return type is incompatible with I.m(), C1.m() Are you seeing something else ? I'm not against releasing your change, but it won't change anything if we eliminate the case of an abstract method in a non-abstract class. If a concrete method is not found, then the assumption would remain valid that all other methods are abstract, so using the method at position 0 is fine. But your change does make the code more readable, even tho it won't change our behaviour.
(In reply to comment #13) > With this case : > [...] > The abstract method m in type C1 can only be defined by an abstract class > The return type is incompatible with I.m(), C1.m() > > Are you seeing something else ? Exactly the same. I understood your patch from bug 249140 should avoid this incompatibility message if the current class can simply implement both inherited abstract methods with just one "String m() { ... }". The example in this bug is "the same" as bug 249140, *if* the error in C1 is a missing class level "abstract". Sure it is difficult to guess what is the "right error" to report since we have to guess whether C1 has too many or too few "abstract" modifiers. In addition to guessing the users intention we can also challenge the solution by checking whether some source-changes can cause confusing changes in the errors reported. In this case: adding/removing "abstract" for class C1 would add/hide an incompatibility error in C2, where the connection between cause and effect is difficult to understand. (may actually require to know that the compiler assumes C1.m() to be intended as non-abstract). So my gut-feeling is: if we guess that C1.m() was _intended_ to be non-abstract, then error messages can change in funny ways, once code changes prove the guess wrong. With less guess-work errors might be more stable. > I'm not against releasing your change, but it won't change anything if we > eliminate the case of an abstract method in a non-abstract class. With my patch instead of your's we could leave the abstract C1.m abstract and would then benefit from the patch from bug 249140, too, no? > If a concrete method is not found, then the assumption would remain valid that > all other methods are abstract, so using the method at position 0 is fine. Is the order in "methods" related to the lexical ordering in the source? Is that why position 0 produces a "better" message, because it's the _first_ place to look for? > But your change does make the code more readable, even tho it won't change our > behaviour. Actually, I didn't mean to trigger a controversial discussion ;-) I just had the smell that the actual code did something different than what it suggested at first reading, which could *either* be a bug *or* missing documentation of the actual intention.
>With my patch instead of your's we could leave the abstract C1.m abstract >and would then benefit from the patch from bug 249140, too, no? Need to change the .class file for C1 so the method is marked as abstract. >Is the order in "methods" related to the lexical ordering in the source? The methods are 'found' by walking the hierarchy - superclasses first. So the method at position 0 is the closest to the type & usually the best choice to complain against. So this is the case from bug 249140 that was 'fixed' to say C2 needs to implement C1.m() [even tho I1.m is more specific] interface I1 { String m(); } abstract class C1 { abstract Object m(); } class C2 extends C1 implements I1 {} When C1 is no longer abstract OR C1.m() is no longer abstract, we're still complaining about incompatible return types. So more work to do...
CRAP! interface I1 { String m(); } class C1 { public Object m() { return null; } } class C2 extends C1 implements I1 {} It makes sense to me that we complain against C2 about incompatible return types in this case. C1.m() is NOT a compatible implementation of I1.m(), and if we only report that C2 needs to implement I1.m(), then a user may never realize that C1.m() was incompatible. Yes, C2 can define String m(), but maybe the user meant to inherit m() from C1. And why should we be different in these 2 cases ? class C1 { public abstract Object m(); } // assume abstract is ignored on m() OR abstract class C1 { public Object m() { return null; } }
(In reply to comment #16) > interface I1 { > String m(); > } > class C1 { > public Object m() { return null; } > } > class C2 extends C1 implements I1 {} > > It makes sense to me that we complain against C2 about incompatible return > types in this case. Sure. > And why should we be different in these 2 cases ? > > class C1 { public abstract Object m(); } // assume abstract is ignored on m() > OR > abstract class C1 { public Object m() { return null; } } maybe because your assumption (abstract being ignored) is questionable? IMHO we'll have to draw a line between reporting incompatibility vs. abstractness. I'm proposing to report incompatibility IFF a real non-abstract method is inherited. I don't see another similarly clear line (except: always report the incompatibility, too).
(sorry for late response, but still travelling) > Re: comment 9 I would be enclined to make the method abstract, if it is defined as such in source, providing the VM is tolerating the presence of an abstract method in non abstract type. The case I have in mind is someone adding an abstract method incrementally into an existing concrete class. He will add abstract modifier to the class after the fact. So the abstract modifier is actually meant on the method. As I said earlier, I am not too concerned about the secondary error, since it is a rare case, where the behavior is actually explainable, if you accept that the abstract method is actually abstract. I agree that method[0] is not always the best choice though. This part I could see us improve a bit to be more relevant; but again if this is a super rare case, then don't we have something more important to fix ? All we are talking about here is a small change in the wording of the error message. Stephan - can you explain why this is so important ? Maybe I am missing something...
> maybe because your assumption (abstract being ignored) is questionable? I agree - I go back & forth myself. > (except: always report the incompatibility, too). I thought about this too & I'm like it more. BTW: javac never reports an incompatibility (in any of the 4 cases) between the superclass' method & the superinterface's method. javac only complains about C2 needing to implement either C1's or I1's method. So when both are valid abstract methods, we'll report about the missing implementation (on the most specific abstract method - give it a try). For the other cases, I'm going to look into reporting both the incompatible return types and the missing interface method. Also need to look into keeping the abstract method in a concrete superclass marked as such in the .class file.
(In reply to comment #18) > Stephan - can you explain why this is so important ? Maybe I am missing > something... don't worry :) Initially I raised this at (severity minor!) simply because I needed to precisely understand that particular code and found this pattern: T[] array = ...; for (int i=array.length-1; i>0;) { if (condition(array[i])) { // <- check each element action(array[0]); // <- don't report checked element, but first return; } } Without any context nor comments this looks like a probable bug to me and I simply wanted to alert you of this finding (perhaps an oversight introduced at some point in the history of MethodVerifier). Eventually Kent and me got carried away by the intellectual challenge of finding the ultimately best errors reported in all cases. As a positive result of this discussion I see that we now better understand the interactions between these issues: - consistency between full and incremental compilation - guessing whether an abstract method was intended to be non-abstract - selecting the most suitable abstract method for reporting (several criteria are possible here) - error stability, i.e., previously hidden errors are not revealed due to seemingly unrelated source code changes. - bug 249140 I'm fully confident that Kent will wrap it all up with a simple and consistent solution (and won't complain any more, promised ;-)
(In reply to comment #19) > > (except: always report the incompatibility, too). > > I thought about this too & I'm like it more. If that's what you'll actually go for, I'd suggest to briefly reconsider the message's wording: "The return type is incompatible with I.m(), C1.m()" leaves me puzzled: what return type is incompatible with these methods? How about something along these lines: "The inherited methods I.m() and C1.m() have incompatible return types" ? > Also need to look into keeping the abstract method in a concrete superclass > marked as such in the .class file. Is that OK for the byte code verifier? A clean (but overkill) solution would be to generate a non-abstract problem method with a byte code attribute IWasAbstractInSource ;-) from which the BinaryTypeBinding would create a fresh ProblemMethodBinding marked as abstract, right?
Created attachment 117235 [details] Proposed patch with testcases This patch adds a new error for the case when an interface method is a better match than a superclass method. For example: interface I { String m(); } abstract class A { abstract Object m(); } class B extends A implements I {} We will now report the error : The type B must implement the inherited abstract method I.m() to override A.m() Any suggestions/improvements?
Created attachment 117239 [details] Proposed patch with testcases Added a binary typeBinding check to the 2 new tests
Could you also add a test with disassembled code to check the shape of the problem abstract method ? (you can only compare the signature fragment). Also, I still find this message cryptic: "The return type is incompatible with I.foo(Number), K.foo(Number), J.foo(Number)\n" I would prefer a message along the lines of what Stephan suggested: "The inherited methods I.m() and C1.m() have incompatible return types"
> Could you also add a test with disassembled code to check the shape of the > problem abstract method ? (you can only compare the signature fragment). Problem methods cannot be tagged as abstract since they have a body to report the error, so they're stored as non-abstract. > Also, I still find this message cryptic: > "The return type is incompatible with I.foo(Number), K.foo(Number), > J.foo(Number)\n" > > I would prefer a message along the lines of what Stephan suggested: > "The inherited methods I.m() and C1.m() have incompatible return types" The issue I have with that message is the 'reason' is at the end AND may not be visible in the UI if the method signatures are long enough. For example: "The inherited methods MyTypeWithBigName.m(String, Comparable<T>, int[]) and AnInterface.m(String, Comparable<T>, int[]) have incompatible return types" You would never know what the error is since 'incompatible return types' would not be visible.
(In reply to comment #25) > The issue I have with that message is the 'reason' is at the end AND may not be > visible in the UI if the method signatures are long enough. I just opened bug 254643 for this :) More directly to this issue: couldn't we simply re-arrange the wording to something like "Incompatible return types in inherited methods I.foo(Number), K.foo(Number), J.foo(Number)" Not being a native English speaker I only have a vague feeling that this is linguistically suboptimal.. I do think the new message from comment 22 is a good idea. Is my understanding correct that the most recent patch does not change the way abstract methods are flagged? So full build and incremental compilation can still produce different errors, right? (Not a complaint, just for clarification).
Since I'm the only one whose native language is English ! ;) I would suggest: "The return types are incompatible for the inherited methods I.foo(Number), K.foo(Number), J.foo(Number)" But Stephan's suggestion is fine too: "Incompatible return types in inherited methods I.foo(Number), K.foo(Number), J.foo(Number)" As for the .class file/abstract tagbit issue... no it wasn't changed but because of the new error message, we report it in both cases (from source vs. .class file) as the binary test shows.
Created attachment 118174 [details] Proposed patch with testcases This patch includes 3 new + 2 different error messages: 363 = The type {0} must be an abstract class to define abstract methods 418 = The type {3} must implement the inherited abstract method {2}.{0}({1}) to override {6}.{4}({5}) 419 = The return types are incompatible for the inherited methods {0} 763 = The enum constant {2} must implement the abstract method {0}({1}) 764 = The enum constant {0} cannot define abstract methods It also includes the change to problem types so abstract methods are dumped as is. Now our full vs. incremental build errors are the exact same.
One small issue: - "1. ERROR in p\\CC.java (at line 4)\n" + - " abstract void m(); // compile-time error \n" + - " ^^^\n" + - "The abstract method m in type CC can only be defined by an abstract class\n" + + "1. ERROR in p\\CC.java (at line 2)\n" + + " class CC {\n" + + " ^^\n" + + "The type CC must be an abstract class to define abstract methods\n" + Will the user still find the offending method m()? I understand the change is due to unifying reporting against source/binary types. Why not at least mention the method in the error message?
> Why not at least mention the method in the error message? Because there can be more than 1 method, but I can change the message to: The type A2 must be an abstract class to define abstract methods: m(), n(), o(), p() Or we let the user find the abstract methods by looking in the outliner for the abstract modifier.
Created attachment 118376 [details] LAST Proposed patch with testcases
Fix and tests released for 3.5M4
The fix in HEAD causes a regression in org.eclipse.jdt.ui.tests.quickfix.ModifierCorrectionsQuickFixTest.testAbstractMethodInNonAbstractClass(): The same is also immediately visible in the workbench when you hover over the problem: You get no quick fixes and an exception in the log. We expect to get an IProblem.AbstractMethodInAbstractClass for each abstract method in a non-abstract class, but now we only get a single problem with the same ID but different semantics. Could you please summarize why you think it's better to get a single problem on the type name rather than individual problems on the affected methods? The problem mentioned in comment 29 and comment 30 is a good indication that the chosen solution is not good. And enumerating multiple methods in the error message is a no-go, since it can explode the message length and make the argument hard to use for offering quick fixes.
Created attachment 118514 [details] Proposed patch with testcases To ensure quick fixes are available against the type and each abstract method, this patch will report problems on the type & the abstract methods. It maintains incremental and full build errors are the same.
(In reply to comment #34) Thanks, added a quick fix for the new error on the class and fixed our tests.
Verified for 3.5M4 using I20081208-1800
