Inform if malformed inner-class attributes are found.
PS1 is same as go/r8g/37040.
PS3 changes compilation failure to just warning, along with more tests
regarding wrong repackaging and D8 cases.
PS6 changes warning to info, along with test infra supports.
Change-Id: I0b004294a8659df6184609e9931c7d45b61d349f
Bug: 120597515, 130706685
diff --git a/src/main/java/com/android/tools/r8/graph/JarClassFileReader.java b/src/main/java/com/android/tools/r8/graph/JarClassFileReader.java
index b87d13c..2a290b6 100644
--- a/src/main/java/com/android/tools/r8/graph/JarClassFileReader.java
+++ b/src/main/java/com/android/tools/r8/graph/JarClassFileReader.java
@@ -30,10 +30,12 @@
import com.android.tools.r8.jar.CfApplicationWriter;
import com.android.tools.r8.origin.Origin;
import com.android.tools.r8.shaking.ProguardKeepAttributes;
+import com.android.tools.r8.utils.DescriptorUtils;
import com.android.tools.r8.utils.FieldSignatureEquivalence;
import com.android.tools.r8.utils.InternalOptions;
import com.android.tools.r8.utils.MethodSignatureEquivalence;
import com.android.tools.r8.utils.StringDiagnostic;
+import com.android.tools.r8.utils.StringUtils;
import com.google.common.base.Equivalence.Wrapper;
import java.io.BufferedInputStream;
import java.io.IOException;
@@ -211,6 +213,20 @@
@Override
public void visitInnerClass(String name, String outerName, String innerName, int access) {
+ if (outerName != null && innerName != null) {
+ String separator =
+ DescriptorUtils.computeInnerClassSeparator(
+ outerName, name, innerName, application.options.isMinifying());
+ if (separator == null) {
+ application.options.reporter.info(
+ new StringDiagnostic(
+ StringUtils.lines(
+ "Malformed inner-class attribute:",
+ "\touterTypeInternal: " + outerName,
+ "\tinnerTypeInternal: " + name,
+ "\tinnerName: " + innerName)));
+ }
+ }
innerClasses.add(
new InnerClassAttribute(
access,
diff --git a/src/main/java/com/android/tools/r8/naming/ClassNameMinifier.java b/src/main/java/com/android/tools/r8/naming/ClassNameMinifier.java
index cc52767..725f500 100644
--- a/src/main/java/com/android/tools/r8/naming/ClassNameMinifier.java
+++ b/src/main/java/com/android/tools/r8/naming/ClassNameMinifier.java
@@ -269,7 +269,7 @@
if (outerClass != null) {
String innerClassSeparator =
computeInnerClassSeparator(
- outerClass, type, getInnerNameForType(type), appView.options());
+ outerClass, type, getInnerNameForType(type), appView.options().isMinifying());
assert innerClassSeparator != null;
state = getStateForOuterClass(outerClass, innerClassSeparator);
}
diff --git a/src/main/java/com/android/tools/r8/naming/MinifiedRenaming.java b/src/main/java/com/android/tools/r8/naming/MinifiedRenaming.java
index c996aee..4e26746 100644
--- a/src/main/java/com/android/tools/r8/naming/MinifiedRenaming.java
+++ b/src/main/java/com/android/tools/r8/naming/MinifiedRenaming.java
@@ -77,7 +77,7 @@
}
String separator =
DescriptorUtils.computeInnerClassSeparator(
- attribute.getOuter(), innerType, innerName, options);
+ attribute.getOuter(), innerType, innerName, options.isMinifying());
assert separator != null;
int index = innerTypeMapped.lastIndexOf(separator);
if (index < 0) {
diff --git a/src/main/java/com/android/tools/r8/utils/DescriptorUtils.java b/src/main/java/com/android/tools/r8/utils/DescriptorUtils.java
index 40c0236..d523610 100644
--- a/src/main/java/com/android/tools/r8/utils/DescriptorUtils.java
+++ b/src/main/java/com/android/tools/r8/utils/DescriptorUtils.java
@@ -281,7 +281,6 @@
return className.replace(JAVA_PACKAGE_SEPARATOR, DESCRIPTOR_PACKAGE_SEPARATOR);
}
-
/**
* Convert a class binary name to a descriptor.
*
@@ -310,20 +309,28 @@
}
public static String computeInnerClassSeparator(
- DexType outerClass, DexType innerClass, String innerName, InternalOptions options) {
- String outerInternalRaw = outerClass.getInternalName();
- String innerInternalRaw = innerClass.getInternalName();
+ DexType outerClass, DexType innerClass, String innerName, boolean isMinifying) {
+ return computeInnerClassSeparator(
+ outerClass.getInternalName(), innerClass.getInternalName(), innerName, isMinifying);
+ }
+
+ public static String computeInnerClassSeparator(
+ String outerDescriptor, String innerDescriptor, String innerName, boolean isMinifying) {
// outer-internal<separator>inner-name == inner-internal
- if (outerInternalRaw.length() + innerName.length() > innerInternalRaw.length()) {
+ if (outerDescriptor.length() + innerName.length() > innerDescriptor.length()) {
+ // But, if the minification is enabled, we can choose $ separator.
+ if (isMinifying) {
+ return String.valueOf(INNER_CLASS_SEPARATOR);
+ }
return null;
}
String separator =
- innerInternalRaw.substring(
- outerInternalRaw.length(), innerInternalRaw.length() - innerName.length());
+ innerDescriptor.substring(
+ outerDescriptor.length(), innerDescriptor.length() - innerName.length());
// Any non-$ separator results in a runtime exception in getCanonicalName.
if (!separator.startsWith(String.valueOf(INNER_CLASS_SEPARATOR))) {
- // But, if the minification is enabled, we can choose $ separator.
- if (options.isMinifying()) {
+ // Again, if the minification is enabled, we can choose $ separator.
+ if (isMinifying) {
return String.valueOf(INNER_CLASS_SEPARATOR);
}
return null;
diff --git a/src/test/java/com/android/tools/r8/TestCompileResult.java b/src/test/java/com/android/tools/r8/TestCompileResult.java
index 2137c84..d63626a 100644
--- a/src/test/java/com/android/tools/r8/TestCompileResult.java
+++ b/src/test/java/com/android/tools/r8/TestCompileResult.java
@@ -5,7 +5,7 @@
import static com.android.tools.r8.TestBase.Backend.DEX;
import static com.android.tools.r8.utils.codeinspector.Matchers.isPresent;
-import static org.junit.Assert.assertThat;
+import static org.hamcrest.MatcherAssert.assertThat;
import com.android.tools.r8.ClassFileConsumer.ArchiveConsumer;
import com.android.tools.r8.TestBase.Backend;
@@ -177,6 +177,16 @@
return self();
}
+ public CR assertInfoMessageThatMatches(Matcher<String> matcher) {
+ getDiagnosticMessages().assertInfoMessageThatMatches(matcher);
+ return self();
+ }
+
+ public CR assertNoInfoMessageThatMatches(Matcher<String> matcher) {
+ getDiagnosticMessages().assertNoInfoMessageThatMatches(matcher);
+ return self();
+ }
+
public CR assertWarningMessageThatMatches(Matcher<String> matcher) {
getDiagnosticMessages().assertWarningMessageThatMatches(matcher);
return self();
diff --git a/src/test/java/com/android/tools/r8/TestDiagnosticMessages.java b/src/test/java/com/android/tools/r8/TestDiagnosticMessages.java
index d1e55f4..125329b 100644
--- a/src/test/java/com/android/tools/r8/TestDiagnosticMessages.java
+++ b/src/test/java/com/android/tools/r8/TestDiagnosticMessages.java
@@ -7,7 +7,6 @@
import java.util.List;
import org.hamcrest.Matcher;
-
public interface TestDiagnosticMessages {
public List<Diagnostic> getInfos();
@@ -28,6 +27,10 @@
public TestDiagnosticMessages assertErrorsCount(int count);
+ public TestDiagnosticMessages assertInfoMessageThatMatches(Matcher<String> matcher);
+
+ public TestDiagnosticMessages assertNoInfoMessageThatMatches(Matcher<String> matcher);
+
public TestDiagnosticMessages assertWarningMessageThatMatches(Matcher<String> matcher);
public TestDiagnosticMessages assertNoWarningMessageThatMatches(Matcher<String> matcher);
diff --git a/src/test/java/com/android/tools/r8/TestDiagnosticMessagesImpl.java b/src/test/java/com/android/tools/r8/TestDiagnosticMessagesImpl.java
index 7ad544b..8737581 100644
--- a/src/test/java/com/android/tools/r8/TestDiagnosticMessagesImpl.java
+++ b/src/test/java/com/android/tools/r8/TestDiagnosticMessagesImpl.java
@@ -91,22 +91,23 @@
return this;
}
- public TestDiagnosticMessages assertWarningMessageThatMatches(Matcher<String> matcher) {
- assertNotEquals(0, getWarnings().size());
- for (int i = 0; i < getWarnings().size(); i++) {
- if (matcher.matches(getWarnings().get(i).getDiagnosticMessage())) {
+ private TestDiagnosticMessages assertMessageThatMatches(
+ List<Diagnostic> diagnostics, String tag, Matcher<String> matcher) {
+ assertNotEquals(0, diagnostics.size());
+ for (int i = 0; i < diagnostics.size(); i++) {
+ if (matcher.matches(diagnostics.get(i).getDiagnosticMessage())) {
return this;
}
}
- StringBuilder builder = new StringBuilder("No warning matches " + matcher.toString());
+ StringBuilder builder = new StringBuilder("No " + tag + " matches " + matcher.toString());
builder.append(System.lineSeparator());
if (getWarnings().size() == 0) {
- builder.append("There were no warnings.");
+ builder.append("There were no " + tag + "s.");
} else {
- builder.append("There were " + getWarnings().size() + " warnings:");
+ builder.append("There were " + diagnostics.size() + " "+ tag + "s:");
builder.append(System.lineSeparator());
- for (int i = 0; i < getWarnings().size(); i++) {
- builder.append(getWarnings().get(i).getDiagnosticMessage());
+ for (int i = 0; i < diagnostics.size(); i++) {
+ builder.append(diagnostics.get(i).getDiagnosticMessage());
builder.append(System.lineSeparator());
}
}
@@ -114,14 +115,31 @@
return this;
}
- public TestDiagnosticMessages assertNoWarningMessageThatMatches(Matcher<String> matcher) {
- assertNotEquals(0, getWarnings().size());
- for (int i = 0; i < getWarnings().size(); i++) {
- String message = getWarnings().get(i).getDiagnosticMessage();
+ private TestDiagnosticMessages assertNoMessageThatMatches(
+ List<Diagnostic> diagnostics, String tag, Matcher<String> matcher) {
+ assertNotEquals(0, diagnostics.size());
+ for (int i = 0; i < diagnostics.size(); i++) {
+ String message = diagnostics.get(i).getDiagnosticMessage();
if (matcher.matches(message)) {
- fail("The warning: \"" + message + "\" + matches " + matcher + ".");
+ fail("The " + tag + ": \"" + message + "\" + matches " + matcher + ".");
}
}
return this;
}
+
+ public TestDiagnosticMessages assertInfoMessageThatMatches(Matcher<String> matcher) {
+ return assertMessageThatMatches(getInfos(), "info", matcher);
+ }
+
+ public TestDiagnosticMessages assertNoInfoMessageThatMatches(Matcher<String> matcher) {
+ return assertNoMessageThatMatches(getInfos(), "info", matcher);
+ }
+
+ public TestDiagnosticMessages assertWarningMessageThatMatches(Matcher<String> matcher) {
+ return assertMessageThatMatches(getWarnings(), "warning", matcher);
+ }
+
+ public TestDiagnosticMessages assertNoWarningMessageThatMatches(Matcher<String> matcher) {
+ return assertNoMessageThatMatches(getWarnings(), "warning", matcher);
+ }
}
diff --git a/src/test/java/com/android/tools/r8/ir/optimize/reflection/InnerClassNameTestDump.java b/src/test/java/com/android/tools/r8/ir/optimize/reflection/InnerClassNameTestDump.java
index e3a6336..3e54cfa 100644
--- a/src/test/java/com/android/tools/r8/ir/optimize/reflection/InnerClassNameTestDump.java
+++ b/src/test/java/com/android/tools/r8/ir/optimize/reflection/InnerClassNameTestDump.java
@@ -36,7 +36,12 @@
MethodVisitor methodVisitor;
classWriter.visit(
- V1_8, ACC_SUPER, config.getOuterInternalType(), null, "java/lang/Object", null);
+ V1_8,
+ ACC_PUBLIC | ACC_SUPER,
+ config.getOuterInternalType(),
+ null,
+ "java/lang/Object",
+ null);
classWriter.visitSource("InnerClassNameTest.java", null);
@@ -69,7 +74,12 @@
MethodVisitor methodVisitor;
classWriter.visit(
- V1_8, ACC_SUPER, config.getInnerInternalType(), null, "java/lang/Object", null);
+ V1_8,
+ ACC_PUBLIC | ACC_SUPER,
+ config.getInnerInternalType(),
+ null,
+ "java/lang/Object",
+ null);
classWriter.visitSource("InnerClassNameTest.java", null);
diff --git a/src/test/java/com/android/tools/r8/ir/optimize/reflection/InnerClassNameTestRunner.java b/src/test/java/com/android/tools/r8/ir/optimize/reflection/InnerClassNameTestRunner.java
index 7d44d4b..bc6e13c 100644
--- a/src/test/java/com/android/tools/r8/ir/optimize/reflection/InnerClassNameTestRunner.java
+++ b/src/test/java/com/android/tools/r8/ir/optimize/reflection/InnerClassNameTestRunner.java
@@ -4,13 +4,15 @@
package com.android.tools.r8.ir.optimize.reflection;
import static org.hamcrest.CoreMatchers.containsString;
-import static org.junit.Assert.assertTrue;
+import static org.junit.Assume.assumeTrue;
-import com.android.tools.r8.CompilationFailedException;
+import com.android.tools.r8.D8TestCompileResult;
+import com.android.tools.r8.D8TestRunResult;
import com.android.tools.r8.JvmTestRunResult;
import com.android.tools.r8.R8TestCompileResult;
import com.android.tools.r8.R8TestRunResult;
import com.android.tools.r8.TestBase;
+import com.android.tools.r8.TestCompileResult;
import com.android.tools.r8.TestParameters;
import com.android.tools.r8.TestRuntime.CfVm;
import com.android.tools.r8.ToolHelper.DexVm;
@@ -19,9 +21,7 @@
import com.android.tools.r8.utils.DescriptorUtils;
import com.android.tools.r8.utils.StringUtils;
import com.android.tools.r8.utils.codeinspector.CodeInspector;
-import java.io.IOException;
import java.util.Collection;
-import java.util.concurrent.ExecutionException;
import org.junit.Test;
import org.junit.runner.RunWith;
import org.junit.runners.Parameterized;
@@ -32,6 +32,8 @@
private static final Class<?> MAIN_CLASS = InnerClassNameTest.class;
private static final String PACKAGE = "com/android/tools/r8/ir/optimize/reflection/";
+ private static final String REPACKAGE =
+ "com/android/some/other/repackage/tools/r8/ir/optimize/reflection/";
public enum TestNamingConfig {
DEFAULT,
@@ -39,6 +41,7 @@
EMTPY_SEPARATOR,
UNDERBAR_SEPARATOR,
NON_NESTED_INNER,
+ WRONG_REPACKAGE,
OUTER_ENDS_WITH_DOLLAR,
$_$_$;
@@ -49,6 +52,7 @@
case EMTPY_SEPARATOR:
case UNDERBAR_SEPARATOR:
case NON_NESTED_INNER:
+ case WRONG_REPACKAGE:
return "OuterClass";
case OUTER_ENDS_WITH_DOLLAR:
return "OuterClass$";
@@ -61,6 +65,7 @@
public String getSeparator() {
switch (this) {
case DEFAULT:
+ case WRONG_REPACKAGE:
case OUTER_ENDS_WITH_DOLLAR:
case $_$_$:
return "$";
@@ -82,6 +87,7 @@
case $_$_$:
case UNDERBAR_SEPARATOR:
case NON_NESTED_INNER:
+ case WRONG_REPACKAGE:
case EMTPY_SEPARATOR:
return "$";
case DOLLAR2_SEPARATOR:
@@ -97,10 +103,11 @@
public String getInnerTypeRaw() {
switch (this) {
case DEFAULT:
- case OUTER_ENDS_WITH_DOLLAR:
case DOLLAR2_SEPARATOR:
case EMTPY_SEPARATOR:
case UNDERBAR_SEPARATOR:
+ case WRONG_REPACKAGE:
+ case OUTER_ENDS_WITH_DOLLAR:
case $_$_$:
return getOuterTypeRaw() + getSeparator() + getInnerClassName();
case NON_NESTED_INNER:
@@ -110,7 +117,8 @@
}
public String getInnerInternalType() {
- return PACKAGE + getInnerTypeRaw();
+ // b/130706685: Intentionally repackage inner type only.
+ return (this == WRONG_REPACKAGE ? REPACKAGE : PACKAGE) + getInnerTypeRaw();
}
public String getOuterInternalType() {
@@ -150,8 +158,47 @@
this.config = config;
}
+ private void checkWarningsAboutMalformedAttribute(TestCompileResult<?, ?> result) {
+ switch (config) {
+ case DEFAULT:
+ case OUTER_ENDS_WITH_DOLLAR:
+ case $_$_$:
+ case DOLLAR2_SEPARATOR:
+ result.assertNoMessages();
+ break;
+ case EMTPY_SEPARATOR:
+ case UNDERBAR_SEPARATOR:
+ case NON_NESTED_INNER:
+ case WRONG_REPACKAGE:
+ if (!minify) {
+ result
+ .assertInfoMessageThatMatches(containsString("Malformed inner-class attribute"))
+ .assertInfoMessageThatMatches(containsString(config.getOuterTypeRaw()))
+ .assertInfoMessageThatMatches(containsString(config.getInnerTypeRaw()));
+ } else {
+ result.assertNoMessages();
+ }
+ break;
+ default:
+ throw new Unreachable("Unexpected test configuration: " + config);
+ }
+ }
+
@Test
- public void test() throws IOException, CompilationFailedException, ExecutionException {
+ public void testD8() throws Exception {
+ assumeTrue("Only run D8 for Dex backend", parameters.isDexRuntime() && !minify);
+ D8TestCompileResult d8CompileResult =
+ testForD8()
+ .addProgramClassFileData(InnerClassNameTestDump.dump(config, parameters))
+ .setMinApi(parameters.getRuntime())
+ .compile();
+ checkWarningsAboutMalformedAttribute(d8CompileResult);
+ D8TestRunResult d8RunResult = d8CompileResult.run(parameters.getRuntime(), MAIN_CLASS);
+ d8RunResult.assertSuccessWithOutput(getExpectedNonMinified(config.getInnerClassName()));
+ }
+
+ @Test
+ public void testR8() throws Exception {
JvmTestRunResult runResult = null;
if (parameters.isCfRuntime()) {
runResult =
@@ -160,9 +207,7 @@
.run(parameters.getRuntime(), MAIN_CLASS);
}
- R8TestCompileResult r8CompileResult;
- try {
- r8CompileResult =
+ R8TestCompileResult r8CompileResult =
testForR8(parameters.getBackend())
.addKeepMainRule(MAIN_CLASS)
.addKeepRules("-keep,allowobfuscation class * { *; }")
@@ -171,11 +216,7 @@
.minification(minify)
.setMinApi(parameters.getRuntime())
.compile();
- } catch (CompilationFailedException e) {
- assertTrue("b/120597515", config == TestNamingConfig.NON_NESTED_INNER);
- assertTrue("b/120597515", minify);
- return;
- }
+ checkWarningsAboutMalformedAttribute(r8CompileResult);
CodeInspector inspector = r8CompileResult.inspector();
R8TestRunResult r8RunResult = r8CompileResult.run(parameters.getRuntime(), MAIN_CLASS);
switch (config) {
@@ -205,6 +246,8 @@
break;
case EMTPY_SEPARATOR:
case UNDERBAR_SEPARATOR:
+ case NON_NESTED_INNER:
+ case WRONG_REPACKAGE:
if (!minify
&& parameters.isCfRuntime()
&& parameters.getRuntime().asCf().getVm().lessThanOrEqual(CfVm.JDK8)) {
@@ -217,21 +260,6 @@
r8RunResult.assertSuccessWithOutput(getExpectedMinified(inspector));
}
break;
- case NON_NESTED_INNER:
- if (minify) {
- // TODO(b/120597515): better fail early while reading class file.
- throw new Unreachable("Expect to see compilation error: " + config);
- }
- if (parameters.isCfRuntime()
- && parameters.getRuntime().asCf().getVm().lessThanOrEqual(CfVm.JDK8)) {
- // Any non-$ separator results in a runtime exception in getCanonicalName.
- r8RunResult.assertFailureWithErrorThatMatches(containsString("Malformed class name"));
- } else {
- assert parameters.isDexRuntime()
- || !parameters.getRuntime().asCf().getVm().lessThanOrEqual(CfVm.JDK8);
- r8RunResult.assertSuccessWithOutput(getExpectedMinified(inspector));
- }
- break;
default:
throw new Unreachable("Unexpected test configuration: " + config);
}
