Community
Participate
Working Groups
There should be a preference (on the Code Style page?) where users can configure modifiers to be generated for new interface members. The JLS allows to redundantly add any of - "public static final" to a field declaration, - "public static" to a class declaration, - "public abstract" to a method declaration in an interface. I'd suggest to add 4 checkboxes with label "Add redundant modifiers to interface members:". Move Static Members and Extract Interface should respect this preference.
Martin, comments ?
I think that's an overkill. Wouldn't do that.
Won't fix.
Extract Interface now fills its wizard with checkboxes to add public and abstract modifiers. Pull Up just does what it wants, e.g. when pulling up into interface it always adds "public". Move Static Members adds modifiers that are necessary in the target, but never removes redundant modifiers, e.g. moving "int CONST= 1;" from an interface to a class and then moving it back results in "public static final int CONST= 1;". Quick fix to create a method in a super interface never adds any modifiers, but creating in a super class copies them (gives compile error when abstract...). This is confusing and should be streamlined, and I don't see how a proper solution could go without a code style preference. Furthermore, a Clean Up could then be written to add/remove redundant modifiers.
*** Bug 141874 has been marked as a duplicate of this bug. ***
.
> Furthermore, a Clean Up could then be written to add/remove redundant > modifiers. See bug 267335.
The JLS7 says: "It is permitted, but discouraged as a matter of style, to redundantly specify the public and/or abstract modifier for a method declared in an interface." => We don't need a preference for that -- we just shouldn't ever add these. For redundant modifiers on field and class declarations in an interface, it just says the modifiers are permitted. => We should also stop creating/copying the redundant modifiers in these cases. A preference could be added later if really necessary. Bug 130888 is for a clean up to remove redundant modifiers.
*** Bug 390677 has been marked as a duplicate of this bug. ***
I am working on this one. Already done the cleaning to remove the public/abstract modifiers checkboxes from the "Extract Interface wizard. So now, I never get these modifiers in the generated interface. Is is enough to get this issue fixed, or I missed something ?
See comment 4 for other refactorings / quick assists that also produce redundant modifiers. Would be good to fix them all at once to clear the plate.
ok, my patch now fixes the 3 first points of comment 4, that is: - Extract Interface - Pull Up - Move Static Members For the last one (Quick fix to create a method), I'm not sure what is the expected result when the method is created in an interface. Any advice ?
(In reply to Jerome Cambon from comment #12) > For the last one (Quick fix to create a method), I'm not sure what is the > expected result when the method is created in an interface. > Any advice ? The "Create 'method()' in super type 'Interface'" quick fix looks fine (doesn't create public nor abstract modifiers). If the super type is a class, the quick fix should keep copying all modifier keywords, but it should not add a method body if the method is abstract.
(In reply to Markus Keller from comment #13) > (In reply to Jerome Cambon from comment #12) > > For the last one (Quick fix to create a method), I'm not sure what is the > > expected result when the method is created in an interface. > > Any advice ? > > The "Create 'method()' in super type 'Interface'" quick fix looks fine > (doesn't create public nor abstract modifiers). If the super type is a > class, the quick fix should keep copying all modifier keywords, but it > should not add a method body if the method is abstract. ok, so perhaps this one should be logged as a separate bug, since it is not related to the Summary.
*** Bug 432971 has been marked as a duplicate of this bug. ***
(In reply to Jerome Cambon from comment #14) > ok, so perhaps this one should be logged as a separate bug, since it is not > related to the Summary. Feel free to open a separate bug. The fix should be trivial, and for such things, I don't care much under which bug a problem is fixed.
(In reply to Markus Keller from comment #16) > Feel free to open a separate bug. The fix should be trivial, and for such > things, I don't care much under which bug a problem is fixed. I have included this fix in my patch.
You can assign this one to me. I have a patch that I hope to be allowed to publish soon.
Created attachment 243183 [details] My patch to review
Please add an explicit CoO sign-off comment here. http://wiki.eclipse.org/Development_Resources/Contributing_via_Git#via_Bugzilla Here are some initial review comments: 1. Executed AllAllRefactoringTests.java and with this patch i have 110 failures. The tests needs to be fixed. 2. While using "Move" refactoring to move 'public static final int CHROME= 100;' from one class to another interface, it did not remove the redundant "public static final" modifiers. Only 'public' modifier was removed. 3. Same applies while moving 'public static final int CHROME= 100;' from one interface to another interface. 4. I can see that the pending issues from bug 432971 is handled by this patch. It will be good to add a testcase for moving static method from class to interface. 5. Quick fix to create a method in a super class gives compile error when abstract, this is fixed with this patch. A testcase can be added for this scenario as well. 6. Copyright year of all modified files should be updated. e.g ExtractInterfaceWizard 7. Use the below template for updating the contributors list in all the modified file header. (https://wiki.eclipse.org/JDT_UI How_to_Contribute#Coding_Conventions): Your Name <email@example.com> - Bug Title - https://bugs.eclipse.org/BUG_NUMBER
This contribution complies with http://www.eclipse.org/legal/CoO.php
(In reply to Manju Mathew from comment #20) Thanks for the review. > Please add an explicit CoO sign-off comment here. > http://wiki.eclipse.org/Development_Resources/ > Contributing_via_Git#via_Bugzilla Done > > Here are some initial review comments: > 1. Executed AllAllRefactoringTests.java and with this patch i have 110 > failures. The tests needs to be fixed. Done > 2. While using "Move" refactoring to move 'public static final int CHROME= > 100;' from one class to another interface, it did not remove the redundant > "public static final" modifiers. Only 'public' modifier was removed. Done > 3. Same applies while moving 'public static final int CHROME= 100;' from one > interface to another interface. Done > 4. I can see that the pending issues from bug 432971 is handled by this > patch. It will be good to add a testcase for moving static method from class > to interface. This seems to be already covered by MoveMemberTests18, test18_1 > 5. Quick fix to create a method in a super class gives compile error when > abstract, this is fixed with this patch. A testcase can be added for this > scenario as well. Could you please elaborate on how to do this ? I see how to create a method from Quick fix in the current class, but not in its super class. > 6. Copyright year of all modified files should be updated. e.g > ExtractInterfaceWizard Done > 7. Use the below template for updating the contributors list in all the > modified file header. > (https://wiki.eclipse.org/JDT_UI How_to_Contribute#Coding_Conventions): > Your Name <email@example.com> - Bug Title - > https://bugs.eclipse.org/BUG_NUMBER Done
Created attachment 243921 [details] New patch including Manju's comments
(In reply to Jerome Cambon from comment #22) > (In reply to Manju Mathew from comment #20) > > 4. I can see that the pending issues from bug 432971 is handled by this > > patch. It will be good to add a testcase for moving static method from class > > to interface. > This seems to be already covered by MoveMemberTests18, test18_1 Right, i missed this. We can also add tests in PullUpTests18 for: => pull up a static method from class to interface => pull up a static method from interface to interface > > > 5. Quick fix to create a method in a super class gives compile error when > > abstract, this is fixed with this patch. A testcase can be added for this > > scenario as well. > Could you please elaborate on how to do this ? I see how to create a method > from Quick fix in the current class, but not in its super class. public abstract class ClassB { } public abstract class ClassA extends ClassB { @Override public abstract void m1(); } Invoke the quick fix to create #m1 in ClassB, this usecase earlier resulted in compiler error as the abstract method was created with body in the super class. Now with your fix in AbstractMethodCorrectionProposal this issue is resolved. A testcase would be good for it. Thanks for updating all the failing tests, AllAllRefactoringTests is green now. Except the missing testcase the fix looks good to go.
(In reply to Manju Mathew from comment #24) > Right, i missed this. We can also add tests in PullUpTests18 for: > => pull up a static method from class to interface It results in a compile error. Do we really want to test this ? > => pull up a static method from interface to interface This results in the error: "Members in interface cannot be moved" > public abstract class ClassB { > > } > > public abstract class ClassA extends ClassB { > > @Override > public abstract void m1(); > } > > Invoke the quick fix to create #m1 in ClassB, this usecase earlier resulted > in compiler error as the abstract method was created with body in the super > class. Now with your fix in AbstractMethodCorrectionProposal this issue is > resolved. A testcase would be good for it. Ok, thanks. In which package this test would be located? I didn't find similar tests in the repo.
The change in MoveStaticMembersProcessor is wrong. Consider the below code snippet: public class ClassA implements InterfaceA { private static void staticM1() { System.out.println("Something"); } } public interface InterfaceA { ] Invoke "Move" refactoring on #staticM1(). After the refactoring an abstract method with body is created in InterfaceA which results in compiler error. (In reply to Jerome Cambon from comment #26) > > => pull up a static method from class to interface > It results in a compile error. Do we really want to test this ? This is an existing bug, the fix has to go in PullUpRefactoringProcessor#getModifiersWithUpdatedVisibility. If you see it as a simple fix, then you can fix it here. Else I have put a note in bug 435999, we can handle it there. > > => pull up a static method from interface to interface > This results in the error: > "Members in interface cannot be moved" Sorry for the confusion, this can also be handled in Bug 435999. > Ok, thanks. In which package this test would be located? I didn't find > similar tests in the repo. I also couldn't find a test class that handles this particular case. UnresolvedMethodsQuickFixTest is the closest match i could find.
(In reply to Manju Mathew from comment #27) > (In reply to Jerome Cambon from comment #26) > > Ok, thanks. In which package this test would be located? I didn't find > > similar tests in the repo. > I also couldn't find a test class that handles this particular case. > UnresolvedMethodsQuickFixTest is the closest match i could find. Yeah, there's no perfect place. The QF is implemented in ModifierCorrectionSubProcessor#removeOverrideAnnotationProposal(..), where it piggy-backs on QuickAssistProcessor#getCreateInSuperClassProposals(..). We already have a similar test ModifierCorrectionsQuickFixTest#testOverrideAnnotationButNotOverriding(), so I think we best put the new test after that one. One way to find that test: - open QuickAssistProcessor#getCreateInSuperClassProposals(..) - choose left ruler context menu > Show Annotations - open the History view - click the QuickAssistProcessor.getCreateInSuperClassProposals line in the ruler -> look for changed files in a *.tests project in that commit
(In reply to Manju Mathew from comment #27) > The change in MoveStaticMembersProcessor is wrong. > Consider the below code snippet: > public class ClassA implements InterfaceA { > private static void staticM1() { > System.out.println("Something"); > } > } > public interface InterfaceA { > ] > > Invoke "Move" refactoring on #staticM1(). After the refactoring an abstract > method with body is created in InterfaceA which results in compiler error. I propose to create a new bug for this, since it is not the purpose of this bug, which is about modifiers only. I guess the "Move" on the example above would be different if 1.8 (this would create a default implementation) or not (error) > This is an existing bug, the fix has to go in > PullUpRefactoringProcessor#getModifiersWithUpdatedVisibility. If you see it > as a simple fix, then you can fix it here. Else I have put a note in bug > 435999, we can handle it there. Yes please, let's handle it there. > I also couldn't find a test class that handles this particular case. > UnresolvedMethodsQuickFixTest is the closest match i could find. I have added a new test ModifierCorrectionsQuickFixTest.testCreateMethodWhenOverrideAnnotation(), as suggested by Markus.
Created attachment 244284 [details] New patch with additional test, to review
(In reply to Jerome Cambon from comment #30) > Created attachment 244284 [details] [diff] > New patch with additional test, to review Jerome, i am unable to apply the latest patch that you have uploaded. I have conflicts in RenameResourceWizard. Looks like this patch contains the files modified for bug 391389. (In reply to Jerome Cambon from comment #29) > (In reply to Manju Mathew from comment #27) > > The change in MoveStaticMembersProcessor is wrong. > > Consider the below code snippet: > > public class ClassA implements InterfaceA { > > private static void staticM1() { > > System.out.println("Something"); > > } > > } > > public interface InterfaceA { > > ] > > > > Invoke "Move" refactoring on #staticM1(). After the refactoring an abstract > > method with body is created in InterfaceA which results in compiler error. > > I propose to create a new bug for this, since it is not the purpose of this > bug, which is about modifiers only. I guess the "Move" on the example above > would be different if 1.8 (this would create a default implementation) or > not (error) The above bug does not exist now. It will be introduced if the changes in MoveStaticMembersProcessor is released. Hence if the method being moved is static, then we should not clear all the modifier flags as you have currently done. > I have added a new test > ModifierCorrectionsQuickFixTest.testCreateMethodWhenOverrideAnnotation(), > as suggested by Markus. This patch does not contain the new tests in ModifierCorrectionsQuickFixTest as you mentioned. Kindly have a look and post an updated patch. Also note that Git 'master' branch is closed for the Luna release and hence we are releasing the reviewed patches in Git 'marsTemp' branch. It will be better to create patches against 'marsTemp'.
(In reply to Manju Mathew from comment #31) > Jerome, i am unable to apply the latest patch that you have uploaded. I have > conflicts in RenameResourceWizard. Looks like this patch contains the files > modified for bug 391389. Done, I removed it. > (In reply to Jerome Cambon from comment #29) > > (In reply to Manju Mathew from comment #27) > > > The change in MoveStaticMembersProcessor is wrong. > > > Consider the below code snippet: > > > public class ClassA implements InterfaceA { > > > private static void staticM1() { > > > System.out.println("Something"); > > > } > > > } > > > public interface InterfaceA { > > > ] > > > > > > Invoke "Move" refactoring on #staticM1(). After the refactoring an abstract > > > method with body is created in InterfaceA which results in compiler error. > > > > I propose to create a new bug for this, since it is not the purpose of this > > bug, which is about modifiers only. I guess the "Move" on the example above > > would be different if 1.8 (this would create a default implementation) or > > not (error) > The above bug does not exist now. It will be introduced if the changes in > MoveStaticMembersProcessor is released. Hence if the method being moved is > static, then we should not clear all the modifier flags as you have > currently done. Yes of course, sorry for the confusion. I've removed the 'static' keyword removal in this case. > > > I have added a new test > > ModifierCorrectionsQuickFixTest.testCreateMethodWhenOverrideAnnotation(), > > as suggested by Markus. > This patch does not contain the new tests in ModifierCorrectionsQuickFixTest > as you mentioned. Kindly have a look and post an updated patch. Done. > > Also note that Git 'master' branch is closed for the Luna release and hence > we are releasing the reviewed patches in Git 'marsTemp' branch. It will be > better to create patches against 'marsTemp'. ok, done.
Created attachment 244427 [details] New patch to review
Created attachment 244428 [details] New patch to review Added my name in ModifierCorrectionsQuickFixTest.java
(In reply to Jerome Cambon from comment #32) > > The above bug does not exist now. It will be introduced if the changes in > > MoveStaticMembersProcessor is released. Hence if the method being moved is > > static, then we should not clear all the modifier flags as you have > > currently done. > > Yes of course, sorry for the confusion. I've removed the 'static' keyword > removal in this case. The latest patch results in 2 test failures in MoveMembersTests as the redundant static modifier of the field is not removed and 2 test failures in MoveMembersTests18.
Created attachment 244539 [details] New patch with test fixed.
(In reply to Jerome Cambon from comment #36) > Created attachment 244539 [details] [diff] > New patch with test fixed. MoveMembersTests#test23 and MoveMembersTests#test34 still fails. Revisit the changes made in MoveStaticMembersProcessor#getUpdatedMemberSource.
Created attachment 244850 [details] New patch to review
(In reply to Manju Mathew from comment #37) > (In reply to Jerome Cambon from comment #36) > > Created attachment 244539 [details] [diff] [diff] > > New patch with test fixed. > > MoveMembersTests#test23 and MoveMembersTests#test34 still fails. Revisit the > changes made in MoveStaticMembersProcessor#getUpdatedMemberSource. I've updated the MoveStaticMembersProcessor#getUpdatedMemberSource() method, and re-run all the tests related to this bug. Should be ok now.
All the tests are green with this patch. The issues related to pull up refactoring involving static methods will be handled in bug 435999, otherwise the changes looks fine. We do not include workspace preference file (this patch had org.eclipse.jdt.launching.prefs and org.eclipse.jdt.core.prefs) as part of the patch. Released the patch with: http://git.eclipse.org/c/jdt/eclipse.jdt.ui.git/commit/?id=858d103d16a2bbcadeed578269c733af077fb381 The unused message strings are removed with: http://git.eclipse.org/c/jdt/eclipse.jdt.ui.git/commit/?id=ff1873c0c156f2f8d56117143ea38ffc2f2df29a
