From 0282663d35ad5ad3e92e6dceaddfe11b886d4ef2 Mon Sep 17 00:00:00 2001 From: Samuel Freilich Date: Fri, 25 Aug 2023 12:12:33 -0400 Subject: [PATCH] Add shrinker-test examples with never-referenced constructors R8 can do some optimizations that assume values of types whose constructors are never referenced are `null`. (A bit confused why the rules here are sufficient to avoid that optimization in the case of a class whose sole constructor has arguments and is unreferenced, since the keep rule only refers to `()` instead of `(...)`. Does R8 have special behavior for `()` where attempting to keep that makes it assume a class is constructable even if it doesn't have a no-args constructor?) Also rename some test classes/names to refer to `NoArgs` instead of `Default` constructors. The examples here care about whether a no-argument constructor is present. While a no-argument constructor (explicitly defined or not) is used by default in some circumstances, the Java documentation consistently uses "default constructor" to refer only to constructors whose implementation is implicitly provided by the complier (all default constructors are no-argument constructors, but not vice versa). --- shrinker-test/common.pro | 2 +- shrinker-test/proguard.pro | 2 +- shrinker-test/r8.pro | 2 +- ....java => ClassWithHasArgsConstructor.java} | 6 +- ...r.java => ClassWithNoArgsConstructor.java} | 4 +- ...assWithUnreferencedHasArgsConstructor.java | 19 ++++++ ...lassWithUnreferencedNoArgsConstructor.java | 18 +++++ .../src/main/java/com/example/Main.java | 65 +++++++++++++++++-- .../com/example/NoSerializedNameMain.java | 14 ++-- .../java/com/google/gson/it/ShrinkingIT.java | 34 ++++++++-- 10 files changed, 137 insertions(+), 29 deletions(-) rename shrinker-test/src/main/java/com/example/{ClassWithoutDefaultConstructor.java => ClassWithHasArgsConstructor.java} (61%) rename shrinker-test/src/main/java/com/example/{ClassWithDefaultConstructor.java => ClassWithNoArgsConstructor.java} (74%) create mode 100644 shrinker-test/src/main/java/com/example/ClassWithUnreferencedHasArgsConstructor.java create mode 100644 shrinker-test/src/main/java/com/example/ClassWithUnreferencedNoArgsConstructor.java diff --git a/shrinker-test/common.pro b/shrinker-test/common.pro index b30faa1329..9211e67460 100644 --- a/shrinker-test/common.pro +++ b/shrinker-test/common.pro @@ -18,7 +18,7 @@ -keep class com.example.NoSerializedNameMain { public static java.lang.String runTest(); public static java.lang.String runTestNoJdkUnsafe(); - public static java.lang.String runTestNoDefaultConstructor(); + public static java.lang.String runTestConstructorHasArgs(); } diff --git a/shrinker-test/proguard.pro b/shrinker-test/proguard.pro index ee96073372..4fe03c845a 100644 --- a/shrinker-test/proguard.pro +++ b/shrinker-test/proguard.pro @@ -12,6 +12,6 @@ -keepclassmembernames class com.example.NoSerializedNameMain$TestClassNotAbstract { ; } --keepclassmembernames class com.example.NoSerializedNameMain$TestClassWithoutDefaultConstructor { +-keepclassmembernames class com.example.NoSerializedNameMain$TestClassConstructorHasArgs { ; } diff --git a/shrinker-test/r8.pro b/shrinker-test/r8.pro index 01b8c84ee2..e916214609 100644 --- a/shrinker-test/r8.pro +++ b/shrinker-test/r8.pro @@ -11,7 +11,7 @@ # Don't obfuscate class name, to check it in exception message -keep,allowshrinking,allowoptimization class com.example.NoSerializedNameMain$TestClass --keep,allowshrinking,allowoptimization class com.example.NoSerializedNameMain$TestClassWithoutDefaultConstructor +-keep,allowshrinking,allowoptimization class com.example.NoSerializedNameMain$TestClassConstructorHasArgs # This rule has the side-effect that R8 still removes the no-args constructor, but does not make the class abstract -keep class com.example.NoSerializedNameMain$TestClassNotAbstract { diff --git a/shrinker-test/src/main/java/com/example/ClassWithoutDefaultConstructor.java b/shrinker-test/src/main/java/com/example/ClassWithHasArgsConstructor.java similarity index 61% rename from shrinker-test/src/main/java/com/example/ClassWithoutDefaultConstructor.java rename to shrinker-test/src/main/java/com/example/ClassWithHasArgsConstructor.java index 2af573677a..c65281e08e 100644 --- a/shrinker-test/src/main/java/com/example/ClassWithoutDefaultConstructor.java +++ b/shrinker-test/src/main/java/com/example/ClassWithHasArgsConstructor.java @@ -3,15 +3,15 @@ import com.google.gson.annotations.SerializedName; /** - * Class without no-args default constructor, but with field annotated with + * Class without no-args constructor, but with field annotated with * {@link SerializedName}. */ -public class ClassWithoutDefaultConstructor { +public class ClassWithHasArgsConstructor { @SerializedName("myField") public int i; // Specify explicit constructor with args to remove implicit no-args default constructor - public ClassWithoutDefaultConstructor(int i) { + public ClassWithHasArgsConstructor(int i) { this.i = i; } } diff --git a/shrinker-test/src/main/java/com/example/ClassWithDefaultConstructor.java b/shrinker-test/src/main/java/com/example/ClassWithNoArgsConstructor.java similarity index 74% rename from shrinker-test/src/main/java/com/example/ClassWithDefaultConstructor.java rename to shrinker-test/src/main/java/com/example/ClassWithNoArgsConstructor.java index 6cff119011..9130aaadc5 100644 --- a/shrinker-test/src/main/java/com/example/ClassWithDefaultConstructor.java +++ b/shrinker-test/src/main/java/com/example/ClassWithNoArgsConstructor.java @@ -6,11 +6,11 @@ * Class with no-args default constructor and with field annotated with * {@link SerializedName}. */ -public class ClassWithDefaultConstructor { +public class ClassWithNoArgsConstructor { @SerializedName("myField") public int i; - public ClassWithDefaultConstructor() { + public ClassWithNoArgsConstructor() { i = -3; } } diff --git a/shrinker-test/src/main/java/com/example/ClassWithUnreferencedHasArgsConstructor.java b/shrinker-test/src/main/java/com/example/ClassWithUnreferencedHasArgsConstructor.java new file mode 100644 index 0000000000..b0178bf613 --- /dev/null +++ b/shrinker-test/src/main/java/com/example/ClassWithUnreferencedHasArgsConstructor.java @@ -0,0 +1,19 @@ +package com.example; + +import com.google.gson.annotations.SerializedName; + +/** + * Class without no-args constructor, but with field annotated with + * {@link SerializedName}. The constructor should not actually be used in the + * code, but this shouldn't lead to R8 concluding that values of the type are + * not constructible and therefore must be null. + */ +public class ClassWithUnreferencedHasArgsConstructor { + @SerializedName("myField") + public int i; + + // Specify explicit constructor with args to remove implicit no-args default constructor + public ClassWithUnreferencedHasArgsConstructor(int i) { + this.i = i; + } +} diff --git a/shrinker-test/src/main/java/com/example/ClassWithUnreferencedNoArgsConstructor.java b/shrinker-test/src/main/java/com/example/ClassWithUnreferencedNoArgsConstructor.java new file mode 100644 index 0000000000..2d43032140 --- /dev/null +++ b/shrinker-test/src/main/java/com/example/ClassWithUnreferencedNoArgsConstructor.java @@ -0,0 +1,18 @@ +package com.example; + +import com.google.gson.annotations.SerializedName; + +/** + * Class with no-args constructor and with field annotated with + * {@link SerializedName}. The constructor should not actually be used in the + * code, but this shouldn't lead to R8 concluding that values of the type are + * not constructible and therefore must be null. + */ +public class ClassWithUnreferencedNoArgsConstructor { + @SerializedName("myField") + public int i; + + public ClassWithUnreferencedNoArgsConstructor() { + i = -3; + } +} diff --git a/shrinker-test/src/main/java/com/example/Main.java b/shrinker-test/src/main/java/com/example/Main.java index 488bc03b82..d2d88021e1 100644 --- a/shrinker-test/src/main/java/com/example/Main.java +++ b/shrinker-test/src/main/java/com/example/Main.java @@ -32,7 +32,11 @@ public static void runTests(BiConsumer outputConsumer) { testNamedFields(outputConsumer); testSerializedName(outputConsumer); - testNoDefaultConstructor(outputConsumer); + testConstructorNoArgs(outputConsumer); + testConstructorHasArgs(outputConsumer); + testUnreferencedConstructorNoArgs(outputConsumer); + testUnreferencedConstructorHasArgs(outputConsumer); + testNoJdkUnsafe(outputConsumer); testEnum(outputConsumer); @@ -90,12 +94,57 @@ private static void testSerializedName(BiConsumer outputConsumer }); } - private static void testNoDefaultConstructor(BiConsumer outputConsumer) { + private static void testConstructorNoArgs(BiConsumer outputConsumer) { + Gson gson = new GsonBuilder().setPrettyPrinting().create(); + TestExecutor.run(outputConsumer, "Write: No args constructor", () -> toJson( + gson, new ClassWithNoArgsConstructor())); + TestExecutor.run(outputConsumer, "Read: No args constructor; initial constructor value", () -> { + ClassWithNoArgsConstructor deserialized = fromJson(gson, "{}", ClassWithNoArgsConstructor.class); + return Integer.toString(deserialized.i); + }); + TestExecutor.run(outputConsumer, "Read: No args constructor; custom value", () -> { + ClassWithNoArgsConstructor deserialized = fromJson(gson, "{\"myField\": 3}", ClassWithNoArgsConstructor.class); + return Integer.toString(deserialized.i); + }); + } + + private static void testConstructorHasArgs(BiConsumer outputConsumer) { Gson gson = new GsonBuilder().setPrettyPrinting().create(); - TestExecutor.run(outputConsumer, "Write: No default constructor", () -> toJson(gson, new ClassWithoutDefaultConstructor(2))); + TestExecutor.run(outputConsumer, "Write: Constructor with args", () -> toJson( + gson, new ClassWithHasArgsConstructor(2))); + // This most likely relies on JDK Unsafe (unless the shrinker rewrites the constructor in some way) + TestExecutor.run(outputConsumer, "Read: Constructor with args", () -> { + ClassWithHasArgsConstructor deserialized = fromJson( + gson, "{\"myField\": 3}", ClassWithHasArgsConstructor.class); + return Integer.toString(deserialized.i); + }); + } + + private static void testUnreferencedConstructorNoArgs(BiConsumer outputConsumer) { + Gson gson = new GsonBuilder().setPrettyPrinting().create(); + // No write because we're not referencing this class's constructor. + + // This runs the no-args constructor. + TestExecutor.run(outputConsumer, "Read: Unreferenced no args constructor; initial constructor value", () -> { + ClassWithUnreferencedNoArgsConstructor deserialized = fromJson( + gson, "{}", ClassWithUnreferencedNoArgsConstructor.class); + return Integer.toString(deserialized.i); + }); + TestExecutor.run(outputConsumer, "Read: Unreferenced no args constructor; custom value", () -> { + ClassWithUnreferencedNoArgsConstructor deserialized = fromJson( + gson, "{\"myField\": 3}", ClassWithUnreferencedNoArgsConstructor.class); + return Integer.toString(deserialized.i); + }); + } + + private static void testUnreferencedConstructorHasArgs(BiConsumer outputConsumer) { + Gson gson = new GsonBuilder().setPrettyPrinting().create(); + // No write because we're not referencing this class's constructor. + // This most likely relies on JDK Unsafe (unless the shrinker rewrites the constructor in some way) - TestExecutor.run(outputConsumer, "Read: No default constructor", () -> { - ClassWithoutDefaultConstructor deserialized = fromJson(gson, "{\"myField\": 3}", ClassWithoutDefaultConstructor.class); + TestExecutor.run(outputConsumer, "Read: Unreferenced constructor with args", () -> { + ClassWithUnreferencedHasArgsConstructor deserialized = fromJson( + gson, "{\"myField\": 3}", ClassWithUnreferencedHasArgsConstructor.class); return Integer.toString(deserialized.i); }); } @@ -103,11 +152,13 @@ private static void testNoDefaultConstructor(BiConsumer outputCo private static void testNoJdkUnsafe(BiConsumer outputConsumer) { Gson gson = new GsonBuilder().disableJdkUnsafe().create(); TestExecutor.run(outputConsumer, "Read: No JDK Unsafe; initial constructor value", () -> { - ClassWithDefaultConstructor deserialized = fromJson(gson, "{}", ClassWithDefaultConstructor.class); + ClassWithNoArgsConstructor deserialized = fromJson( + gson, "{}", ClassWithNoArgsConstructor.class); return Integer.toString(deserialized.i); }); TestExecutor.run(outputConsumer, "Read: No JDK Unsafe; custom value", () -> { - ClassWithDefaultConstructor deserialized = fromJson(gson, "{\"myField\": 3}", ClassWithDefaultConstructor.class); + ClassWithNoArgsConstructor deserialized = fromJson( + gson, "{\"myField\": 3}", ClassWithNoArgsConstructor.class); return Integer.toString(deserialized.i); }); } diff --git a/shrinker-test/src/main/java/com/example/NoSerializedNameMain.java b/shrinker-test/src/main/java/com/example/NoSerializedNameMain.java index cd70af34a7..2bf30f61b2 100644 --- a/shrinker-test/src/main/java/com/example/NoSerializedNameMain.java +++ b/shrinker-test/src/main/java/com/example/NoSerializedNameMain.java @@ -19,17 +19,17 @@ static class TestClassNotAbstract { public String s; } - static class TestClassWithoutDefaultConstructor { + static class TestClassConstructorHasArgs { public String s; // Specify explicit constructor with args to remove implicit no-args default constructor - public TestClassWithoutDefaultConstructor(String s) { + public TestClassConstructorHasArgs(String s) { this.s = s; } } /** - * Main entrypoint, called by {@code ShrinkingIT.testNoSerializedName_DefaultConstructor()}. + * Main entrypoint, called by {@code ShrinkingIT.testNoSerializedName_ConstructorHasArgs()}. */ public static String runTest() { TestClass deserialized = new Gson().fromJson("{\"s\":\"value\"}", same(TestClass.class)); @@ -37,7 +37,7 @@ public static String runTest() { } /** - * Main entrypoint, called by {@code ShrinkingIT.testNoSerializedName_DefaultConstructorNoJdkUnsafe()}. + * Main entrypoint, called by {@code ShrinkingIT.testNoSerializedName_NoArgsConstructorNoJdkUnsafe()}. */ public static String runTestNoJdkUnsafe() { Gson gson = new GsonBuilder().disableJdkUnsafe().create(); @@ -46,10 +46,10 @@ public static String runTestNoJdkUnsafe() { } /** - * Main entrypoint, called by {@code ShrinkingIT.testNoSerializedName_NoDefaultConstructor()}. + * Main entrypoint, called by {@code ShrinkingIT.testNoSerializedName_ConstructorHasArgs()}. */ - public static String runTestNoDefaultConstructor() { - TestClassWithoutDefaultConstructor deserialized = new Gson().fromJson("{\"s\":\"value\"}", same(TestClassWithoutDefaultConstructor.class)); + public static String runTestConstructorHasArgs() { + TestClassConstructorHasArgs deserialized = new Gson().fromJson("{\"s\":\"value\"}", same(TestClassConstructorHasArgs.class)); return deserialized.s; } } diff --git a/shrinker-test/src/test/java/com/google/gson/it/ShrinkingIT.java b/shrinker-test/src/test/java/com/google/gson/it/ShrinkingIT.java index be5a6213f7..bfe32ada81 100644 --- a/shrinker-test/src/test/java/com/google/gson/it/ShrinkingIT.java +++ b/shrinker-test/src/test/java/com/google/gson/it/ShrinkingIT.java @@ -129,12 +129,32 @@ public void test() throws Exception { "Read: SerializedName", "3", "===", - "Write: No default constructor", + "Write: No args constructor", + "{", + " \"myField\": -3", + "}", + "===", + "Read: No args constructor; initial constructor value", + "-3", + "===", + "Read: No args constructor; custom value", + "3", + "===", + "Write: Constructor with args", "{", " \"myField\": 2", "}", "===", - "Read: No default constructor", + "Read: Constructor with args", + "3", + "===", + "Read: Unreferenced no args constructor; initial constructor value", + "-3", + "===", + "Read: Unreferenced no args constructor; custom value", + "3", + "===", + "Read: Unreferenced constructor with args", "3", "===", "Read: No JDK Unsafe; initial constructor value", @@ -191,7 +211,7 @@ public void test() throws Exception { } @Test - public void testNoSerializedName_DefaultConstructor() throws Exception { + public void testNoSerializedName_NoArgsConstructor() throws Exception { runTest("com.example.NoSerializedNameMain", c -> { Method m = c.getMethod("runTest"); @@ -211,7 +231,7 @@ public void testNoSerializedName_DefaultConstructor() throws Exception { } @Test - public void testNoSerializedName_DefaultConstructorNoJdkUnsafe() throws Exception { + public void testNoSerializedName_NoArgsConstructorNoJdkUnsafe() throws Exception { runTest("com.example.NoSerializedNameMain", c -> { Method m = c.getMethod("runTestNoJdkUnsafe"); @@ -232,9 +252,9 @@ public void testNoSerializedName_DefaultConstructorNoJdkUnsafe() throws Exceptio } @Test - public void testNoSerializedName_NoDefaultConstructor() throws Exception { + public void testNoSerializedName_ConstructorHasArgs() throws Exception { runTest("com.example.NoSerializedNameMain", c -> { - Method m = c.getMethod("runTestNoDefaultConstructor"); + Method m = c.getMethod("runTestConstructorHasArgs"); if (jarToTest.equals(PROGUARD_RESULT_PATH)) { Object result = m.invoke(null); @@ -244,7 +264,7 @@ public void testNoSerializedName_NoDefaultConstructor() throws Exception { Exception e = assertThrows(InvocationTargetException.class, () -> m.invoke(null)); assertThat(e).hasCauseThat().hasMessageThat().isEqualTo( "Abstract classes can't be instantiated! Adjust the R8 configuration or register an InstanceCreator" - + " or a TypeAdapter for this type. Class name: com.example.NoSerializedNameMain$TestClassWithoutDefaultConstructor" + + " or a TypeAdapter for this type. Class name: com.example.NoSerializedNameMain$TestClassConstructorHasArgs" + "\nSee https://github.com/google/gson/blob/main/Troubleshooting.md#r8-abstract-class" ); }