Community
Participate
Eclipse IDE
If you change the getType() of VariableDeclarationStatement class to return double instead of int, three tests are failing in org.eclipse.jdt.ui.tests.refactoring.PromoteTempToFieldTests. They are test8, test13 and test15 (existing tests in 3.1). Three new tests are also failing. testMultiVariableDeclFragment01, testMultiVariableDeclFragment02, testMultiVariableDeclFragment03. These tests are new since 3.1 is out. The problem comes from the fact that the type of the variable declaration statement is not set. So a default type (int) is retrieved. I will attach a patch for it. This would be a candidate for 3.1.1.
Created attachment 24563 [details] Proposed fix This patch is based on 3.1 maintenance stream. Apply on jdt.ui project.
It is important to keep all tests clean at any time or create branch for each test suite based on 3.1. Otherwise it is difficult to find out why tests are failing after changes. I don't release code if I find a "potential" regression and I thought this was a regression coming from my changes.
Created attachment 24568 [details] Proposed fix New patch that also preserves the existing modifiers of the multiple local declaration. That declaration can potentially have modifiers (like final or an annotation). They must be preserve after the promotion. New tests would need to be added to the 3.1.1 stream. I can provide a patch for an updated test that has been added post 3.1.
Current code doesn't support annotation among the modifiers. I will provide a new patch for this.
Created attachment 24572 [details] Proposed fix With this patch the formatting of the modifiers is not preserved, but at least existing annotations are preserved as well as existing modifiers. This patch needs to be reviewed and all existing tests run.
Created attachment 24573 [details] Regression test Regression test based on HEAD contents because there is no branch for jdt/ui refactoring tests.
Update title according to latest comments.
Olivier, it would be really helpful to understand what JDT/Core changes in 3.1.1 so that JDT/UI has to fix this. I discussed this with Philippe and we agreed that fixes that break JDT/UI tests even if there is a bug in JDT/UI should be treated special Very likely a dup of 101980. We are shipping with this quite a while and we should be really careful about what goes into 3.1.1 and what not. Can you please comment on why this is critical from your point of view.
Yes, this seems to be a dup of bug 101980 for half of it. We didn't change anything in core that broke this. This is broken for a while and I think it should be fixed, because this refactoring is useless without a good support for multiple local declarations. As soon as you want to promote a local within a multiple local declaration to a field, the type for remaining locals is set to int. Pretty annoying. I would vote to get this fix for 3.1.1 since the fix is trivial as described in bug 101980. 101980 covers only the type issue whereas this bug also refers to an issue with the modifiers. The modifiers are simply removed.
Also I don't like the fact that existing tests are based on the fact that int is returned as the type of a variable declaration statement when none is specified.
+1 for 3.1.1
Olivier - pls can you explain exactly why this is critical ? Your first comment says that "If you change the getType() of VariableDeclarationStatement"; but are we changing it in 3.1.1 ?
Markus, can you please investigate and see how it relates to bug 101980. Can you please check what the problem with the modifiers is that Olivier is referring to. Why are the new tests failing in HEAD. Bug 101980 got fixed there and therefore the HEAD test cases shouldn't fail anymore. Olivier, when you run the HEAD test cases did you have HEAD from JDT/UI also ?
No, I don't have JDT/UI from HEAD. I have the contents of the 3.1 maintenance branch. But I took the tests from HEAD since there is no 3.1 maintenance branch for the tests.
(In reply to comment #12) > Olivier - pls can you explain exactly why this is critical ? Your first comment > says that "If you change the getType() of VariableDeclarationStatement"; but are > we changing it in 3.1.1 ? No, we don't plan to change it. I simply did that to expose the fact that existing tests rely on this "default" type from being returned when no type has been set. I see it as critical because this refactoring without a fix doesn't support multiple locals propertly. If the type of the local is not int, then the type of the remaining locals after refactorting is always wrong. The existing modifiers are also not preserved.
Bug 101980 was only about the return type. That one is already fixed in HEAD. With HEAD, tests (including new tests for bug 101980) don't fail any more when changing the default type in VariableDeclarationStatement. Nothing has been released to R3_1_maintenance so far, but the branch is there for jdt.ui, jdt.ui.tests, and jdt.ui.tests.refactoring. The bug is not critical (no crash, loss of data, etc.), but I agree that it should be fixed in 3.1.1. I'll fix it in HEAD and then attach patches for 3.1.1 here. Dirk, please approve for 3.1.1.
*** Bug 101980 has been marked as a duplicate of this bug. ***
Created attachment 25695 [details] Improved Fix This fix retains formatting/comments in the modifiers list.
Created attachment 25696 [details] Tests for improved fix
Released fix to HEAD.
Patch looks fine for me. Martin, can you please do a review as well.
Patch looks fine for me.
Released Improved Fix to R3_1_Maintenance.
start verifying...
Verified the the type and modifiers are correctly preserved for single and multi variable declarations. Verified on M20050831-1200 and plug-in export.
