Community
Participate
Working Groups
Build Identifier: 20110615-0604 Applying the move method refactoring introduces a compilation error Reproducible: Always Steps to Reproduce: 1. Create the classes public class A { public B b; private long m(long l) { return 0; } public long m(int i) { return 1; } } public class B extends A { public long test() { return super.m(2); } } 2. Apply the move method refactoring to move m to B public class A { public B b; private long m(long l) { return 0; } } public class B extends A { public long test() { return super.m(2); } public long m(int i) { return 1; } } 3. The resulting program does not compile: The method m from the type A is not visible
Refactoring is to move A#m(int) to B (move to the subclass). The problem is that we don't handle the super.m(2) call in B at all. Should at least issue an error in this case.
*** Bug 355329 has been marked as a duplicate of this bug. ***
Created attachment 234680 [details] propsed patch This contribution complies with http://www.eclipse.org/legal/CoO.php This fixes the reported issue. However I can't help feeling that there are corner cases that I have missed out. Even if I have missed out some cases it is still worth checking this patch in because it improves the situation.
Few more common cases can also be handled in this patch. Have a look at the impl for "if (node instanceof MethodInvocation)" just below your fix to cover such cases, like : 1. super method invocation changes to "null.m(2);" : public class A { public long m(B b, int i) { return 1; } } public class B extends A { public long test() { return super.m(null, 2); } } 2. Results in compilation error if target node was inserted while moving: public class A { public int i= 0; public long m(B b, int i) { return this.i + i; } } public class B extends A { public long test() { return super.m(new B(), 2); } } etc.
Created attachment 234705 [details] More test cases This contribution complies with http://www.eclipse.org/legal/CoO.php Ok I think I got all the corner cases.
Comment on attachment 234705 [details] More test cases - You are always adding ThisExpression as the *first* argument of new method invocation. This won't work here: public class A { public int i= 0; public long m(int i, B b) { return this.i + i; } } public class B extends A { public long test() { return super.m(2, new B()); } } - It is good to use 'if-else if' instead of consecutive 'if's wherever possible. - Change the copyright addition to follow this one line pattern: Name <e-mail> - summary - https://bugs.eclipse.org/BUG_NUMBER - Please refer http://wiki.eclipse.org/JDT_UI/How_to_Contribute to configure your workspace with JDT UI specific Save Actions.
Created attachment 234747 [details] even more test cases covered This contribution complies with http://www.eclipse.org/legal/CoO.php Thanks Noopur, Here is a patch with your comments incorporated
Thanks Nikolay, the patch looks good now. Released after some refactoring and formatting, please have a look: http://git.eclipse.org/c/jdt/eclipse.jdt.ui.git/commit/?id=bd8e7065888ed4d82ba11a6485106ef4ebad93ae
Thanks Noopur, Looks good
