Attachment 224492 Details for Bug 58648 – Code fix and test cases

[patch] Code fix and test cases

bug58648.patch (text/plain), 13.19 KB, created by Jean-Noel Rouvignac

on 2012-12-08 12:05:32 EST

(hide)

Description:

Filename:

MIME Type:

Creator: Jean-Noel Rouvignac

Created: 2012-12-08 12:05:32 EST

Size: 13.19 KB

patch

obsolete

> .../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 <bmuskalla@eclipsesource.com> - [extract method] extracting return value results in compile error - https://bugs.eclipse.org/bugs/show_bug.cgi?id=264606
>+ *     Jean-Noël Rouvignac <jn.rouvignac@gmail.com> - [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<Match> fResult= new ArrayList<Match>(2);

Actions: View | Diff

Attachments on bug 58648: 224492 | 224493 | 227226