.../ui/tests/refactoring/ExtractMethodTests.java | 18 ++- .../refactoring/code/ExtractMethodRefactoring.java | 15 +- .../corext/refactoring/code/SnippetFinder.java | 163 +++++++++++++++++--- 3 files changed, 165 insertions(+), 31 deletions(-) diff --git a/org.eclipse.jdt.ui.tests.refactoring/test cases/org/eclipse/jdt/ui/tests/refactoring/ExtractMethodTests.java b/org.eclipse.jdt.ui.tests.refactoring/test cases/org/eclipse/jdt/ui/tests/refactoring/ExtractMethodTests.java index 6d649d8..731031c 100644 --- a/org.eclipse.jdt.ui.tests.refactoring/test cases/org/eclipse/jdt/ui/tests/refactoring/ExtractMethodTests.java +++ b/org.eclipse.jdt.ui.tests.refactoring/test cases/org/eclipse/jdt/ui/tests/refactoring/ExtractMethodTests.java @@ -968,7 +968,23 @@ public class ExtractMethodTests extends AbstractSelectionTestCase { public void test374() throws Exception { validSelectionTestChecked(); } - + + public void test375() throws Exception { + validSelectionTestChecked(); //https://bugs.eclipse.org/bugs/show_bug.cgi?id=58648 + } + + public void test376() throws Exception { + validSelectionTestChecked(); //https://bugs.eclipse.org/bugs/show_bug.cgi?id=58648 + } + + public void test377() throws Exception { + validSelectionTestChecked(); //https://bugs.eclipse.org/bugs/show_bug.cgi?id=58648 + } + + public void test378() throws Exception { + validSelectionTestChecked(); //https://bugs.eclipse.org/bugs/show_bug.cgi?id=58648 + } + //==================================================================================== // Testing Extracted result //==================================================================================== diff --git a/org.eclipse.jdt.ui/core refactoring/org/eclipse/jdt/internal/corext/refactoring/code/ExtractMethodRefactoring.java b/org.eclipse.jdt.ui/core refactoring/org/eclipse/jdt/internal/corext/refactoring/code/ExtractMethodRefactoring.java index 0279d0b..54ae73c 100644 --- a/org.eclipse.jdt.ui/core refactoring/org/eclipse/jdt/internal/corext/refactoring/code/ExtractMethodRefactoring.java +++ b/org.eclipse.jdt.ui/core refactoring/org/eclipse/jdt/internal/corext/refactoring/code/ExtractMethodRefactoring.java @@ -505,11 +505,11 @@ public class ExtractMethodRefactoring extends Refactoring { TextEditGroup substituteDesc= new TextEditGroup(Messages.format(RefactoringCoreMessages.ExtractMethodRefactoring_substitute_with_call, BasicElementLabels.getJavaElementName(fMethodName))); result.addTextEditGroup(substituteDesc); - MethodDeclaration mm= createNewMethod(selectedNodes, fCUnit.findRecommendedLineSeparator(), substituteDesc); + MethodDeclaration extractedMethod= createNewMethod(selectedNodes, fCUnit.findRecommendedLineSeparator(), substituteDesc); if (fLinkedProposalModel != null) { LinkedProposalPositionGroup typeGroup= fLinkedProposalModel.getPositionGroup(KEY_TYPE, true); - typeGroup.addPosition(fRewriter.track(mm.getReturnType2()), false); + typeGroup.addPosition(fRewriter.track(extractedMethod.getReturnType2()), false); ITypeBinding typeBinding= fAnalyzer.getReturnTypeBinding(); if (typeBinding != null) { @@ -520,9 +520,9 @@ public class ExtractMethodRefactoring extends Refactoring { } LinkedProposalPositionGroup nameGroup= fLinkedProposalModel.getPositionGroup(KEY_NAME, true); - nameGroup.addPosition(fRewriter.track(mm.getName()), false); + nameGroup.addPosition(fRewriter.track(extractedMethod.getName()), false); - ModifierCorrectionSubProcessor.installLinkedVisibilityProposals(fLinkedProposalModel, fRewriter, mm.modifiers(), false); + ModifierCorrectionSubProcessor.installLinkedVisibilityProposals(fLinkedProposalModel, fRewriter, extractedMethod.modifiers(), false); } TextEditGroup insertDesc= new TextEditGroup(Messages.format(RefactoringCoreMessages.ExtractMethodRefactoring_add_method, BasicElementLabels.getJavaElementName(fMethodName))); @@ -531,13 +531,13 @@ public class ExtractMethodRefactoring extends Refactoring { if (fDestination == fDestinations[0]) { ChildListPropertyDescriptor desc= (ChildListPropertyDescriptor)declaration.getLocationInParent(); ListRewrite container= fRewriter.getListRewrite(declaration.getParent(), desc); - container.insertAfter(mm, declaration, insertDesc); + container.insertAfter(extractedMethod, declaration, insertDesc); } else { BodyDeclarationRewrite container= BodyDeclarationRewrite.create(fRewriter, fDestination); - container.insert(mm, insertDesc); + container.insert(extractedMethod, insertDesc); } - replaceDuplicates(result, mm.getModifiers()); + replaceDuplicates(result, extractedMethod.getModifiers()); replaceBranches(result); if (fImportRewriter.hasRecordedChanges()) { @@ -553,7 +553,6 @@ public class ExtractMethodRefactoring extends Refactoring { } finally { pm.done(); } - } private void replaceBranches(final CompilationUnitChange result) { diff --git a/org.eclipse.jdt.ui/core refactoring/org/eclipse/jdt/internal/corext/refactoring/code/SnippetFinder.java b/org.eclipse.jdt.ui/core refactoring/org/eclipse/jdt/internal/corext/refactoring/code/SnippetFinder.java index 6fb18b9..dee2834 100644 --- a/org.eclipse.jdt.ui/core refactoring/org/eclipse/jdt/internal/corext/refactoring/code/SnippetFinder.java +++ b/org.eclipse.jdt.ui/core refactoring/org/eclipse/jdt/internal/corext/refactoring/code/SnippetFinder.java @@ -8,6 +8,7 @@ * Contributors: * IBM Corporation - initial API and implementation * Benjamin Muskalla - [extract method] extracting return value results in compile error - https://bugs.eclipse.org/bugs/show_bug.cgi?id=264606 + * Jean-Noël Rouvignac - [extract method] disregard 'this.' when finding duplicates (was: find simple aliasing) - https://bugs.eclipse.org/bugs/show_bug.cgi?id=58648 *******************************************************************************/ package org.eclipse.jdt.internal.corext.refactoring.code; @@ -31,9 +32,11 @@ import org.eclipse.jdt.core.dom.FieldAccess; import org.eclipse.jdt.core.dom.IBinding; import org.eclipse.jdt.core.dom.IVariableBinding; import org.eclipse.jdt.core.dom.MethodDeclaration; +import org.eclipse.jdt.core.dom.Name; import org.eclipse.jdt.core.dom.QualifiedName; import org.eclipse.jdt.core.dom.SimpleName; import org.eclipse.jdt.core.dom.SuperFieldAccess; +import org.eclipse.jdt.core.dom.ThisExpression; import org.eclipse.jdt.core.dom.TypeDeclaration; import org.eclipse.jdt.internal.corext.dom.ASTNodes; @@ -86,9 +89,9 @@ import org.eclipse.jdt.internal.corext.dom.GenericVisitor; /** * Tests if the whole duplicate is the full body of a method. If so * don't replace it since we would replace a method body with a new - * method body which doesn't make to much sense. + * method body which doesn't make too much sense. * - * @return whether the duplicte is the whole method body + * @return whether the duplicate is the whole method body */ public boolean isMethodBody() { ASTNode first= fNodes.get(0); @@ -108,34 +111,150 @@ import org.eclipse.jdt.internal.corext.dom.GenericVisitor; private class Matcher extends ASTMatcher { @Override + public boolean match(FieldAccess candidate, Object s) { + if (s instanceof FieldAccess) { + FieldAccess snippet= (FieldAccess) s; + IBinding cb= candidate.resolveFieldBinding(); + IBinding sb= snippet.resolveFieldBinding(); + if (cb == null || sb == null) + return false; + if (Bindings.equals(cb, sb)) { + Expression ce= candidate.getExpression(); + Expression se= snippet.getExpression(); + if (ce.getNodeType() == ASTNode.THIS_EXPRESSION && + se.getNodeType() == ASTNode.THIS_EXPRESSION) { + ThisExpression cte= (ThisExpression) ce; + ThisExpression ste= (ThisExpression) se; + return isSame(cte.getQualifier(), ste.getQualifier(), cte, ste); + } + } + } else if (s instanceof SimpleName) { + return match((SimpleName) s, candidate); + } + return false; + } + + @Override public boolean match(SimpleName candidate, Object s) { - if (!(s instanceof SimpleName)) - return false; + if (s instanceof SimpleName) { + SimpleName snippet= (SimpleName) s; + if (candidate.isDeclaration() != snippet.isDeclaration()) + return false; - SimpleName snippet= (SimpleName)s; - if (candidate.isDeclaration() != snippet.isDeclaration()) - return false; + IBinding cb= candidate.resolveBinding(); + IBinding sb= snippet.resolveBinding(); + if (cb == null || sb == null) + return false; + IVariableBinding vcb= ASTNodes.getVariableBinding(candidate); + IVariableBinding vsb= ASTNodes.getVariableBinding(snippet); + if (vcb == null || vsb == null) + return Bindings.equals(cb, sb); + if (!vcb.isField() && !vsb.isField() && Bindings.equals(vcb.getType(), vsb.getType())) { + SimpleName mapped= fMatch.getMappedName(vsb); + if (mapped != null) { + IVariableBinding mappedBinding= ASTNodes.getVariableBinding(mapped); + if (!Bindings.equals(vcb, mappedBinding)) + return false; + } + fMatch.addLocal(vsb, candidate); + return true; + } + return Bindings.equals(cb, sb); + } else if (s instanceof FieldAccess) { + return match(candidate, (FieldAccess) s); + } + return false; + } - IBinding cb= candidate.resolveBinding(); - IBinding sb= snippet.resolveBinding(); - if (cb == null || sb == null) + private boolean match(SimpleName sn, FieldAccess fa) { + if (!isOverarchingNameOrFieldAccess(sn)) { return false; - IVariableBinding vcb= ASTNodes.getVariableBinding(candidate); - IVariableBinding vsb= ASTNodes.getVariableBinding(snippet); - if (vcb == null || vsb == null) - return Bindings.equals(cb, sb); - if (!vcb.isField() && !vsb.isField() && Bindings.equals(vcb.getType(), vsb.getType())) { - SimpleName mapped= fMatch.getMappedName(vsb); - if (mapped != null) { - IVariableBinding mappedBinding= ASTNodes.getVariableBinding(mapped); - if (!Bindings.equals(vcb, mappedBinding)) - return false; + } + IBinding snb= sn.resolveBinding(); + IBinding fab= fa.resolveFieldBinding(); + if (snb == null || fab == null) + return false; + + if (Bindings.equals(snb, fab)) { + Expression e= fa.getExpression(); + if (e.getNodeType() == ASTNode.THIS_EXPRESSION) { + ThisExpression te= (ThisExpression) e; + return te.getQualifier() == null || isEnclosingTypeFor(te.getQualifier(), te); } - fMatch.addLocal(vsb, candidate); + } + return false; + } + + /** + * @param name the name from where to start looking + * @return true if there is no Name/FieldAccess above the given Name, false otherwise. + */ + private boolean isOverarchingNameOrFieldAccess(Name name) { + final ASTNode parent= name.getParent(); + if (parent == null) { return true; } - return Bindings.equals(cb, sb); + int parentType= parent.getNodeType(); + return parentType != ASTNode.FIELD_ACCESS && parentType != ASTNode.QUALIFIED_NAME; + } + + private boolean isSame(Name name1, Name name2, ASTNode parent1, ASTNode parent2) { + if (name1 == null) { + return name2 == null || isEnclosingTypeFor(name2, parent1); + } else if (name2 == null) { + return isEnclosingTypeFor(name1, parent2); + } else if (name1.isSimpleName()) { + SimpleName sn1= (SimpleName) name1; + if (name2.isSimpleName()) { + return isSame(sn1, (SimpleName) name2); + } else { + return isSame(sn1, (QualifiedName) name2, parent2); + } + } else if (name1.isQualifiedName()) { + QualifiedName qual1= (QualifiedName) name1; + if (name2.isSimpleName()) { + return isSame((SimpleName) name2, qual1, parent1); + } else { + QualifiedName qual2= (QualifiedName) name2; + return isSame(qual1.getName(), qual2.getName()) && isSame(qual1.getQualifier(), qual2.getQualifier(), parent1, parent2); + } + } + return false; + } + + private boolean isSame(SimpleName name1, SimpleName name2) { + return name1.getIdentifier().equals(name2.getIdentifier()); + } + + private boolean isSame(SimpleName sn, QualifiedName qual, ASTNode qualParent) { + if (sn.getIdentifier().equals(qual.getName().getIdentifier())) { + TypeDeclaration typeDecl= (TypeDeclaration) ASTNodes.getParent(qualParent, TypeDeclaration.class); + if (typeDecl.getName().getIdentifier().equals(qual.getName().getIdentifier())) { + return isEnclosingTypeFor(qual.getQualifier(), typeDecl.getParent()); + } + } + return false; } + + private boolean isEnclosingTypeFor(Name name, ASTNode node) { + if (name == null || node == null || node.getParent() == null) { + return false; + } + ASTNode parent= node.getParent(); + if (node.getNodeType() == ASTNode.TYPE_DECLARATION) { + final TypeDeclaration typeDecl= (TypeDeclaration) node; + final SimpleName typeName= typeDecl.getName(); + if (name.isSimpleName()) { + SimpleName sn= (SimpleName) name; + return sn.getIdentifier().equals(typeName.getIdentifier()); + } else if (name.isQualifiedName()) { + QualifiedName qn= (QualifiedName) name; + return qn.getName().getIdentifier().equals(typeName.getIdentifier()) && isEnclosingTypeFor(qn.getQualifier(), parent); + } + } + return isEnclosingTypeFor(name, parent); + } + } private List fResult= new ArrayList(2);