Check for missing reservation state for classes not visited
Bug: 173184123
Bug: 172895918
Change-Id: Ia7f205c8d89aaaea817a205401b9129926dba549
diff --git a/src/main/java/com/android/tools/r8/naming/InterfaceMethodNameMinifier.java b/src/main/java/com/android/tools/r8/naming/InterfaceMethodNameMinifier.java
index 86dac13..a8bd96c 100644
--- a/src/main/java/com/android/tools/r8/naming/InterfaceMethodNameMinifier.java
+++ b/src/main/java/com/android/tools/r8/naming/InterfaceMethodNameMinifier.java
@@ -104,7 +104,7 @@
// Used for iterating the sub trees that has this node as root.
final Set<DexType> children = new HashSet<>();
// Collection of the frontier reservation types and the interface type itself.
- final Set<DexType> reservationTypes = new HashSet<>();
+ private final Set<DexType> reservationTypes = new HashSet<>();
InterfaceReservationState(DexClass iface) {
this.iface = iface;
@@ -139,6 +139,10 @@
return isReserved == null ? null : method.getName();
}
+ void addReservationType(DexType type) {
+ this.reservationTypes.add(type);
+ }
+
void reserveName(DexString reservedName, DexEncodedMethod method) {
forAll(
s -> {
@@ -162,7 +166,7 @@
}
return null;
});
- return result == null ? true : result;
+ return result == null || result;
}
void addRenaming(DexString newName, DexEncodedMethod method) {
@@ -304,7 +308,7 @@
}
return null;
});
- return result == null ? true : result;
+ return result == null || result;
}
void addRenaming(DexString newName, MethodNameMinifier.State minifierState) {
@@ -398,7 +402,7 @@
assert iface.isInterface();
minifierState.allocateReservationStateAndReserve(iface.type, iface.type);
InterfaceReservationState iFaceState = new InterfaceReservationState(iface);
- iFaceState.reservationTypes.add(iface.type);
+ iFaceState.addReservationType(iface.type);
interfaceStateMap.put(iface.type, iFaceState);
}
}
@@ -644,8 +648,12 @@
InterfaceReservationState iState = interfaceStateMap.get(directlyImplemented);
if (iState != null) {
DexType frontierType = minifierState.getFrontier(clazz.type);
- assert minifierState.getReservationState(frontierType) != null;
- iState.reservationTypes.add(frontierType);
+ iState.addReservationType(frontierType);
+ // The reservation state should already be added, but if a class is extending
+ // an interface, we will not visit the class during the sub-type traversel
+ if (minifierState.getReservationState(clazz.type) == null) {
+ minifierState.allocateReservationStateAndReserve(clazz.type, frontierType);
+ }
}
}
}
diff --git a/src/main/java/com/android/tools/r8/naming/Minifier.java b/src/main/java/com/android/tools/r8/naming/Minifier.java
index 9893fb7..4bc638a 100644
--- a/src/main/java/com/android/tools/r8/naming/Minifier.java
+++ b/src/main/java/com/android/tools/r8/naming/Minifier.java
@@ -26,6 +26,7 @@
import com.android.tools.r8.utils.SymbolGenerationUtils;
import com.android.tools.r8.utils.SymbolGenerationUtils.MixedCasing;
import com.android.tools.r8.utils.Timing;
+import java.util.Comparator;
import java.util.HashSet;
import java.util.List;
import java.util.Set;
@@ -47,7 +48,7 @@
assert appView.options().isMinifying();
SubtypingInfo subtypingInfo = appView.appInfo().computeSubtypingInfo();
timing.begin("ComputeInterfaces");
- Set<DexClass> interfaces = new TreeSet<>((a, b) -> a.type.compareTo(b.type));
+ Set<DexClass> interfaces = new TreeSet<>(Comparator.comparing(a -> a.type));
interfaces.addAll(appView.appInfo().computeReachableInterfaces());
timing.end();
timing.begin("MinifyClasses");
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 8e65d5c..32cd619 100644
--- a/src/main/java/com/android/tools/r8/shaking/Enqueuer.java
+++ b/src/main/java/com/android/tools/r8/shaking/Enqueuer.java
@@ -649,6 +649,33 @@
}
}
+ private void warnIfClassExtendsInterfaceOrImplementsClass(DexProgramClass clazz) {
+ if (clazz.superType != null) {
+ DexClass superClass = definitionFor(clazz.superType);
+ if (superClass != null && superClass.isInterface()) {
+ options.reporter.warning(
+ new StringDiagnostic(
+ "Class "
+ + clazz.toSourceString()
+ + " extends "
+ + superClass.toSourceString()
+ + " which is an interface"));
+ }
+ }
+ for (DexType iface : clazz.interfaces.values) {
+ DexClass ifaceClass = definitionFor(iface);
+ if (ifaceClass != null && !ifaceClass.isInterface()) {
+ options.reporter.warning(
+ new StringDiagnostic(
+ "Class "
+ + clazz.toSourceString()
+ + " implements "
+ + ifaceClass.toSourceString()
+ + " which is not an interface"));
+ }
+ }
+ }
+
private void enqueueRootItems(Map<DexReference, Set<ProguardKeepRuleBase>> items) {
items.entrySet().forEach(this::enqueueRootItem);
}
@@ -1681,6 +1708,9 @@
markTypeAsLive(holder.superType, reason);
}
+ // Warn if the class extends an interface or implements a class
+ warnIfLibraryExtendsInterfaceOrImplementsClass(holder);
+
// If this is an interface that has just become live, then report previously seen but unreported
// implemented-by edges.
transitionUnusedInterfaceToLive(holder);
diff --git a/src/test/java/com/android/tools/r8/ExternalR8TestCompileResult.java b/src/test/java/com/android/tools/r8/ExternalR8TestCompileResult.java
index ea6b2f7..a7c0baf 100644
--- a/src/test/java/com/android/tools/r8/ExternalR8TestCompileResult.java
+++ b/src/test/java/com/android/tools/r8/ExternalR8TestCompileResult.java
@@ -5,7 +5,6 @@
package com.android.tools.r8;
import com.android.tools.r8.ToolHelper.ProcessResult;
-import com.android.tools.r8.errors.Unimplemented;
import com.android.tools.r8.utils.AndroidApp;
import com.android.tools.r8.utils.codeinspector.CodeInspector;
import java.io.IOException;
@@ -41,14 +40,6 @@
return proguardMap;
}
- public String stdout() {
- return processResult.stdout;
- }
-
- public String stderr() {
- return processResult.stdout;
- }
-
@Override
public ExternalR8TestCompileResult self() {
return this;
@@ -61,12 +52,12 @@
@Override
public String getStdout() {
- throw new Unimplemented("Unexpected attempt to access stdout from external R8");
+ return processResult.stdout;
}
@Override
public String getStderr() {
- throw new Unimplemented("Unexpected attempt to access stderr from external R8");
+ return processResult.stderr;
}
@Override
diff --git a/src/test/java/com/android/tools/r8/cf/bootstrap/BootstrapCurrentEqualityTest.java b/src/test/java/com/android/tools/r8/cf/bootstrap/BootstrapCurrentEqualityTest.java
index 070d6c8..f2660ef 100644
--- a/src/test/java/com/android/tools/r8/cf/bootstrap/BootstrapCurrentEqualityTest.java
+++ b/src/test/java/com/android/tools/r8/cf/bootstrap/BootstrapCurrentEqualityTest.java
@@ -257,8 +257,8 @@
.setMode(mode)
.compile();
// Check that the process outputs (exit code, stdout, stderr) are the same.
- assertEquals(result.stdout(), runR8R8.stdout());
- assertEquals(result.stderr(), runR8R8.stderr());
+ assertEquals(result.getStdout(), runR8R8.getStdout());
+ assertEquals(result.getStderr(), runR8R8.getStderr());
// Check that the output jars are the same.
assertProgramsEqual(result.outputJar(), runR8R8.outputJar());
}
diff --git a/src/test/java/com/android/tools/r8/naming/b173184123/ClassExtendsInterfaceNamingTest.java b/src/test/java/com/android/tools/r8/naming/b173184123/ClassExtendsInterfaceNamingTest.java
index daf872a..fccc7b2 100644
--- a/src/test/java/com/android/tools/r8/naming/b173184123/ClassExtendsInterfaceNamingTest.java
+++ b/src/test/java/com/android/tools/r8/naming/b173184123/ClassExtendsInterfaceNamingTest.java
@@ -5,8 +5,6 @@
package com.android.tools.r8.naming.b173184123;
import static org.hamcrest.CoreMatchers.containsString;
-import static org.hamcrest.MatcherAssert.assertThat;
-import static org.junit.Assert.assertThrows;
import com.android.tools.r8.NeverClassInline;
import com.android.tools.r8.NeverInline;
@@ -52,28 +50,28 @@
.setSuper(DescriptorUtils.javaTypeToDescriptor(Interface.class.getTypeName()))
.transform())
.build();
- // TODO(b/173184123): Should not result in an error.
- AssertionError assertionError =
- assertThrows(
- AssertionError.class,
- () -> {
- testForExternalR8(
- parameters.getBackend(),
- parameters.isCfRuntime()
- ? parameters.getRuntime()
- : TestRuntime.getCheckedInJdk11())
- .addProgramFiles(classFiles)
- .enableAssertions(false)
- .setMinApi(parameters.getApiLevel())
- .addKeepMainRule(Main.class)
- .addKeepClassAndMembersRules(Interface.class)
- .addKeepRules("-neverclassinline @com.android.tools.r8.NeverClassInline class *")
- .addKeepRules("-neverinline class * { @**.NeverInline *; }")
- .allowTestProguardOptions(true)
- .compile();
- });
- assertThat(
- assertionError.getMessage(), containsString("Error: java.lang.NullPointerException"));
+ testForExternalR8(
+ parameters.getBackend(),
+ parameters.isCfRuntime() ? parameters.getRuntime() : TestRuntime.getCheckedInJdk11())
+ .addProgramFiles(classFiles)
+ .enableAssertions(false)
+ .useR8WithRelocatedDeps()
+ .setMinApi(parameters.getApiLevel())
+ .addKeepMainRule(Main.class)
+ .addKeepClassAndMembersRules(Interface.class)
+ .addKeepRules("-neverclassinline @com.android.tools.r8.NeverClassInline class *")
+ .addKeepRules("-neverinline class * { @**.NeverInline *; }")
+ .allowTestProguardOptions(true)
+ .compile()
+ .assertStderrThatMatches(
+ containsString(
+ "Class "
+ + ConcreteClass.class.getTypeName()
+ + " extends "
+ + Interface.class.getTypeName()
+ + " which is an interface"))
+ .run(parameters.getRuntime(), Main.class)
+ .assertFailureWithErrorThatThrows(IncompatibleClassChangeError.class);
}
public interface Interface {
