Community
Participate
Working Groups
The fix for bug 388795 accidentally interacts with the solution in bug 317719, which manifests in more errors being reported in MethodVerifyTest.testBug317719f in 1.6 mode. Ideally, in 1.6 mode this test should be silent to mimic the bug in javac6.
Bug 388739 contains changes on top of bug 388795 which also fix this bug. I'll take a look how much of those changes should be considered for master.
Stephan, I guess this moves to M6.
(In reply to comment #2) > Stephan, I guess this moves to M6. I agree, bug 388739 is a bit delicate, backporting parts of it shouldn't be done in a haste.
Stephan, do you think this can still go in for M6?
No time for this in M6. Let's target M7.
First result of investigation: The reason, the error was not reported prior to bug 388795 was indeed just in the order in which we looked at the two methods in question. The error was masked because isSubstituteParameterSubsignature(inheritedMethod, otherInheritedMethod) answered false, while the necessary additional check isSubstituteParameterSubsignature(otherInheritedMethod, inheritedMethod) indeed answers true. This means a point-fix without backporting more from BETA_JAVA8 is probably not possible.
Back porting worked pretty painlessly. Still I suggest to weigh the impact of this change against its benefit. Some test results change as discussed in bug 388739 comment 1. I *believe* these changes to be OK. The implementation changes two aspects at the heart of the method verifier: - the way how super types are collected, now uses topological sort (as it was motivated by new rules for default methods) - the logic for finding duplicates / clashes etc. has been re-worked from a single loop to a two-phase approach and also introducing two-way comparison in some places As for the benefit: All this is only to re-establish one aspect of bug compatibility against javac6. Well, bug 317719 did show some very strong opinions. I'm currently checking which parts of the patch can be omitted for Kepler (method sorting seems to be necessary only for default methods etc.). Once that's done and tested I'll post a patch.
Created attachment 229504 [details] proposed fix Here's the patch back ported from bug 388795 and slightly trimmed. All JDT/Core tests pass.
A gentle reminder that Dani suggested a review for this one.
(In reply to comment #9) > A gentle reminder that Dani suggested a review for this one. Sorry for the delay - I have been focused on wrapping up lambda code generation. I'll review it early next week.
Having studied the comments in this and all the related bugs and glanced through the patch, I recommend that this be retargetted for 4.4 M1 - My observations obviously have nothing to do with QOI concerns, but only with the timing and the need for risk mitigation and weighing in the purported benefits. As comment#7 calls out, this fix calls for fairly involved changes at the heart of the method verifier in return for bug compatibility. Given this bug is internally raised (well, I agree that is a tenuous argument to go by, any one or more of the open bugs could be a duplicate of this) and further given that for the test, difference in compiler behavior on which is the raison d'être for the present bug, javac 7 has changed behavior to reject the program, the scales are tilted I would think. I agree bug 317719 generated a fair bit of unwelcome/unfair noise - particularly since ECJ behavior was correct - but all I would change about that particular exercise is for us to have stopped the push back mode just a tad sooner but no more sooner. As found out in that exercise, bug compatibility has its own perils and there were numerous cases where we could make neither head nor tail of javac behavior.
(In reply to comment #11) > Given this bug is internally raised (well, I agree that is a tenuous argument > to go by, any one or more of the open bugs could be a duplicate of this) Duh. I missed looking at the test name. Sigh. So it is not a question of some JDK6 incompatibility we uncovered in the course of the fix for bug 388795 but the very same incompatibility that generated a good bit of heat earlier. OK, Jay is doing some stress testing of the fix and I'll continue with my review and we'll take a call soon - sorry about the noise.
Patch looks good. Since it fixes a regression on a behavior that caused quite a bit of grief, it is a good candidate for early inclusion. Jay got a good bit of stress testing done on it and the results are green Stephan, please release asap, Thanks !
Thanks for the review and testing. Released for 4.3 M7 via commit 4dd974a226271180ff02d909a12722017f80ff3a
Verified for 4.3 M7 Build id: I20130501-2000
