Skip to content

Commit

Permalink
Fix change signature to recognize a possible logic issue after rename (…
Browse files Browse the repository at this point in the history 
…#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 <jdt-bot@eclipse.org>
  • Loading branch information
@jjohnstn @eclipse-jdt-bot
jjohnstn and eclipse-jdt-bot authored Nov 27, 2024
1 parent f1dfac5 commit 9c7fbe4
Show file tree
Hide file tree
Showing 7 changed files with 205 additions and 2 deletions.
2 changes: 2 additions & 0 deletions ...n/core extension/org/eclipse/jdt/internal/corext/refactoring/RefactoringCoreMessages.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
1 change: 1 addition & 0 deletions ...ulation/core extension/org/eclipse/jdt/internal/corext/refactoring/refactoring.properties
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
178 changes: 178 additions & 0 deletions ...nsion/org/eclipse/jdt/internal/corext/refactoring/structure/ChangeSignatureProcessor.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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;
Expand All @@ -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;
Expand All @@ -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;
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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;
Expand All @@ -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;
}
}
Expand All @@ -523,6 +629,78 @@ private boolean recursiveShadowCheck(ITypeBinding typeBinding, String origPackag
return false;
}

private Set<String> 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<String> fFloatSet= new HashSet<>(List.of("float", "double", "Float", "Double")); //$NON-NLS-1$ //$NON-NLS-2$ //$NON-NLS-3$ //$NON-NLS-4$ //
private Set<String> fBooleanSet= new HashSet<>(List.of("boolean", "Boolean")); //$NON-NLS-1$ //$NON-NLS-2$ //
private Set<String> 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) {
Expand Down
2 changes: 1 addition & 1 deletion org.eclipse.jdt.ui.tests.refactoring/META-INF/MANIFEST.MF
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
2 changes: 1 addition & 1 deletion org.eclipse.jdt.ui.tests.refactoring/pom.xml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@
</parent>
<groupId>org.eclipse.jdt</groupId>
<artifactId>org.eclipse.jdt.ui.tests.refactoring</artifactId>
<version>3.15.700-SNAPSHOT</version>
<version>3.15.800-SNAPSHOT</version>
<packaging>eclipse-test-plugin</packaging>
<properties>
<testSuite>${project.artifactId}</testSuite>
Expand Down
16 changes: 16 additions & 0 deletions ....jdt.ui.tests.refactoring/resources/ChangeSignature/cannotModify/A_testFailIssue1750.java
View file
Original file line number Diff line number Diff line change
@@ -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);
}
}
}
6 changes: 6 additions & 0 deletions ...sts.refactoring/test cases/org/eclipse/jdt/ui/tests/refactoring/ChangeSignatureTests.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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= {};
Expand Down

0 comments on commit 9c7fbe4

Please sign in to comment.