From 9c7fbe475bb753438f2938cd91283e65e202cdd5 Mon Sep 17 00:00:00 2001 From: Jeff Johnston Date: Wed, 27 Nov 2024 18:50:43 -0500 Subject: [PATCH] Fix change signature to recognize a possible logic issue after rename (#1790) * Fix change signature to recognize a possible logic issue after rename - modify ChangeSignatureProcessor to add new checkShadowing2 method which recognizes when rename of method might affect an implementor of the class which is using a method of same name already - add new test to ChangeSignatureTests - fixes #1750 * Version bump(s) for 4.35 stream --------- Co-authored-by: Eclipse JDT Bot --- .../refactoring/RefactoringCoreMessages.java | 2 + .../corext/refactoring/refactoring.properties | 1 + .../structure/ChangeSignatureProcessor.java | 178 ++++++++++++++++++ .../META-INF/MANIFEST.MF | 2 +- org.eclipse.jdt.ui.tests.refactoring/pom.xml | 2 +- .../cannotModify/A_testFailIssue1750.java | 16 ++ .../refactoring/ChangeSignatureTests.java | 6 + 7 files changed, 205 insertions(+), 2 deletions(-) create mode 100644 org.eclipse.jdt.ui.tests.refactoring/resources/ChangeSignature/cannotModify/A_testFailIssue1750.java diff --git a/org.eclipse.jdt.core.manipulation/core extension/org/eclipse/jdt/internal/corext/refactoring/RefactoringCoreMessages.java b/org.eclipse.jdt.core.manipulation/core extension/org/eclipse/jdt/internal/corext/refactoring/RefactoringCoreMessages.java index fcf9febf578..01fcba416be 100644 --- a/org.eclipse.jdt.core.manipulation/core extension/org/eclipse/jdt/internal/corext/refactoring/RefactoringCoreMessages.java +++ b/org.eclipse.jdt.core.manipulation/core extension/org/eclipse/jdt/internal/corext/refactoring/RefactoringCoreMessages.java @@ -107,6 +107,8 @@ public final class RefactoringCoreMessages extends NLS { public static String ChangeSignatureRefactoring_method_name_will_shadow; + public static String ChangeSignatureRefactoring_method_name_will_shadow2; + public static String ChangeSignatureRefactoring_method_name_not_empty; public static String ChangeSignatureRefactoring_modify_Parameters; diff --git a/org.eclipse.jdt.core.manipulation/core extension/org/eclipse/jdt/internal/corext/refactoring/refactoring.properties b/org.eclipse.jdt.core.manipulation/core extension/org/eclipse/jdt/internal/corext/refactoring/refactoring.properties index 39686dbd695..d353adbe6c2 100644 --- a/org.eclipse.jdt.core.manipulation/core extension/org/eclipse/jdt/internal/corext/refactoring/refactoring.properties +++ b/org.eclipse.jdt.core.manipulation/core extension/org/eclipse/jdt/internal/corext/refactoring/refactoring.properties @@ -809,6 +809,7 @@ ChangeSignatureRefactoring_native=Method ''{0}'' declared in type ''{1}'' is nat 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_will_shadow2=Renaming method ''{0}'' to ''{1}'' causes potential logic change due to an existing ''{1}'' 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/org.eclipse.jdt.core.manipulation/core extension/org/eclipse/jdt/internal/corext/refactoring/structure/ChangeSignatureProcessor.java b/org.eclipse.jdt.core.manipulation/core extension/org/eclipse/jdt/internal/corext/refactoring/structure/ChangeSignatureProcessor.java index fbadfe5f166..e42ef25cbbd 100644 --- a/org.eclipse.jdt.core.manipulation/core extension/org/eclipse/jdt/internal/corext/refactoring/structure/ChangeSignatureProcessor.java +++ b/org.eclipse.jdt.core.manipulation/core extension/org/eclipse/jdt/internal/corext/refactoring/structure/ChangeSignatureProcessor.java @@ -62,6 +62,7 @@ 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.ASTVisitor; import org.eclipse.jdt.core.dom.AbstractTypeDeclaration; import org.eclipse.jdt.core.dom.AnonymousClassDeclaration; import org.eclipse.jdt.core.dom.Block; @@ -96,6 +97,7 @@ import org.eclipse.jdt.core.dom.TagElement; import org.eclipse.jdt.core.dom.TextElement; import org.eclipse.jdt.core.dom.Type; +import org.eclipse.jdt.core.dom.TypeDeclaration; import org.eclipse.jdt.core.dom.VariableDeclaration; import org.eclipse.jdt.core.dom.VariableDeclarationFragment; import org.eclipse.jdt.core.dom.rewrite.ASTRewrite; @@ -116,6 +118,7 @@ import org.eclipse.jdt.core.search.SearchEngine; import org.eclipse.jdt.core.search.SearchMatch; import org.eclipse.jdt.core.search.SearchPattern; +import org.eclipse.jdt.core.search.TypeReferenceMatch; import org.eclipse.jdt.internal.core.manipulation.JavaElementLabelsCore; import org.eclipse.jdt.internal.core.manipulation.JavaManipulationPlugin; @@ -127,6 +130,7 @@ import org.eclipse.jdt.internal.corext.codemanipulation.ContextSensitiveImportRewriteContext; import org.eclipse.jdt.internal.corext.dom.ASTNodeFactory; import org.eclipse.jdt.internal.corext.dom.ASTNodes; +import org.eclipse.jdt.internal.corext.dom.AbortSearchException; import org.eclipse.jdt.internal.corext.dom.Bindings; import org.eclipse.jdt.internal.corext.dom.IASTSharedValues; import org.eclipse.jdt.internal.corext.dom.ModifierRewrite; @@ -426,6 +430,8 @@ private RefactoringStatus checkSignature(boolean resolveBindings) { if (result.hasFatalError()) return result; checkShadowing(result); + if (!result.hasError()) + checkShadowing2(result); if (result.hasFatalError()) return result; checkParameterNamesAndValues(result); @@ -494,6 +500,99 @@ private void checkShadowing(RefactoringStatus result) { } } + private class ShadowedMethodVisitor extends ASTVisitor { + private MethodInvocation fMethodInvocation; + + public MethodInvocation getMethod() { + return fMethodInvocation; + } + + @Override + public boolean visit(MethodInvocation node) { + if (node.getName().getFullyQualifiedName().equals(fMethodName) && + node.getExpression() == null) { + IMethodBinding binding= node.resolveMethodBinding(); + if (binding.getParameterTypes().length == fParameterInfos.size()) { + for (int i= 0; i < binding.getParameterTypes().length; ++i) { + String simpleName= binding.getParameterTypes()[0].getName(); + String newTypeName= fParameterInfos.get(i).getNewTypeName(); + if (!simpleName.equals(newTypeName) && isKnownIncompatible(simpleName, newTypeName)) { + return true; + } + } + fMethodInvocation= node; + throw new AbortSearchException(); + } + } + return true; + } + } + + private void checkShadowing2(RefactoringStatus result) { + try { + if (!fMethodName.equals(fMethod.getElementName())) { + ICompilationUnit icu= fMethod.getCompilationUnit(); + CompilationUnit cu= Checks.convertICUtoCU(icu); + if (cu == null) { + return; + } + ShadowedMethodVisitor visitor= new ShadowedMethodVisitor(); + try { + AbstractTypeDeclaration methodTypeDecl= ASTNodeSearchUtil.getAbstractTypeDeclarationNode(fMethod.getDeclaringType(), cu); + if (methodTypeDecl != null) { + methodTypeDecl.accept(visitor); + } + } catch (AbortSearchException e) { + MethodInvocation method= visitor.getMethod(); + RefactoringStatusContext context= JavaStatusContext.create(icu, new SourceRange(method.getStartPosition(), method.getLength())); + String msg= Messages.format(RefactoringCoreMessages.ChangeSignatureRefactoring_method_name_will_shadow2, new Object[] {fMethod.getElementName(), fMethodName}); + if (fMethod.getParameterNames().length == 0) { + result.addFatalError(msg, context); + } else { + result.addError(msg, context); + } + return; + } + if (!Modifier.isPrivate(fMethod.getFlags())) { + SearchResultGroup[] matches= findImplementors(fMethod.getDeclaringType(), new NullProgressMonitor()); + for (SearchResultGroup match : matches) { + icu= match.getCompilationUnit(); + cu= Checks.convertICUtoCU(icu); + if (cu == null) { + return; + } + for (SearchMatch matchResult : match.getSearchResults()) { + if (matchResult instanceof TypeReferenceMatch typeMatch && typeMatch.getElement() instanceof IType type) { + final TypeDeclaration typeDecl= ASTNodeSearchUtil.getTypeDeclarationNode(type, cu); + if (typeDecl != null) { + ITypeBinding typeBinding= typeDecl.resolveBinding(); + if (Modifier.isPublic(fMethod.getFlags()) || Modifier.isProtected(fMethod.getFlags()) || + typeBinding != null && typeBinding.getPackage().getName().equals(fMethod.getDeclaringType().getPackageFragment().getElementName())) { + visitor= new ShadowedMethodVisitor(); + try { + typeDecl.accept(visitor); + } catch (AbortSearchException e) { + MethodInvocation method= visitor.getMethod(); + RefactoringStatusContext context= JavaStatusContext.create(icu, new SourceRange(method.getStartPosition(), method.getLength())); + String msg= Messages.format(RefactoringCoreMessages.ChangeSignatureRefactoring_method_name_will_shadow2, new Object[] {fMethod.getElementName(), fMethodName}); + if (fMethod.getParameterNames().length == 0) { + result.addFatalError(msg, context); + } 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; @@ -503,6 +602,13 @@ private boolean recursiveShadowCheck(ITypeBinding typeBinding, String origPackag 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))) { + for (int i= 0; i < method.getParameterTypes().length; ++i) { + String simpleName= method.getParameterTypes()[0].getName(); + String newTypeName= fParameterInfos.get(i).getNewTypeName(); + if (!simpleName.equals(newTypeName) && isKnownIncompatible(simpleName, newTypeName)) { + return false; + } + } return true; } } @@ -523,6 +629,78 @@ private boolean recursiveShadowCheck(ITypeBinding typeBinding, String origPackag return false; } + private Set fIntSet= new HashSet<>(List.of("char", "int", "long", "short", "Integer", "Long", "Short")); //$NON-NLS-1$ //$NON-NLS-2$ //$NON-NLS-3$ //$NON-NLS-4$ //$NON-NLS-5$ //$NON-NLS-6$ //$NON-NLS-7$ + private Set fFloatSet= new HashSet<>(List.of("float", "double", "Float", "Double")); //$NON-NLS-1$ //$NON-NLS-2$ //$NON-NLS-3$ //$NON-NLS-4$ // + private Set fBooleanSet= new HashSet<>(List.of("boolean", "Boolean")); //$NON-NLS-1$ //$NON-NLS-2$ // + private Set fWellUsedSet= new HashSet<>(List.of( + "Date", //$NON-NLS-1$ + "String", //$NON-NLS-1$ + "Integer", //$NON-NLS-1$ + "Long", //$NON-NLS-1$ + "Short", //$NON-NLS-1$ + "Character", //$NON-NLS-1$ + "Float", //$NON-NLS-1$ + "Double", //$NON-NLS-1$ + "Boolean", //$NON-NLS-1$ + "List", //$NON-NLS-1$ + "Map", //$NON-NLS-1$ + "Set", //$NON-NLS-1$ + "Iterable", //$NON-NLS-1$ + "Byte", //$NON-NLS-1$ + "int", //$NON-NLS-1$ + "long", //$NON-NLS-1$ + "short", //$NON-NLS-1$ + "float", //$NON-NLS-1$ + "double", //$NON-NLS-1$ + "boolean", //$NON-NLS-1$ + "char" //$NON-NLS-1$ + )); + + + private boolean isKnownIncompatible(String simpleName, String newTypeName) { + if (simpleName.isEmpty() || newTypeName.isEmpty()) { + return true; + } + if (simpleName.endsWith("[]") || newTypeName.endsWith("[]")) { //$NON-NLS-1$ //$NON-NLS-2$ + return true; + } + if (simpleName.indexOf('<') >= 0) { + simpleName= simpleName.substring(0, simpleName.indexOf('<')); + } + if (newTypeName.indexOf('<') >= 0) { + newTypeName= newTypeName.substring(0, newTypeName.indexOf('<')); + } + if (fIntSet.contains(simpleName) && fIntSet.contains(newTypeName)) { + return false; + } + if (fFloatSet.contains(simpleName) && fFloatSet.contains(newTypeName)) { + return false; + } + if (fBooleanSet.contains(simpleName) && fBooleanSet.contains(newTypeName)) { + return false; + } + if (fWellUsedSet.contains(simpleName) && fWellUsedSet.contains(newTypeName)) { + return true; + } + if (Character.isLowerCase(simpleName.charAt(0)) != Character.isLowerCase(newTypeName.charAt(0))) { + return true; + } + return false; + } + + private SearchResultGroup[] findImplementors(final IType type, final IProgressMonitor monitor) throws JavaModelException { + SearchPattern pattern= SearchPattern.createPattern(type, IJavaSearchConstants.IMPLEMENTORS, 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(type)); + engine.searchPattern(monitor); + return (SearchResultGroup[]) engine.getResults(); + } + 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) { diff --git a/org.eclipse.jdt.ui.tests.refactoring/META-INF/MANIFEST.MF b/org.eclipse.jdt.ui.tests.refactoring/META-INF/MANIFEST.MF index a24eba260ed..2efd1cf0d36 100644 --- a/org.eclipse.jdt.ui.tests.refactoring/META-INF/MANIFEST.MF +++ b/org.eclipse.jdt.ui.tests.refactoring/META-INF/MANIFEST.MF @@ -3,7 +3,7 @@ Automatic-Module-Name: org.eclipse.jdt.ui.tests.refactoring Bundle-ManifestVersion: 2 Bundle-Name: %Plugin.name Bundle-SymbolicName: org.eclipse.jdt.ui.tests.refactoring; singleton:=true -Bundle-Version: 3.15.700.qualifier +Bundle-Version: 3.15.800.qualifier Bundle-Activator: org.eclipse.jdt.ui.tests.refactoring.infra.RefactoringTestPlugin Bundle-ActivationPolicy: lazy Bundle-Vendor: %Plugin.providerName diff --git a/org.eclipse.jdt.ui.tests.refactoring/pom.xml b/org.eclipse.jdt.ui.tests.refactoring/pom.xml index ea744d4d55d..d022ae62b87 100644 --- a/org.eclipse.jdt.ui.tests.refactoring/pom.xml +++ b/org.eclipse.jdt.ui.tests.refactoring/pom.xml @@ -19,7 +19,7 @@ org.eclipse.jdt org.eclipse.jdt.ui.tests.refactoring - 3.15.700-SNAPSHOT + 3.15.800-SNAPSHOT eclipse-test-plugin ${project.artifactId} diff --git a/org.eclipse.jdt.ui.tests.refactoring/resources/ChangeSignature/cannotModify/A_testFailIssue1750.java b/org.eclipse.jdt.ui.tests.refactoring/resources/ChangeSignature/cannotModify/A_testFailIssue1750.java new file mode 100644 index 00000000000..e003f63ecec --- /dev/null +++ b/org.eclipse.jdt.ui.tests.refactoring/resources/ChangeSignature/cannotModify/A_testFailIssue1750.java @@ -0,0 +1,16 @@ +package p; + +class A_testFailIssue1750 { + public void k(Number x) { + } + // change method signature 'm' to 'k' + public void m(Long x) { + } + + class B { + void foo() { + long i = 1; + k(i); + } + } +} \ No newline at end of file diff --git a/org.eclipse.jdt.ui.tests.refactoring/test cases/org/eclipse/jdt/ui/tests/refactoring/ChangeSignatureTests.java b/org.eclipse.jdt.ui.tests.refactoring/test cases/org/eclipse/jdt/ui/tests/refactoring/ChangeSignatureTests.java index ffc8edf4513..063c1bfaf28 100644 --- a/org.eclipse.jdt.ui.tests.refactoring/test cases/org/eclipse/jdt/ui/tests/refactoring/ChangeSignatureTests.java +++ b/org.eclipse.jdt.ui.tests.refactoring/test cases/org/eclipse/jdt/ui/tests/refactoring/ChangeSignatureTests.java @@ -607,6 +607,12 @@ public void testFail1() throws Exception{ helperFail(new String[0], new String[0], RefactoringStatus.FATAL); } + @Test + public void testFailIssue1750() throws Exception { + String[] signature= {"QLong;"}; + helperRenameMethodFail(signature, "k", RefactoringStatus.ERROR, false, true, "A_testFailIssue1750"); + } + @Test public void testFailIssue1751() throws Exception { String[] signature= {};