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 6edf3626b6a..18745ee67f6 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 @@ -1,5 +1,5 @@ /******************************************************************************* - * Copyright (c) 2000, 2025 IBM Corporation and others. + * Copyright (c) 2000, 2026 IBM Corporation and others. * * This program and the accompanying materials * are made available under the terms of the Eclipse Public License 2.0 @@ -1405,6 +1405,8 @@ public final class RefactoringCoreMessages extends NLS { public static String MoveInstanceMethodProcessor_target_name_already_used; + public static String MoveInstanceMethodProcessor_target_null_comparison; + public static String MoveInstanceMethodProcessor_this_reference; public static String MoveInstanceMethodProcessor_uses_super; @@ -2550,6 +2552,7 @@ public final class RefactoringCoreMessages extends NLS { public static String ConvertToRecordRefactoring_member_types_not_supported; + static { NLS.initializeMessages(BUNDLE_NAME, RefactoringCoreMessages.class); } 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 899ee0f3006..9b143b3c671 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 @@ -934,6 +934,7 @@ MoveInstanceMethodProcessor_this_reference=A reference to 'this' has been found MoveInstanceMethodProcessor_no_resolved_target=The target of the method could not be resolved. MoveInstanceMethodProcessor_no_null_argument=The method invocation ''{0}'' cannot be updated, since it uses null as argument. MoveInstanceMethodProcessor_target_name_already_used=The name of the target conflicts with the method parameter ''{0}''. +MoveInstanceMethodProcessor_target_null_comparison=The move will cause ''{0}'' being replaced by ''this'' and compared to null which is invalid logic and may imply an NPE can occur. MoveInstanceMethodProcessor_target_element_pattern=Target element: ''{0}'' MoveInstanceMethodProcessor_present_type_parameter_warning=The type parameter ''{0}'' is already present in the target type ''{1}'' and will be removed from the method. MoveInstanceMethodProcessor_remove_original_method=Remove method declaration diff --git a/org.eclipse.jdt.core.manipulation/core extension/org/eclipse/jdt/internal/corext/refactoring/structure/MoveInstanceMethodProcessor.java b/org.eclipse.jdt.core.manipulation/core extension/org/eclipse/jdt/internal/corext/refactoring/structure/MoveInstanceMethodProcessor.java index 7a0c602fcad..2d064115457 100644 --- a/org.eclipse.jdt.core.manipulation/core extension/org/eclipse/jdt/internal/corext/refactoring/structure/MoveInstanceMethodProcessor.java +++ b/org.eclipse.jdt.core.manipulation/core extension/org/eclipse/jdt/internal/corext/refactoring/structure/MoveInstanceMethodProcessor.java @@ -1,5 +1,5 @@ /******************************************************************************* - * Copyright (c) 2000, 2025 IBM Corporation and others. + * Copyright (c) 2000, 2026 IBM Corporation and others. * * This program and the accompanying materials * are made available under the terms of the Eclipse Public License 2.0 @@ -97,6 +97,8 @@ 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.InfixExpression; +import org.eclipse.jdt.core.dom.InfixExpression.Operator; import org.eclipse.jdt.core.dom.Javadoc; import org.eclipse.jdt.core.dom.MethodDeclaration; import org.eclipse.jdt.core.dom.MethodInvocation; @@ -1713,7 +1715,7 @@ public RefactoringStatus checkFinalConditions(final IProgressMonitor monitor, fi final RefactoringStatus status= new RefactoringStatus(); fChangeManager= new TextChangeManager(); try { - monitor.beginTask("", 6); //$NON-NLS-1$ + monitor.beginTask("", 7); //$NON-NLS-1$ monitor.setTaskName(RefactoringCoreMessages.MoveInstanceMethodProcessor_checking); status.merge(Checks.checkIfCuBroken(fMethod)); if (!status.hasError()) { @@ -1732,6 +1734,7 @@ public RefactoringStatus checkFinalConditions(final IProgressMonitor monitor, fi checkConflictingTarget(Progress.subMonitor(monitor, 1), status); checkConflictingMethod(Progress.subMonitor(monitor, 1), status); checkOverrideOuterMethod(Progress.subMonitor(monitor, 1), status); + checkTargetNullCheck(Progress.subMonitor(monitor, 1), status); checkFinalMethod(status); Checks.addModifiedFilesToChecker(computeModifiedFiles(fMethod.getCompilationUnit(), type.getCompilationUnit()), context); @@ -1751,6 +1754,44 @@ public RefactoringStatus checkFinalConditions(final IProgressMonitor monitor, fi return status; } + private void checkTargetNullCheck(IProgressMonitor monitor, RefactoringStatus status) throws JavaModelException { + Assert.isNotNull(monitor); + Assert.isNotNull(status); + try { + monitor.beginTask("", 1); //$NON-NLS-1$ + monitor.setTaskName(RefactoringCoreMessages.MoveInstanceMethodProcessor_checking); + ASTVisitor visitor= new ASTVisitor() { + @Override + public boolean visit(InfixExpression node) { + if (node.getOperator() == Operator.EQUALS || node.getOperator() == Operator.NOT_EQUALS) { + Expression rightOperand= node.getRightOperand(); + if (rightOperand instanceof NullLiteral) { + Expression leftOperand= node.getLeftOperand(); + if (leftOperand instanceof Name name) { + IBinding nameBinding= name.resolveBinding(); + if (nameBinding.isEqualTo(fTarget)) { + throw new AbortSearchException(); + } + } + } + } + return true; + } + }; + try { + final MethodDeclaration declaration= ASTNodeSearchUtil.getMethodDeclarationNode(fMethod, fSourceRewrite.getRoot()); + if (declaration != null) { + declaration.accept(visitor); + } + } catch (AbortSearchException e) { + status.merge(RefactoringStatus.createErrorStatus( + Messages.format(RefactoringCoreMessages.MoveInstanceMethodProcessor_target_null_comparison, new String[] {fTarget.getName()}), JavaStatusContext.create(fMethod))); + } + } finally { + monitor.done(); + } + } + /** * Checks whether the target is a type variable or a generic type. * diff --git a/org.eclipse.jdt.ui.tests.refactoring/resources/MoveInstanceMethod/cannotMove/testFail21/in/A.java b/org.eclipse.jdt.ui.tests.refactoring/resources/MoveInstanceMethod/cannotMove/testFail21/in/A.java new file mode 100644 index 00000000000..f185d91c015 --- /dev/null +++ b/org.eclipse.jdt.ui.tests.refactoring/resources/MoveInstanceMethod/cannotMove/testFail21/in/A.java @@ -0,0 +1,10 @@ +package p1; + +class A { + B b; + + int m() { + if (b != null) return 2; + return 0; + } +} diff --git a/org.eclipse.jdt.ui.tests.refactoring/resources/MoveInstanceMethod/cannotMove/testFail21/in/B.java b/org.eclipse.jdt.ui.tests.refactoring/resources/MoveInstanceMethod/cannotMove/testFail21/in/B.java new file mode 100644 index 00000000000..df33307636d --- /dev/null +++ b/org.eclipse.jdt.ui.tests.refactoring/resources/MoveInstanceMethod/cannotMove/testFail21/in/B.java @@ -0,0 +1,5 @@ +package p1; + +public class B { + +} \ No newline at end of file diff --git a/org.eclipse.jdt.ui.tests.refactoring/test cases/org/eclipse/jdt/ui/tests/refactoring/MoveInstanceMethodTests.java b/org.eclipse.jdt.ui.tests.refactoring/test cases/org/eclipse/jdt/ui/tests/refactoring/MoveInstanceMethodTests.java index 62b577c4068..063f3ab0f34 100644 --- a/org.eclipse.jdt.ui.tests.refactoring/test cases/org/eclipse/jdt/ui/tests/refactoring/MoveInstanceMethodTests.java +++ b/org.eclipse.jdt.ui.tests.refactoring/test cases/org/eclipse/jdt/ui/tests/refactoring/MoveInstanceMethodTests.java @@ -789,6 +789,48 @@ public void testFail1() throws Exception { failHelper1("p1.A", 5, 26, 5, 29, PARAMETER, "b", true, true); } + // Cannot move static method + @Test + public void testFail2() throws Exception { + failHelper1(new String[] { "p1.A", "p2.B"}, "p1.A", 6, 23, 6, 24, PARAMETER, "b", true, true); + } + + // Cannot move native method + @Test + public void testFail3() throws Exception { + failHelper1(new String[] { "p1.A", "p2.B"}, "p1.A", 6, 23, 6, 24, PARAMETER, "b", true, true); + } + + // Cannot move method that references "super" + @Test + public void testFail4() throws Exception { + failHelper1(new String[] { "p1.A", "p2.B"}, "p1.A", 11, 20, 11, 21, PARAMETER, "b", true, true); + } + + // Cannot move method that references an enclosing instance + @Test + public void testFail5() throws Exception { + failHelper1(new String[] { "p1.A", "p2.B"}, "p1.A", 8, 21, 8, 21, PARAMETER, "b", true, true); + } + + // Cannot move potentially directly recursive method + @Test + public void testFail6() throws Exception { + failHelper1(new String[] { "p1.A", "p2.B"}, "p1.A", 6, 16, 6, 17, PARAMETER, "b", true, true); + } + + // Cannot move synchronized method + @Test + public void testFail8() throws Exception { + failHelper1(new String[] { "p1.A", "p2.B"}, "p1.A", 6, 29, 6, 29, PARAMETER, "b", true, true); + } + + // Cannot move method if there's no new potential receiver + @Test + public void testFail9() throws Exception { + failHelper1(new String[] { "p1.A", "p2.B", "p3.C"}, "p1.A", 7, 17, 7, 20, PARAMETER, "b", true, true); + } + // Cannot move method if there's no new potential receiver @Test public void testFail10() throws Exception { @@ -857,45 +899,9 @@ public void testFail20() throws Exception { failHelper1(new String[] { "p1.A" }, "p1.A", 6, 17, 6, 23, FIELD, "b", true, true); } - // Cannot move static method - @Test - public void testFail2() throws Exception { - failHelper1(new String[] { "p1.A", "p2.B"}, "p1.A", 6, 23, 6, 24, PARAMETER, "b", true, true); - } - - // Cannot move native method - @Test - public void testFail3() throws Exception { - failHelper1(new String[] { "p1.A", "p2.B"}, "p1.A", 6, 23, 6, 24, PARAMETER, "b", true, true); - } - - // Cannot move method that references "super" - @Test - public void testFail4() throws Exception { - failHelper1(new String[] { "p1.A", "p2.B"}, "p1.A", 11, 20, 11, 21, PARAMETER, "b", true, true); - } - - // Cannot move method that references an enclosing instance - @Test - public void testFail5() throws Exception { - failHelper1(new String[] { "p1.A", "p2.B"}, "p1.A", 8, 21, 8, 21, PARAMETER, "b", true, true); - } - - // Cannot move potentially directly recursive method - @Test - public void testFail6() throws Exception { - failHelper1(new String[] { "p1.A", "p2.B"}, "p1.A", 6, 16, 6, 17, PARAMETER, "b", true, true); - } - - // Cannot move synchronized method - @Test - public void testFail8() throws Exception { - failHelper1(new String[] { "p1.A", "p2.B"}, "p1.A", 6, 29, 6, 29, PARAMETER, "b", true, true); - } - - // Cannot move method if there's no new potential receiver + // Issue 3035 @Test - public void testFail9() throws Exception { - failHelper1(new String[] { "p1.A", "p2.B", "p3.C"}, "p1.A", 7, 17, 7, 20, PARAMETER, "b", true, true); + public void testFail21() throws Exception { + failHelper1(new String[] { "p1.A", "p1.B" }, "p1.A", 6, 9, 6, 10, FIELD, "b", true, true); } }