From a00fd899f6fe748d9012705d902cf153a0215f40 Mon Sep 17 00:00:00 2001 From: Jeff Johnston Date: Fri, 27 Sep 2024 22:41:35 -0400 Subject: [PATCH] Warn about a pull up method overriding calls to super class method (#1682) - add logic ot PullUpRefactoringProcessor to check if pulling up a method may override existing calls to a super class method of the same name and change program semantics - add new test to PullUpTests - fixes #1677 --- .../corext/refactoring/refactoring.properties | 2 +- .../structure/PullUpRefactoringProcessor.java | 45 ++++++++++++++++++- .../resources/PullUp/testFail35/in/A.java | 19 ++++++++ .../jdt/ui/tests/refactoring/PullUpTests.java | 5 +++ 4 files changed, 68 insertions(+), 3 deletions(-) create mode 100644 org.eclipse.jdt.ui.tests.refactoring/resources/PullUp/testFail35/in/A.java 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 6444f308e14..a68e1430a05 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 @@ -663,7 +663,7 @@ PullUpRefactoring_calculating_required=Computing required members... PullUpRefactoring_category_description=Changes to pull up members PullUpRefactoring_Field_declared_in_class=Field ''{0}'' is declared in class ''{1}''. Moving it may result in changed program semantics. PullUpRefactoring_add_override_annotation=Add '@Override' annotation -PullUpRefactoring_Method_declared_in_class=Method ''{0}'' is declared in class ''{1}''. Moving it may result in changed program semantics. +PullUpRefactoring_Method_declared_in_class=Pulling up ''{1}'' up may override existing calls to ''{2}'' and change program semantics. PullUpRefactoring_field_cannot_be_accessed=Field ''{0}'' cannot be accessed from ''{1}'' PullUpRefactoring_method_cannot_be_accessed=Method ''{0}'' cannot be accessed from ''{1}'' PullUpRefactoring_Type_declared_in_class=Type ''{0}'' is declared in class ''{1}''. Moving it may result in changed program semantics. diff --git a/org.eclipse.jdt.core.manipulation/refactoring/org/eclipse/jdt/internal/corext/refactoring/structure/PullUpRefactoringProcessor.java b/org.eclipse.jdt.core.manipulation/refactoring/org/eclipse/jdt/internal/corext/refactoring/structure/PullUpRefactoringProcessor.java index c14200dafdc..848be021875 100644 --- a/org.eclipse.jdt.core.manipulation/refactoring/org/eclipse/jdt/internal/corext/refactoring/structure/PullUpRefactoringProcessor.java +++ b/org.eclipse.jdt.core.manipulation/refactoring/org/eclipse/jdt/internal/corext/refactoring/structure/PullUpRefactoringProcessor.java @@ -804,6 +804,9 @@ private static void upgradeMap(final Map> result, final Ma /** The cached set of skipped supertypes */ private Set fCachedSkippedSuperTypes; + /** The cached set of all supertypes */ + private Set fCachedSuperTypes; + /** Whether an argument has been added to handle this references in pulled up methods */ private boolean fArgumentAddedToHandleThis; @@ -1160,7 +1163,7 @@ private void checkFieldTypes(final IProgressMonitor monitor, final RefactoringSt @Override public RefactoringStatus checkFinalConditions(final IProgressMonitor monitor, final CheckConditionsContext context) throws CoreException, OperationCanceledException { try { - SubMonitor subMonitor= SubMonitor.convert(monitor, RefactoringCoreMessages.PullUpRefactoring_checking, 13); + SubMonitor subMonitor= SubMonitor.convert(monitor, RefactoringCoreMessages.PullUpRefactoring_checking, 14); clearCaches(); final RefactoringStatus result= new RefactoringStatus(); @@ -1177,6 +1180,7 @@ public RefactoringStatus checkFinalConditions(final IProgressMonitor monitor, fi result.merge(checkAccesses(subMonitor.newChild(1))); result.merge(checkMembersInTypeAndAllSubtypes(subMonitor.newChild(2))); result.merge(checkIfSkippingOverElements(subMonitor.newChild(1))); + result.merge(checkIfOverridingSuperClass(subMonitor.newChild(1))); if (monitor.isCanceled()) throw new OperationCanceledException(); if (!JdtFlags.isAbstract(getDestinationType()) && getAbstractMethods().length > 0) @@ -1385,7 +1389,7 @@ private RefactoringStatus checkIfMethodDeclaredIn(final IMethod iMethod, final I final IMethod methodInType= JavaModelUtil.findMethod(iMethod.getElementName(), iMethod.getParameterTypes(), iMethod.isConstructor(), type); if (methodInType == null || !methodInType.exists()) return null; - final String[] keys= { JavaElementLabelsCore.getTextLabel(methodInType, JavaElementLabelsCore.ALL_FULLY_QUALIFIED), JavaElementLabelsCore.getTextLabel(type, JavaElementLabelsCore.ALL_FULLY_QUALIFIED)}; + final String[] keys= { JavaElementLabelsCore.getTextLabel(methodInType, JavaElementLabelsCore.ALL_FULLY_QUALIFIED), iMethod.getElementName(), JavaElementLabelsCore.getTextLabel(methodInType, JavaElementLabelsCore.ALL_FULLY_QUALIFIED) }; final String msg= Messages.format(RefactoringCoreMessages.PullUpRefactoring_Method_declared_in_class, keys); final RefactoringStatusContext context= JavaStatusContext.create(methodInType); return RefactoringStatus.createWarningStatus(msg, context); @@ -1408,6 +1412,23 @@ private RefactoringStatus checkIfSkippingOverElements(final IProgressMonitor mon } } + private RefactoringStatus checkIfOverridingSuperClass(final IProgressMonitor monitor) throws JavaModelException { + SubMonitor subMonitor= SubMonitor.convert(monitor, RefactoringCoreMessages.PullUpRefactoring_checking, 1); + try { + final Set superTypeSet= getAllSuperTypes(subMonitor.newChild(1)); + final IType[] superTypes= superTypeSet.toArray(new IType[superTypeSet.size()]); + final RefactoringStatus result= new RefactoringStatus(); + for (final IMember element : fMembersToMove) { + for (IType element2 : superTypes) { + result.merge(checkIfDeclaredIn(element, element2)); + } + } + return result; + } finally { + monitor.done(); + } + + } private RefactoringStatus checkIfTypeDeclaredIn(final IType iType, final IType type) { final IType typeInType= type.getType(iType.getElementName()); if (!typeInType.exists()) @@ -2284,6 +2305,26 @@ private Set getSkippedSuperTypes(final IProgressMonitor monitor) throws J } } + // All super classes of the destination + private Set getAllSuperTypes(final IProgressMonitor monitor) throws JavaModelException { + SubMonitor subMonitor= SubMonitor.convert(monitor, RefactoringCoreMessages.PullUpRefactoring_checking, 1); + try { + if (fCachedSuperTypes != null) + return fCachedSkippedSuperTypes; + final ITypeHierarchy hierarchy= getDestinationTypeHierarchy(subMonitor.newChild(1)); + fCachedSuperTypes= new HashSet<>(2); + IType current= hierarchy.getSuperclass(getDeclaringType()); + while (current != null && !current.getFullyQualifiedName().equals("java.lang.Object")) { //$NON-NLS-1$ + fCachedSuperTypes.add(current); + current= hierarchy.getSuperclass(current); + } + return fCachedSuperTypes; + } finally { + monitor.done(); + } + + } + private RefactoringStatus initialize(final JavaRefactoringArguments extended) { String handle= extended.getAttribute(JavaRefactoringDescriptorUtil.ATTRIBUTE_INPUT); if (handle != null) { diff --git a/org.eclipse.jdt.ui.tests.refactoring/resources/PullUp/testFail35/in/A.java b/org.eclipse.jdt.ui.tests.refactoring/resources/PullUp/testFail35/in/A.java new file mode 100644 index 00000000000..74f585c9b6d --- /dev/null +++ b/org.eclipse.jdt.ui.tests.refactoring/resources/PullUp/testFail35/in/A.java @@ -0,0 +1,19 @@ +package p; + +class A { + void method() { + System.out.println("ParentClass method"); + } +} + +class B extends SubClass { + void method() { + System.out.println("Class B method"); + } +} + +class SubClass extends A { + void test() { + method(); + } +} diff --git a/org.eclipse.jdt.ui.tests.refactoring/test cases/org/eclipse/jdt/ui/tests/refactoring/PullUpTests.java b/org.eclipse.jdt.ui.tests.refactoring/test cases/org/eclipse/jdt/ui/tests/refactoring/PullUpTests.java index 50eb14112ae..2da48de7d33 100644 --- a/org.eclipse.jdt.ui.tests.refactoring/test cases/org/eclipse/jdt/ui/tests/refactoring/PullUpTests.java +++ b/org.eclipse.jdt.ui.tests.refactoring/test cases/org/eclipse/jdt/ui/tests/refactoring/PullUpTests.java @@ -1857,6 +1857,11 @@ public void testFail34() throws Exception { helper2(new String[] { "foo" }, new String[][] { new String[0] }, true, false, 0); } + @Test + public void testFail35() throws Exception { + helper2(new String[] { "method" }, new String[][] { new String[0] }, true, false, 0); + } + //---------------------------------------------------------- @Test public void testField0() throws Exception {