Fix change method signature to recognize name collision (#1767)

* Fix change method signature to recognize name collision - modify ChangeSignatureProcessor to check for the case where the new name is already a method accessible to a location that is calling the old name - make method in Checks class public to convert ICompilationUnit to CompilationUnit - add new test to ChangeSignatureTests - fixes #1751
eclipse-jdt · Nov 6, 2024 · 0734ebc · 0734ebc
1 parent 324bdfd
commit 0734ebc
Show file tree

Hide file tree

Showing 6 changed files with 164 additions and 4 deletions.
diff --git a/....core.manipulation/core extension/org/eclipse/jdt/internal/corext/refactoring/Checks.java b/....core.manipulation/core extension/org/eclipse/jdt/internal/corext/refactoring/Checks.java
@@ -915,7 +915,7 @@ public static boolean isDeclaredIn(VariableDeclaration tempDeclaration, Class<?
 		return true;
 	}
 
-	private static  CompilationUnit convertICUtoCU(ICompilationUnit compilationUnit) {
+	public static CompilationUnit convertICUtoCU(ICompilationUnit compilationUnit) {
 		ASTParser parser= ASTParser.newParser(IASTSharedValues.SHARED_AST_LEVEL);
 		parser.setKind(ASTParser.K_COMPILATION_UNIT);
 		parser.setSource(compilationUnit);

diff --git a/...n/core extension/org/eclipse/jdt/internal/corext/refactoring/RefactoringCoreMessages.java b/...n/core extension/org/eclipse/jdt/internal/corext/refactoring/RefactoringCoreMessages.java
@@ -105,6 +105,8 @@ public final class RefactoringCoreMessages extends NLS {
 
 	public static String ChangeSignatureRefactoring_method_deleted;
 
+	public static String ChangeSignatureRefactoring_method_name_will_shadow;
+
 	public static String ChangeSignatureRefactoring_method_name_not_empty;
 
 	public static String ChangeSignatureRefactoring_modify_Parameters;

diff --git a/...ulation/core extension/org/eclipse/jdt/internal/corext/refactoring/refactoring.properties b/...ulation/core extension/org/eclipse/jdt/internal/corext/refactoring/refactoring.properties
@@ -808,6 +808,7 @@ ChangeSignatureRefactoring_method_deleted=The selected method has been deleted f
 ChangeSignatureRefactoring_native=Method ''{0}'' declared in type ''{1}'' is native. Reordering parameters will cause UnsatisfiedLinkError on runtime if you do not update your native libraries.
 ChangeSignatureRefactoring_duplicate_name=Duplicate parameter name: ''{0}''.
 ChangeSignatureRefactoring_return_type_contains_type_variable=The return type ''{0}'' contains the type variable ''{1}'', which may not be available in related methods.
+ChangeSignatureRefactoring_method_name_will_shadow=Renaming method ''{0}'' to ''{1}'' causes potential logic change due to an existing ''{0}'' method accessible at a call location.
 ChangeSignatureRefactoring_method_name_not_empty=The method name cannot be empty.
 ChangeSignatureRefactoring_default_value=Enter the default value for parameter ''{0}''.
 ChangeSignatureRefactoring_default_visibility=(package)

diff --git a/...nsion/org/eclipse/jdt/internal/corext/refactoring/structure/ChangeSignatureProcessor.java b/...nsion/org/eclipse/jdt/internal/corext/refactoring/structure/ChangeSignatureProcessor.java
@@ -1,5 +1,5 @@
 /*******************************************************************************
- * Copyright (c) 2000, 2018 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
@@ -13,6 +13,7 @@
  *******************************************************************************/
 package org.eclipse.jdt.internal.corext.refactoring.structure;
 
+import java.lang.ref.Cleaner;
 import java.util.ArrayList;
 import java.util.HashMap;
 import java.util.HashSet;
@@ -48,16 +49,21 @@
 import org.eclipse.jdt.core.ICompilationUnit;
 import org.eclipse.jdt.core.IJavaElement;
 import org.eclipse.jdt.core.IJavaProject;
+import org.eclipse.jdt.core.IMember;
 import org.eclipse.jdt.core.IMethod;
 import org.eclipse.jdt.core.IType;
 import org.eclipse.jdt.core.ITypeHierarchy;
+import org.eclipse.jdt.core.JavaCore;
 import org.eclipse.jdt.core.JavaModelException;
 import org.eclipse.jdt.core.Signature;
+import org.eclipse.jdt.core.SourceRange;
+import org.eclipse.jdt.core.WorkingCopyOwner;
 import org.eclipse.jdt.core.compiler.IProblem;
 import org.eclipse.jdt.core.dom.AST;
 import org.eclipse.jdt.core.dom.ASTNode;
 import org.eclipse.jdt.core.dom.ASTParser;
 import org.eclipse.jdt.core.dom.AbstractTypeDeclaration;
+import org.eclipse.jdt.core.dom.AnonymousClassDeclaration;
 import org.eclipse.jdt.core.dom.Block;
 import org.eclipse.jdt.core.dom.BodyDeclaration;
 import org.eclipse.jdt.core.dom.ClassInstanceCreation;
@@ -137,6 +143,7 @@
 import org.eclipse.jdt.internal.corext.refactoring.RefactoringCoreMessages;
 import org.eclipse.jdt.internal.corext.refactoring.RefactoringScopeFactory;
 import org.eclipse.jdt.internal.corext.refactoring.RefactoringSearchEngine;
+import org.eclipse.jdt.internal.corext.refactoring.RefactoringSearchEngine2;
 import org.eclipse.jdt.internal.corext.refactoring.ReturnTypeInfo;
 import org.eclipse.jdt.internal.corext.refactoring.SearchResultGroup;
 import org.eclipse.jdt.internal.corext.refactoring.StubTypeContext;
@@ -183,6 +190,8 @@ public class ChangeSignatureProcessor extends RefactoringProcessor implements ID
 	private List<ExceptionInfo> fExceptionInfos;
 	private TextChangeManager fChangeManager;
 
+	private Cleaner fCleaner= Cleaner.create();
+	private CleanableWorkingCopyOwner fOwner= new CleanableWorkingCopyOwner();
 	private IMethod fMethod;
 	private IMethod fTopMethod;
 	private IMethod[] fRippleMethods;
@@ -206,6 +215,7 @@ public class ChangeSignatureProcessor extends RefactoringProcessor implements ID
 
 	public ChangeSignatureProcessor(JavaRefactoringArguments arguments, RefactoringStatus status) throws JavaModelException {
 		this((IMethod) null);
+		fCleaner.register(this, fOwner);
 		status.merge(initialize(arguments));
 	}
 
@@ -228,6 +238,26 @@ public ChangeSignatureProcessor(IMethod method) throws JavaModelException {
 		}
 	}
 
+	static class CleanableWorkingCopyOwner extends WorkingCopyOwner implements Runnable {
+        @Override
+		public void run() {
+            resetWorkingCopies(this);
+        }
+	}
+
+	/**
+	 * Resets the working copies.
+	 */
+	private static void resetWorkingCopies(WorkingCopyOwner owner) {
+		for (ICompilationUnit unit : JavaCore.getWorkingCopies(owner)) {
+			try {
+				unit.discardWorkingCopy();
+			} catch (Exception exception) {
+				// Do nothing
+			}
+		}
+	}
+
 	private static List<ParameterInfo> createParameterInfoList(IMethod method) {
 		try {
 			String[] typeNames= method.getParameterTypes();
@@ -395,7 +425,9 @@ private RefactoringStatus checkSignature(boolean resolveBindings) {
 		checkMethodName(result);
 		if (result.hasFatalError())
 			return result;
-
+		checkShadowing(result);
+		if (result.hasFatalError())
+			return result;
 		checkParameterNamesAndValues(result);
 		if (result.hasFatalError())
 			return result;
@@ -424,6 +456,86 @@ private RefactoringStatus checkSignature(boolean resolveBindings) {
 		return result;
 	}
 
+	private void checkShadowing(RefactoringStatus result) {
+		if (!fMethodName.equals(fMethod.getElementName())) {
+			try {
+				SearchResultGroup[] matches= findReferences(fMethod, new NullProgressMonitor());
+				for (SearchResultGroup match : matches) {
+					ICompilationUnit cu= match.getCompilationUnit();
+
+					for (SearchMatch matchResult : match.getSearchResults()) {
+						if (matchResult instanceof MethodReferenceMatch methodMatch && methodMatch.getElement() instanceof IMethod method) {
+							final MethodDeclaration methodDecl= ASTNodeSearchUtil.getMethodDeclarationNode(method, Checks.convertICUtoCU(cu));
+							ASTNode typeParent= ASTNodes.getFirstAncestorOrNull(methodDecl, AbstractTypeDeclaration.class, AnonymousClassDeclaration.class);
+							ITypeBinding typeBinding= null;
+							if (typeParent instanceof AbstractTypeDeclaration atd) {
+								typeBinding= atd.resolveBinding();
+							} else if (typeParent instanceof AnonymousClassDeclaration acd) {
+								typeBinding= acd.resolveBinding();
+							}
+							if (typeBinding != null) {
+								if (recursiveShadowCheck(typeBinding, typeBinding.getPackage().getName(), true)) {
+									RefactoringStatusContext context= JavaStatusContext.create(method.getTypeRoot(), new SourceRange(methodMatch.getOffset(), methodMatch.getLength()));
+									String msg= Messages.format(RefactoringCoreMessages.ChangeSignatureRefactoring_method_name_will_shadow, new Object[] {fMethod.getElementName(), fMethodName});
+									if (method.getParameterNames().length == 0 && fMethodName.equals(methodDecl.getName().getFullyQualifiedName())) {
+										result.addFatalError(msg, context);
+										return;
+									} else {
+										result.addError(msg, context);
+									}
+								}
+							}
+						}
+					}
+				}
+			} catch (JavaModelException e) {
+				// ignore and exit
+			}
+		}
+	}
+
+	private boolean recursiveShadowCheck(ITypeBinding typeBinding, String origPackage, boolean allowPrivate) {
+		if (typeBinding == null) {
+			return false;
+		}
+		IMethodBinding[] methods= typeBinding.getDeclaredMethods();
+		for (IMethodBinding method : methods) {
+			if (method.getName().equals(fMethodName) && method.getParameterNames().length == fParameterInfos.size() &&
+					(allowPrivate || Modifier.isPublic(method.getModifiers()) || Modifier.isProtected(method.getModifiers())
+							|| !Modifier.isPrivate(method.getModifiers()) && typeBinding.getPackage().getName().equals(origPackage))) {
+				return true;
+			}
+		}
+		if (recursiveShadowCheck(typeBinding.getSuperclass(), origPackage, false)) {
+			return true;
+		}
+		ITypeBinding[] interfaces= typeBinding.getInterfaces();
+		for (ITypeBinding implemented : interfaces) {
+			if (recursiveShadowCheck(implemented, origPackage, false)) {
+				return true;
+			}
+		}
+		if (typeBinding.isMember()) {
+			if (recursiveShadowCheck(typeBinding.getDeclaringClass(), origPackage, true)) {
+				return true;
+			}
+		}
+		return false;
+	}
+
+	private SearchResultGroup[] findReferences(final IMember member, final IProgressMonitor monitor) throws JavaModelException {
+		SearchPattern pattern= SearchPattern.createPattern(member, IJavaSearchConstants.REFERENCES, SearchUtils.GENERICS_AGNOSTIC_MATCH_RULE);
+		if (pattern == null) {
+			return new SearchResultGroup[0];
+		}
+		final RefactoringSearchEngine2 engine= new RefactoringSearchEngine2(pattern);
+		engine.setOwner(fOwner);
+		engine.setFiltering(true, true);
+		engine.setScope(RefactoringScopeFactory.create(member));
+		engine.searchPattern(monitor);
+		return (SearchResultGroup[]) engine.getResults();
+	}
+
 	public boolean isSignatureSameAsInitial() throws JavaModelException {
 		if (! isVisibilitySameAsInitial())
 			return false;

diff --git a/....jdt.ui.tests.refactoring/resources/ChangeSignature/cannotModify/A_testFailIssue1751.java b/....jdt.ui.tests.refactoring/resources/ChangeSignature/cannotModify/A_testFailIssue1751.java
@@ -0,0 +1,13 @@
+package p;
+
+class A_testFailIssue1751 {
+	// change method signature 'm' to 'k'
+	public void m() {
+	}
+
+	class B {
+		void k() {
+			m();
+		}
+	}
+}
diff --git a/...sts.refactoring/test cases/org/eclipse/jdt/ui/tests/refactoring/ChangeSignatureTests.java b/...sts.refactoring/test cases/org/eclipse/jdt/ui/tests/refactoring/ChangeSignatureTests.java
@@ -1,5 +1,5 @@
 /*******************************************************************************
- * Copyright (c) 2000, 2020 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
@@ -197,6 +197,32 @@ private void helperRenameMethod(String[] signature, String newMethodName, boolea
 		assertParticipant(classType);
 	}
 
+	/*
+	 * Rename method 'm(signature)' to 'newMethodName(signature)'
+	 */
+	private void helperRenameMethodFail(String[] signature, String newMethodName, int expectedSeverity, boolean createDelegate, boolean markAsDeprecated, String typeName) throws Exception {
+		ICompilationUnit cu= createCUfromTestFile(getPackageP(), false, true);
+		IType classType= getType(cu, typeName);
+		IMethod method = classType.getMethod("m", signature);
+		assertTrue("method m does not exist in " +typeName, method.exists());
+		assertTrue("refactoring not available", RefactoringAvailabilityTester.isChangeSignatureAvailable(method));
+
+		ChangeSignatureProcessor processor= new ChangeSignatureProcessor(method);
+		Refactoring ref= new ProcessorBasedRefactoring(processor);
+
+		processor.setNewMethodName(newMethodName);
+		processor.setDelegateUpdating(createDelegate);
+		processor.setDeprecateDelegates(markAsDeprecated);
+		FussyProgressMonitor testMonitor= new FussyProgressMonitor();
+		ref.checkInitialConditions(testMonitor);
+		testMonitor.assertUsedUp();
+
+		JavaRefactoringDescriptor descriptor= processor.createDescriptor();
+		RefactoringStatus result= performRefactoring(descriptor);
+		assertNotNull("precondition was supposed to fail", result);
+		assertEquals("Severity:", expectedSeverity, result.getSeverity());
+	}
+
 	private void helperDoAll(String typeName,
 								String methodName,
 							  	String[] signature,
@@ -581,6 +607,12 @@ public void testFail1() throws Exception{
 		helperFail(new String[0], new String[0], RefactoringStatus.FATAL);
 	}
 
+	@Test
+	public void testFailIssue1751() throws Exception {
+		String[] signature= {};
+		helperRenameMethodFail(signature, "k", RefactoringStatus.FATAL, false, true, "A_testFailIssue1751");
+	}
+
 	@Test
 	public void testFailAdd2() throws Exception{
 		String[] signature= {"I"};