Only publicize package-private methods if it is not overridden
Bug: 150589374
Bug: 172254047
Change-Id: I91ee861c2ded5c6748eb7c800b5674ef1c575c71
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 55c58c6cd..0922f50 100644
--- a/src/main/java/com/android/tools/r8/optimize/ClassAndMemberPublicizer.java
+++ b/src/main/java/com/android/tools/r8/optimize/ClassAndMemberPublicizer.java
@@ -13,6 +13,7 @@
import com.android.tools.r8.graph.DexClass;
import com.android.tools.r8.graph.DexEncodedField;
import com.android.tools.r8.graph.DexEncodedMethod;
+import com.android.tools.r8.graph.DexMethod;
import com.android.tools.r8.graph.DexProgramClass;
import com.android.tools.r8.graph.DexType;
import com.android.tools.r8.graph.GraphLens;
@@ -21,12 +22,15 @@
import com.android.tools.r8.graph.ProgramDefinition;
import com.android.tools.r8.graph.ProgramMethod;
import com.android.tools.r8.graph.SubtypingInfo;
+import com.android.tools.r8.ir.optimize.MemberPoolCollection.MemberPool;
import com.android.tools.r8.ir.optimize.MethodPoolCollection;
import com.android.tools.r8.optimize.PublicizerLens.PublicizedLensBuilder;
import com.android.tools.r8.shaking.AppInfoWithLiveness;
import com.android.tools.r8.shaking.KeepInfoCollection;
+import com.android.tools.r8.utils.MethodSignatureEquivalence;
import com.android.tools.r8.utils.OptionalBool;
import com.android.tools.r8.utils.Timing;
+import com.google.common.base.Equivalence.Wrapper;
import java.util.LinkedHashSet;
import java.util.Set;
import java.util.concurrent.ExecutionException;
@@ -159,13 +163,28 @@
}
}
- if (!accessFlags.isPrivate() || appView.dexItemFactory().isConstructor(method.getReference())) {
- // TODO(b/150589374): This should check for dispatch targets or just abandon in
- // package-private.
+ if (method.getDefinition().isInstanceInitializer() || accessFlags.isProtected()) {
doPublicize(method);
return false;
}
+ if (accessFlags.isPackagePrivate()) {
+ // If we publicize a package private method we have to ensure there is no overrides of it. We
+ // could potentially publicize a method if it only has package-private overrides, but for know
+ // we just check if it is seen below.
+ // Note that we will not publize private methods if there exists a package-private override,
+ // and there is therefore no need to check the hierarchy above.
+ MemberPool<DexMethod> memberPool = methodPoolCollection.get(method.getHolder());
+ Wrapper<DexMethod> methodKey = MethodSignatureEquivalence.get().wrap(method.getReference());
+ if (memberPool.hasSeenStrictlyBelow(methodKey)) {
+ return false;
+ }
+ doPublicize(method);
+ return false;
+ }
+
+ assert accessFlags.isPrivate();
+
if (accessFlags.isStatic()) {
// For private static methods we can just relax the access to public, since
// even though JLS prevents from declaring static method in derived class if
diff --git a/src/test/java/com/android/tools/r8/accessrelaxation/PackagePrivateNonOverridePublicizerTest.java b/src/test/java/com/android/tools/r8/accessrelaxation/PackagePrivateNonOverridePublicizerTest.java
new file mode 100644
index 0000000..b4cc748
--- /dev/null
+++ b/src/test/java/com/android/tools/r8/accessrelaxation/PackagePrivateNonOverridePublicizerTest.java
@@ -0,0 +1,107 @@
+// 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.CoreMatchers.containsString;
+import static org.hamcrest.CoreMatchers.not;
+import static org.hamcrest.MatcherAssert.assertThat;
+
+import com.android.tools.r8.NeverClassInline;
+import com.android.tools.r8.NeverInline;
+import com.android.tools.r8.NoVerticalClassMerging;
+import com.android.tools.r8.TestBase;
+import com.android.tools.r8.TestParameters;
+import com.android.tools.r8.TestRunResult;
+import com.android.tools.r8.TestShrinkerBuilder;
+import com.android.tools.r8.resolution.virtualtargets.package_a.ViewModel;
+import com.android.tools.r8.utils.BooleanUtils;
+import com.android.tools.r8.utils.codeinspector.ClassSubject;
+import java.util.List;
+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 PackagePrivateNonOverridePublicizerTest extends TestBase {
+
+ private final TestParameters parameters;
+ private final boolean allowAccessModification;
+ private final String[] EXPECTED =
+ new String[] {"SubViewModel.clearNotOverriding()", "ViewModel.clear()"};
+
+ @Parameters(name = "{0}, allowAccessModification: {1}")
+ public static List<Object[]> data() {
+ return buildParameters(
+ getTestParameters().withAllRuntimesAndApiLevels().build(), BooleanUtils.values());
+ }
+
+ public PackagePrivateNonOverridePublicizerTest(
+ TestParameters parameters, boolean allowAccessModification) {
+ this.parameters = parameters;
+ this.allowAccessModification = allowAccessModification;
+ }
+
+ @Test
+ public void testRuntime() throws Exception {
+ testForRuntime(parameters)
+ .addProgramClasses(ViewModel.class, SubViewModel.class, Main.class)
+ .run(parameters.getRuntime(), Main.class)
+ .apply(this::assertSuccessOutput);
+ }
+
+ @Test
+ public void testR8() throws Exception {
+ testForR8(parameters.getBackend())
+ .addProgramClasses(ViewModel.class, SubViewModel.class, Main.class)
+ .addKeepMainRule(Main.class)
+ .setMinApi(parameters.getApiLevel())
+ .enableInliningAnnotations()
+ .enableNeverClassInliningAnnotations()
+ .enableNoVerticalClassMergingAnnotations()
+ .applyIf(allowAccessModification, TestShrinkerBuilder::allowAccessModification)
+ .run(parameters.getRuntime(), Main.class)
+ .apply(this::assertSuccessOutput)
+ .inspect(
+ inspector -> {
+ ClassSubject clazz = inspector.clazz(ViewModel.class);
+ // ViewModel.clear() is package private. When we publicize the method, we can inline
+ // the clearBridge() into Main and thereby remove the ViewModel class entirely.
+ if (allowAccessModification) {
+ assertThat(clazz, not(isPresent()));
+ } else {
+ assertThat(clazz, isPresent());
+ }
+ });
+ }
+
+ private void assertSuccessOutput(TestRunResult<?> result) {
+ if (parameters.isDexRuntime() && parameters.getDexRuntimeVersion().isDalvik()) {
+ result.assertFailureWithErrorThatMatches(containsString("overrides final"));
+ } else {
+ result.assertSuccessWithOutputLines(EXPECTED);
+ }
+ }
+
+ @NeverClassInline
+ @NoVerticalClassMerging
+ public static class SubViewModel extends ViewModel {
+
+ @NeverInline
+ public void clearNotOverriding() {
+ System.out.println("SubViewModel.clearNotOverriding()");
+ }
+ }
+
+ public static class Main {
+
+ public static void main(String[] args) {
+ SubViewModel subViewModel = new SubViewModel();
+ subViewModel.clearNotOverriding();
+ subViewModel.clearBridge();
+ }
+ }
+}
diff --git a/src/test/java/com/android/tools/r8/accessrelaxation/PackagePrivateOverridePublicizerTest.java b/src/test/java/com/android/tools/r8/accessrelaxation/PackagePrivateOverridePublicizerTest.java
index 1d41225..ccd0b21 100644
--- a/src/test/java/com/android/tools/r8/accessrelaxation/PackagePrivateOverridePublicizerTest.java
+++ b/src/test/java/com/android/tools/r8/accessrelaxation/PackagePrivateOverridePublicizerTest.java
@@ -23,7 +23,6 @@
private final TestParameters parameters;
private final String[] EXPECTED = new String[] {"SubViewModel.clear()", "ViewModel.clear()"};
- private final String[] R8_OUT = new String[] {"SubViewModel.clear()", "SubViewModel.clear()"};
@Parameters(name = "{0}")
public static TestParametersCollection data() {
@@ -52,7 +51,7 @@
.enableNeverClassInliningAnnotations()
.allowAccessModification()
.run(parameters.getRuntime(), Main.class)
- .assertSuccessWithOutputLines(R8_OUT);
+ .apply(this::assertSuccessOutput);
}
private void assertSuccessOutput(TestRunResult<?> result) {
