Reproduce abstract method on non-abstract class warnings
Bug: 158018192
Change-Id: Idef706e9f2ea79b621813747eeb24e1f4892cf17
diff --git a/src/test/java/com/android/tools/r8/R8TestBuilder.java b/src/test/java/com/android/tools/r8/R8TestBuilder.java
index 1a86ba0..9d0684d 100644
--- a/src/test/java/com/android/tools/r8/R8TestBuilder.java
+++ b/src/test/java/com/android/tools/r8/R8TestBuilder.java
@@ -136,7 +136,8 @@
box.proguardConfiguration,
box.syntheticProguardRules,
proguardMapBuilder.toString(),
- graphConsumer);
+ graphConsumer,
+ builder.getMinApiLevel());
switch (allowedDiagnosticMessages) {
case ALL:
compileResult.assertDiagnosticThatMatches(new IsAnything<>());
diff --git a/src/test/java/com/android/tools/r8/R8TestCompileResult.java b/src/test/java/com/android/tools/r8/R8TestCompileResult.java
index 3a16807..db3e876 100644
--- a/src/test/java/com/android/tools/r8/R8TestCompileResult.java
+++ b/src/test/java/com/android/tools/r8/R8TestCompileResult.java
@@ -14,7 +14,6 @@
import java.io.IOException;
import java.nio.file.Path;
import java.util.List;
-import java.util.concurrent.ExecutionException;
import java.util.function.Consumer;
public class R8TestCompileResult extends TestCompileResult<R8TestCompileResult, R8TestRunResult> {
@@ -23,6 +22,7 @@
private final List<ProguardConfigurationRule> syntheticProguardRules;
private final String proguardMap;
private final CollectingGraphConsumer graphConsumer;
+ private final int minApiLevel;
R8TestCompileResult(
TestState state,
@@ -31,12 +31,14 @@
ProguardConfiguration proguardConfiguration,
List<ProguardConfigurationRule> syntheticProguardRules,
String proguardMap,
- CollectingGraphConsumer graphConsumer) {
+ CollectingGraphConsumer graphConsumer,
+ int minApiLevel) {
super(state, app, outputMode);
this.proguardConfiguration = proguardConfiguration;
this.syntheticProguardRules = syntheticProguardRules;
this.proguardMap = proguardMap;
this.graphConsumer = graphConsumer;
+ this.minApiLevel = minApiLevel;
}
@Override
@@ -65,11 +67,20 @@
}
@Override
- public CodeInspector inspector() throws IOException, ExecutionException {
- return new CodeInspector(app, proguardMap);
+ 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;
+ }
+ });
}
- public GraphInspector graphInspector() throws IOException, ExecutionException {
+ public GraphInspector graphInspector() throws IOException {
assert graphConsumer != null;
return new GraphInspector(graphConsumer, inspector());
}
diff --git a/src/test/java/com/android/tools/r8/maindexlist/MainDexListTests.java b/src/test/java/com/android/tools/r8/maindexlist/MainDexListTests.java
index cacf66b..26186aa 100644
--- a/src/test/java/com/android/tools/r8/maindexlist/MainDexListTests.java
+++ b/src/test/java/com/android/tools/r8/maindexlist/MainDexListTests.java
@@ -635,9 +635,10 @@
.forEach(
p -> {
try {
- CodeInspector i = new CodeInspector(AndroidApp.builder().addProgramFiles(p).build());
+ CodeInspector i =
+ new CodeInspector(AndroidApp.builder().addProgramFiles(p).build());
assertFalse("Found " + clazz + " in file " + p, i.clazz(clazz).isPresent());
- } catch (IOException | ExecutionException e) {
+ } catch (IOException e) {
e.printStackTrace();
}
});
diff --git a/src/test/java/com/android/tools/r8/shaking/AbstractMethodOnNonAbstractClassTest.java b/src/test/java/com/android/tools/r8/shaking/AbstractMethodOnNonAbstractClassTest.java
new file mode 100644
index 0000000..d9970f3
--- /dev/null
+++ b/src/test/java/com/android/tools/r8/shaking/AbstractMethodOnNonAbstractClassTest.java
@@ -0,0 +1,149 @@
+// 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.shaking;
+
+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 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;
+import org.junit.runner.RunWith;
+import org.junit.runners.Parameterized;
+
+@RunWith(Parameterized.class)
+public class AbstractMethodOnNonAbstractClassTest extends TestBase {
+
+ private static final String DEX2OAT_WARNING =
+ "is abstract, but the declaring class is neither abstract nor an interface";
+
+ private final TestParameters parameters;
+
+ @Parameterized.Parameters(name = "{0}")
+ public static TestParametersCollection data() {
+ return getTestParameters().withAllRuntimesAndApiLevels().build();
+ }
+
+ public AbstractMethodOnNonAbstractClassTest(TestParameters parameters) {
+ this.parameters = parameters;
+ }
+
+ @Test
+ public void testCompat() throws Exception {
+ R8TestCompileResult compileResult =
+ testForR8Compat(parameters.getBackend())
+ .addInnerClasses(AbstractMethodOnNonAbstractClassTest.class)
+ .addKeepMainRule(TestClass.class)
+ .setMinApi(parameters.getApiLevel())
+ .compile();
+
+ // A is not made abstract in compat mode.
+ ClassSubject classSubject = compileResult.inspector().clazz(A.class);
+ assertThat(classSubject, isPresent());
+ assertFalse(classSubject.isAbstract());
+
+ MethodSubject methodSubject = classSubject.uniqueMethodWithName("m");
+ assertThat(methodSubject, isPresent());
+
+ 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
+ .run(parameters.getRuntime(), TestClass.class)
+ .assertSuccessWithOutputLines("Hello world!");
+ }
+
+ @Test
+ public void testFull() throws Exception {
+ R8TestCompileResult compileResult =
+ testForR8(parameters.getBackend())
+ .addInnerClasses(AbstractMethodOnNonAbstractClassTest.class)
+ .addKeepMainRule(TestClass.class)
+ .setMinApi(parameters.getApiLevel())
+ .compile();
+
+ // A is made abstract in full mode.
+ ClassSubject classSubject = compileResult.inspector().clazz(A.class);
+ assertThat(classSubject, isPresent());
+ assertTrue(classSubject.isAbstract());
+
+ // A.m() is also made abstract in full mode.
+ MethodSubject methodSubject = classSubject.uniqueMethodWithName("m");
+ assertThat(methodSubject, isPresent());
+ assertTrue(methodSubject.isAbstract());
+
+ if (parameters.isDexRuntime()) {
+ // There is no warning due to both A and A.m() being abstract.
+ compileResult
+ .runDex2Oat(parameters.getRuntime())
+ .assertStderrMatches(not(containsString(DEX2OAT_WARNING)));
+ }
+
+ compileResult
+ .run(parameters.getRuntime(), TestClass.class)
+ .assertSuccessWithOutputLines("Hello world!");
+ }
+
+ static class TestClass {
+
+ public static void main(String[] args) {
+ A b = System.currentTimeMillis() > 0 ? new B() : new C();
+ b.m();
+ }
+ }
+
+ static class A {
+
+ // Never called directly on A, thus can be made abstract.
+ void m() {}
+ }
+
+ static class B extends A {
+
+ @Override
+ void m() {
+ System.out.println("Hello world!");
+ }
+ }
+
+ static class C extends A {
+
+ @Override
+ void m() {
+ throw new RuntimeException();
+ }
+ }
+}
diff --git a/src/test/java/com/android/tools/r8/smali/SmaliBuildTest.java b/src/test/java/com/android/tools/r8/smali/SmaliBuildTest.java
index 912915b..ac43338 100644
--- a/src/test/java/com/android/tools/r8/smali/SmaliBuildTest.java
+++ b/src/test/java/com/android/tools/r8/smali/SmaliBuildTest.java
@@ -13,7 +13,6 @@
import com.android.tools.r8.utils.codeinspector.ClassSubject;
import com.android.tools.r8.utils.codeinspector.CodeInspector;
import java.io.IOException;
-import java.util.concurrent.ExecutionException;
import org.junit.Test;
public class SmaliBuildTest extends SmaliTestBase {
@@ -23,7 +22,7 @@
CodeInspector inspector = new CodeInspector(application);
ClassSubject clazz = inspector.clazz("java.lang.String");
assertEquals(present, clazz.isPresent());
- } catch (IOException | ExecutionException e) {
+ } catch (IOException e) {
throw new RuntimeException(e);
}
}
diff --git a/src/test/java/com/android/tools/r8/smali/SmaliTestBase.java b/src/test/java/com/android/tools/r8/smali/SmaliTestBase.java
index efcc42f..10962c8 100644
--- a/src/test/java/com/android/tools/r8/smali/SmaliTestBase.java
+++ b/src/test/java/com/android/tools/r8/smali/SmaliTestBase.java
@@ -126,7 +126,7 @@
ClassSubject clazz = inspector.clazz(className);
assertTrue(clazz.isPresent());
return clazz.getDexProgramClass();
- } catch (IOException | ExecutionException e) {
+ } catch (IOException e) {
throw new RuntimeException(e);
}
}
@@ -135,7 +135,7 @@
try {
CodeInspector inspector = new CodeInspector(appPath);
return getMethodSubject(inspector, signature);
- } catch (IOException | ExecutionException e) {
+ } catch (IOException e) {
throw new RuntimeException(e);
}
}
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 39f7c44..1156e55 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
@@ -55,7 +55,6 @@
import java.util.Collections;
import java.util.List;
import java.util.Map;
-import java.util.concurrent.ExecutionException;
import java.util.function.BiConsumer;
import java.util.function.BiFunction;
import java.util.function.Consumer;
@@ -72,25 +71,25 @@
public static MethodSignature MAIN =
new MethodSignature("main", "void", new String[] {"java.lang.String[]"});
- public CodeInspector(String path) throws IOException, ExecutionException {
+ public CodeInspector(String path) throws IOException {
this(Paths.get(path));
}
- public CodeInspector(Path file, String mappingFile) throws IOException, ExecutionException {
+ public CodeInspector(Path file, String mappingFile) throws IOException {
this(Collections.singletonList(file), mappingFile, null);
}
- public CodeInspector(Path file) throws IOException, ExecutionException {
+ public CodeInspector(Path file) throws IOException {
this(Collections.singletonList(file), null, null);
}
- public CodeInspector(List<Path> files) throws IOException, ExecutionException {
+ public CodeInspector(List<Path> files) throws IOException {
this(files, null, null);
}
public CodeInspector(
List<Path> files, String mappingFile, Consumer<InternalOptions> optionsConsumer)
- throws IOException, ExecutionException {
+ throws IOException {
Path mappingPath = mappingFile != null ? Paths.get(mappingFile) : null;
if (mappingPath != null && Files.exists(mappingPath)) {
mapping = ClassNameMapper.mapperFromFile(mappingPath);
@@ -109,14 +108,14 @@
application = new ApplicationReader(input, options, timing).read();
}
- public CodeInspector(AndroidApp app) throws IOException, ExecutionException {
+ public CodeInspector(AndroidApp app) throws IOException {
this(
new ApplicationReader(app, runOptionsConsumer(null), Timing.empty())
.read(app.getProguardMapOutputData()));
}
public CodeInspector(AndroidApp app, Consumer<InternalOptions> optionsConsumer)
- throws IOException, ExecutionException {
+ throws IOException {
this(
new ApplicationReader(app, runOptionsConsumer(optionsConsumer), Timing.empty())
.read(app.getProguardMapOutputData()));
@@ -130,17 +129,21 @@
return internalOptions;
}
- public CodeInspector(AndroidApp app, Path proguardMapFile)
- throws IOException, ExecutionException {
+ public CodeInspector(AndroidApp app, Path proguardMapFile) throws IOException {
this(
new ApplicationReader(app, runOptionsConsumer(null), Timing.empty())
.read(StringResource.fromFile(proguardMapFile)));
}
- public CodeInspector(AndroidApp app, String proguardMapContent)
- throws IOException, ExecutionException {
+ 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(null), Timing.empty())
+ new ApplicationReader(app, runOptionsConsumer(optionsConsumer), Timing.empty())
.read(StringResource.fromString(proguardMapContent, Origin.unknown())));
}
