Skip to content

Commit

Permalink
Fix inline method to check access modifiers of inlined code (#1381)
Browse files Browse the repository at this point in the history
* Fix inline method to check access modifiers of inlined code

- add new checkAccessCompatible() method to SourceProvider that
  checks if code from declaration can be moved to target without
  causing an access violation
- add new AccessVisitor to SourceAnalyzer to log non-public
  accesses (private, package-private, and protected)
- add call to checkAccessCompatible() in CallInliner
- add new tests to InlineMethodTests
- fixes #1358
  • Loading branch information
jjohnstn committed May 2, 2024
1 parent 8a46c86 commit 6333016
Show file tree
Hide file tree
Showing 12 changed files with 265 additions and 2 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -2520,6 +2520,27 @@ public static <T extends ASTNode> T getParent(ASTNode node, Class<T> parentClass
return parentClass.cast(node);
}

/**
* Returns the top-level AbstractTypeDeclaration that is the parent of the node.
*
* @param node the node
* @return the top-level AbstractTypeDeclaration node for the file that node is in
*/
public static AbstractTypeDeclaration getTopLevelTypeDeclaration(ASTNode node) {
AbstractTypeDeclaration result= null;
if (node instanceof AbstractTypeDeclaration) {
result= (AbstractTypeDeclaration) node;
}
ASTNode parent= node.getParent();
while (parent != null) {
if (parent instanceof AbstractTypeDeclaration) {
result= (AbstractTypeDeclaration) parent;
}
parent= parent.getParent();
}
return result;
}

/**
* Returns the closest ancestor of <code>node</code> whose type is <code>nodeType</code>, or <code>null</code> if none.
* <p>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,8 @@ public final class RefactoringCoreMessages extends NLS {

public static String CallInliner_super_into_this_expression;

public static String CallInliner_unexpected_model_exception;

public static String Change_does_not_exist;

public static String ChangeSignatureRefactoring_add_constructor;
Expand Down Expand Up @@ -855,6 +857,12 @@ public final class RefactoringCoreMessages extends NLS {

public static String InlineMethodRefactoring_SourceAnalyzer_declaration_has_errors;

public static String InlineMethodRefactoring_SourceAnalyzer_methoddeclaration_accesses_package_private;

public static String InlineMethodRefactoring_SourceAnalyzer_methoddeclaration_accesses_private;

public static String InlineMethodRefactoring_SourceAnalyzer_methoddeclaration_accesses_protected;

public static String InlineMethodRefactoring_SourceAnalyzer_methoddeclaration_has_errors;

public static String InlineMethodRefactoring_SourceAnalyzer_native_methods;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,7 @@
import org.eclipse.ltk.core.refactoring.RefactoringStatusEntry;

import org.eclipse.jdt.core.ICompilationUnit;
import org.eclipse.jdt.core.JavaModelException;
import org.eclipse.jdt.core.dom.AST;
import org.eclipse.jdt.core.dom.ASTNode;
import org.eclipse.jdt.core.dom.ASTVisitor;
Expand Down Expand Up @@ -265,6 +266,7 @@ public RefactoringStatus initialize(ASTNode invocation, int severity) {
JavaManipulationPlugin.log(exception);
}
checkInvocationContext(result, severity);
checkAccessCompatibility(result, severity);

return result;
}
Expand Down Expand Up @@ -317,6 +319,17 @@ private void checkMethodDeclaration(RefactoringStatus result, int severity) {
}
}

private void checkAccessCompatibility(RefactoringStatus result, int severity) {
try {
result.merge(fSourceProvider.checkAccessCompatible(fTargetNode));
} catch (JavaModelException e) {
result.addEntry(new RefactoringStatusEntry(
severity,
RefactoringCoreMessages.CallInliner_unexpected_model_exception,
JavaStatusContext.create(fCUnit, fInvocation)));
}
}

private void checkInvocationContext(RefactoringStatus result, int severity) {
if (fInvocation.getNodeType() == ASTNode.METHOD_INVOCATION) {
if (((MethodInvocation)fInvocation).resolveTypeBinding() == null) {
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*******************************************************************************
* Copyright (c) 2000, 2019 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
Expand Down Expand Up @@ -361,6 +361,41 @@ public boolean visit(ArrayAccess node) {
}
}

private class AccessAnalyzer extends ASTVisitor {
public boolean accessesPrivate;
public boolean accessesProtected;
public boolean accessesPackagePrivate;
@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;
}
}
accessesPrivate= accessesPrivate || isPrivate;
accessesProtected= accessesProtected || isProtected;
accessesPackagePrivate= accessesPackagePrivate || (!isPublic && !isPrivate && !isProtected);
}
return true;
}
}

private ITypeRoot fTypeRoot;
private MethodDeclaration fDeclaration;
private Map<IVariableBinding, ParameterData> fParameters;
Expand All @@ -381,6 +416,10 @@ public boolean visit(ArrayAccess node) {

private boolean fInterruptedExecutionFlow;

private boolean fAccessesPrivate;
private boolean fAccessesProtected;
private boolean fAccessesPackagePrivate;

public SourceAnalyzer(ITypeRoot typeRoot, MethodDeclaration declaration) {
super();
fTypeRoot= typeRoot;
Expand Down Expand Up @@ -420,6 +459,11 @@ public RefactoringStatus checkActivation() throws JavaModelException {
result.addFatalError(RefactoringCoreMessages.InlineMethodRefactoring_SourceAnalyzer_methoddeclaration_has_errors, JavaStatusContext.create(fTypeRoot));
return result;
}
AccessAnalyzer accessAnalyzer= new AccessAnalyzer();
fDeclaration.accept(accessAnalyzer);
fAccessesPrivate= accessAnalyzer.accessesPrivate;
fAccessesProtected= accessAnalyzer.accessesProtected;
fAccessesPackagePrivate= accessAnalyzer.accessesPackagePrivate;
ActivationAnalyzer analyzer= new ActivationAnalyzer();
fDeclaration.accept(analyzer);
result.merge(analyzer.status);
Expand Down Expand Up @@ -536,4 +580,20 @@ private ASTNode[] getStatements() {
return statements.toArray(new ASTNode[statements.size()]);
}

public boolean accessesPrivate() {
return fAccessesPrivate;
}

public boolean accessesProtected() {
return fAccessesProtected;
}

public boolean accessesPackagePrivate() {
return fAccessesPackagePrivate;
}

public boolean onlyAccessesPublic() {
return !fAccessesPrivate && !fAccessesProtected && !fAccessesPackagePrivate;
}

}
Original file line number Diff line number Diff line change
Expand Up @@ -51,12 +51,14 @@

import org.eclipse.jdt.core.ICompilationUnit;
import org.eclipse.jdt.core.IJavaProject;
import org.eclipse.jdt.core.IPackageDeclaration;
import org.eclipse.jdt.core.IType;
import org.eclipse.jdt.core.ITypeRoot;
import org.eclipse.jdt.core.JavaModelException;
import org.eclipse.jdt.core.dom.AST;
import org.eclipse.jdt.core.dom.ASTNode;
import org.eclipse.jdt.core.dom.ASTVisitor;
import org.eclipse.jdt.core.dom.AbstractTypeDeclaration;
import org.eclipse.jdt.core.dom.Block;
import org.eclipse.jdt.core.dom.CastExpression;
import org.eclipse.jdt.core.dom.ChildPropertyDescriptor;
Expand All @@ -79,6 +81,7 @@
import org.eclipse.jdt.core.dom.MethodInvocation;
import org.eclipse.jdt.core.dom.Modifier;
import org.eclipse.jdt.core.dom.Name;
import org.eclipse.jdt.core.dom.PackageDeclaration;
import org.eclipse.jdt.core.dom.ParenthesizedExpression;
import org.eclipse.jdt.core.dom.ReturnStatement;
import org.eclipse.jdt.core.dom.SimpleName;
Expand All @@ -101,7 +104,9 @@
import org.eclipse.jdt.internal.corext.dom.ASTNodes;
import org.eclipse.jdt.internal.corext.dom.Bindings;
import org.eclipse.jdt.internal.corext.dom.CodeScopeBuilder;
import org.eclipse.jdt.internal.corext.refactoring.RefactoringCoreMessages;
import org.eclipse.jdt.internal.corext.refactoring.code.SourceAnalyzer.NameData;
import org.eclipse.jdt.internal.corext.refactoring.util.JavaStatusContext;
import org.eclipse.jdt.internal.corext.refactoring.util.RefactoringFileBuffers;
import org.eclipse.jdt.internal.corext.util.CodeFormatterUtil;

Expand Down Expand Up @@ -801,4 +806,58 @@ private boolean isSingleControlStatementWithoutBlock() {
}
return false;
}

public RefactoringStatus checkAccessCompatible(ASTNode targetNode) throws JavaModelException {
RefactoringStatus result= new RefactoringStatus();
if (fAnalyzer.onlyAccessesPublic()) {
return result;
}
IMethodBinding declBinding= fDeclaration.resolveBinding();
if (declBinding == null) {
result.addFatalError(RefactoringCoreMessages.InlineMethodRefactoring_SourceAnalyzer_methoddeclaration_has_errors, JavaStatusContext.create(fTypeRoot));
return result;
}
ASTNode targetRoot= targetNode.getRoot();
if (targetRoot instanceof CompilationUnit cu && (fTypeRoot instanceof ICompilationUnit rootICU)) {
AbstractTypeDeclaration typeNode= ASTNodes.getTopLevelTypeDeclaration(targetNode);
AbstractTypeDeclaration declTypeNode= ASTNodes.getTopLevelTypeDeclaration(fDeclaration);
if (typeNode != null && declTypeNode != null) {
ITypeBinding typeNodeBinding= typeNode.resolveBinding();
ITypeBinding declTypeNodeBinding= declTypeNode.resolveBinding();
if (typeNodeBinding != null && declTypeNodeBinding != null) {
if (typeNodeBinding.isEqualTo(declTypeNodeBinding)) {
return result;
}
}
}
if (fAnalyzer.accessesPrivate()) {
result.addFatalError(RefactoringCoreMessages.InlineMethodRefactoring_SourceAnalyzer_methoddeclaration_accesses_private, JavaStatusContext.create(fTypeRoot));
return result;
}
PackageDeclaration packageDecl= cu.getPackage();
IPackageDeclaration[] rootPackageDecls= rootICU.getPackageDeclarations();
if (packageDecl.getName().getFullyQualifiedName().equals(rootPackageDecls[0].getElementName())) {
return result;
}
if (fAnalyzer.accessesPackagePrivate()) {
result.addFatalError(RefactoringCoreMessages.InlineMethodRefactoring_SourceAnalyzer_methoddeclaration_accesses_package_private, JavaStatusContext.create(fTypeRoot));
return result;
}
ITypeBinding typeBinding= declBinding.getDeclaringClass();
if (typeBinding == null) {
result.addFatalError(RefactoringCoreMessages.InlineMethodRefactoring_SourceAnalyzer_methoddeclaration_has_errors, JavaStatusContext.create(fTypeRoot));
return result;
}
if (typeNode != null) {
ITypeBinding targetTypeBinding= typeNode.resolveBinding();
if (targetTypeBinding != null) {
if (ASTNodes.findImplementedType(targetTypeBinding, typeBinding.getQualifiedName()) == null
|| (targetNode instanceof MethodInvocation invocation && invocation.getExpression() != null && !(invocation.getExpression() instanceof ThisExpression))) {
result.addFatalError(RefactoringCoreMessages.InlineMethodRefactoring_SourceAnalyzer_methoddeclaration_accesses_protected, JavaStatusContext.create(fTypeRoot));
}
}
}
}
return result;
}
}
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
###############################################################################
# Copyright (c) 2000, 2022 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
Expand Down Expand Up @@ -197,6 +197,9 @@ InlineMethodRefactoring_checking_implements_error= Method to be inlined implemen

InlineMethodRefactoring_SourceAnalyzer_recursive_call=Method declaration contains recursive call.
InlineMethodRefactoring_SourceAnalyzer_native_methods=Cannot inline native methods
InlineMethodRefactoring_SourceAnalyzer_methoddeclaration_accesses_package_private=The method declaration references at least one package-private member that would be inaccessible after inlining.
InlineMethodRefactoring_SourceAnalyzer_methoddeclaration_accesses_private=The method declaration references at least one private member that would be inaccessible after inlining.
InlineMethodRefactoring_SourceAnalyzer_methoddeclaration_accesses_protected=The method declaration references at least one protected member that would be inaccessible after inlining.
InlineMethodRefactoring_SourceAnalyzer_declaration_has_errors=The method declaration contains compile errors. To perform the operation you will need to fix the errors.
InlineMethodRefactoring_SourceAnalyzer_typedeclaration_has_errors=The type declaration contains compile errors. To perform the operation you will need to fix the errors.
InlineMethodRefactoring_SourceAnalyzer_methoddeclaration_has_errors=The method declaration contains compile errors. To perform the operation you will need to fix the errors.
Expand All @@ -218,6 +221,7 @@ CallInliner_constructors=Cannot inline a constructor invocation that is used as
CallInliner_cast_analysis_error=Cannot analyze call context to determine if implicit cast is needed.
CallInliner_create_sync_block_error=Cannot get type binding to create static class reference.
CallInliner_cannot_synchronize_error=Source method is synchronized and it is not possible to synchronize inlined content.
CallInliner_unexpected_model_exception=Unexpected Java model exception.

TargetProvider_inaccurate_match=Inaccurate references to method found. References will be ignored.
TargetProvider_method_declaration_not_unique=Cannot uniquely resolve method to be inlined.
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
package bugs_in;

class MyClass {
static int value= 10;

public static void staticMethod() {
System.out.println("Value: " + value);
}
}

public class Test_issue_1358_2 {
private int value= 20;

public void callStaticMethod() {
new MyClass()./*]*/staticMethod()/*[*/;
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
package bugs_in;

public class Test_issue_1358_3 {
class MyClass {
private int value= 10;

public void method() {
System.out.println("Value: " + value);
}
}
private int value= 20;

public void callMethod() {
new MyClass()./*]*/method()/*[*/;
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
package bugs_in;

class MyClass {
static int value= 10;

public static void staticMethod() {
System.out.println("Value: " + value);
}
}

public class Test_issue_1358_2 {
private int value= 20;

public void callStaticMethod() {
System.out.println("Value: " + MyClass.value);
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
package bugs_in;

public class Test_issue_1358_3 {
class MyClass {
private int value= 10;

public void method() {
System.out.println("Value: " + value);
}
}
private int value= 20;

public void callMethod() {
System.out.println("Value: " + new MyClass().value);
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
package invalid;

class MyClass {
private static int value= 10;

public static void staticMethod() {
System.out.println("Value: " + value);
}
}

class AnotherClass {
private int value= 20;

public void callStaticMethod() {
new MyClass()./*]*/staticMethod()/*[*/;
}
}

0 comments on commit 6333016

Please sign in to comment.