Version 1.2.26
Warn about and remove invalid field and method signatures
CL: https://r8-review.googlesource.com/c/r8/+/22381
Fix rewriting of inner classes not found in signatures
CL: https://r8-review.googlesource.com/c/r8/+/22380
Change-Id: I72c4c631c126824ef39c71b353fe4fecf1f38c3b
diff --git a/src/main/java/com/android/tools/r8/Version.java b/src/main/java/com/android/tools/r8/Version.java
index 6e6a120..98a6d86 100644
--- a/src/main/java/com/android/tools/r8/Version.java
+++ b/src/main/java/com/android/tools/r8/Version.java
@@ -11,7 +11,7 @@
// This field is accessed from release scripts using simple pattern matching.
// Therefore, changing this field could break our release scripts.
- public static final String LABEL = "1.2.25";
+ public static final String LABEL = "1.2.26";
private Version() {
}
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 e3ec746..01fff01 100644
--- a/src/main/java/com/android/tools/r8/naming/ClassNameMinifier.java
+++ b/src/main/java/com/android/tools/r8/naming/ClassNameMinifier.java
@@ -161,7 +161,7 @@
}
private void parseError(DexItem item, Origin origin, GenericSignatureFormatError e) {
- StringBuilder message = new StringBuilder("Invalid class signature for ");
+ StringBuilder message = new StringBuilder("Invalid signature for ");
if (item instanceof DexClass) {
message.append("class ");
message.append(((DexClass) item).getType().toSourceString());
@@ -183,12 +183,14 @@
clazz.annotations = rewriteGenericSignatures(clazz.annotations,
genericSignatureParser::parseClassSignature,
e -> parseError(clazz, clazz.getOrigin(), e));
- clazz.forEachField(field -> rewriteGenericSignatures(
- field.annotations, genericSignatureParser::parseFieldSignature,
- e -> parseError(field, clazz.getOrigin(), e)));
- clazz.forEachMethod(method -> rewriteGenericSignatures(
- method.annotations, genericSignatureParser::parseMethodSignature,
- e -> parseError(method, clazz.getOrigin(), e)));
+ clazz.forEachField(field ->
+ field.annotations = rewriteGenericSignatures(
+ field.annotations, genericSignatureParser::parseFieldSignature,
+ e -> parseError(field, clazz.getOrigin(), e)));
+ clazz.forEachMethod(method ->
+ method.annotations = rewriteGenericSignatures(
+ method.annotations, genericSignatureParser::parseMethodSignature,
+ e -> parseError(method, clazz.getOrigin(), e)));
}
}
@@ -506,11 +508,18 @@
String enclosingRenamedBinaryName =
getClassBinaryNameFromDescriptor(
renaming.getOrDefault(enclosingType, enclosingType.descriptor).toString());
- String renamed =
- getClassBinaryNameFromDescriptor(
- renaming.getOrDefault(type, type.descriptor).toString());
- String outName = renamed.substring(enclosingRenamedBinaryName.length() + 1);
- renamedSignature.append(outName);
+ DexString renamedDescriptor = renaming.get(type);
+ if (renamedDescriptor != null) {
+ // Pick the renamed inner class from the fully renamed binary name.
+ String fullRenamedBinaryName =
+ getClassBinaryNameFromDescriptor(renamedDescriptor.toString());
+ renamedSignature.append(
+ fullRenamedBinaryName.substring(enclosingRenamedBinaryName.length() + 1));
+ } else {
+ // Did not find the class - keep the inner class name as is.
+ // TODO(110085899): Warn about missing classes in signatures?
+ renamedSignature.append(name);
+ }
return type;
}
diff --git a/src/main/java/com/android/tools/r8/naming/signature/GenericSignatureParser.java b/src/main/java/com/android/tools/r8/naming/signature/GenericSignatureParser.java
index 5b549a3..c1f6a7d 100644
--- a/src/main/java/com/android/tools/r8/naming/signature/GenericSignatureParser.java
+++ b/src/main/java/com/android/tools/r8/naming/signature/GenericSignatureParser.java
@@ -84,7 +84,7 @@
throw e;
} catch (Throwable t) {
Error e = new GenericSignatureFormatError(
- "Unknown error parsing generic signature: " + t.getMessage());
+ "Unknown error parsing class signature: " + t.getMessage());
e.addSuppressed(t);
throw e;
}
@@ -100,7 +100,7 @@
throw e;
} catch (Throwable t) {
Error e = new GenericSignatureFormatError(
- "Unknown error parsing generic signature: " + t.getMessage());
+ "Unknown error parsing method signature: " + t.getMessage());
e.addSuppressed(t);
throw e;
}
@@ -112,14 +112,14 @@
setInput(signature);
parseFieldTypeSignature();
actions.stop();
- } catch (GenericSignatureFormatError e) {
- throw e;
- } catch (Throwable t) {
- Error e = new GenericSignatureFormatError(
- "Unknown error parsing generic signature: " + t.getMessage());
- e.addSuppressed(t);
- throw e;
- }
+ } catch (GenericSignatureFormatError e) {
+ throw e;
+ } catch (Throwable t) {
+ Error e = new GenericSignatureFormatError(
+ "Unknown error parsing field signature: " + t.getMessage());
+ e.addSuppressed(t);
+ throw e;
+ }
}
private void setInput(String input) {
diff --git a/src/test/java/com/android/tools/r8/naming/GenericSignatureParserTest.java b/src/test/java/com/android/tools/r8/naming/GenericSignatureParserTest.java
index 1d1604b..957bc1d 100644
--- a/src/test/java/com/android/tools/r8/naming/GenericSignatureParserTest.java
+++ b/src/test/java/com/android/tools/r8/naming/GenericSignatureParserTest.java
@@ -350,7 +350,8 @@
forMethodSignatures(this::parseMethodSignature);
}
- private void failingParseAction(Consumer<GenericSignatureParser<String>> parse)
+ private void failingParseAction(
+ Consumer<GenericSignatureParser<String>> parse, String errorMessageType)
throws Exception {
class ThrowsInParserActionBase<E extends Error> extends ReGenerateGenericSignatureRewriter {
protected Supplier<? extends E> exceptionSupplier;
@@ -452,7 +453,8 @@
assertEquals("ERROR", e.getMessage());
} else {
plainErrorCount++;
- assertEquals("Unknown error parsing generic signature: ERROR", e.getMessage());
+ assertEquals("Unknown error parsing "
+ + errorMessageType + " signature: ERROR", e.getMessage());
}
}
}
@@ -463,10 +465,10 @@
public void failingParseAction() throws Exception {
// These signatures hits all action callbacks.
failingParseAction(parser -> parser.parseClassSignature(
- "<U:Ljava/lang/Object;>LOuter<TT;>.Inner;Ljava/util/List<TU;>;"));
+ "<U:Ljava/lang/Object;>LOuter<TT;>.Inner;Ljava/util/List<TU;>;"), "class");
failingParseAction(
- parser -> parser.parseFieldSignature("LOuter$InnerInterface<TU;>.Inner;"));
+ parser -> parser.parseFieldSignature("LOuter$InnerInterface<TU;>.Inner;"), "field");
failingParseAction(
- parser -> parser.parseMethodSignature("(LOuter$InnerInterface<TU;>.Inner;)V"));
+ parser -> parser.parseMethodSignature("(LOuter$InnerInterface<TU;>.Inner;)V"), "method");
}
}
diff --git a/src/test/java/com/android/tools/r8/naming/MinifierClassSignatureTest.java b/src/test/java/com/android/tools/r8/naming/MinifierClassSignatureTest.java
index 68cd36f..b9ff7ea 100644
--- a/src/test/java/com/android/tools/r8/naming/MinifierClassSignatureTest.java
+++ b/src/test/java/com/android/tools/r8/naming/MinifierClassSignatureTest.java
@@ -424,7 +424,7 @@
@Test
public void classSignatureOuter_classNotFound() throws Exception {
- String signature = "<T:LNotFound;>LNotFound;";
+ String signature = "<T:LNotFound;>LAlsoNotFound;";
testSingleClass("Outer", signature, this::noWarnings, inspector -> {
assertThat(inspector.clazz("NotFound"), not(isPresent()));
ClassSubject outer = inspector.clazz("Outer");
@@ -433,11 +433,51 @@
}
@Test
+ public void classSignatureExtendsInner_innerClassNotFound() throws Exception {
+ String signature = "LOuter<TT;>.NotFound;";
+ testSingleClass("Outer$ExtendsInner", signature, this::noWarnings, inspector -> {
+ assertThat(inspector.clazz("NotFound"), not(isPresent()));
+ ClassSubject outer = inspector.clazz("Outer$ExtendsInner");
+ assertEquals(signature, outer.getOriginalSignatureAttribute());
+ });
+ }
+
+ @Test
+ public void classSignatureExtendsInner_outerAndInnerClassNotFound() throws Exception {
+ String signature = "LNotFound<TT;>.AlsoNotFound;";
+ testSingleClass("Outer$ExtendsInner", signature, this::noWarnings, inspector -> {
+ assertThat(inspector.clazz("NotFound"), not(isPresent()));
+ ClassSubject outer = inspector.clazz("Outer$ExtendsInner");
+ assertEquals(signature, outer.getOriginalSignatureAttribute());
+ });
+ }
+
+ @Test
+ public void classSignatureExtendsInner_nestedInnerClassNotFound() throws Exception {
+ String signature = "LOuter<TT;>.Inner.NotFound;";
+ testSingleClass("Outer$ExtendsInner", signature, this::noWarnings, inspector -> {
+ assertThat(inspector.clazz("NotFound"), not(isPresent()));
+ ClassSubject outer = inspector.clazz("Outer$ExtendsInner");
+ assertEquals(signature, outer.getOriginalSignatureAttribute());
+ });
+ }
+
+ @Test
+ public void classSignatureExtendsInner_multipleMestedInnerClassesNotFound() throws Exception {
+ String signature = "LOuter<TT;>.NotFound.AlsoNotFound;";
+ testSingleClass("Outer$ExtendsInner", signature, this::noWarnings, inspector -> {
+ assertThat(inspector.clazz("NotFound"), not(isPresent()));
+ ClassSubject outer = inspector.clazz("Outer$ExtendsInner");
+ assertEquals(signature, outer.getOriginalSignatureAttribute());
+ });
+ }
+
+ @Test
public void classSignatureOuter_invalid() throws Exception {
testSingleClass("Outer", "X", diagnostics -> {
assertEquals(1, diagnostics.warnings.size());
DiagnosticsChecker.checkDiagnostic(diagnostics.warnings.get(0), this::isOriginUnknown,
- "Invalid class signature for class Outer", "Expected L at position 1");
+ "Invalid signature for class Outer", "Expected L at position 1");
}, inspector -> noSignatureAttribute(inspector.clazz("Outer")));
}
@@ -446,7 +486,7 @@
testSingleClass("Outer", "<L", diagnostics -> {
assertEquals(1, diagnostics.warnings.size());
DiagnosticsChecker.checkDiagnostic(diagnostics.warnings.get(0), this::isOriginUnknown,
- "Invalid class signature for class Outer", "Unexpected end of signature at position 3");
+ "Invalid signature for class Outer", "Unexpected end of signature at position 3");
}, inspector -> noSignatureAttribute(inspector.clazz("Outer")));
}
@@ -455,7 +495,7 @@
testSingleClass("Outer$ExtendsInner", "X", diagnostics -> {
assertEquals(1, diagnostics.warnings.size());
DiagnosticsChecker.checkDiagnostic(diagnostics.warnings.get(0), this::isOriginUnknown,
- "Invalid class signature for class Outer$ExtendsInner", "Expected L at position 1");
+ "Invalid signature for class Outer$ExtendsInner", "Expected L at position 1");
}, inspector -> noSignatureAttribute(inspector.clazz("Outer$ExtendsInner")));
}
@@ -464,7 +504,7 @@
testSingleClass("Outer$Inner$ExtendsInnerInner", "X", diagnostics -> {
assertEquals(1, diagnostics.warnings.size());
DiagnosticsChecker.checkDiagnostic(diagnostics.warnings.get(0), this::isOriginUnknown,
- "Invalid class signature for class Outer$Inner$ExtendsInnerInner",
+ "Invalid signature for class Outer$Inner$ExtendsInnerInner",
"Expected L at position 1");
}, inspector -> noSignatureAttribute(inspector.clazz("Outer$Inner$ExtendsInnerInner")));
}
@@ -488,7 +528,7 @@
testSingleClass("Outer$ExtendsInner", signature, diagnostics -> {
assertEquals(1, diagnostics.warnings.size());
DiagnosticsChecker.checkDiagnostic(diagnostics.warnings.get(0), this::isOriginUnknown,
- "Invalid class signature for class Outer$ExtendsInner", "Expected ; at position 16");
+ "Invalid signature for class Outer$ExtendsInner", "Expected ; at position 16");
}, inspector -> {
noSignatureAttribute(inspector.clazz("Outer$ExtendsInner"));
});
diff --git a/src/test/java/com/android/tools/r8/naming/MinifierFieldSignatureTest.java b/src/test/java/com/android/tools/r8/naming/MinifierFieldSignatureTest.java
new file mode 100644
index 0000000..e48ce56
--- /dev/null
+++ b/src/test/java/com/android/tools/r8/naming/MinifierFieldSignatureTest.java
@@ -0,0 +1,289 @@
+// Copyright (c) 2018, 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.naming;
+
+import static com.android.tools.r8.utils.DexInspectorMatchers.isPresent;
+import static com.android.tools.r8.utils.DexInspectorMatchers.isRenamed;
+import static org.hamcrest.CoreMatchers.not;
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertNull;
+import static org.junit.Assert.assertSame;
+import static org.junit.Assert.assertThat;
+import static org.objectweb.asm.Opcodes.ACC_FINAL;
+import static org.objectweb.asm.Opcodes.ACC_PUBLIC;
+import static org.objectweb.asm.Opcodes.ACC_SUPER;
+import static org.objectweb.asm.Opcodes.ACC_SYNTHETIC;
+import static org.objectweb.asm.Opcodes.ALOAD;
+import static org.objectweb.asm.Opcodes.INVOKESPECIAL;
+import static org.objectweb.asm.Opcodes.PUTFIELD;
+import static org.objectweb.asm.Opcodes.RETURN;
+import static org.objectweb.asm.Opcodes.V1_8;
+
+import com.android.tools.r8.DexIndexedConsumer;
+import com.android.tools.r8.DiagnosticsChecker;
+import com.android.tools.r8.R8Command;
+import com.android.tools.r8.StringConsumer;
+import com.android.tools.r8.TestBase;
+import com.android.tools.r8.ToolHelper;
+import com.android.tools.r8.graph.invokesuper.Consumer;
+import com.android.tools.r8.origin.Origin;
+import com.android.tools.r8.utils.DexInspector;
+import com.android.tools.r8.utils.DexInspector.ClassSubject;
+import com.android.tools.r8.utils.DexInspector.FieldSubject;
+import com.google.common.collect.ImmutableList;
+import com.google.common.collect.ImmutableMap;
+import java.util.Map;
+import org.junit.Test;
+import org.objectweb.asm.ClassWriter;
+import org.objectweb.asm.FieldVisitor;
+import org.objectweb.asm.MethodVisitor;
+
+
+public class MinifierFieldSignatureTest extends TestBase {
+ /*
+
+ class Fields<X extends String> {
+ class Inner {
+ }
+ public X anX;
+ public X[] anArrayOfX;
+ public Fields<X> aFieldsOfX;
+ public Fields<X>.Inner aFieldsOfXInner;
+ }
+
+ */
+
+ private String anXSignature = "TX;";
+ private String anArrayOfXSignature = "[TX;";
+ private String aFieldsOfXSignature = "LFields<TX;>;";
+ private String aFieldsOfXInnerSignature = "LFields<TX;>.Inner;";
+
+ public byte[] dumpFields(Map<String, String> signatures) throws Exception {
+
+ ClassWriter cw = new ClassWriter(0);
+ FieldVisitor fv;
+ MethodVisitor mv;
+ String signature;
+
+ cw.visit(V1_8, ACC_SUPER, "Fields", "<X:Ljava/lang/String;>Ljava/lang/Object;",
+ "java/lang/Object", null);
+
+ cw.visitInnerClass("Fields$Inner", "Fields", "Inner", 0);
+
+ {
+ signature = signatures.get("anX");
+ signature = signature == null ? anXSignature : signature;
+ fv = cw.visitField(ACC_PUBLIC, "anX", "Ljava/lang/String;", signature, null);
+ fv.visitEnd();
+ }
+ {
+ signature = signatures.get("anArrayOfX");
+ signature = signature == null ? anArrayOfXSignature : signature;
+ fv = cw.visitField(
+ ACC_PUBLIC, "anArrayOfX", "[Ljava/lang/String;", signature, null);
+ fv.visitEnd();
+ }
+ {
+ signature = signatures.get("aFieldsOfX");
+ signature = signature == null ? aFieldsOfXSignature : signature;
+ fv = cw.visitField(ACC_PUBLIC, "aFieldsOfX", "LFields;", signature, null);
+ fv.visitEnd();
+ }
+ {
+ signature = signatures.get("aFieldsOfXInner");
+ signature = signature == null ? aFieldsOfXInnerSignature : signature;
+ fv = cw.visitField(
+ ACC_PUBLIC, "aFieldsOfXInner", "LFields$Inner;", signature, null);
+ fv.visitEnd();
+ }
+ {
+ mv = cw.visitMethod(0, "<init>", "()V", null, null);
+ mv.visitCode();
+ mv.visitVarInsn(ALOAD, 0);
+ mv.visitMethodInsn(INVOKESPECIAL, "java/lang/Object", "<init>", "()V", false);
+ mv.visitInsn(RETURN);
+ mv.visitMaxs(1, 1);
+ mv.visitEnd();
+ }
+ cw.visitEnd();
+
+ return cw.toByteArray();
+ }
+
+ public byte[] dumpInner() throws Exception {
+
+ ClassWriter cw = new ClassWriter(0);
+ FieldVisitor fv;
+ MethodVisitor mv;
+
+ cw.visit(V1_8, ACC_SUPER, "Fields$Inner", null, "java/lang/Object", null);
+
+ cw.visitInnerClass("Fields$Inner", "Fields", "Inner", 0);
+
+ {
+ fv = cw.visitField(ACC_FINAL + ACC_SYNTHETIC, "this$0", "LFields;", null, null);
+ fv.visitEnd();
+ }
+ {
+ mv = cw.visitMethod(0, "<init>", "(LFields;)V", null, null);
+ mv.visitCode();
+ mv.visitVarInsn(ALOAD, 0);
+ mv.visitVarInsn(ALOAD, 1);
+ mv.visitFieldInsn(PUTFIELD, "Fields$Inner", "this$0", "LFields;");
+ mv.visitVarInsn(ALOAD, 0);
+ mv.visitMethodInsn(INVOKESPECIAL, "java/lang/Object", "<init>", "()V", false);
+ mv.visitInsn(RETURN);
+ mv.visitMaxs(2, 2);
+ mv.visitEnd();
+ }
+ cw.visitEnd();
+
+ return cw.toByteArray();
+ }
+
+ private FieldSubject lookupAnX(DexInspector inspector) {
+ ClassSubject clazz = inspector.clazz("Fields");
+ return clazz.field("java.lang.String", "anX");
+ }
+
+ private FieldSubject lookupAnArrayOfX(DexInspector inspector) {
+ ClassSubject clazz = inspector.clazz("Fields");
+ return clazz.field("java.lang.String[]", "anArrayOfX");
+ }
+
+ private FieldSubject lookupAFieldsOfX(DexInspector inspector) {
+ ClassSubject clazz = inspector.clazz("Fields");
+ return clazz.field("Fields", "aFieldsOfX");
+ }
+
+ public void runTest(
+ ImmutableMap<String, String> signatures,
+ Consumer<DiagnosticsChecker> diagnostics,
+ Consumer<DexInspector> inspect)
+ throws Exception {
+ DiagnosticsChecker checker = new DiagnosticsChecker();
+ DexInspector inspector = new DexInspector(
+ ToolHelper.runR8(R8Command.builder(checker)
+ .addClassProgramData(dumpFields(signatures), Origin.unknown())
+ .addClassProgramData(dumpInner(), Origin.unknown())
+ .addProguardConfiguration(ImmutableList.of(
+ "-keepattributes InnerClasses,EnclosingMethod,Signature",
+ "-keep,allowobfuscation class ** { *; }"
+ ), Origin.unknown())
+ .setProgramConsumer(DexIndexedConsumer.emptyConsumer())
+ .setProguardMapConsumer(StringConsumer.emptyConsumer())
+ .build()));
+ // All classes are kept, and renamed.
+ ClassSubject clazz = inspector.clazz("Fields");
+ assertThat(clazz, isRenamed());
+ assertThat(inspector.clazz("Fields$Inner"), isRenamed());
+
+ FieldSubject anX = lookupAnX(inspector);
+ FieldSubject anArrayOfX = lookupAnArrayOfX(inspector);
+ FieldSubject aFieldsOfX =lookupAFieldsOfX(inspector);
+ FieldSubject aFieldsOfXInner = clazz.field("Fields$Inner", "aFieldsOfXInner");
+
+ // Check that all fields have been renamed
+ assertThat(anX, isRenamed());
+ assertThat(anArrayOfX, isRenamed());
+ assertThat(aFieldsOfX, isRenamed());
+ assertThat(aFieldsOfXInner, isRenamed());
+
+ //System.out.println(generic.getFinalSignatureAttribute());
+ //System.out.println(generic.getOriginalSignatureAttribute());
+
+ // Test that methods have their original signature if the default was provided.
+ if (!signatures.containsKey("anX")) {
+ assertEquals(anXSignature, anX.getOriginalSignatureAttribute());
+ }
+ if (!signatures.containsKey("anArrayOfX")) {
+ assertEquals(anArrayOfXSignature, anArrayOfX.getOriginalSignatureAttribute());
+ }
+ if (!signatures.containsKey("aFieldsOfX")) {
+ assertEquals(
+ aFieldsOfXSignature, aFieldsOfX.getOriginalSignatureAttribute());
+ }
+ if (!signatures.containsKey("aFieldsOfXInner")) {
+ assertEquals(
+ aFieldsOfXInnerSignature, aFieldsOfXInner.getOriginalSignatureAttribute());
+ }
+
+ diagnostics.accept(checker);
+ inspect.accept(inspector);
+ }
+
+ private void testSingleField(String name, String signature,
+ Consumer<DiagnosticsChecker> diagnostics,
+ Consumer<DexInspector> inspector)
+ throws Exception {
+ ImmutableMap<String, String> signatures = ImmutableMap.of(name, signature);
+ runTest(signatures, diagnostics, inspector);
+ }
+
+ private void isOriginUnknown(Origin origin) {
+ assertSame(Origin.unknown(), origin);
+ }
+
+ private void noWarnings(DiagnosticsChecker checker) {
+ assertEquals(0, checker.warnings.size());
+ }
+
+ private void noInspection(DexInspector inspector) {
+ }
+
+ private void noSignatureAttribute(FieldSubject field) {
+ assertThat(field, isPresent());
+ assertNull(field.getFinalSignatureAttribute());
+ assertNull(field.getOriginalSignatureAttribute());
+ }
+
+ @Test
+ public void originalJavacSignatures() throws Exception {
+ // Test using the signatures generated by javac.
+ runTest(ImmutableMap.of(), this::noWarnings, this::noInspection);
+ }
+
+ @Test
+ public void signatureEmpty() throws Exception {
+ testSingleField("anX", "", this::noWarnings,
+ inspector -> noSignatureAttribute(lookupAnX(inspector)));
+ }
+
+ @Test
+ public void signatureInvalid() throws Exception {
+ testSingleField("anX", "X", diagnostics -> {
+ assertEquals(1, diagnostics.warnings.size());
+ // TODO(sgjesse): The position 2 reported here is one off.
+ DiagnosticsChecker.checkDiagnostic(diagnostics.warnings.get(0), this::isOriginUnknown,
+ "Invalid signature for field",
+ "java.lang.String Fields.anX",
+ "Expected L, [ or T at position 2");
+ }, inspector -> noSignatureAttribute(lookupAnX(inspector)));
+ }
+
+ @Test
+ public void classNotFound() throws Exception {
+ String signature = "LNotFound<TX;>.InnerNotFound.InnerAlsoNotFound;";
+ testSingleField("anX", signature, this::noWarnings, inspector -> {
+ assertThat(inspector.clazz("NotFound"), not(isPresent()));
+ assertEquals(signature, lookupAnX(inspector).getOriginalSignatureAttribute());
+ });
+ }
+
+ @Test
+ public void multipleWarnings() throws Exception {
+ runTest(ImmutableMap.of(
+ "anX", "X",
+ "anArrayOfX", "X",
+ "aFieldsOfX", "X"
+ ), diagnostics -> {
+ assertEquals(3, diagnostics.warnings.size());
+ }, inspector -> {
+ noSignatureAttribute(lookupAnX(inspector));
+ noSignatureAttribute(lookupAnArrayOfX(inspector));
+ noSignatureAttribute(lookupAFieldsOfX(inspector));
+ });
+ }
+}
diff --git a/src/test/java/com/android/tools/r8/naming/MinifierMethodSignatureTest.java b/src/test/java/com/android/tools/r8/naming/MinifierMethodSignatureTest.java
new file mode 100644
index 0000000..41750ae
--- /dev/null
+++ b/src/test/java/com/android/tools/r8/naming/MinifierMethodSignatureTest.java
@@ -0,0 +1,314 @@
+// Copyright (c) 2018, 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.naming;
+
+import static com.android.tools.r8.utils.DexInspectorMatchers.isPresent;
+import static com.android.tools.r8.utils.DexInspectorMatchers.isRenamed;
+import static org.hamcrest.CoreMatchers.not;
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertNull;
+import static org.junit.Assert.assertSame;
+import static org.junit.Assert.assertThat;
+import static org.objectweb.asm.Opcodes.ACC_FINAL;
+import static org.objectweb.asm.Opcodes.ACC_PUBLIC;
+import static org.objectweb.asm.Opcodes.ACC_STATIC;
+import static org.objectweb.asm.Opcodes.ACC_SUPER;
+import static org.objectweb.asm.Opcodes.ACC_SYNTHETIC;
+import static org.objectweb.asm.Opcodes.ACONST_NULL;
+import static org.objectweb.asm.Opcodes.ALOAD;
+import static org.objectweb.asm.Opcodes.ARETURN;
+import static org.objectweb.asm.Opcodes.INVOKESPECIAL;
+import static org.objectweb.asm.Opcodes.PUTFIELD;
+import static org.objectweb.asm.Opcodes.RETURN;
+import static org.objectweb.asm.Opcodes.V1_8;
+
+import com.android.tools.r8.DexIndexedConsumer;
+import com.android.tools.r8.DiagnosticsChecker;
+import com.android.tools.r8.R8Command;
+import com.android.tools.r8.StringConsumer;
+import com.android.tools.r8.TestBase;
+import com.android.tools.r8.ToolHelper;
+import com.android.tools.r8.graph.invokesuper.Consumer;
+import com.android.tools.r8.origin.Origin;
+import com.android.tools.r8.utils.DexInspector;
+import com.android.tools.r8.utils.DexInspector.ClassSubject;
+import com.android.tools.r8.utils.DexInspector.MethodSubject;
+import com.google.common.collect.ImmutableList;
+import com.google.common.collect.ImmutableMap;
+import java.util.Map;
+import org.junit.Test;
+import org.objectweb.asm.ClassWriter;
+import org.objectweb.asm.FieldVisitor;
+import org.objectweb.asm.MethodVisitor;
+
+public class MinifierMethodSignatureTest extends TestBase {
+ /*
+
+ class Methods<X extends Throwable> {
+ class Inner {
+ }
+ public static <T extends Throwable> T generic(T a, Methods<T>.Inner b) { return null; }
+ public Methods<X>.Inner parameterizedReturn() { return null; }
+ public void parameterizedArguments(X a, Methods<X>.Inner b) { }
+ public void parametrizedThrows() throws X { }
+ }
+
+ */
+
+ private String genericSignature = "<T:Ljava/lang/Throwable;>(TT;LMethods<TT;>.Inner;)TT;";
+ private String parameterizedReturnSignature = "()LMethods<TX;>.Inner;";
+ private String parameterizedArgumentsSignature = "(TX;LMethods<TX;>.Inner;)V";
+ private String parametrizedThrowsSignature = "()V^TX;";
+
+ private byte[] dumpMethods(Map<String, String> signatures) throws Exception {
+
+ ClassWriter cw = new ClassWriter(0);
+ MethodVisitor mv;
+ String signature;
+
+ cw.visit(V1_8, ACC_SUPER, "Methods", "<X:Ljava/lang/Throwable;>Ljava/lang/Object;",
+ "java/lang/Object", null);
+
+ cw.visitInnerClass("Methods$Inner", "Methods", "Inner", 0);
+
+ {
+ mv = cw.visitMethod(0, "<init>", "()V", null, null);
+ mv.visitCode();
+ mv.visitVarInsn(ALOAD, 0);
+ mv.visitMethodInsn(INVOKESPECIAL, "java/lang/Object", "<init>", "()V", false);
+ mv.visitInsn(RETURN);
+ mv.visitMaxs(1, 1);
+ mv.visitEnd();
+ }
+ {
+ signature = signatures.get("generic");
+ signature = signature == null ? genericSignature : signature;
+ mv = cw.visitMethod(ACC_PUBLIC + ACC_STATIC, "generic",
+ "(Ljava/lang/Throwable;LMethods$Inner;)Ljava/lang/Throwable;",
+ signature, null);
+ mv.visitCode();
+ mv.visitInsn(ACONST_NULL);
+ mv.visitInsn(ARETURN);
+ mv.visitMaxs(1, 2);
+ mv.visitEnd();
+ }
+ {
+ signature = signatures.get("parameterizedReturn");
+ signature = signature == null ? parameterizedReturnSignature : signature;
+ mv = cw.visitMethod(ACC_PUBLIC, "parameterizedReturn", "()LMethods$Inner;",
+ signature, null);
+ mv.visitCode();
+ mv.visitInsn(ACONST_NULL);
+ mv.visitInsn(ARETURN);
+ mv.visitMaxs(1, 1);
+ mv.visitEnd();
+ }
+ {
+ signature = signatures.get("parameterizedArguments");
+ signature = signature == null ? parameterizedArgumentsSignature : signature;
+ mv = cw.visitMethod(ACC_PUBLIC, "parameterizedArguments",
+ "(Ljava/lang/Throwable;LMethods$Inner;)V", signature, null);
+ mv.visitCode();
+ mv.visitInsn(RETURN);
+ mv.visitMaxs(0, 3);
+ mv.visitEnd();
+ }
+ {
+ signature = signatures.get("parametrizedThrows");
+ signature = signature == null ? parametrizedThrowsSignature : signature;
+ mv = cw.visitMethod(ACC_PUBLIC, "parametrizedThrows", "()V", signature,
+ new String[] { "java/lang/Throwable" });
+ mv.visitCode();
+ mv.visitInsn(RETURN);
+ mv.visitMaxs(0, 1);
+ mv.visitEnd();
+ }
+ cw.visitEnd();
+
+ return cw.toByteArray();
+ }
+
+ private byte[] dumpInner() throws Exception {
+
+ ClassWriter cw = new ClassWriter(0);
+ FieldVisitor fv;
+ MethodVisitor mv;
+
+ cw.visit(V1_8, ACC_SUPER, "Methods$Inner", null, "java/lang/Object", null);
+
+ cw.visitInnerClass("Methods$Inner", "Methods", "Inner", 0);
+
+ {
+ fv = cw.visitField(ACC_FINAL + ACC_SYNTHETIC, "this$0", "LMethods;", null, null);
+ fv.visitEnd();
+ }
+ {
+ mv = cw.visitMethod(0, "<init>", "(LMethods;)V", null, null);
+ mv.visitCode();
+ mv.visitVarInsn(ALOAD, 0);
+ mv.visitVarInsn(ALOAD, 1);
+ mv.visitFieldInsn(PUTFIELD, "Methods$Inner", "this$0", "LMethods;");
+ mv.visitVarInsn(ALOAD, 0);
+ mv.visitMethodInsn(INVOKESPECIAL, "java/lang/Object", "<init>", "()V", false);
+ mv.visitInsn(RETURN);
+ mv.visitMaxs(2, 2);
+ mv.visitEnd();
+ }
+ cw.visitEnd();
+
+ return cw.toByteArray();
+ }
+
+ private MethodSubject lookupGeneric(DexInspector inspector) {
+ ClassSubject clazz = inspector.clazz("Methods");
+ return clazz.method(
+ "java.lang.Throwable", "generic", ImmutableList.of("java.lang.Throwable", "Methods$Inner"));
+ }
+
+ private MethodSubject lookupParameterizedReturn(DexInspector inspector) {
+ ClassSubject clazz = inspector.clazz("Methods");
+ return clazz.method(
+ "Methods$Inner", "parameterizedReturn", ImmutableList.of());
+ }
+
+ private MethodSubject lookupParameterizedArguments(DexInspector inspector) {
+ ClassSubject clazz = inspector.clazz("Methods");
+ return clazz.method(
+ "void", "parameterizedArguments", ImmutableList.of("java.lang.Throwable", "Methods$Inner"));
+ }
+
+ public void runTest(
+ ImmutableMap<String, String> signatures,
+ Consumer<DiagnosticsChecker> diagnostics,
+ Consumer<DexInspector> inspect)
+ throws Exception {
+ DiagnosticsChecker checker = new DiagnosticsChecker();
+ DexInspector inspector = new DexInspector(
+ ToolHelper.runR8(R8Command.builder(checker)
+ .addClassProgramData(dumpMethods(signatures), Origin.unknown())
+ .addClassProgramData(dumpInner(), Origin.unknown())
+ .addProguardConfiguration(ImmutableList.of(
+ "-keepattributes InnerClasses,EnclosingMethod,Signature",
+ "-keep,allowobfuscation class ** { *; }"
+ ), Origin.unknown())
+ .setProgramConsumer(DexIndexedConsumer.emptyConsumer())
+ .setProguardMapConsumer(StringConsumer.emptyConsumer())
+ .build()));
+ // All classes are kept, and renamed.
+ ClassSubject clazz = inspector.clazz("Methods");
+ assertThat(clazz, isRenamed());
+ assertThat(inspector.clazz("Methods$Inner"), isRenamed());
+
+ MethodSubject generic = lookupGeneric(inspector);
+ MethodSubject parameterizedReturn = lookupParameterizedReturn(inspector);
+ MethodSubject parameterizedArguments = lookupParameterizedArguments(inspector);
+ MethodSubject parametrizedThrows =
+ clazz.method("void", "parametrizedThrows", ImmutableList.of());
+
+ // Check that all methods have been renamed
+ assertThat(generic, isRenamed());
+ assertThat(parameterizedReturn, isRenamed());
+ assertThat(parameterizedArguments, isRenamed());
+ assertThat(parametrizedThrows, isRenamed());
+
+ // Test that methods have their original signature if the default was provided.
+ if (!signatures.containsKey("generic")) {
+ assertEquals(genericSignature, generic.getOriginalSignatureAttribute());
+ }
+ if (!signatures.containsKey("parameterizedReturn")) {
+ assertEquals(
+ parameterizedReturnSignature, parameterizedReturn.getOriginalSignatureAttribute());
+ }
+ if (!signatures.containsKey("parameterizedArguments")) {
+ assertEquals(
+ parameterizedArgumentsSignature, parameterizedArguments.getOriginalSignatureAttribute());
+ }
+ if (!signatures.containsKey("parametrizedThrows")) {
+ assertEquals(
+ parametrizedThrowsSignature, parametrizedThrows.getOriginalSignatureAttribute());
+ }
+
+ diagnostics.accept(checker);
+ inspect.accept(inspector);
+ }
+
+ private void testSingleMethod(String name, String signature,
+ Consumer<DiagnosticsChecker> diagnostics,
+ Consumer<DexInspector> inspector)
+ throws Exception {
+ ImmutableMap<String, String> signatures = ImmutableMap.of(name, signature);
+ runTest(signatures, diagnostics, inspector);
+ }
+
+ private void isOriginUnknown(Origin origin) {
+ assertSame(Origin.unknown(), origin);
+ }
+
+ private void noWarnings(DiagnosticsChecker checker) {
+ assertEquals(0, checker.warnings.size());
+ }
+
+ private void noInspection(DexInspector inspector) {
+ }
+
+ private void noSignatureAttribute(MethodSubject method) {
+ assertThat(method, isPresent());
+ assertNull(method.getFinalSignatureAttribute());
+ assertNull(method.getOriginalSignatureAttribute());
+ }
+
+ @Test
+ public void originalJavacSignatures() throws Exception {
+ // Test using the signatures generated by javac.
+ runTest(ImmutableMap.of(), this::noWarnings, this::noInspection);
+ }
+
+ @Test
+ public void signatureEmpty() throws Exception {
+ testSingleMethod("generic", "", this::noWarnings, inspector -> {
+ noSignatureAttribute(lookupGeneric(inspector));
+ });
+ }
+
+ @Test
+ public void signatureInvalid() throws Exception {
+ testSingleMethod("generic", "X", diagnostics -> {
+ assertEquals(1, diagnostics.warnings.size());
+ DiagnosticsChecker.checkDiagnostic(diagnostics.warnings.get(0), this::isOriginUnknown,
+ "Invalid signature for method",
+ "java.lang.Throwable Methods.generic(java.lang.Throwable, Methods$Inner)",
+ "Expected ( at position 1");
+ }, inspector -> noSignatureAttribute(lookupGeneric(inspector)));
+ }
+
+ @Test
+ public void classNotFound() throws Exception {
+ String signature = "<T:LNotFound;>(TT;LAlsoNotFound<TT;>.InnerNotFound.InnerAlsoNotFound;)TT;";
+ testSingleMethod("generic", signature, this::noWarnings,
+ inspector -> {
+ ClassSubject methods = inspector.clazz("Methods");
+ MethodSubject method =
+ methods.method("java.lang.Throwable", "generic",
+ ImmutableList.of("java.lang.Throwable", "Methods$Inner"));
+ assertThat(inspector.clazz("NotFound"), not(isPresent()));
+ assertEquals(signature, method.getOriginalSignatureAttribute());
+ });
+ }
+
+ @Test
+ public void multipleWarnings() throws Exception {
+ runTest(ImmutableMap.of(
+ "generic", "X",
+ "parameterizedReturn", "X",
+ "parameterizedArguments", "X"
+ ), diagnostics -> {
+ assertEquals(3, diagnostics.warnings.size());
+ }, inspector -> {
+ noSignatureAttribute(lookupGeneric(inspector));
+ noSignatureAttribute(lookupParameterizedReturn(inspector));
+ noSignatureAttribute(lookupParameterizedArguments(inspector));
+ });
+ }
+}
diff --git a/src/test/java/com/android/tools/r8/utils/DexInspector.java b/src/test/java/com/android/tools/r8/utils/DexInspector.java
index e7e882a..fd32ec7 100644
--- a/src/test/java/com/android/tools/r8/utils/DexInspector.java
+++ b/src/test/java/com/android/tools/r8/utils/DexInspector.java
@@ -886,7 +886,31 @@
@Override
public MethodSignature getOriginalSignature() {
MethodSignature signature = getFinalSignature();
- MemberNaming memberNaming = clazz.naming != null ? clazz.naming.lookup(signature) : null;
+ if (clazz.naming == null) {
+ return signature;
+ }
+
+ // Map the parameters and return type to original names. This is needed as the in the
+ // Proguard map the names on the left side are the original names. E.g.
+ //
+ // X -> a
+ // X method(X) -> a
+ //
+ // whereas the final signature is for X.a is "a (a)"
+ String[] OriginalParameters = new String[signature.parameters.length];
+ for (int i = 0; i < OriginalParameters.length; i++) {
+ String obfuscated = signature.parameters[i];
+ String original = originalToObfuscatedMapping.inverse().get(obfuscated);
+ OriginalParameters[i] = original != null ? original : obfuscated;
+ }
+ String obfuscatedReturnType = signature.type;
+ String originalReturnType = originalToObfuscatedMapping.inverse().get(obfuscatedReturnType);
+ String returnType = originalReturnType != null ? originalReturnType : obfuscatedReturnType;
+
+ MethodSignature lookupSignature =
+ new MethodSignature(signature.name, returnType, OriginalParameters);
+
+ MemberNaming memberNaming = clazz.naming.lookup(lookupSignature);
return memberNaming != null
? (MethodSignature) memberNaming.getOriginalSignature()
: signature;
@@ -1035,7 +1059,24 @@
@Override
public FieldSignature getOriginalSignature() {
FieldSignature signature = getFinalSignature();
- MemberNaming memberNaming = clazz.naming != null ? clazz.naming.lookup(signature) : null;
+ if (clazz.naming == null) {
+ return signature;
+ }
+
+ // Map the type to the original name. This is needed as the in the Proguard map the
+ // names on the left side are the original names. E.g.
+ //
+ // X -> a
+ // X field -> a
+ //
+ // whereas the final signature is for X.a is "a a"
+ String obfuscatedType = signature.type;
+ String originalType = originalToObfuscatedMapping.inverse().get(obfuscatedType);
+ String fieldType = originalType != null ? originalType : obfuscatedType;
+
+ FieldSignature lookupSignature = new FieldSignature(signature.name, fieldType);
+
+ MemberNaming memberNaming = clazz.naming.lookup(lookupSignature);
return memberNaming != null
? (FieldSignature) memberNaming.getOriginalSignature()
: signature;
