Community
Participate
Working Groups
BETA_JAVA8 JLS 8.1.1.1 defines the criteria when a class needs to be declared as abstract. These rules have been rewritten to account for default methods. The new rules rely on two notions that have been updated/introduced for Java8: - methods inherited by a given class (8.4.8 - modified) - a method "overrides" another method "from C" (8.4.8.1 - modified, concept "override from C" is new)
My current patch affects some expected test results. Most of these simply change the order of types in the error message (which is expected). One change, however, is somewhat more significant, so for the records: See org.eclipse.jdt.core.tests.compiler.regression.GenericTypeTest.test0987() Previously we expected a secondary error against GLinkElementView: - The return types are incompatible for the inherited methods EditPart.getViewer(), AbstractLinkView<M>.getViewer() IMO, this message is useless, since the same conflict is already reported against the superclass AbstractLinkView and GLinkElementView adds nothing to the problem. For comparison: javac7 reports the same error both against AbstractLinkView and against GLinkElementView: - getViewer() in AbstractLinkView cannot implement getViewer() in EditPart return type ISheetViewer is not compatible with EditPartViewer Thus my patch introduces a slight difference between javac and ecj but only regarding a secondary error. The primary error is reported in similar ways. The same holds for test0988()
Implementation notes of the patch-under-construction: This feature builds on and improves the result of bug 388795 (see bug 388795 comment 7 for explanations): (a) when doing two-way comparison of methods (as introduced in bug 388795), we need to know which direction succeeded -> moving control from areMethodsCompatible() to its caller (b) while working on the patch I saw one message in MethodVerifierTest.test140() change: mentioning also I.foo() although that one is overridden by J.foo(), which is already mentioned itself. -> fixed by collecting another bool array 'isOverridden' and passing down up-to ProblemReporter.inheritedMethodsHaveIncompatibleReturnTypes(..) -> to ensure that actually all pairs of methods are tested for overriding, I split the main loop inside checkMethods() into two subsequent loops. The first loop ignores the skip flags, which are only tested in the second loop. (c) while changing the signature of checkInheritedMethods(), I realized that the super version in MethodVerifier is never called because MethodVerifier is never instantiated -> made MethodVerifier and its checkInheritedMethods() abstract. (d) The main part in this implementation is the use of a new Sorting facility to ensure that we see super types in topological order. -> previously, various loops inside computeInheritedMethods collected the super interfaces in no specific order. -> now this is simplified into collectAllDistinctSuperInterfaces() and the result is then topologically sorted. (e) Also methods are sorted before checking to ensure that if we have a concrete methods this one shows up in position [0] (needed by DefaultMethodtTest.testAbstract02a()). The net effect is that all new tests in DefaultMethodTest.testAbstract*() pass with hardly any special handling in the code. I've pushed my current work as a remote branch to http://git.eclipse.org/c/jdt/eclipse.jdt.core.git/log/?h=sherrmann/Bug388739
Although I've seen all tests pass with this current patch, some critique is due: The code contains some FIXMEs where in my understanding the code should actually look differently. Unfortunately, the "correct" version creates regressions in tests and only this version with awkward tweaks passes the tests. -> My understanding is obviously wrong and I need to find out why this version works and the 'correct' one doesn't Also some refactoring is called for to make checkMethods() better readable, but again straight-forward code-"improvement" leads to regressions. My uneasy feelings are amplified by observations where methods were processed in seemingly arbitrary order, and only one specific order would let tests pass, while another - seemingly correct - order would cause regressions. Ergo, I need to better understand *why* the tests are currently passing. Perhaps more bugs a la bug 388795 are lurking here.
I succeeded with the desired cleanup (see comment 3) and pushed the result to the branch (commit 7b88699d7247f89fea7506d2e4eadd50330f19b3). Nothing magic, actually, just getting all relevant details right simultaneously. Incidentally, this also fixed bug 395681.
After more testing I'm confident that the result in the branch is what we want. Pushed to BETA_JAVA8 as commit 1e8305535ff304200778a08eb6eb4025791a09c1. I'll keep the feature branch because the intermediate commit (squashed in BETA_JAVA8) may be useful for documenting the genesis of this change.
