Reland "Fix inadequate detection of package private accesses"
This reverts commit b9028365137393aa8632b29dbabcbec522ebad8f.
Bug: b/243955191
Change-Id: If19f6bb899ced9d0dcb0598c06b3d7814542c0b1
diff --git a/src/main/java/com/android/tools/r8/horizontalclassmerging/PolicyScheduler.java b/src/main/java/com/android/tools/r8/horizontalclassmerging/PolicyScheduler.java
index 2ff1bfb..42f4731 100644
--- a/src/main/java/com/android/tools/r8/horizontalclassmerging/PolicyScheduler.java
+++ b/src/main/java/com/android/tools/r8/horizontalclassmerging/PolicyScheduler.java
@@ -279,7 +279,7 @@
new SameNestHost(appView),
new SameParentClass(),
new SyntheticItemsPolicy(appView, mode),
- new RespectPackageBoundaries(appView),
+ new RespectPackageBoundaries(appView, mode),
new NoDifferentApiReferenceLevel(appView),
new NoIndirectRuntimeTypeChecks(appView, runtimeTypeCheckInfo),
new NoWeakerAccessPrivileges(appView, immediateSubtypingInfo),
diff --git a/src/main/java/com/android/tools/r8/horizontalclassmerging/policies/RespectPackageBoundaries.java b/src/main/java/com/android/tools/r8/horizontalclassmerging/policies/RespectPackageBoundaries.java
index e3986c8..ded7e1d 100644
--- a/src/main/java/com/android/tools/r8/horizontalclassmerging/policies/RespectPackageBoundaries.java
+++ b/src/main/java/com/android/tools/r8/horizontalclassmerging/policies/RespectPackageBoundaries.java
@@ -7,11 +7,13 @@
import com.android.tools.r8.graph.AppInfoWithClassHierarchy;
import com.android.tools.r8.graph.AppView;
import com.android.tools.r8.graph.DexClass;
+import com.android.tools.r8.graph.DexClassAndField;
import com.android.tools.r8.graph.DexDefinition;
import com.android.tools.r8.graph.DexEncodedField;
import com.android.tools.r8.graph.DexEncodedMember;
import com.android.tools.r8.graph.DexProgramClass;
import com.android.tools.r8.graph.DexType;
+import com.android.tools.r8.horizontalclassmerging.HorizontalClassMerger.Mode;
import com.android.tools.r8.horizontalclassmerging.MergeGroup;
import com.android.tools.r8.horizontalclassmerging.MultiClassPolicy;
import com.android.tools.r8.shaking.VerticalClassMerger.IllegalAccessDetector;
@@ -25,9 +27,11 @@
public class RespectPackageBoundaries extends MultiClassPolicy {
private final AppView<? extends AppInfoWithClassHierarchy> appView;
+ private final Mode mode;
- public RespectPackageBoundaries(AppView<? extends AppInfoWithClassHierarchy> appView) {
+ public RespectPackageBoundaries(AppView<? extends AppInfoWithClassHierarchy> appView, Mode mode) {
this.appView = appView;
+ this.mode = mode;
}
boolean shouldRestrictMergingAcrossPackageBoundary(DexProgramClass clazz) {
@@ -83,7 +87,29 @@
method -> {
boolean foundIllegalAccess =
method.registerCodeReferencesWithResult(
- new IllegalAccessDetector(appView, method));
+ new IllegalAccessDetector(appView, method) {
+
+ @Override
+ protected boolean checkRewrittenFieldType(DexClassAndField field) {
+ if (mode.isInitial()) {
+ // If the type of the field is package private, we need to keep the
+ // current class in its package in case we end up synthesizing a
+ // check-cast for the field type after relaxing the type of the field
+ // after instance field merging.
+ DexType fieldBaseType = field.getType().toBaseType(dexItemFactory());
+ if (fieldBaseType.isClassType()) {
+ DexClass fieldBaseClass = appView.definitionFor(fieldBaseType);
+ if (fieldBaseClass == null || !fieldBaseClass.isPublic()) {
+ return setFoundPackagePrivateAccess();
+ }
+ }
+ } else {
+ // No relaxing of field types, hence no insertion of casts where we need
+ // to guarantee visibility.
+ }
+ return continueSearchForPackagePrivateAccess();
+ }
+ });
if (foundIllegalAccess) {
return TraversalContinuation.doBreak();
}
diff --git a/src/main/java/com/android/tools/r8/shaking/VerticalClassMerger.java b/src/main/java/com/android/tools/r8/shaking/VerticalClassMerger.java
index 95ba96a..a875269 100644
--- a/src/main/java/com/android/tools/r8/shaking/VerticalClassMerger.java
+++ b/src/main/java/com/android/tools/r8/shaking/VerticalClassMerger.java
@@ -23,6 +23,7 @@
import com.android.tools.r8.graph.DefaultInstanceInitializerCode;
import com.android.tools.r8.graph.DexApplication;
import com.android.tools.r8.graph.DexClass;
+import com.android.tools.r8.graph.DexClassAndField;
import com.android.tools.r8.graph.DexClassAndMethod;
import com.android.tools.r8.graph.DexEncodedField;
import com.android.tools.r8.graph.DexEncodedMember;
@@ -2106,8 +2107,8 @@
}
}
- // Searches for a reference to a non-public class, field or method declared in the same package
- // as [source].
+ // Searches for a reference to a non-private, non-public class, field or method declared in the
+ // same package as [source].
public static class IllegalAccessDetector extends UseRegistryWithResult<Boolean, ProgramMethod> {
private final AppView<? extends AppInfoWithClassHierarchy> appView;
@@ -2118,106 +2119,144 @@
this.appView = appView;
}
- private boolean checkFieldReference(DexField field) {
- DexType baseType =
- appView.graphLens().lookupType(field.holder.toBaseType(appView.dexItemFactory()));
- if (baseType.isClassType() && baseType.isSamePackage(getContext().getHolderType())) {
- if (checkTypeReference(field.holder) || checkTypeReference(field.type)) {
- return true;
- }
+ protected boolean checkFoundPackagePrivateAccess() {
+ assert getResult();
+ return true;
+ }
- DexEncodedField definition = appView.appInfo().resolveField(field).getResolvedField();
- if (definition == null || !definition.accessFlags.isPublic()) {
- setResult(true);
- return true;
- }
- }
+ protected boolean setFoundPackagePrivateAccess() {
+ setResult(true);
+ return true;
+ }
+
+ protected static boolean continueSearchForPackagePrivateAccess() {
return false;
}
- private boolean checkMethodReference(DexMethod method, OptionalBool isInterface) {
- DexType baseType =
- appView.graphLens().lookupType(method.holder.toBaseType(appView.dexItemFactory()));
- if (baseType.isClassType() && baseType.isSamePackage(getContext().getHolderType())) {
- if (checkTypeReference(method.holder)
- || checkTypeReference(method.proto.returnType)
- || Iterables.any(method.getParameters(), this::checkTypeReference)) {
- return true;
- }
+ private boolean checkFieldReference(DexField field) {
+ return checkRewrittenFieldReference(appView.graphLens().lookupField(field));
+ }
- MethodResolutionResult resolutionResult =
- isInterface.isUnknown()
- ? appView.appInfo().unsafeResolveMethodDueToDexFormat(method)
- : appView.appInfo().resolveMethod(method, isInterface.isTrue());
- if (!resolutionResult.isSingleResolution()
- || !resolutionResult.asSingleResolution().getResolvedMethod().isPublic()) {
- setResult(true);
- return true;
+ private boolean checkRewrittenFieldReference(DexField field) {
+ assert field.getHolderType().isClassType();
+ DexType fieldHolder = field.getHolderType();
+ if (fieldHolder.isSamePackage(getContext().getHolderType())) {
+ if (checkRewrittenTypeReference(fieldHolder)) {
+ return checkFoundPackagePrivateAccess();
+ }
+ DexClassAndField resolvedField = appView.appInfo().resolveField(field).getResolutionPair();
+ if (resolvedField == null) {
+ return setFoundPackagePrivateAccess();
+ }
+ if (resolvedField.getHolder() != getContext().getHolder()
+ && !resolvedField.getAccessFlags().isPublic()) {
+ return setFoundPackagePrivateAccess();
+ }
+ if (checkRewrittenFieldType(resolvedField)) {
+ return checkFoundPackagePrivateAccess();
}
}
- return false;
+ return continueSearchForPackagePrivateAccess();
+ }
+
+ protected boolean checkRewrittenFieldType(DexClassAndField field) {
+ return continueSearchForPackagePrivateAccess();
+ }
+
+ private boolean checkRewrittenMethodReference(
+ DexMethod rewrittenMethod, OptionalBool isInterface) {
+ DexType baseType = rewrittenMethod.getHolderType().toBaseType(appView.dexItemFactory());
+ if (baseType.isClassType() && baseType.isSamePackage(getContext().getHolderType())) {
+ if (checkTypeReference(rewrittenMethod.getHolderType())) {
+ return checkFoundPackagePrivateAccess();
+ }
+ MethodResolutionResult resolutionResult =
+ isInterface.isUnknown()
+ ? appView.appInfo().unsafeResolveMethodDueToDexFormat(rewrittenMethod)
+ : appView.appInfo().resolveMethod(rewrittenMethod, isInterface.isTrue());
+ if (!resolutionResult.isSingleResolution()) {
+ return setFoundPackagePrivateAccess();
+ }
+ DexClassAndMethod resolvedMethod =
+ resolutionResult.asSingleResolution().getResolutionPair();
+ if (resolvedMethod.getHolder() != getContext().getHolder()
+ && !resolvedMethod.getAccessFlags().isPublic()) {
+ return setFoundPackagePrivateAccess();
+ }
+ }
+ return continueSearchForPackagePrivateAccess();
}
private boolean checkTypeReference(DexType type) {
- DexType baseType = appView.graphLens().lookupType(type.toBaseType(appView.dexItemFactory()));
+ return internalCheckTypeReference(type, appView.graphLens());
+ }
+
+ private boolean checkRewrittenTypeReference(DexType type) {
+ return internalCheckTypeReference(type, GraphLens.getIdentityLens());
+ }
+
+ private boolean internalCheckTypeReference(DexType type, GraphLens graphLens) {
+ DexType baseType = graphLens.lookupType(type.toBaseType(appView.dexItemFactory()));
if (baseType.isClassType() && baseType.isSamePackage(getContext().getHolderType())) {
DexClass clazz = appView.definitionFor(baseType);
- if (clazz == null || !clazz.accessFlags.isPublic()) {
- setResult(true);
- return true;
+ if (clazz == null || !clazz.isPublic()) {
+ return setFoundPackagePrivateAccess();
}
}
- return false;
+ return continueSearchForPackagePrivateAccess();
}
@Override
public void registerInitClass(DexType clazz) {
- checkTypeReference(clazz);
+ if (appView.initClassLens().isFinal()) {
+ // The InitClass lens is always rewritten up until the most recent graph lens, so first map
+ // the class type to the most recent graph lens.
+ DexType rewrittenType = appView.graphLens().lookupType(clazz);
+ DexField initClassField = appView.initClassLens().getInitClassField(rewrittenType);
+ checkRewrittenFieldReference(initClassField);
+ } else {
+ checkTypeReference(clazz);
+ }
}
@Override
public void registerInvokeVirtual(DexMethod method) {
- MethodLookupResult lookup =
- appView.graphLens().lookupMethod(method, getContext().getReference(), VIRTUAL);
- checkMethodReference(lookup.getReference(), OptionalBool.FALSE);
+ MethodLookupResult lookup = appView.graphLens().lookupInvokeVirtual(method, getContext());
+ checkRewrittenMethodReference(lookup.getReference(), OptionalBool.FALSE);
}
@Override
public void registerInvokeDirect(DexMethod method) {
- MethodLookupResult lookup =
- appView.graphLens().lookupMethod(method, getContext().getReference(), DIRECT);
- checkMethodReference(lookup.getReference(), OptionalBool.UNKNOWN);
+ MethodLookupResult lookup = appView.graphLens().lookupInvokeDirect(method, getContext());
+ checkRewrittenMethodReference(lookup.getReference(), OptionalBool.UNKNOWN);
}
@Override
public void registerInvokeStatic(DexMethod method) {
- MethodLookupResult lookup =
- appView.graphLens().lookupMethod(method, getContext().getReference(), Type.STATIC);
- checkMethodReference(lookup.getReference(), OptionalBool.UNKNOWN);
+ MethodLookupResult lookup = appView.graphLens().lookupInvokeStatic(method, getContext());
+ checkRewrittenMethodReference(lookup.getReference(), OptionalBool.UNKNOWN);
}
@Override
public void registerInvokeInterface(DexMethod method) {
- MethodLookupResult lookup =
- appView.graphLens().lookupMethod(method, getContext().getReference(), Type.INTERFACE);
- checkMethodReference(lookup.getReference(), OptionalBool.TRUE);
+ MethodLookupResult lookup = appView.graphLens().lookupInvokeInterface(method, getContext());
+ checkRewrittenMethodReference(lookup.getReference(), OptionalBool.TRUE);
}
@Override
public void registerInvokeSuper(DexMethod method) {
- MethodLookupResult lookup =
- appView.graphLens().lookupMethod(method, getContext().getReference(), Type.SUPER);
- checkMethodReference(lookup.getReference(), OptionalBool.UNKNOWN);
+ MethodLookupResult lookup = appView.graphLens().lookupInvokeSuper(method, getContext());
+ checkRewrittenMethodReference(lookup.getReference(), OptionalBool.UNKNOWN);
}
@Override
public void registerInstanceFieldWrite(DexField field) {
- checkFieldReference(appView.graphLens().lookupField(field));
+ checkFieldReference(field);
}
@Override
public void registerInstanceFieldRead(DexField field) {
- checkFieldReference(appView.graphLens().lookupField(field));
+ checkFieldReference(field);
}
@Override
@@ -2227,12 +2266,12 @@
@Override
public void registerStaticFieldRead(DexField field) {
- checkFieldReference(appView.graphLens().lookupField(field));
+ checkFieldReference(field);
}
@Override
public void registerStaticFieldWrite(DexField field) {
- checkFieldReference(appView.graphLens().lookupField(field));
+ checkFieldReference(field);
}
@Override
diff --git a/src/test/java/com/android/tools/r8/kotlin/R8KotlinAccessorTest.java b/src/test/java/com/android/tools/r8/kotlin/R8KotlinAccessorTest.java
index 907ca27..483f18b 100644
--- a/src/test/java/com/android/tools/r8/kotlin/R8KotlinAccessorTest.java
+++ b/src/test/java/com/android/tools/r8/kotlin/R8KotlinAccessorTest.java
@@ -19,6 +19,7 @@
import com.android.tools.r8.kotlin.TestKotlinClass.AccessorKind;
import com.android.tools.r8.kotlin.TestKotlinClass.Visibility;
import com.android.tools.r8.naming.MemberNaming;
+import com.android.tools.r8.utils.AndroidApiLevel;
import com.android.tools.r8.utils.AndroidApp;
import com.android.tools.r8.utils.BooleanUtils;
import com.android.tools.r8.utils.codeinspector.ClassSubject;
@@ -219,7 +220,9 @@
runTest(PROPERTIES_PACKAGE_NAME, mainClass)
.inspect(
inspector -> {
- if (allowAccessModification) {
+ if (allowAccessModification
+ || (testParameters.isDexRuntime()
+ && testParameters.getApiLevel().isGreaterThan(AndroidApiLevel.B))) {
checkClassIsRemoved(inspector, testedClass.getOuterClassName());
return;
}
diff --git a/src/test/java/com/android/tools/r8/kotlin/R8KotlinPropertiesTest.java b/src/test/java/com/android/tools/r8/kotlin/R8KotlinPropertiesTest.java
index 59ba99b..527e568 100644
--- a/src/test/java/com/android/tools/r8/kotlin/R8KotlinPropertiesTest.java
+++ b/src/test/java/com/android/tools/r8/kotlin/R8KotlinPropertiesTest.java
@@ -11,6 +11,7 @@
import com.android.tools.r8.kotlin.TestKotlinClass.Visibility;
import com.android.tools.r8.naming.MemberNaming;
import com.android.tools.r8.naming.MemberNaming.MethodSignature;
+import com.android.tools.r8.utils.AndroidApiLevel;
import com.android.tools.r8.utils.BooleanUtils;
import com.android.tools.r8.utils.InternalOptions;
import com.android.tools.r8.utils.codeinspector.ClassSubject;
@@ -509,7 +510,9 @@
inspector -> {
checkClassIsRemoved(inspector, testedClass.getClassName());
- if (allowAccessModification) {
+ if (allowAccessModification
+ || (testParameters.isDexRuntime()
+ && testParameters.getApiLevel().isGreaterThan(AndroidApiLevel.B))) {
checkClassIsRemoved(inspector, testedClass.getOuterClassName());
return;
}
