Fix push down to recognize super class name collisions (#1690)

- fix PushDownRefactoringProcessor to look for method invocations and field accesses to super class members that have names that collide with members in any of the destination types and reference them using super expression - add new test to PushDownTests - fixes #1679
eclipse-jdt · Oct 3, 2024 · 829094c · 829094c
1 parent fd6cb03
commit 829094c
Show file tree

Hide file tree

Showing 4 changed files with 185 additions and 1 deletion.
diff --git a/...g/org/eclipse/jdt/internal/corext/refactoring/structure/PushDownRefactoringProcessor.java b/...g/org/eclipse/jdt/internal/corext/refactoring/structure/PushDownRefactoringProcessor.java
@@ -26,6 +26,7 @@
 import org.eclipse.core.runtime.Assert;
 import org.eclipse.core.runtime.CoreException;
 import org.eclipse.core.runtime.IProgressMonitor;
+import org.eclipse.core.runtime.NullProgressMonitor;
 import org.eclipse.core.runtime.OperationCanceledException;
 import org.eclipse.core.runtime.SubMonitor;
 
@@ -64,8 +65,13 @@
 import org.eclipse.jdt.core.dom.ClassInstanceCreation;
 import org.eclipse.jdt.core.dom.CompilationUnit;
 import org.eclipse.jdt.core.dom.Expression;
+import org.eclipse.jdt.core.dom.FieldAccess;
 import org.eclipse.jdt.core.dom.FieldDeclaration;
+import org.eclipse.jdt.core.dom.IBinding;
 import org.eclipse.jdt.core.dom.IExtendedModifier;
+import org.eclipse.jdt.core.dom.IMethodBinding;
+import org.eclipse.jdt.core.dom.ITypeBinding;
+import org.eclipse.jdt.core.dom.IVariableBinding;
 import org.eclipse.jdt.core.dom.MarkerAnnotation;
 import org.eclipse.jdt.core.dom.MethodDeclaration;
 import org.eclipse.jdt.core.dom.MethodInvocation;
@@ -74,6 +80,8 @@
 import org.eclipse.jdt.core.dom.NodeFinder;
 import org.eclipse.jdt.core.dom.SimpleName;
 import org.eclipse.jdt.core.dom.SimpleType;
+import org.eclipse.jdt.core.dom.SuperFieldAccess;
+import org.eclipse.jdt.core.dom.SuperMethodInvocation;
 import org.eclipse.jdt.core.dom.ThisExpression;
 import org.eclipse.jdt.core.dom.Type;
 import org.eclipse.jdt.core.dom.TypeDeclaration;
@@ -667,6 +675,123 @@ public final boolean visit(ThisExpression node) {
 		}
 	}
 
+	/**
+	 * AST node visitor which performs check of name collision in target and adds super qualifiers to
+	 * members as needed.
+	 */
+	public static class MemberVisitor extends ASTVisitor {
+
+		private final ITypeHierarchy fDeclaringTypeHierarchy;
+		private final IType fDeclaringType;
+		private final IType[] fDestinationTypes;
+		private final ASTRewrite fRewrite;
+
+		public MemberVisitor(final ASTRewrite rewrite, final IType declaringType, final ITypeHierarchy declaringTypeHierarchy, final IType[] destinationTypes) {
+			Assert.isNotNull(rewrite);
+			Assert.isNotNull(declaringTypeHierarchy);
+			Assert.isNotNull(declaringType);
+			Assert.isNotNull(destinationTypes);
+			fRewrite= rewrite;
+			fDeclaringTypeHierarchy= declaringTypeHierarchy;
+			fDeclaringType= declaringType;
+			fDestinationTypes= destinationTypes;
+		}
+
+		@Override
+		public final boolean visit(SimpleName node) {
+			if (node.getLocationInParent() != MethodInvocation.NAME_PROPERTY
+					&& node.getLocationInParent() != FieldAccess.NAME_PROPERTY) {
+				IBinding nodeBinding= node.resolveBinding();
+				if (nodeBinding instanceof IVariableBinding varBinding && varBinding.isField()) {
+					ITypeBinding typeBinding= varBinding.getDeclaringClass();
+					if (typeBinding != null && !typeBinding.getQualifiedName().equals(fDeclaringType.getFullyQualifiedName())) {
+						boolean mayNeedSuper= false;
+						for (IType destinationType : fDestinationTypes) {
+							try {
+								IField[] fields= destinationType.getFields();
+								for (IField field : fields) {
+									if (field.getElementName().equals(node.getFullyQualifiedName())) {
+										mayNeedSuper= true;
+										break;
+									}
+								}
+							} catch (JavaModelException e) {
+								// do nothing
+							}
+						}
+						if (mayNeedSuper) {
+							IType[] superTypes= fDeclaringTypeHierarchy.getAllSuperclasses(fDeclaringType);
+							for (IType superType : superTypes) {
+								if (superType.getFullyQualifiedName().equals(typeBinding.getQualifiedName())) {
+									AST ast= fRewrite.getAST();
+									SuperFieldAccess superFieldAccess= ast.newSuperFieldAccess();
+									superFieldAccess.setName(ast.newSimpleName(node.getFullyQualifiedName()));
+									fRewrite.replace(node, superFieldAccess, null);
+									return true;
+								}
+							}
+						}
+
+					}
+				}
+			}
+			return true;
+		}
+
+		@Override
+		public final boolean visit(MethodInvocation node) {
+			Expression exp= node.getExpression();
+			if (exp == null) {
+				IMethodBinding methodBinding= node.resolveMethodBinding();
+				if (methodBinding != null) {
+					ITypeBinding typeBinding= methodBinding.getDeclaringClass();
+					if (typeBinding != null && !typeBinding.getQualifiedName().equals(fDeclaringType.getFullyQualifiedName())) {
+						boolean mayNeedSuper= false;
+						for (IType destinationType : fDestinationTypes) {
+							try {
+								IMethod[] methods= destinationType.getMethods();
+								for (IMethod method : methods) {
+									if (method.getElementName().equals(node.getName().getFullyQualifiedName()) &&
+											method.getNumberOfParameters() == node.arguments().size()) {
+										mayNeedSuper= true;
+										break;
+									}
+								}
+							} catch (JavaModelException e) {
+								// do nothing
+							}
+						}
+						if (mayNeedSuper) {
+							IType[] superTypes= fDeclaringTypeHierarchy.getAllSuperclasses(fDeclaringType);
+							for (IType superType : superTypes) {
+								if (superType.getFullyQualifiedName().equals(typeBinding.getQualifiedName())) {
+									AST ast= fRewrite.getAST();
+									SuperMethodInvocation superMethodInvocation= ast.newSuperMethodInvocation();
+									superMethodInvocation.setName(ast.newSimpleName(node.getName().getFullyQualifiedName()));
+									ListRewrite typeArgs= fRewrite.getListRewrite(node, MethodInvocation.TYPE_ARGUMENTS_PROPERTY);
+									List<Type> originalTypeList= typeArgs.getOriginalList();
+									if (originalTypeList.size() > 0) {
+										ASTNode typeArgsCopy= typeArgs.createCopyTarget(originalTypeList.get(0), originalTypeList.get(originalTypeList.size() - 1));
+										superMethodInvocation.typeArguments().add(typeArgsCopy);
+									}
+									ListRewrite args= fRewrite.getListRewrite(node, MethodInvocation.ARGUMENTS_PROPERTY);
+									List<Type> originalArgsList= args.getOriginalList();
+									if (originalArgsList.size() > 0) {
+										ASTNode argsCopy= typeArgs.createCopyTarget(originalArgsList.get(0), originalArgsList.get(originalTypeList.size() - 1));
+										superMethodInvocation.arguments().add(argsCopy);
+									}
+									fRewrite.replace(node, superMethodInvocation, null);
+									return true;
+								}
+							}
+						}
+					}
+				}
+			}
+			return true;
+		}
+	}
+
 	private void copyBodyOfPushedDownMethod(ASTRewrite targetRewrite, IMethod method, MethodDeclaration oldMethod, MethodDeclaration newMethod, TypeVariableMaplet[] mapping) throws JavaModelException {
 		Block body= oldMethod.getBody();
 		if (body == null) {
@@ -679,6 +804,7 @@ private void copyBodyOfPushedDownMethod(ASTRewrite targetRewrite, IMethod method
 			final ITrackedNodePosition position= rewriter.track(body);
 			body.accept(new TypeVariableMapper(rewriter, mapping));
 			body.accept(new ThisVisitor(rewriter, fCachedDeclaringType));
+			body.accept(new MemberVisitor(rewriter, fCachedDeclaringType, getHierarchyOfDeclaringClass(new NullProgressMonitor()), getAbstractDestinations(new NullProgressMonitor())));
 			rewriter.rewriteAST(document, getDeclaringType().getCompilationUnit().getOptions(true)).apply(document, TextEdit.NONE);
 			String content= document.get(position.getStartPosition(), position.getLength());
 			String[] lines= Strings.convertIntoLines(content);

diff --git a/org.eclipse.jdt.ui.tests.refactoring/resources/PushDown/test39/in/A.java b/org.eclipse.jdt.ui.tests.refactoring/resources/PushDown/test39/in/A.java
@@ -0,0 +1,20 @@
+package p;
+
+class A extends ParentClass {
+	void m() {
+		f = 3;
+		method();
+	}
+}
+class ParentClass {
+	int f;
+	void method() {
+		System.out.println("ParentClass method");
+	}
+}
+class B extends A {
+	int f;
+	void method() {
+		System.out.println("Class B method");
+	}
+}
diff --git a/org.eclipse.jdt.ui.tests.refactoring/resources/PushDown/test39/out/A.java b/org.eclipse.jdt.ui.tests.refactoring/resources/PushDown/test39/out/A.java
@@ -0,0 +1,20 @@
+package p;
+
+class A extends ParentClass {
+}
+class ParentClass {
+	int f;
+	void method() {
+		System.out.println("ParentClass method");
+	}
+}
+class B extends A {
+	int f;
+	void method() {
+		System.out.println("Class B method");
+	}
+	void m() {
+		super.f = 3;
+		super.method();
+	}
+}
diff --git a/...t.ui.tests.refactoring/test cases/org/eclipse/jdt/ui/tests/refactoring/PushDownTests.java b/...t.ui.tests.refactoring/test cases/org/eclipse/jdt/ui/tests/refactoring/PushDownTests.java
@@ -1,5 +1,5 @@
 /*******************************************************************************
- * Copyright (c) 2000, 2021 IBM Corporation and others.
+ * Copyright (c) 2000, 2024 IBM Corporation and others.
  *
  * This program and the accompanying materials
  * are made available under the terms of the Eclipse Public License 2.0
@@ -945,6 +945,24 @@ public void test38() throws Exception {
 				namesOfMethodsToDeclareAbstract, signaturesOfMethodsToDeclareAbstract, null, null);
 	}
 
+	@Test
+	public void test39() throws Exception { //https://github.com/eclipse-jdt/eclipse.jdt.ui/issues/1679
+		String[] selectedMethodNames= { "m" };
+		String[][] selectedMethodSignatures= { new String[0] };
+		String[] selectedFieldNames= {};
+		String[] namesOfMethodsToPushDown= selectedMethodNames;
+		String[][] signaturesOfMethodsToPushDown= selectedMethodSignatures;
+		String[] namesOfFieldsToPushDown= {};
+		String[] namesOfMethodsToDeclareAbstract= {};
+		String[][] signaturesOfMethodsToDeclareAbstract= {};
+
+		helper(selectedMethodNames, selectedMethodSignatures,
+				selectedFieldNames,
+				namesOfMethodsToPushDown, signaturesOfMethodsToPushDown,
+				namesOfFieldsToPushDown,
+				namesOfMethodsToDeclareAbstract, signaturesOfMethodsToDeclareAbstract, null, null);
+	}
+
 	@Test
 	public void testFail0() throws Exception {
 		String[] selectedMethodNames= {"f"};