Revert "Fix inadequate detection of package private accesses" This reverts commit 65be8bcf65bac299cf8d84d600c65119ff0a2258. Reason for revert: Failures on internal bot Change-Id: I08e1f0e7fa5dead0a4a273600002f3a69a9b2110
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 68700e4..8760fb8 100644 --- a/src/main/java/com/android/tools/r8/shaking/VerticalClassMerger.java +++ b/src/main/java/com/android/tools/r8/shaking/VerticalClassMerger.java
@@ -24,7 +24,6 @@ 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; @@ -2107,8 +2106,8 @@ } } - // Searches for a reference to a non-private, non-public class, field or method declared in the - // same package as [source]. + // Searches for a reference to a 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; @@ -2119,143 +2118,106 @@ this.appView = appView; } - private boolean checkFoundPackagePrivateAccess() { - assert getResult(); - return true; - } + 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; + } - private boolean setFoundPackagePrivateAccess() { - setResult(true); - return true; - } - - private static boolean continueSearchForPackagePrivateAccess() { + DexEncodedField definition = appView.appInfo().resolveField(field).getResolvedField(); + if (definition == null || !definition.accessFlags.isPublic()) { + setResult(true); + return true; + } + } return false; } - private boolean checkFieldReference(DexField field) { - return internalCheckFieldReference(field, appView.graphLens()); - } - - private boolean checkRewrittenFieldReference(DexField field) { - return internalCheckFieldReference(field, GraphLens.getIdentityLens()); - } - - private boolean internalCheckFieldReference(DexField field, GraphLens graphLens) { - DexField rewrittenField = graphLens.lookupField(field); - assert rewrittenField.getHolderType().isClassType(); - DexType rewrittenFieldHolder = rewrittenField.getHolderType(); - if (rewrittenFieldHolder.isSamePackage(getContext().getHolderType())) { - if (checkRewrittenTypeReference(rewrittenFieldHolder)) { - return checkFoundPackagePrivateAccess(); - } - DexClassAndField resolvedField = - appView.appInfo().resolveField(rewrittenField).getResolutionPair(); - if (resolvedField == null) { - return setFoundPackagePrivateAccess(); - } - if (resolvedField.getHolder() != getContext().getHolder() - && !resolvedField.getAccessFlags().isPublic()) { - return setFoundPackagePrivateAccess(); - } - } - return continueSearchForPackagePrivateAccess(); - } - - private boolean checkRewrittenMethodReference( - DexMethod rewrittenMethod, OptionalBool isInterface) { - DexType baseType = rewrittenMethod.getHolderType().toBaseType(appView.dexItemFactory()); + 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(rewrittenMethod.getHolderType())) { - return checkFoundPackagePrivateAccess(); + if (checkTypeReference(method.holder) + || checkTypeReference(method.proto.returnType) + || Iterables.any(method.getParameters(), this::checkTypeReference)) { + return true; } + 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(); + ? appView.appInfo().unsafeResolveMethodDueToDexFormatLegacy(method) + : appView.appInfo().resolveMethodLegacy(method, isInterface.isTrue()); + if (!resolutionResult.isSingleResolution() + || !resolutionResult.asSingleResolution().getResolvedMethod().isPublic()) { + setResult(true); + return true; } } - return continueSearchForPackagePrivateAccess(); + return false; } private boolean checkTypeReference(DexType type) { - 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())); + DexType baseType = appView.graphLens().lookupType(type.toBaseType(appView.dexItemFactory())); if (baseType.isClassType() && baseType.isSamePackage(getContext().getHolderType())) { DexClass clazz = appView.definitionFor(baseType); - if (clazz == null || !clazz.isPublic()) { - return setFoundPackagePrivateAccess(); + if (clazz == null || !clazz.accessFlags.isPublic()) { + setResult(true); + return true; } } - return continueSearchForPackagePrivateAccess(); + return false; } @Override public void registerInitClass(DexType 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); - } + checkTypeReference(clazz); } @Override public void registerInvokeVirtual(DexMethod method) { - MethodLookupResult lookup = appView.graphLens().lookupInvokeVirtual(method, getContext()); - checkRewrittenMethodReference(lookup.getReference(), OptionalBool.FALSE); + MethodLookupResult lookup = + appView.graphLens().lookupMethod(method, getContext().getReference(), VIRTUAL); + checkMethodReference(lookup.getReference(), OptionalBool.FALSE); } @Override public void registerInvokeDirect(DexMethod method) { - MethodLookupResult lookup = appView.graphLens().lookupInvokeDirect(method, getContext()); - checkRewrittenMethodReference(lookup.getReference(), OptionalBool.UNKNOWN); + MethodLookupResult lookup = + appView.graphLens().lookupMethod(method, getContext().getReference(), DIRECT); + checkMethodReference(lookup.getReference(), OptionalBool.UNKNOWN); } @Override public void registerInvokeStatic(DexMethod method) { - MethodLookupResult lookup = appView.graphLens().lookupInvokeStatic(method, getContext()); - checkRewrittenMethodReference(lookup.getReference(), OptionalBool.UNKNOWN); + MethodLookupResult lookup = + appView.graphLens().lookupMethod(method, getContext().getReference(), Type.STATIC); + checkMethodReference(lookup.getReference(), OptionalBool.UNKNOWN); } @Override public void registerInvokeInterface(DexMethod method) { - MethodLookupResult lookup = appView.graphLens().lookupInvokeInterface(method, getContext()); - checkRewrittenMethodReference(lookup.getReference(), OptionalBool.TRUE); + MethodLookupResult lookup = + appView.graphLens().lookupMethod(method, getContext().getReference(), Type.INTERFACE); + checkMethodReference(lookup.getReference(), OptionalBool.TRUE); } @Override public void registerInvokeSuper(DexMethod method) { - MethodLookupResult lookup = appView.graphLens().lookupInvokeSuper(method, getContext()); - checkRewrittenMethodReference(lookup.getReference(), OptionalBool.UNKNOWN); + MethodLookupResult lookup = + appView.graphLens().lookupMethod(method, getContext().getReference(), Type.SUPER); + checkMethodReference(lookup.getReference(), OptionalBool.UNKNOWN); } @Override public void registerInstanceFieldWrite(DexField field) { - checkFieldReference(field); + checkFieldReference(appView.graphLens().lookupField(field)); } @Override public void registerInstanceFieldRead(DexField field) { - checkFieldReference(field); + checkFieldReference(appView.graphLens().lookupField(field)); } @Override @@ -2265,12 +2227,12 @@ @Override public void registerStaticFieldRead(DexField field) { - checkFieldReference(field); + checkFieldReference(appView.graphLens().lookupField(field)); } @Override public void registerStaticFieldWrite(DexField field) { - checkFieldReference(field); + checkFieldReference(appView.graphLens().lookupField(field)); } @Override
diff --git a/src/test/java/com/android/tools/r8/classmerging/horizontal/InitClassToPackagePrivateFieldWithCrossPackageMergingTest.java b/src/test/java/com/android/tools/r8/classmerging/horizontal/InitClassToPackagePrivateFieldWithCrossPackageMergingTest.java index af9f880..ed2130d 100644 --- a/src/test/java/com/android/tools/r8/classmerging/horizontal/InitClassToPackagePrivateFieldWithCrossPackageMergingTest.java +++ b/src/test/java/com/android/tools/r8/classmerging/horizontal/InitClassToPackagePrivateFieldWithCrossPackageMergingTest.java
@@ -9,7 +9,7 @@ import com.android.tools.r8.TestBase; import com.android.tools.r8.TestParameters; import com.android.tools.r8.TestParametersCollection; -import com.android.tools.r8.utils.codeinspector.HorizontallyMergedClassesInspector; +import com.android.tools.r8.references.Reference; import com.google.common.collect.ImmutableList; import java.util.List; import org.junit.Test; @@ -47,13 +47,16 @@ .addProgramClassFileData(getProgramClassFileData()) .addKeepMainRule(Main.class) .addHorizontallyMergedClassesInspector( - HorizontallyMergedClassesInspector::assertNoClassesMerged) + inspector -> + inspector.assertMergedInto( + Reference.classFromClass(A.class), + Reference.classFromDescriptor(NEW_B_DESCRIPTOR))) .enableInliningAnnotations() .enableNoHorizontalClassMergingAnnotations() .setMinApi(parameters.getApiLevel()) .compile() .run(parameters.getRuntime(), Main.class) - .assertSuccessWithOutputLines("Hello, world!"); + .assertFailureWithErrorThatThrows(IllegalAccessError.class); } private List<byte[]> getProgramClassFileData() throws Exception {
diff --git a/src/test/java/com/android/tools/r8/internal/YouTubeV1719Test.java b/src/test/java/com/android/tools/r8/internal/YouTubeV1719Test.java index c78212b..dbeb66b 100644 --- a/src/test/java/com/android/tools/r8/internal/YouTubeV1719Test.java +++ b/src/test/java/com/android/tools/r8/internal/YouTubeV1719Test.java
@@ -40,7 +40,6 @@ import java.nio.file.Paths; import java.util.concurrent.ExecutionException; import org.junit.After; -import org.junit.Ignore; import org.junit.Test; import org.junit.runner.RunWith; import org.junit.runners.Parameterized; @@ -192,7 +191,6 @@ * Validates that when all protos are kept and the proto optmization is enabled, the generated * proto schemas are identical to the proto schemas in the input. */ - @Ignore @Test public void testProtoRewriting() throws Exception { assumeTrue(shouldRunSlowTests());
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 483f18b..907ca27 100644 --- a/src/test/java/com/android/tools/r8/kotlin/R8KotlinAccessorTest.java +++ b/src/test/java/com/android/tools/r8/kotlin/R8KotlinAccessorTest.java
@@ -19,7 +19,6 @@ 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; @@ -220,9 +219,7 @@ runTest(PROPERTIES_PACKAGE_NAME, mainClass) .inspect( inspector -> { - if (allowAccessModification - || (testParameters.isDexRuntime() - && testParameters.getApiLevel().isGreaterThan(AndroidApiLevel.B))) { + if (allowAccessModification) { 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 527e568..59ba99b 100644 --- a/src/test/java/com/android/tools/r8/kotlin/R8KotlinPropertiesTest.java +++ b/src/test/java/com/android/tools/r8/kotlin/R8KotlinPropertiesTest.java
@@ -11,7 +11,6 @@ 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; @@ -510,9 +509,7 @@ inspector -> { checkClassIsRemoved(inspector, testedClass.getClassName()); - if (allowAccessModification - || (testParameters.isDexRuntime() - && testParameters.getApiLevel().isGreaterThan(AndroidApiLevel.B))) { + if (allowAccessModification) { checkClassIsRemoved(inspector, testedClass.getOuterClassName()); return; }