Do not publicize private members that are kept
Bug: 131130038, 155816423
Change-Id: Ica01d13e2c6905b2a6f6ad983b87d9c666d80cae
diff --git a/src/main/java/com/android/tools/r8/optimize/ClassAndMemberPublicizer.java b/src/main/java/com/android/tools/r8/optimize/ClassAndMemberPublicizer.java
index 8315925..5da8e5f 100644
--- a/src/main/java/com/android/tools/r8/optimize/ClassAndMemberPublicizer.java
+++ b/src/main/java/com/android/tools/r8/optimize/ClassAndMemberPublicizer.java
@@ -6,11 +6,13 @@
import static com.android.tools.r8.dex.Constants.ACC_PRIVATE;
import static com.android.tools.r8.dex.Constants.ACC_PROTECTED;
import static com.android.tools.r8.dex.Constants.ACC_PUBLIC;
+import static com.android.tools.r8.graph.DexProgramClass.asProgramClassOrNull;
import com.android.tools.r8.graph.AppView;
import com.android.tools.r8.graph.DexApplication;
import com.android.tools.r8.graph.DexClass;
import com.android.tools.r8.graph.DexEncodedMethod;
+import com.android.tools.r8.graph.DexProgramClass;
import com.android.tools.r8.graph.DexType;
import com.android.tools.r8.graph.GraphLense;
import com.android.tools.r8.graph.InnerClassAttribute;
@@ -82,43 +84,69 @@
}
private void publicizeType(DexType type) {
- DexClass clazz = application.definitionFor(type);
- if (clazz != null && clazz.isProgramClass()) {
- clazz.accessFlags.promoteToPublic();
-
- // Publicize fields.
- clazz.forEachField(field -> field.accessFlags.promoteToPublic());
-
- // Publicize methods.
- Set<DexEncodedMethod> privateInstanceEncodedMethods = new LinkedHashSet<>();
- clazz.forEachMethod(encodedMethod -> {
- if (publicizeMethod(clazz, encodedMethod)) {
- privateInstanceEncodedMethods.add(encodedMethod);
- }
- });
- if (!privateInstanceEncodedMethods.isEmpty()) {
- clazz.virtualizeMethods(privateInstanceEncodedMethods);
- }
-
- // Publicize inner class attribute.
- InnerClassAttribute attr = clazz.getInnerClassAttributeForThisClass();
- if (attr != null) {
- int accessFlags = ((attr.getAccess() | ACC_PUBLIC) & ~ACC_PRIVATE) & ~ACC_PROTECTED;
- clazz.replaceInnerClassAttributeForThisClass(
- new InnerClassAttribute(
- accessFlags, attr.getInner(), attr.getOuter(), attr.getInnerName()));
- }
+ DexProgramClass clazz = asProgramClassOrNull(application.definitionFor(type));
+ if (clazz != null) {
+ publicizeClass(clazz);
}
-
subtypingInfo.forAllImmediateExtendsSubtypes(type, this::publicizeType);
}
- private boolean publicizeMethod(DexClass holder, DexEncodedMethod encodedMethod) {
- MethodAccessFlags accessFlags = encodedMethod.accessFlags;
+ private void publicizeClass(DexProgramClass clazz) {
+ clazz.accessFlags.promoteToPublic();
+
+ // Publicize fields.
+ clazz.forEachField(
+ field -> {
+ if (field.isPublic()) {
+ return;
+ }
+ if (appView.appInfo().isPinned(field.field)) {
+ // TODO(b/131130038): Also do not publicize package-private and protected fields that
+ // are kept.
+ if (field.isPrivate()) {
+ return;
+ }
+ }
+ field.accessFlags.promoteToPublic();
+ });
+
+ // Publicize methods.
+ Set<DexEncodedMethod> privateInstanceMethods = new LinkedHashSet<>();
+ clazz.forEachMethod(
+ method -> {
+ if (publicizeMethod(clazz, method)) {
+ privateInstanceMethods.add(method);
+ }
+ });
+ if (!privateInstanceMethods.isEmpty()) {
+ clazz.virtualizeMethods(privateInstanceMethods);
+ }
+
+ // Publicize inner class attribute.
+ InnerClassAttribute attr = clazz.getInnerClassAttributeForThisClass();
+ if (attr != null) {
+ int accessFlags = ((attr.getAccess() | ACC_PUBLIC) & ~ACC_PRIVATE) & ~ACC_PROTECTED;
+ clazz.replaceInnerClassAttributeForThisClass(
+ new InnerClassAttribute(
+ accessFlags, attr.getInner(), attr.getOuter(), attr.getInnerName()));
+ }
+ }
+
+ private boolean publicizeMethod(DexProgramClass holder, DexEncodedMethod method) {
+ MethodAccessFlags accessFlags = method.accessFlags;
if (accessFlags.isPublic()) {
return false;
}
- if (!accessFlags.isPrivate() || appView.dexItemFactory().isConstructor(encodedMethod.method)) {
+ // If this method is mentioned in keep rules, do not transform (rule applications changed).
+ if (appView.appInfo().isPinned(method.method)) {
+ // TODO(b/131130038): Also do not publicize package-private and protected methods that are
+ // kept.
+ if (method.isPrivate()) {
+ return false;
+ }
+ }
+
+ if (!accessFlags.isPrivate() || appView.dexItemFactory().isConstructor(method.method)) {
// TODO(b/150589374): This should check for dispatch targets or just abandon in
// package-private.
accessFlags.promoteToPublic();
@@ -126,10 +154,6 @@
}
if (!accessFlags.isStatic()) {
- // If this method is mentioned in keep rules, do not transform (rule applications changed).
- if (appView.appInfo().isPinned(encodedMethod.method)) {
- return false;
- }
// We can't publicize private instance methods in interfaces or methods that are copied from
// interfaces to lambda-desugared classes because this will be added as a new default method.
@@ -138,20 +162,20 @@
return false;
}
- boolean wasSeen = methodPoolCollection.markIfNotSeen(holder, encodedMethod.method);
+ boolean wasSeen = methodPoolCollection.markIfNotSeen(holder, method.method);
if (wasSeen) {
// We can't do anything further because even renaming is not allowed due to the keep rule.
- if (appView.rootSet().mayNotBeMinified(encodedMethod.method, appView)) {
+ if (appView.rootSet().mayNotBeMinified(method.method, appView)) {
return false;
}
// TODO(b/111118390): Renaming will enable more private instance methods to be publicized.
return false;
}
- lenseBuilder.add(encodedMethod.method);
+ lenseBuilder.add(method.method);
accessFlags.promoteToFinal();
accessFlags.promoteToPublic();
// The method just became public and is therefore not a library override.
- encodedMethod.setLibraryMethodOverride(OptionalBool.FALSE);
+ method.setLibraryMethodOverride(OptionalBool.FALSE);
return true;
}
diff --git a/src/test/java/com/android/tools/r8/accessrelaxation/AccessRelaxationProguardCompatTest.java b/src/test/java/com/android/tools/r8/accessrelaxation/AccessRelaxationProguardCompatTest.java
index 7ea2765..3efb9b1 100644
--- a/src/test/java/com/android/tools/r8/accessrelaxation/AccessRelaxationProguardCompatTest.java
+++ b/src/test/java/com/android/tools/r8/accessrelaxation/AccessRelaxationProguardCompatTest.java
@@ -15,10 +15,7 @@
import com.android.tools.r8.utils.codeinspector.FieldSubject;
import org.junit.Test;
-/**
- * Tests that both R8 and Proguard may change the visibility of a field or method that is explicitly
- * kept.
- */
+/** Tests that Proguard may change the visibility of a field or method that is explicitly kept. */
public class AccessRelaxationProguardCompatTest extends TestBase {
private static Class<?> clazz = AccessRelaxationProguardCompatTestClass.class;
@@ -35,7 +32,7 @@
"}")
.allowAccessModification()
.compile()
- .inspect(AccessRelaxationProguardCompatTest::inspect);
+ .inspect(inspector -> inspect(inspector, true));
}
@Test
@@ -49,10 +46,10 @@
"}")
.allowAccessModification()
.compile()
- .inspect(AccessRelaxationProguardCompatTest::inspect);
+ .inspect(inspector -> inspect(inspector, false));
}
- private static void inspect(CodeInspector inspector) {
+ private static void inspect(CodeInspector inspector, boolean isR8) {
ClassSubject classSubject = inspector.clazz(clazzWithGetter);
assertThat(classSubject, isPresent());
@@ -60,7 +57,11 @@
assertThat(fieldSubject, isPresent());
// Although this field was explicitly kept, it is no longer private.
- assertThat(fieldSubject, not(isPrivate()));
+ if (isR8) {
+ assertThat(fieldSubject, isPrivate());
+ } else {
+ assertThat(fieldSubject, not(isPrivate()));
+ }
}
}
diff --git a/src/test/java/com/android/tools/r8/accessrelaxation/PrivateKeptMembersPublicizerTest.java b/src/test/java/com/android/tools/r8/accessrelaxation/PrivateKeptMembersPublicizerTest.java
new file mode 100644
index 0000000..940479a
--- /dev/null
+++ b/src/test/java/com/android/tools/r8/accessrelaxation/PrivateKeptMembersPublicizerTest.java
@@ -0,0 +1,67 @@
+// Copyright (c) 2020, the R8 project authors. Please see the AUTHORS file
+// for details. All rights reserved. Use of this source code is governed by a
+// BSD-style license that can be found in the LICENSE file.
+
+package com.android.tools.r8.accessrelaxation;
+
+import static com.android.tools.r8.utils.codeinspector.Matchers.isPresent;
+import static org.hamcrest.MatcherAssert.assertThat;
+import static org.junit.Assert.assertTrue;
+
+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.ClassSubject;
+import com.android.tools.r8.utils.codeinspector.CodeInspector;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.junit.runners.Parameterized;
+import org.junit.runners.Parameterized.Parameters;
+
+@RunWith(Parameterized.class)
+public class PrivateKeptMembersPublicizerTest extends TestBase {
+
+ private final TestParameters parameters;
+
+ @Parameters(name = "{0}")
+ public static TestParametersCollection data() {
+ return getTestParameters().withAllRuntimesAndApiLevels().build();
+ }
+
+ public PrivateKeptMembersPublicizerTest(TestParameters parameters) {
+ this.parameters = parameters;
+ }
+
+ @Test
+ public void test() throws Exception {
+ testForR8(parameters.getBackend())
+ .addInnerClasses(PrivateKeptMembersPublicizerTest.class)
+ .addKeepClassAndMembersRules(TestClass.class)
+ .allowAccessModification()
+ .setMinApi(parameters.getApiLevel())
+ .compile()
+ .inspect(this::inspect)
+ .run(parameters.getRuntime(), TestClass.class)
+ .assertSuccessWithOutputLines("Hello world!");
+ }
+
+ private void inspect(CodeInspector inspector) {
+ ClassSubject classSubject = inspector.clazz(TestClass.class);
+ assertThat(classSubject, isPresent());
+ assertTrue(classSubject.uniqueFieldWithName("greeting").isPrivate());
+ assertTrue(classSubject.uniqueMethodWithName("greet").isPrivate());
+ }
+
+ static class TestClass {
+
+ private static String greeting = "Hello world!";
+
+ public static void main(String[] args) {
+ greet(greeting);
+ }
+
+ private static void greet(String message) {
+ System.out.println(message);
+ }
+ }
+}
