Community
Participate
Eclipse IDE
3.1RC4 public class AA { boolean foo(boolean b, Object x, List l){ if (b){ /*[*/ if (x instanceof Integer) return false; return true; /*]*/ } else { for (Iterator iter = l.iterator(); iter.hasNext();) { Object next= iter.next(); if (next instanceof Integer) return false; } return true; } } } 'Replace duplicate fragments' In the loop, the call to the new method is missing
the code in the loop shouldn't be replaced at all code loss, but not a stop ship candidate for 3.1.1
bug is in the SnippetFinder: It matches the extracted code although the last statements is in an outer scope (block). The ending of a block seems to be ignored. This bug seems to occur with all kind of scopes: blocks, if statements..., even over more than one scope. When replacing, a list rewriter on block is used, but doesn't work for the last node which isn't in the block. bug 99113 has been filed that the rewriter should throw an IllegalArgumentException when this happens. This example is also matched and fails with an assertion exception: boolean foo(boolean b, Object x, List l){ if (b){ /*[*/ if (x instanceof Integer) l.add(null); l.add(new Object()); l.add(new Object()); /*]*/ } else { Iterator iter = l.iterator(); Object next= iter.next(); { if (true) if (x instanceof Integer) l.add(null); l.add(new Object()); } l.add(new Object()); } return true; } at org.eclipse.jdt.internal.corext.Assert.isTrue(Assert.java:139) at org.eclipse.jdt.internal.corext.Assert.isTrue(Assert.java:124) at org.eclipse.jdt.internal.corext.dom.ReplaceRewrite.<init>(ReplaceRewrite.java:42) at org.eclipse.jdt.internal.corext.dom.StatementRewrite.<init>(StatementRewrite.java:26) at org.eclipse.jdt.internal.corext.refactoring.code.ExtractMethodRefactoring.replaceDuplicates(ExtractMethodRefactoring.java:710) at org.eclipse.jdt.internal.corext.refactoring.code.ExtractMethodRefactoring.createChange(ExtractMethodRefactoring.java:451) at org.eclipse.ltk.core.refactoring.CreateChangeOperation.run(CreateChangeOperation.java:117) at org.eclipse.core.internal.resources.Workspace.run(Workspace.java:1719) at org.eclipse.ltk.internal.ui.refactoring.WorkbenchRunnableAdapter.run(WorkbenchRunnableAdapter.java:86) at org.eclipse.jface.operation.ModalContext$ModalContextThread.run(ModalContext.java:113)
Created attachment 24526 [details] Code patch
Created attachment 24527 [details] Additional test cases
Fix is low is and very local. The missing piece is that the snippet finder doesn't handle endVisit blocks and therefore skipped the closing block in th snippet.
I haven't tested it, but it seems to me that this still ignores the problem with for/if/while statements that are not blocks.
Good catch ! I will attach a new patch that checks for the parent of the found snippet instead of checking for blocks. That's what the extract refactoring does for the selected statements as well to ensure that they don't spawn scope boundaries.
Created attachment 24528 [details] Improved code patch
Created attachment 24529 [details] Additional test cases
REviewed with Tom Eicher and Martin Aeschlimann. Found another case that needs special treatment. public void foo() { foo(); foo(); if (true) foo(); else foo(); } Selecting foo(); foo(); will match against the foo in the if statements.
Created attachment 25300 [details] New code patch
Created attachment 25301 [details] New test cases
New fix reviewed by Martin Aeschlimann again and discussed with Tom Eicher. Added 5 new test cases 965 - 969.
verified in M20050808-2000 (3.1.1)
also verified in I20050808-2000 (3.2 M1)
Not a bug, but I just noticed, that the class names in test cases 965 - 969 are constantly A_test964, which is probably not intended ;-)
