Do not mark initializers with @AssumeMayHaveSideEffects as trivial
Bug: 142772856
Change-Id: I640118222c200b71b7d0b611779bdd5d1118b9bb
diff --git a/src/main/java/com/android/tools/r8/ir/conversion/IRConverter.java b/src/main/java/com/android/tools/r8/ir/conversion/IRConverter.java
index b7f7a4d..4935103 100644
--- a/src/main/java/com/android/tools/r8/ir/conversion/IRConverter.java
+++ b/src/main/java/com/android/tools/r8/ir/conversion/IRConverter.java
@@ -205,7 +205,6 @@
.map(prefix -> "L" + DescriptorUtils.getPackageBinaryNameFromJavaType(prefix))
.map(options.itemFactory::createString)
.collect(Collectors.toList());
- this.methodOptimizationInfoCollector = new MethodOptimizationInfoCollector(appView);
if (options.isDesugaredLibraryCompilation()) {
// Specific L8 Settings.
// BackportedMethodRewriter is needed for retarget core library members and backports.
@@ -241,6 +240,7 @@
this.stringSwitchRemover = null;
this.desugaredLibraryAPIConverter = null;
this.serviceLoaderRewriter = null;
+ this.methodOptimizationInfoCollector = null;
return;
}
this.lambdaRewriter = options.enableDesugaring ? new LambdaRewriter(appView, this) : null;
@@ -295,6 +295,8 @@
this.outliner = new Outliner(appViewWithLiveness, this);
this.memberValuePropagation =
options.enableValuePropagation ? new MemberValuePropagation(appViewWithLiveness) : null;
+ this.methodOptimizationInfoCollector =
+ new MethodOptimizationInfoCollector(appViewWithLiveness);
if (options.isMinifying()) {
this.identifierNameStringMarker = new IdentifierNameStringMarker(appViewWithLiveness);
} else {
@@ -330,6 +332,7 @@
this.d8NestBasedAccessDesugaring =
options.shouldDesugarNests() ? new D8NestBasedAccessDesugaring(appView) : null;
this.serviceLoaderRewriter = null;
+ this.methodOptimizationInfoCollector = null;
}
this.stringSwitchRemover =
options.isStringSwitchConversionEnabled()
diff --git a/src/main/java/com/android/tools/r8/ir/optimize/info/MethodOptimizationInfoCollector.java b/src/main/java/com/android/tools/r8/ir/optimize/info/MethodOptimizationInfoCollector.java
index e23fa01..8cbc644 100644
--- a/src/main/java/com/android/tools/r8/ir/optimize/info/MethodOptimizationInfoCollector.java
+++ b/src/main/java/com/android/tools/r8/ir/optimize/info/MethodOptimizationInfoCollector.java
@@ -61,11 +61,11 @@
import java.util.function.Function;
public class MethodOptimizationInfoCollector {
- private final AppView<?> appView;
+ private final AppView<AppInfoWithLiveness> appView;
private final InternalOptions options;
private final DexItemFactory dexItemFactory;
- public MethodOptimizationInfoCollector(AppView<?> appView) {
+ public MethodOptimizationInfoCollector(AppView<AppInfoWithLiveness> appView) {
this.appView = appView;
this.options = appView.options();
this.dexItemFactory = appView.dexItemFactory();
@@ -264,7 +264,9 @@
private void identifyTrivialInitializer(
DexEncodedMethod method, IRCode code, OptimizationFeedback feedback) {
- if (!method.isInstanceInitializer() && !method.isClassInitializer()) {
+ assert !appView.appInfo().isPinned(method.method);
+
+ if (!method.isInitializer()) {
return;
}
@@ -272,8 +274,13 @@
return;
}
+ if (appView.appInfo().mayHaveSideEffects.containsKey(method.method)) {
+ return;
+ }
+
DexClass clazz = appView.appInfo().definitionFor(method.method.holder);
if (clazz == null) {
+ assert false;
return;
}
diff --git a/src/test/java/com/android/tools/r8/shaking/librarymethodoverride/LibraryMethodOverrideTest.java b/src/test/java/com/android/tools/r8/shaking/librarymethodoverride/LibraryMethodOverrideTest.java
index ca354f9..a5fbc59 100644
--- a/src/test/java/com/android/tools/r8/shaking/librarymethodoverride/LibraryMethodOverrideTest.java
+++ b/src/test/java/com/android/tools/r8/shaking/librarymethodoverride/LibraryMethodOverrideTest.java
@@ -8,7 +8,6 @@
import static org.hamcrest.CoreMatchers.not;
import static org.hamcrest.MatcherAssert.assertThat;
-import com.android.tools.r8.AssumeMayHaveSideEffects;
import com.android.tools.r8.NeverClassInline;
import com.android.tools.r8.NeverMerge;
import com.android.tools.r8.TestBase;
@@ -29,7 +28,7 @@
@Parameterized.Parameters(name = "{0}")
public static TestParametersCollection data() {
- return getTestParameters().withAllRuntimes().build();
+ return getTestParameters().withAllRuntimesAndApiLevels().build();
}
public LibraryMethodOverrideTest(TestParameters parameters) {
@@ -44,8 +43,7 @@
.addOptionsModification(options -> options.enableTreeShakingOfLibraryMethodOverrides = true)
.enableClassInliningAnnotations()
.enableMergeAnnotations()
- .enableSideEffectAnnotations()
- .setMinApi(parameters.getRuntime())
+ .setMinApi(parameters.getApiLevel())
.compile()
.inspect(this::verifyOutput)
.run(parameters.getRuntime(), TestClass.class)
@@ -68,7 +66,16 @@
for (Class<?> nonEscapingClass : nonEscapingClasses) {
ClassSubject classSubject = inspector.clazz(nonEscapingClass);
assertThat(classSubject, isPresent());
- assertThat(classSubject.uniqueMethodWithName("toString"), not(isPresent()));
+
+ // TODO(b/142772856): None of the non-escaping classes should have a toString() method. It is
+ // a requirement that the instance initializers are considered trivial for this to work,
+ // though, even when they have a side effect (as long as the receiver does not escape via the
+ // side effecting instruction).
+ if (nonEscapingClass == DoesNotEscapeWithSubThatDoesNotOverrideSub.class) {
+ assertThat(classSubject.uniqueMethodWithName("toString"), not(isPresent()));
+ } else {
+ assertThat(classSubject.uniqueMethodWithName("toString"), isPresent());
+ }
}
}
@@ -128,8 +135,12 @@
@NeverClassInline
static class DoesNotEscape {
- @AssumeMayHaveSideEffects
- DoesNotEscape() {}
+ // TODO(b/142772856): Should be classified as a trivial instance initializer although it has a
+ // side effect.
+ DoesNotEscape() {
+ // Side effect to ensure that the constructor is not removed from main().
+ System.out.print("");
+ }
@Override
public String toString() {
@@ -140,8 +151,12 @@
@NeverClassInline
static class DoesNotEscapeWithSubThatDoesNotOverride {
- @AssumeMayHaveSideEffects
- DoesNotEscapeWithSubThatDoesNotOverride() {}
+ // TODO(b/142772856): Should be classified as a trivial instance initializer although it has a
+ // side effect.
+ DoesNotEscapeWithSubThatDoesNotOverride() {
+ // Side effect to ensure that the constructor is not removed from main().
+ System.out.print("");
+ }
@Override
public String toString() {
@@ -153,15 +168,23 @@
static class DoesNotEscapeWithSubThatDoesNotOverrideSub
extends DoesNotEscapeWithSubThatDoesNotOverride {
- @AssumeMayHaveSideEffects
- DoesNotEscapeWithSubThatDoesNotOverrideSub() {}
+ // TODO(b/142772856): Should be classified as a trivial instance initializer although it has a
+ // side effect.
+ DoesNotEscapeWithSubThatDoesNotOverrideSub() {
+ // Side effect to ensure that the constructor is not removed from main().
+ System.out.print("");
+ }
}
@NeverClassInline
static class DoesNotEscapeWithSubThatOverrides {
- @AssumeMayHaveSideEffects
- DoesNotEscapeWithSubThatOverrides() {}
+ // TODO(b/142772856): Should be classified as a trivial instance initializer although it has a
+ // side effect.
+ DoesNotEscapeWithSubThatOverrides() {
+ // Side effect to ensure that the constructor is not removed from main().
+ System.out.print("");
+ }
@Override
public String toString() {
@@ -172,8 +195,12 @@
@NeverClassInline
static class DoesNotEscapeWithSubThatOverridesSub extends DoesNotEscapeWithSubThatOverrides {
- @AssumeMayHaveSideEffects
- DoesNotEscapeWithSubThatOverridesSub() {}
+ // TODO(b/142772856): Should be classified as a trivial instance initializer although it has a
+ // side effect.
+ DoesNotEscapeWithSubThatOverridesSub() {
+ // Side effect to ensure that the constructor is not removed from main().
+ System.out.print("");
+ }
@Override
public String toString() {
@@ -188,8 +215,12 @@
@NeverClassInline
static class DoesNotEscapeWithSubThatOverridesAndEscapes {
- @AssumeMayHaveSideEffects
- DoesNotEscapeWithSubThatOverridesAndEscapes() {}
+ // TODO(b/142772856): Should be classified as a trivial instance initializer although it has a
+ // side effect.
+ DoesNotEscapeWithSubThatOverridesAndEscapes() {
+ // Side effect to ensure that the constructor is not removed from main().
+ System.out.print("");
+ }
@Override
public String toString() {
@@ -201,8 +232,12 @@
static class DoesNotEscapeWithSubThatOverridesAndEscapesSub
extends DoesNotEscapeWithSubThatOverridesAndEscapes {
- @AssumeMayHaveSideEffects
- DoesNotEscapeWithSubThatOverridesAndEscapesSub() {}
+ // TODO(b/142772856): Should be classified as a trivial instance initializer although it has a
+ // side effect.
+ DoesNotEscapeWithSubThatOverridesAndEscapesSub() {
+ // Side effect to ensure that the constructor is not removed from main().
+ System.out.print("");
+ }
@Override
public String toString() {
