Add check to move member in case member moved to non-public class (#1407

)

* Add check to move member in case member moved to non-public class

- flag any references to a moved member where the move will be to
  a package-private class that is referenced by a field since the
  access in another package will cause an error
- add new failure test to MoveInstanceMethodTests
- fixes #1404

org.eclipse.jdt.core.manipulation/core extension/org/eclipse/jdt/internal/corext/refactoring/RefactoringCoreMessages.java

@@ -1319,6 +1319,8 @@ public final class RefactoringCoreMessages extends NLS {
 
 	public static String MoveInstanceMethodProcessor_add_moved_method;
 
+	public static String MoveInstanceMethodProcessor_cannot_access_or_adjust;
+
 	public static String MoveInstanceMethodProcessor_cannot_be_moved;
 
 	public static String MoveInstanceMethodProcessor_checking;
@@ -1335,6 +1337,14 @@ public final class RefactoringCoreMessages extends NLS {
 
 	public static String MoveInstanceMethodProcessor_inline_overridden;
 
+	public static String MoveInstanceMethodProcessor_methoddeclaration_accesses_protected;
+
+	public static String MoveInstanceMethodProcessor_methoddeclaration_has_errors;
+
+	public static String MoveInstanceMethodProcessor_methoddeclaration_accesses_package_private;
+
+	public static String MoveInstanceMethodProcessor_methoddeclaration_accesses_private;
+
 	public static String MoveInstanceMethodProcessor_method_already_exists;
 
 	public static String MoveInstanceMethodProcessor_method_final_to_interface;

org.eclipse.jdt.core.manipulation/core extension/org/eclipse/jdt/internal/corext/refactoring/refactoring.properties

@@ -475,7 +475,7 @@ RenameMethodRefactoring_no_read_only=Related method ''{0}'' (declared in ''{1}''
 RenameMethodRefactoring_not_in_model=Related method ''{0}'' (declared in ''{1}'') does not exist in the model.
 RenameMethodRefactoring_overrides_static_name=This name collides with a static import name.
 RenameMethodRefactoring_overrides_static_name2=The static import ''{0}'' in type ''{1}'' already has the assigned name.
-enameMethodRefactoring_same_name=This name already exists.
+RenameMethodRefactoring_same_name=This name already exists.
 RenameMethodRefactoring_same_name2=The method ''{0}'' in type ''{1}'' already has the assigned name.
 RenameMethodRefactoring_update_occurrence=Update method reference
 RenameMethodRefactoring_update_declaration=Update method declaration
@@ -907,11 +907,16 @@ MoveInstanceMethodProcessor_present_type_parameter_warning=The type parameter ''
 MoveInstanceMethodProcessor_remove_original_method=Remove method declaration
 MoveInstanceMethodProcessor_inline_method_invocation=Update method invocation
 MoveInstanceMethodProcessor_add_moved_method=Add method declaration
+MoveInstanceMethodProcessor_cannot_access_or_adjust=Move would create a field access error on non-public type ''{0}''.
 MoveInstanceMethodProcessor_descriptor_description=Move method ''{0}'' to ''{1}''
 MoveInstanceMethodProcessor_parameter_name_pattern=Parameter name: ''{0}''
 MoveInstanceMethodProcessor_descriptor_description_short=Move method ''{0}''
 MoveInstanceMethodProcessor_inline_inaccurate=A method invocation to the original method in ''{0}'' could not be fully resolved.
 MoveInstanceMethodProcessor_inline_overridden=The method invocations to ''{0}'' cannot be updated, since the original method is used polymorphically.
+MoveInstanceMethodProcessor_methoddeclaration_accesses_protected=Method to move accesses protected members that cannot be accessed at target location.
+MoveInstanceMethodProcessor_methoddeclaration_has_errors=Method to move has compilation errors.  Fix compilation errors and retry.
+MoveInstanceMethodProcessor_methoddeclaration_accesses_package_private=Method to move access package-private members that cannot be accessed at target location.
+MoveInstanceMethodProcessor_methoddeclaration_accesses_private=Method to move accesses private members that cannot be accessed at target location.
 MoveInstanceMethodProcessor_method_already_exists=A method with name ''{0}'' already exists in the target type ''{1}''.
 MoveInstanceMethodProcessor_method_final_to_interface=Method with name ''{0}'' is ''final'' so moving to interface ''{1}'' requires removing ''final'' modifier.
 MoveInstanceMethodProcessor_moved_element_pattern=Moved method: {0}

org.eclipse.jdt.core.manipulation/core extension/org/eclipse/jdt/internal/corext/refactoring/structure/MoveInstanceMethodProcessor.java

@@ -1,5 +1,5 @@
 /*******************************************************************************
- * Copyright (c) 2000, 2021 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
@@ -272,6 +272,57 @@ public final RefactoringStatus getStatus() {
 		}
 	}
 
+	protected static class AccessAnalyzer extends ASTVisitor {
+		public boolean fAccessesPrivate;
+		public boolean fAccessesProtected;
+		public boolean fAccessesPackagePrivate;
+		@Override
+		public boolean visit(SimpleName node) {
+			IBinding binding= node.resolveBinding();
+			if (binding != null) {
+				int modifiers= binding.getModifiers();
+				boolean isPublic= Modifier.isPublic(modifiers);
+				boolean isPrivate= Modifier.isPrivate(modifiers);
+				boolean isProtected= Modifier.isProtected(modifiers);
+				if (!isPublic) {
+					if (binding instanceof IVariableBinding varBinding) {
+						ITypeBinding declClass= varBinding.getDeclaringClass();
+						if (!varBinding.isField() || declClass == null || declClass.isLocal()) {
+							return true;
+						}
+					} else if (binding instanceof IMethodBinding methodBinding) {
+						ITypeBinding declClass= methodBinding.getDeclaringClass();
+						if (declClass == null || declClass.isLocal()) {
+							return true;
+						}
+					} else {
+						return true;
+					}
+				}
+				fAccessesPrivate= fAccessesPrivate || isPrivate;
+				fAccessesProtected= fAccessesProtected || isProtected;
+				fAccessesPackagePrivate= fAccessesPackagePrivate || (!isPublic && !isPrivate && !isProtected);
+			}
+			return true;
+		}
+
+		public boolean accessesPrivate() {
+			return fAccessesPrivate;
+		}
+
+		public boolean accessesProtected() {
+			return fAccessesProtected;
+		}
+
+		public boolean accessesPackagePrivate() {
+			return fAccessesPackagePrivate;
+		}
+
+		public boolean onlyAccessesPublic() {
+			return !fAccessesPrivate && !fAccessesProtected && !fAccessesPackagePrivate;
+		}
+	}
+
 	class DelegateInstanceMethodCreator extends DelegateMethodCreator {
 
 		private Map<IMember, IncomingMemberVisibilityAdjustment> fAdjustments;
@@ -1469,8 +1520,10 @@ protected void checkMethodBody(final IProgressMonitor monitor, final MethodDecla
 				declaration.accept(finder);
 				if (!finder.getStatus().isOK())
 					status.merge(finder.getStatus());
+
 				monitor.worked(1);
 			}
+
 		} finally {
 			monitor.done();
 		}
@@ -1691,7 +1744,7 @@ private static boolean is18OrHigherInterface(ITypeBinding binding) {
 	 * @throws JavaModelException
 	 *             if an error occurs while accessing the target expression
 	 */
-	protected Expression createAdjustedTargetExpression(final IJavaElement enclosingElement, final Expression expression, final Map<IMember, IncomingMemberVisibilityAdjustment> adjustments, final ASTRewrite rewrite) throws JavaModelException {
+	protected Expression createAdjustedTargetExpression(final IJavaElement enclosingElement, final Expression expression, final Map<IMember, IncomingMemberVisibilityAdjustment> adjustments, final ASTRewrite rewrite, RefactoringStatus status) throws JavaModelException {
 		Assert.isNotNull(enclosingElement);
 		Assert.isNotNull(adjustments);
 		Assert.isNotNull(rewrite);
@@ -1717,8 +1770,12 @@ protected Expression createAdjustedTargetExpression(final IJavaElement enclosing
 						}
 					}
 				}
-				if (MemberVisibilityAdjustor.hasLowerVisibility(field.getFlags(), (keyword == null ? Modifier.NONE : keyword.toFlagValue())) && MemberVisibilityAdjustor.needsVisibilityAdjustments(field, keyword, adjustments))
+				if (MemberVisibilityAdjustor.hasLowerVisibility(field.getFlags(), (keyword == null ? Modifier.NONE : keyword.toFlagValue())) && MemberVisibilityAdjustor.needsVisibilityAdjustments(field, keyword, adjustments)) {
+					if (MemberVisibilityAdjustor.hasLowerVisibility(fTarget.getType().getModifiers(), keyword == null ? Modifier.NONE : keyword.toFlagValue())) {
+						status.merge(RefactoringStatus.createErrorStatus(Messages.format(RefactoringCoreMessages.MoveInstanceMethodProcessor_cannot_access_or_adjust, new String[] { BindingLabelProviderCore.getBindingLabel(fTarget.getType(), JavaElementLabelsCore.ALL_FULLY_QUALIFIED)}), JavaStatusContext.create(field)));
+					}
 					adjustments.put(field, new MemberVisibilityAdjustor.OutgoingMemberVisibilityAdjustment(field, keyword, RefactoringStatus.createWarningStatus(Messages.format(RefactoringCoreMessages.MemberVisibilityAdjustor_change_visibility_field_warning, new String[] { BindingLabelProviderCore.getBindingLabel(fTarget, JavaElementLabelsCore.ALL_FULLY_QUALIFIED), modifier }), JavaStatusContext.create(field))));
+				}
 			}
 		}
 		return null;
@@ -1971,6 +2028,10 @@ protected boolean createInlinedMethodInvocation(CompilationUnitRewrite rewriter,
 				Expression access= null;
 				if (invocation.getExpression() != null) {
 					access= createInlinedTargetExpression(rewriter, (IJavaElement) match.getElement(), invocation.getExpression(), adjustments, status);
+					if (status.hasError()) {
+						result= false;
+						return result;
+					}
 					rewrite.set(invocation, MethodInvocation.EXPRESSION_PROPERTY, access, group);
 				} else
 					rewrite.set(invocation, MethodInvocation.EXPRESSION_PROPERTY, rewrite.getAST().newSimpleName(fTarget.getName()), group);
@@ -2055,8 +2116,8 @@ protected Expression createInlinedTargetExpression(final CompilationUnitRewrite
 		Assert.isNotNull(status);
 		Assert.isTrue(fTarget.isField());
 		final Expression expression= (Expression) ASTNode.copySubtree(fSourceRewrite.getASTRewrite().getAST(), original);
-		final Expression result= createAdjustedTargetExpression(enclosingElement, expression, adjustments, fSourceRewrite.getASTRewrite());
-		if (result == null) {
+		final Expression result= createAdjustedTargetExpression(enclosingElement, expression, adjustments, fSourceRewrite.getASTRewrite(), status);
+		if (result == null && !status.hasError()) {
 			final FieldAccess access= fSourceRewrite.getASTRewrite().getAST().newFieldAccess();
 			access.setExpression(expression);
 			access.setName(fSourceRewrite.getASTRewrite().getAST().newSimpleName(fTarget.getName()));

org.eclipse.jdt.ui.tests.refactoring/resources/MoveInstanceMethod/cannotMove/testFail17/in/A.java

@@ -0,0 +1,18 @@
+package p1;
+
+class A1 extends A {
+
+}
+
+public class InputClass {
+
+    A1 a1;
+
+    int value = 6;
+
+    public int getValue() {
+        return value;
+    }
+
+}
+

org.eclipse.jdt.ui.tests.refactoring/resources/MoveInstanceMethod/cannotMove/testFail17/in/B.java

@@ -0,0 +1,11 @@
+package p2;
+
+import p1.A;
+
+public class B {
+
+	public void foo() {
+		A a = new A();
+		a.getValue();
+	}
+}

org.eclipse.jdt.ui.tests.refactoring/test cases/org/eclipse/jdt/ui/tests/refactoring/MoveInstanceMethodTests.java

@@ -755,6 +755,12 @@ public void testFail16() throws Exception {
 		failHelper1(new String[] { "p1.A", "p1.B"}, "p1.A", 8, 14, 8, 15, PARAMETER, "b", true, true);
 	}
 
+	// Issue 1404
+	@Test
+	public void testFail17() throws Exception {
+		failHelper1(new String[] { "p1.A", "p2.B"}, "p1.A", 13, 16, 13, 24, FIELD, "a1", true, true);
+	}
+
 	// Cannot move static method
 	@Test
 	public void testFail2() throws Exception {