Only unbox enums where instance field puts are inside enum constructors
This is a somewhat coarse-grained fix to ensure that we do not try to unbox enums that has field put references outside their constructors. A better fix would be to unbox instance-put on enums if possible.
Bug: b/247146910
Bug: b/249752942
Change-Id: If2f870944573e64ec980cf48137fd9420d733635
diff --git a/src/main/java/com/android/tools/r8/graph/AppView.java b/src/main/java/com/android/tools/r8/graph/AppView.java
index 2412ecc..b4926fe 100644
--- a/src/main/java/com/android/tools/r8/graph/AppView.java
+++ b/src/main/java/com/android/tools/r8/graph/AppView.java
@@ -690,9 +690,6 @@
}
public boolean validateUnboxedEnumsHaveBeenPruned() {
- if (app().options.testing.allowNotPrunedUnboxedEnums) {
- return true;
- }
for (DexType unboxedEnum : unboxedEnums.getUnboxedEnums()) {
assert appInfo.definitionForWithoutExistenceAssert(unboxedEnum) == null
: "Enum " + unboxedEnum + " has been unboxed but is still in the program.";
diff --git a/src/main/java/com/android/tools/r8/ir/optimize/enums/EnumUnboxerImpl.java b/src/main/java/com/android/tools/r8/ir/optimize/enums/EnumUnboxerImpl.java
index b1ea459..7f0fd6f 100644
--- a/src/main/java/com/android/tools/r8/ir/optimize/enums/EnumUnboxerImpl.java
+++ b/src/main/java/com/android/tools/r8/ir/optimize/enums/EnumUnboxerImpl.java
@@ -1155,12 +1155,17 @@
if (field == null) {
return Reason.INVALID_FIELD_PUT;
}
- DexProgramClass dexClass = appView.programDefinitionFor(field.getHolderType(), code.context());
- if (dexClass == null) {
+ DexProgramClass holderClass =
+ appView.programDefinitionFor(field.getHolderType(), code.context());
+ if (holderClass == null) {
return Reason.INVALID_FIELD_PUT;
}
if (fieldPut.isInstancePut() && fieldPut.asInstancePut().object() == enumValue) {
- return Reason.ELIGIBLE;
+ // TODO(b/249752942): The requirement to be inside an initializer of the enum can be relaxed
+ // if we support puts.
+ return context.getHolder() == enumClass && context.getDefinition().isInstanceInitializer()
+ ? Reason.ELIGIBLE
+ : Reason.ASSIGNMENT_OUTSIDE_INIT;
}
// The put value has to be of the field type.
if (field.getReference().type.toBaseType(factory) != enumClass.type) {
diff --git a/src/main/java/com/android/tools/r8/ir/optimize/enums/eligibility/Reason.java b/src/main/java/com/android/tools/r8/ir/optimize/enums/eligibility/Reason.java
index 63fc7fd..3e17a0a 100644
--- a/src/main/java/com/android/tools/r8/ir/optimize/enums/eligibility/Reason.java
+++ b/src/main/java/com/android/tools/r8/ir/optimize/enums/eligibility/Reason.java
@@ -34,6 +34,7 @@
public static final Reason INVALID_ARRAY_PUT = new StringReason("INVALID_ARRAY_PUT");
public static final Reason TYPE_MISMATCH_FIELD_PUT = new StringReason("TYPE_MISMATCH_FIELD_PUT");
public static final Reason INVALID_IF_TYPES = new StringReason("INVALID_IF_TYPES");
+ public static final Reason ASSIGNMENT_OUTSIDE_INIT = new StringReason("ASSIGNMENT_OUTSIDE_INIT");
public static final Reason ENUM_METHOD_CALLED_WITH_NULL_RECEIVER =
new StringReason("ENUM_METHOD_CALLED_WITH_NULL_RECEIVER");
public static final Reason OTHER_UNSUPPORTED_INSTRUCTION =
diff --git a/src/main/java/com/android/tools/r8/shaking/Enqueuer.java b/src/main/java/com/android/tools/r8/shaking/Enqueuer.java
index 5823c33..0c40434 100644
--- a/src/main/java/com/android/tools/r8/shaking/Enqueuer.java
+++ b/src/main/java/com/android/tools/r8/shaking/Enqueuer.java
@@ -2060,8 +2060,7 @@
|| appView.appInfo().getMainDexInfo().isTracedRoot(clazz, appView.getSyntheticItems())
: "Class " + clazz.toSourceString() + " was not a main dex root in the first round";
- assert appView.options().testing.allowNotPrunedUnboxedEnums
- || !appView.unboxedEnums().isUnboxedEnum(clazz)
+ assert !appView.unboxedEnums().isUnboxedEnum(clazz)
: "Enum " + clazz.toSourceString() + " has been unboxed but is still in the program.";
if (options.isGeneratingClassFiles() && clazz.hasPermittedSubclassAttributes()) {
diff --git a/src/main/java/com/android/tools/r8/utils/InternalOptions.java b/src/main/java/com/android/tools/r8/utils/InternalOptions.java
index f7e258e..2269e25 100644
--- a/src/main/java/com/android/tools/r8/utils/InternalOptions.java
+++ b/src/main/java/com/android/tools/r8/utils/InternalOptions.java
@@ -1980,7 +1980,6 @@
System.getProperty("com.android.tools.r8.disableMarkingClassesFinal") != null;
public boolean testEnableTestAssertions = false;
public boolean keepMetadataInR8IfNotRewritten = true;
- public boolean allowNotPrunedUnboxedEnums = false;
// If set, pruned record fields are not used in hashCode/equals/toString and toString prints
// minified field names instead of original field names.
diff --git a/src/test/java/com/android/tools/r8/enumunboxing/EnumInEnumFieldTest.java b/src/test/java/com/android/tools/r8/enumunboxing/EnumInEnumFieldTest.java
index 632b24a..66e7ee5 100644
--- a/src/test/java/com/android/tools/r8/enumunboxing/EnumInEnumFieldTest.java
+++ b/src/test/java/com/android/tools/r8/enumunboxing/EnumInEnumFieldTest.java
@@ -4,13 +4,7 @@
package com.android.tools.r8.enumunboxing;
-import static org.hamcrest.CoreMatchers.containsString;
-import static org.junit.Assert.assertThrows;
-
-import com.android.tools.r8.CompilationFailedException;
-import com.android.tools.r8.DiagnosticsMatcher;
import com.android.tools.r8.NeverClassInline;
-import com.android.tools.r8.R8FullTestBuilder;
import com.android.tools.r8.TestParameters;
import java.util.List;
import org.junit.Test;
@@ -48,36 +42,16 @@
@Test
public void testEnumUnboxing() throws Exception {
- // TODO(b/247146910): Should not throw compilation error.
- assertThrows(CompilationFailedException.class, setupTest()::compile);
- }
-
- @Test
- public void testEnumUnboxingAllowingTypeErrors() throws Exception {
- // TODO(b/247146910): Should not insert throw null.
- setupTest()
- .addOptionsModification(options -> options.testing.allowTypeErrors = true)
- .allowDiagnosticWarningMessages()
- .compileWithExpectedDiagnostics(
- diagnostics ->
- diagnostics.assertAllWarningsMatch(
- DiagnosticsMatcher.diagnosticMessage(
- containsString(
- "does not type check and will be assumed to be unreachable."))))
- .run(parameters.getRuntime(), Main.class)
- .assertFailureWithErrorThatThrows(NullPointerException.class);
- }
-
- private R8FullTestBuilder setupTest() throws Exception {
- return testForR8(parameters.getBackend())
+ testForR8(parameters.getBackend())
.addInnerClasses(EnumInEnumFieldTest.class)
.addKeepMainRule(Main.class)
.addKeepRules(enumKeepRules.getKeepRules())
.addOptionsModification(opt -> enableEnumOptions(opt, enumValueOptimization))
- .addEnumUnboxingInspector(
- inspector -> inspector.assertUnboxed(MyEnum.class, OtherEnum.class))
+ .addEnumUnboxingInspector(inspector -> inspector.assertNotUnboxed(OtherEnum.class))
.addNeverClassInliningAnnotations()
- .setMinApi(parameters.getApiLevel());
+ .setMinApi(parameters.getApiLevel())
+ .run(parameters.getRuntime(), Main.class)
+ .assertSuccessWithOutputLines("0");
}
@NeverClassInline
diff --git a/src/test/java/com/android/tools/r8/enumunboxing/EnumInstanceFieldReferenceInsideAndOutsideTest.java b/src/test/java/com/android/tools/r8/enumunboxing/EnumInstanceFieldReferenceInsideAndOutsideTest.java
index 8be968b..55cdede 100644
--- a/src/test/java/com/android/tools/r8/enumunboxing/EnumInstanceFieldReferenceInsideAndOutsideTest.java
+++ b/src/test/java/com/android/tools/r8/enumunboxing/EnumInstanceFieldReferenceInsideAndOutsideTest.java
@@ -4,13 +4,8 @@
package com.android.tools.r8.enumunboxing;
-import static org.hamcrest.CoreMatchers.containsString;
-import static org.junit.Assert.assertThrows;
-
-import com.android.tools.r8.CompilationFailedException;
import com.android.tools.r8.NeverClassInline;
import com.android.tools.r8.TestParameters;
-import com.android.tools.r8.ToolHelper.DexVm.Version;
import java.util.List;
import org.junit.Test;
import org.junit.runner.RunWith;
@@ -49,54 +44,17 @@
}
@Test
- public void testEnumUnboxing() throws Exception {
- // TODO(b/247146910): Should not throw.
- assertThrows(
- CompilationFailedException.class,
- () ->
- testForR8(parameters.getBackend())
- .addInnerClasses(EnumInstanceFieldReferenceInsideAndOutsideTest.class)
- .addKeepMainRule(Main.class)
- .addKeepRules(enumKeepRules.getKeepRules())
- .addOptionsModification(opt -> enableEnumOptions(opt, enumValueOptimization))
- .addEnumUnboxingInspector(inspector -> inspector.assertUnboxed(MyEnum.class))
- .addNeverClassInliningAnnotations()
- .setMinApi(parameters.getApiLevel())
- .addOptionsModification(options -> options.testing.allowTypeErrors = true)
- .allowDiagnosticWarningMessages()
- .compileWithExpectedDiagnostics(
- diagnostics ->
- diagnostics.assertErrorMessageThatMatches(
- containsString(
- "Enum "
- + typeName(MyEnum.class)
- + " has been unboxed but is still in the program"))));
- }
-
- private boolean hasVerifyError() {
- return parameters.getDexRuntimeVersion().isDalvik()
- || parameters.getDexRuntimeVersion().isNewerThanOrEqual(Version.V12_0_0);
- }
-
- @Test
public void testEnumUnboxingAllowNotPrunedEnums() throws Exception {
testForR8(parameters.getBackend())
.addInnerClasses(EnumInstanceFieldReferenceInsideAndOutsideTest.class)
.addKeepMainRule(Main.class)
.addKeepRules(enumKeepRules.getKeepRules())
- .addOptionsModification(
- opt -> {
- enableEnumOptions(opt, enumValueOptimization);
- opt.testing.allowNotPrunedUnboxedEnums = true;
- })
- .addEnumUnboxingInspector(inspector -> inspector.assertUnboxed(MyEnum.class))
+ .addOptionsModification(opt -> enableEnumOptions(opt, enumValueOptimization))
.addNeverClassInliningAnnotations()
.setMinApi(parameters.getApiLevel())
- .addOptionsModification(options -> options.testing.allowTypeErrors = true)
+ .addEnumUnboxingInspector(inspector -> inspector.assertNotUnboxed(MyEnum.class))
.run(parameters.getRuntime(), Main.class)
- // TODO(b/247146910): Should not fail with runtime error.
- .assertFailureWithErrorThatThrowsIf(hasVerifyError(), VerifyError.class)
- .assertFailureWithErrorThatThrowsIf(!hasVerifyError(), NoSuchFieldError.class);
+ .assertSuccessWithOutputLines("42");
}
@NeverClassInline
diff --git a/src/test/java/com/android/tools/r8/enumunboxing/EnumInstanceFieldReferenceOutsideTest.java b/src/test/java/com/android/tools/r8/enumunboxing/EnumInstanceFieldReferenceOutsideTest.java
index ff0b642..4dac1af 100644
--- a/src/test/java/com/android/tools/r8/enumunboxing/EnumInstanceFieldReferenceOutsideTest.java
+++ b/src/test/java/com/android/tools/r8/enumunboxing/EnumInstanceFieldReferenceOutsideTest.java
@@ -4,13 +4,8 @@
package com.android.tools.r8.enumunboxing;
-import static org.hamcrest.CoreMatchers.containsString;
-import static org.junit.Assert.assertThrows;
-
-import com.android.tools.r8.CompilationFailedException;
import com.android.tools.r8.NeverClassInline;
import com.android.tools.r8.TestParameters;
-import com.android.tools.r8.ToolHelper.DexVm.Version;
import java.util.List;
import org.junit.Test;
import org.junit.runner.RunWith;
@@ -49,54 +44,17 @@
}
@Test
- public void testEnumUnboxing() throws Exception {
- // TODO(b/247146910): Should not throw.
- assertThrows(
- CompilationFailedException.class,
- () ->
- testForR8(parameters.getBackend())
- .addInnerClasses(EnumInstanceFieldReferenceOutsideTest.class)
- .addKeepMainRule(Main.class)
- .addKeepRules(enumKeepRules.getKeepRules())
- .addOptionsModification(opt -> enableEnumOptions(opt, enumValueOptimization))
- .addEnumUnboxingInspector(inspector -> inspector.assertUnboxed(MyEnum.class))
- .addNeverClassInliningAnnotations()
- .setMinApi(parameters.getApiLevel())
- .addOptionsModification(options -> options.testing.allowTypeErrors = true)
- .allowDiagnosticWarningMessages()
- .compileWithExpectedDiagnostics(
- diagnostics ->
- diagnostics.assertErrorMessageThatMatches(
- containsString(
- "Enum "
- + typeName(MyEnum.class)
- + " has been unboxed but is still in the program"))));
- }
-
- private boolean hasVerifyError() {
- return parameters.getDexRuntimeVersion().isDalvik()
- || parameters.getDexRuntimeVersion().isNewerThanOrEqual(Version.V12_0_0);
- }
-
- @Test
public void testEnumUnboxingAllowNotPrunedEnums() throws Exception {
testForR8(parameters.getBackend())
.addInnerClasses(EnumInstanceFieldReferenceOutsideTest.class)
.addKeepMainRule(Main.class)
.addKeepRules(enumKeepRules.getKeepRules())
- .addOptionsModification(
- opt -> {
- enableEnumOptions(opt, enumValueOptimization);
- opt.testing.allowNotPrunedUnboxedEnums = true;
- })
- .addEnumUnboxingInspector(inspector -> inspector.assertUnboxed(MyEnum.class))
+ .addEnumUnboxingInspector(inspector -> inspector.assertNotUnboxed(MyEnum.class))
+ .addOptionsModification(opt -> enableEnumOptions(opt, enumValueOptimization))
.addNeverClassInliningAnnotations()
.setMinApi(parameters.getApiLevel())
- .addOptionsModification(options -> options.testing.allowTypeErrors = true)
.run(parameters.getRuntime(), Main.class)
- // TODO(b/247146910): Should not fail with runtime error.
- .assertFailureWithErrorThatThrowsIf(hasVerifyError(), VerifyError.class)
- .assertFailureWithErrorThatThrowsIf(!hasVerifyError(), NoSuchFieldError.class);
+ .assertSuccessWithOutputLines("42");
}
@NeverClassInline
