Disallow abstract methods on non-abstract classes
Bug: 158018192
Change-Id: Idbf83805290a2599f757e194ada419291c104002
diff --git a/src/main/java/com/android/tools/r8/graph/DexClass.java b/src/main/java/com/android/tools/r8/graph/DexClass.java
index c201290..09aeb11 100644
--- a/src/main/java/com/android/tools/r8/graph/DexClass.java
+++ b/src/main/java/com/android/tools/r8/graph/DexClass.java
@@ -176,12 +176,10 @@
private boolean verifyNoAbstractMethodsOnNonAbstractClasses(
Iterable<DexEncodedMethod> methods, InternalOptions options) {
- if (options.canHaveDalvikAbstractMethodOnNonAbstractClassVerificationBug()) {
- if (!isAbstract()) {
- for (DexEncodedMethod method : methods) {
- assert !method.isAbstract()
- : "Non-abstract method on abstract class: `" + method.method.toSourceString() + "`";
- }
+ if (options.canHaveDalvikAbstractMethodOnNonAbstractClassVerificationBug() && !isAbstract()) {
+ for (DexEncodedMethod method : methods) {
+ assert !method.isAbstract()
+ : "Non-abstract method on abstract class: `" + method.method.toSourceString() + "`";
}
}
return true;
diff --git a/src/main/java/com/android/tools/r8/shaking/TreePruner.java b/src/main/java/com/android/tools/r8/shaking/TreePruner.java
index 74a79ce..ec2ced1 100644
--- a/src/main/java/com/android/tools/r8/shaking/TreePruner.java
+++ b/src/main/java/com/android/tools/r8/shaking/TreePruner.java
@@ -300,8 +300,7 @@
// Final classes cannot be abstract, so we have to keep the method in that case.
// Also some other kinds of methods cannot be abstract, so keep them around.
boolean allowAbstract =
- (!options.canHaveDalvikAbstractMethodOnNonAbstractClassVerificationBug()
- || clazz.isAbstract())
+ (options.canUseAbstractMethodOnNonAbstractClass() || clazz.isAbstract())
&& !method.isFinal()
&& !method.accessFlags.isNative()
&& !method.accessFlags.isStrict()
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 d7ba750..00a2136 100644
--- a/src/main/java/com/android/tools/r8/utils/InternalOptions.java
+++ b/src/main/java/com/android/tools/r8/utils/InternalOptions.java
@@ -1246,6 +1246,22 @@
return minApiLevel >= level.getLevel();
}
+ /**
+ * Dex2Oat issues a warning for abstract methods on non-abstract classes, so we never allow this.
+ *
+ * <p>Note that having an invoke instruction that targets an abstract method on a non-abstract
+ * class will fail with a verification error on Dalvik. Therefore, this must not be more
+ * permissive than {@code return minApiLevel >= AndroidApiLevel.L.getLevel()}.
+ *
+ * <p>See b/132953944.
+ */
+ @SuppressWarnings("ConstantConditions")
+ public boolean canUseAbstractMethodOnNonAbstractClass() {
+ boolean result = false;
+ assert !(result && canHaveDalvikAbstractMethodOnNonAbstractClassVerificationBug());
+ return result;
+ }
+
public boolean canUseConstClassInstructions(int cfVersion) {
assert isGeneratingClassFiles();
return cfVersion >= requiredCfVersionForConstClassInstructions();
diff --git a/src/test/java/com/android/tools/r8/R8TestCompileResult.java b/src/test/java/com/android/tools/r8/R8TestCompileResult.java
index db3e876..5dac954 100644
--- a/src/test/java/com/android/tools/r8/R8TestCompileResult.java
+++ b/src/test/java/com/android/tools/r8/R8TestCompileResult.java
@@ -68,16 +68,7 @@
@Override
public CodeInspector inspector() throws IOException {
- return new CodeInspector(
- app,
- proguardMap,
- options -> {
- // TODO(b/158069726): The dex parser should not transform abstract methods on non-abstract
- // classes into empty throwing methods.
- if (minApiLevel >= 0) {
- options.minApiLevel = minApiLevel;
- }
- });
+ return new CodeInspector(app, proguardMap);
}
public GraphInspector graphInspector() throws IOException {
diff --git a/src/test/java/com/android/tools/r8/shaking/AbstractMethodOnNonAbstractClassTest.java b/src/test/java/com/android/tools/r8/shaking/AbstractMethodOnNonAbstractClassTest.java
index d9970f3..2f95543 100644
--- a/src/test/java/com/android/tools/r8/shaking/AbstractMethodOnNonAbstractClassTest.java
+++ b/src/test/java/com/android/tools/r8/shaking/AbstractMethodOnNonAbstractClassTest.java
@@ -11,14 +11,10 @@
import static org.junit.Assert.assertFalse;
import static org.junit.Assert.assertTrue;
-import com.android.tools.r8.Dex2OatTestRunResult;
import com.android.tools.r8.R8TestCompileResult;
import com.android.tools.r8.TestBase;
import com.android.tools.r8.TestParameters;
import com.android.tools.r8.TestParametersCollection;
-import com.android.tools.r8.ToolHelper.DexVm;
-import com.android.tools.r8.ToolHelper.DexVm.Version;
-import com.android.tools.r8.utils.AndroidApiLevel;
import com.android.tools.r8.utils.codeinspector.ClassSubject;
import com.android.tools.r8.utils.codeinspector.MethodSubject;
import org.junit.Test;
@@ -56,29 +52,15 @@
assertThat(classSubject, isPresent());
assertFalse(classSubject.isAbstract());
+ // A.m() is also not made abstract in compat mode.
MethodSubject methodSubject = classSubject.uniqueMethodWithName("m");
assertThat(methodSubject, isPresent());
+ assertFalse(methodSubject.isAbstract());
if (parameters.isDexRuntime()) {
- Dex2OatTestRunResult dex2OatResult = compileResult.runDex2Oat(parameters.getRuntime());
- if (parameters.getApiLevel().isLessThan(AndroidApiLevel.L)) {
- // Dalvik does not allow abstract methods on non-abstract classes, so R8 emits an empty
- // throwing method instead.
- assertFalse(methodSubject.isAbstract());
- dex2OatResult.assertStderrMatches(not(containsString(DEX2OAT_WARNING)));
- } else {
- // Verify that the method has become abstract.
- assertTrue(methodSubject.isAbstract());
-
- DexVm.Version version = parameters.getRuntime().asDex().getVm().getVersion();
- if (version.equals(Version.V5_1_1) || version.equals(Version.V6_0_1)) {
- // Art 5.1.1 and 6.0.1 did not report a warning for having an abstract method on a
- // non-abstract class.
- dex2OatResult.assertStderrMatches(not(containsString(DEX2OAT_WARNING)));
- } else {
- dex2OatResult.assertStderrMatches(containsString(DEX2OAT_WARNING));
- }
- }
+ compileResult
+ .runDex2Oat(parameters.getRuntime())
+ .assertStderrMatches(not(containsString(DEX2OAT_WARNING)));
}
compileResult
diff --git a/src/test/java/com/android/tools/r8/utils/codeinspector/CodeInspector.java b/src/test/java/com/android/tools/r8/utils/codeinspector/CodeInspector.java
index 1156e55..b2edae2 100644
--- a/src/test/java/com/android/tools/r8/utils/codeinspector/CodeInspector.java
+++ b/src/test/java/com/android/tools/r8/utils/codeinspector/CodeInspector.java
@@ -136,14 +136,8 @@
}
public CodeInspector(AndroidApp app, String proguardMapContent) throws IOException {
- this(app, proguardMapContent, null);
- }
-
- public CodeInspector(
- AndroidApp app, String proguardMapContent, Consumer<InternalOptions> optionsConsumer)
- throws IOException {
this(
- new ApplicationReader(app, runOptionsConsumer(optionsConsumer), Timing.empty())
+ new ApplicationReader(app, runOptionsConsumer(null), Timing.empty())
.read(StringResource.fromString(proguardMapContent, Origin.unknown())));
}
