Fix move instance processor to handle class instance creation accessi…

…bility (#1413)

* Fix move instance processor to recognize an inaccessible constructor

- add logic to the MethodBodyRewriter so it will recognize a
  constructor call that will be inaccessible after moving and
  try to modify the accessibility of the constructor
- fix the ThisReferenceFinder to ignore field references made
  from a local variable
- add new tests to MoveInstanceMethodTests
- fixes #1304

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

@@ -505,12 +505,18 @@ public final class MethodBodyRewriter extends ASTVisitor {
 		/** The anonymous class nesting counter */
 		protected int fAnonymousClass= 0;
 
+		/** The visibility adjustments */
+		private final Map<IMember, IncomingMemberVisibilityAdjustment> fAdjustments;
+
 		/** The method declaration to rewrite */
 		protected final MethodDeclaration fDeclaration;
 
 		/** The source ast rewrite to use */
 		protected final ASTRewrite fRewrite;
 
+		/** The compilation unit rewrites */
+		private final Map<ICompilationUnit, CompilationUnitRewrite> fRewrites;
+
 		/** The existing static imports */
 		protected final Set<IBinding> fStaticImports= new HashSet<>();
 
@@ -523,20 +529,26 @@ public final class MethodBodyRewriter extends ASTVisitor {
 		/**
 		 * Creates a new method body rewriter.
 		 *
+		 * @param rewrites
+		 *            the compilation unit rewrites
 		 * @param targetRewrite
 		 *            the target compilation unit rewrite to use
 		 * @param rewrite
 		 *            the source ast rewrite to use
 		 * @param sourceDeclaration
 		 *            the source method declaration
+		 * @param adjustments
+		 *            the map of elements to visibility adjustments
 		 */
-		public MethodBodyRewriter(final CompilationUnitRewrite targetRewrite, final ASTRewrite rewrite, final MethodDeclaration sourceDeclaration) {
+		public MethodBodyRewriter(final Map<ICompilationUnit, CompilationUnitRewrite> rewrites, final CompilationUnitRewrite targetRewrite, final ASTRewrite rewrite, final MethodDeclaration sourceDeclaration, final Map<IMember, IncomingMemberVisibilityAdjustment> adjustments) {
 			Assert.isNotNull(targetRewrite);
 			Assert.isNotNull(rewrite);
 			Assert.isNotNull(sourceDeclaration);
 			fTargetRewrite= targetRewrite;
 			fRewrite= rewrite;
+			fRewrites= rewrites;
 			fDeclaration= sourceDeclaration;
+			fAdjustments= adjustments;
 			fStaticImports.clear();
 			ImportRewriteUtil.collectImports(fMethod.getJavaProject(), sourceDeclaration, new HashSet<>(), fStaticImports, false);
 		}
@@ -571,7 +583,7 @@ public boolean visit(final AnonymousClassDeclaration node) {
 		public boolean visit(SimpleType node) {
 			ITypeBinding nodeTypeBinding= ASTNodes.getEnclosingType(node);
 			ITypeBinding simpleTypeBinding= node.resolveBinding();
-			if (nodeTypeBinding != null && !nodeTypeBinding.isEqualTo(simpleTypeBinding) && simpleTypeBinding.isMember()) {
+			if (nodeTypeBinding != null && simpleTypeBinding != null && !nodeTypeBinding.isEqualTo(simpleTypeBinding) && simpleTypeBinding.isMember()) {
 				AST ast= node.getAST();
 				if (node.getParent() instanceof ClassInstanceCreation parent && parent.getExpression() == null && !Modifier.isStatic(simpleTypeBinding.getModifiers())) {
 					ClassInstanceCreation newCreation= ast.newClassInstanceCreation();
@@ -649,7 +661,28 @@ public boolean visit(final ClassInstanceCreation node) {
 						fRewrite.replace(node, newCreation, null);
 					}
 				}
-
+				IMethodBinding constructorBinding= node.resolveConstructorBinding();
+				if (constructorBinding != null) {
+					IMethod constructor= (IMethod) constructorBinding.getJavaElement();
+					try {
+						if (constructor != null && !constructor.isBinary() && !constructor.isReadOnly() && !Modifier.isPublic(constructor.getFlags())) {
+							boolean same= false;
+							final CompilationUnitRewrite rewrite= getCompilationUnitRewrite(fRewrites, constructor.getCompilationUnit());
+							final MethodDeclaration declaration= ASTNodeSearchUtil.getMethodDeclarationNode(constructor, rewrite.getRoot());
+							if (declaration != null) {
+								final ITypeBinding declaring= constructorBinding.getDeclaringClass();
+								if (declaring != null && Bindings.equals(declaring.getPackage(), fTarget.getType().getPackage()))
+									same= true;
+								final Modifier.ModifierKeyword keyword= same ? null : Modifier.ModifierKeyword.PUBLIC_KEYWORD;
+								final String modifier= same ? RefactoringCoreMessages.MemberVisibilityAdjustor_change_visibility_default : RefactoringCoreMessages.MemberVisibilityAdjustor_change_visibility_public;
+								if (MemberVisibilityAdjustor.hasLowerVisibility(constructorBinding.getModifiers(), same ? Modifier.NONE : keyword == null ? Modifier.NONE : keyword.toFlagValue()) && MemberVisibilityAdjustor.needsVisibilityAdjustments(constructor, keyword, fAdjustments))
+									fAdjustments.put(constructor, new MemberVisibilityAdjustor.OutgoingMemberVisibilityAdjustment(constructor, keyword, RefactoringStatus.createWarningStatus(Messages.format(RefactoringCoreMessages.MemberVisibilityAdjustor_change_visibility_method_warning, new String[] { BindingLabelProviderCore.getBindingLabel(declaration.resolveBinding(), JavaElementLabelsCore.ALL_FULLY_QUALIFIED), modifier }), JavaStatusContext.create(constructor.getCompilationUnit(), declaration))));
+							}
+						}
+					} catch (JavaModelException e) {
+						// unexpected
+					}
+				}
 			}
 			return super.visit(node);
 		}
@@ -1096,7 +1129,7 @@ public boolean visit(final MethodInvocation node) {
 		@Override
 		public boolean visit(final SimpleName node) {
 			Assert.isNotNull(node);
-			if (isFieldAccess(node) && !isTargetAccess(node)) {
+			if (isFieldAccess(node) && !isLocalQualified(node) && !isTargetAccess(node)) {
 				fResult.add(node);
 				fStatus.merge(RefactoringStatus.createFatalErrorStatus(RefactoringCoreMessages.MoveInstanceMethodProcessor_this_reference, JavaStatusContext.create(fMethod.getCompilationUnit(), node)));
 			} else if (node.getParent() instanceof ClassInstanceCreation constructor && constructor.getExpression() == null) {
@@ -1283,6 +1316,24 @@ protected static boolean isFieldAccess(final SimpleName name) {
 		return !Modifier.isStatic(variable.getModifiers());
 	}
 
+	protected static boolean isLocalQualified(final SimpleName name) {
+		if (name.getParent() instanceof FieldAccess fieldAccess) {
+			Expression exp= fieldAccess.getExpression();
+			if (exp instanceof SimpleName qualifierName) {
+				IBinding qualifierBinding= qualifierName.resolveBinding();
+				if (qualifierBinding instanceof IVariableBinding varBinding && !varBinding.isField()) {
+					return true;
+				}
+			}
+		} else if (name.getParent() instanceof QualifiedName qualifiedName) {
+			IBinding qualifierBinding= qualifiedName.getQualifier().resolveBinding();
+			if (qualifierBinding instanceof IVariableBinding varBinding && !varBinding.isField()) {
+				return true;
+			}
+		}
+		return false;
+	}
+
 	/** The candidate targets */
 	private IVariableBinding[] fCandidateTargets= new IVariableBinding[0];
 
@@ -2315,16 +2366,21 @@ public final ASTNode getTargetNode() throws JavaModelException {
 	/**
 	 * Creates the method body for the target method declaration.
 	 *
+	 * @param rewrites
+	 *            the compilation unit rewrites
 	 * @param rewriter
 	 *            the target compilation unit rewrite
 	 * @param rewrite
 	 *            the source ast rewrite
 	 * @param declaration
 	 *            the source method declaration
+	 * @param adjustments
+	 *            the map of elements to visibility adjustments
 	 */
-	protected void createMethodBody(final CompilationUnitRewrite rewriter, final ASTRewrite rewrite, final MethodDeclaration declaration) {
+	protected void createMethodBody(final Map<ICompilationUnit, CompilationUnitRewrite> rewrites, final CompilationUnitRewrite rewriter,
+			final ASTRewrite rewrite, final MethodDeclaration declaration, final Map<IMember, IncomingMemberVisibilityAdjustment> adjustments) {
 		Assert.isNotNull(declaration);
-		declaration.getBody().accept(new MethodBodyRewriter(rewriter, rewrite, declaration));
+		declaration.getBody().accept(new MethodBodyRewriter(rewrites, rewriter, rewrite, declaration, adjustments));
 	}
 
 	/**
@@ -2514,7 +2570,7 @@ protected void createMethodCopy(IDocument document, MethodDeclaration declaratio
 			createMethodArguments(rewrites, rewrite, declaration, adjustments, status);
 			createMethodTypeParameters(rewrite, declaration, status);
 			createMethodComment(rewrite, declaration);
-			createMethodBody(rewriter, rewrite, declaration);
+			createMethodBody(rewrites, rewriter, rewrite, declaration, adjustments);
 		} finally {
 			if (fMethod.getCompilationUnit().equals(getTargetType().getCompilationUnit()))
 				rewriter.clearImportRewrites();

org.eclipse.jdt.ui.tests.refactoring/resources/MoveInstanceMethod/canMove/test77/in/A.java

@@ -0,0 +1,20 @@
+package p1;
+
+public class A {
+
+    B b;
+    int k;
+
+    private A() {
+
+    }
+
+    public void m() {
+        A a = new A();
+        a.k = 3;
+    }
+}
+
+class B {
+
+}

org.eclipse.jdt.ui.tests.refactoring/resources/MoveInstanceMethod/canMove/test77/out/A.java

@@ -0,0 +1,20 @@
+package p1;
+
+public class A {
+
+    B b;
+    int k;
+
+    A() {
+
+    }
+}
+
+class B {
+
+	public void m() {
+	    A a = new A();
+	    a.k = 3;
+	}
+
+}

org.eclipse.jdt.ui.tests.refactoring/resources/MoveInstanceMethod/canMove/test78/in/A.java

@@ -0,0 +1,16 @@
+package p1;
+
+public class A {
+
+    B b;
+    int k;
+
+    private A() {
+
+    }
+
+    public void m() {
+        A a = new A();
+        a.k = 3;
+    }
+}

org.eclipse.jdt.ui.tests.refactoring/resources/MoveInstanceMethod/canMove/test78/in/B.java

@@ -0,0 +1,5 @@
+package p1;
+
+class B {
+
+}

org.eclipse.jdt.ui.tests.refactoring/resources/MoveInstanceMethod/canMove/test78/out/A.java

@@ -0,0 +1,11 @@
+package p1;
+
+public class A {
+
+    B b;
+    int k;
+
+    A() {
+
+    }
+}

org.eclipse.jdt.ui.tests.refactoring/resources/MoveInstanceMethod/canMove/test78/out/B.java

@@ -0,0 +1,10 @@
+package p1;
+
+class B {
+
+	public void m() {
+	    A a = new A();
+	    a.k = 3;
+	}
+
+}

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

@@ -692,6 +692,18 @@ public void test76() throws Exception {
 		helper1(new String[] { "p1.A", "p1.B" }, "p1.A", 13, 17, 13, 18, FIELD, "b", true, true);
 	}
 
+	// Issue 1304
+	@Test
+	public void test77() throws Exception {
+		helper1(new String[] { "p1.A" }, "p1.A", 12, 17, 12, 18, FIELD, "b", true, true);
+	}
+
+	// Issue 1304
+	@Test
+	public void test78() throws Exception {
+		helper1(new String[] { "p1.A", "p1.B" }, "p1.A", 12, 17, 12, 18, FIELD, "b", true, true);
+	}
+
 	// Move mA1 to field fB, do not inline delegator
 	@Test
 	public void test3() throws Exception {