Implement reflective support for Java Records #1381

ZacSweers · 2021-08-20T20:36:29Z

Porting from https://github.com/ZacSweers/MoshiX/tree/main/moshi-records-reflect

This makes a few changes along the way to make the build all happy with how we're doing this:

Standardizing around Java 8 (which is the minimum supported by gradle toolchains and also soon to be the minimum supported by Kotlin)
Create a separate Java 16 source set for hosting the actual adapter implementation. This is compiled with Java 16 but is then packaged into the main Moshi jar and denoted as a Multi-Release JAR.
We gate access to this record adapter at runtime by checking if the requisite record APIs are available.
We separately have a shim copy of the record adapter in the main source set for linking in BUILT_IN_FACTORIES, but this is booted out by the real one during packaging.
Replace animal sniffer with -release 8, which functionally accomplishes the same
Tests are in a dedicated separate records-test subproject, making it easier to both write new ones and also ensure our packaging rules for the jar is working correctly (we've had success with this in the past with the codegen artifact's shading being tested in a dedicated tests project)

Resolves #1278

Tests are a little skimpy right now, happy to take suggestions on more we should add! Can also look at fleshing them out more in a followup PR

ZacSweers · 2021-08-20T20:53:47Z

It looks like animal sniffer ships a shaded old version of ASM and is also no longer being updated since the -release flag covers the same on JDK 9+. Thing is we want to support 8, so we can't use it yet. Seems like we'll need to drop animal sniffer if we want to proceed. Thoughts? Edit I believe we can just use a newer JDK to compile and set an older release flag, will update

moshi/build.gradle.kts

JakeWharton

Where do you actually pack the MR jar? I don't see any references to META-INF/versions/16/

moshi/src/main/java/com/squareup/moshi/internal/Util.java

JakeWharton · 2021-08-21T02:10:23Z

moshi/src/main/java16/com/squareup/moshi/RecordJsonAdapter.java

+    Constructor<Object> constructor;
+    try {
+      //noinspection unchecked
+      constructor = (Constructor<Object>) rawType.getDeclaredConstructor(constructorParams);


Is there no way to get a MethodHandle from the Class once we know it's a record? Can't believe they expect you to use ancient java.lang.reflect reflection to programmatically instantiate these...

Honestly never used that API, let me look

Good news is we can use MethodHandle for this but bad news is RecordComponent itself still returns only Method for its accessors :/

Constructor bit is in 355422c

I also wrapped the accessor Method with one too, but not sure (lack of familiarity) if this is worthwhile 4700a63. Let me know

moshi/src/main/java16/com/squareup/moshi/RecordJsonAdapter.java

build.gradle.kts

moshi/src/main/java16/com/squareup/moshi/RecordJsonAdapter.java

ZacSweers · 2021-08-21T03:02:11Z

Where do you actually pack the MR jar? I don't see any references to META-INF/versions/16/

Yup removed! I misunderstood the requirements for this. This basically just packages it like a simple jar and gates java 16 code on the Util check

JakeWharton · 2021-08-21T11:58:28Z

No MR jar probably means tools like Graal and D8 (and others) will choke processing the jar.

Instead of a gated use and stub you can make the stub return null from a factory method and make the 16 class return a real implementation. Then you unconditionally call the factory and gate behavior on non-null values.

ZacSweers · 2021-08-21T17:18:16Z

ah good idea. Thanks for bearing with me, learning something new with these MR jars!

This reverts commit 3eb768f.

ZacSweers · 2021-08-21T17:27:52Z

Ok think I've got it working now. This is neat!

We can just enable this everywhere now since we require JDK 16 anyway

ZacSweers · 2021-08-21T17:47:49Z

Being reminded how much the java module system sucks when you have to configure stuff

Gotta be public

Not a regression test in this case

swankjesse · 2021-12-04T14:32:59Z

.github/workflows/build.yml

-          - 13
-          - 14
-          - 15
+          - 16


Not testing on Java 8 is annoying. Something to look into separately from this change.

We can make the main tests run with java 8 via Gradle toolchains

swankjesse · 2021-12-04T14:33:49Z

build.gradle.kts

-    dependencies {
-      "compileOnly"(Dependencies.AnimalSniffer.annotations)
-      "signature"(Dependencies.AnimalSniffer.java7Signature)
+    if (project.name != "records-tests") {


swankjesse · 2021-12-04T14:37:23Z

gradle.properties

@@ -15,9 +15,36 @@
 #

 # For Dokka https://github.com/Kotlin/dokka/issues/1405
-org.gradle.jvmargs=-XX:MaxMetaspaceSize=512m
+org.gradle.jvmargs=-Xmx2048m -Dfile.encoding=UTF-8 \
+  --add-opens=jdk.compiler/com.sun.tools.javac.util=ALL-UNNAMED \


These are ... unfortunate. The Dokka issue above seems unrelated. I think this is the tracking issue.
https://youtrack.jetbrains.com/issue/KT-45545

The kapt parts are no longer relevant or on master now at least with Kotlin 1.6

swankjesse · 2021-12-04T14:37:48Z

kotlin/codegen/build.gradle.kts

-  sourceCompatibility = JavaVersion.VERSION_1_8
-  targetCompatibility = JavaVersion.VERSION_1_8
+tasks.withType<Test>().configureEach {
+  // For kapt to work with kotlin-compile-testing


Tracking here: https://youtrack.jetbrains.com/issue/KT-45545

swankjesse · 2021-12-04T14:38:29Z

moshi/build.gradle.kts

+}
+
+val mainSourceSet by sourceSets.named("main")
+val java16 by sourceSets.creating {


swankjesse · 2021-12-04T14:39:54Z

moshi/build.gradle.kts

@@ -35,6 +70,8 @@ tasks.withType<KotlinCompile>()
  }

 dependencies {
+  // So the j16 source set can "see" main Moshi sources
+  "java16Implementation"(mainSourceSet.output)


off topic: this syntax offends me so much

swankjesse · 2021-12-04T14:41:25Z

moshi/records-tests/src/test/java/com/squareup/moshi/records/RecordsTest.java

+  }
+
+  @Test
+  public void qualifiedValues() throws IOException {


swankjesse · 2021-12-04T14:42:06Z

moshi/src/main/java/com/squareup/moshi/RecordJsonAdapter.java

+import javax.annotation.Nullable;
+
+/**
+ * This is just a simple shim for linking in {@link StandardJsonAdapters} and swapped with a real


I really like how this plugs in

Feels very reminiscent of expect/actual

swankjesse · 2021-12-04T14:43:33Z

moshi/src/main/java16/com/squareup/moshi/RecordJsonAdapter.java

+        Map<String, Type> mappedTypeArgs = null;
+        if (type instanceof ParameterizedType parameterizedType) {
+          Type[] typeArgs = parameterizedType.getActualTypeArguments();
+          var typeVars = rawType.getTypeParameters();


var! (My first time seeing this in real life)

swankjesse · 2021-12-04T14:47:22Z

moshi/src/main/java16/com/squareup/moshi/RecordJsonAdapter.java

+        }
+
+        Map<String, Type> mappedTypeArgs = null;
+        if (type instanceof ParameterizedType parameterizedType) {


This reads to me as very similar to Util.resolve(). Presumably we can’t use that here because this needs more records stuff? Either way this behavior really wants to be extracted into a separately-testable thing

Fix: https://github.com/square/moshi/pull/1442/files

swankjesse · 2021-12-04T14:48:17Z

moshi/src/main/java16/com/squareup/moshi/RecordJsonAdapter.java

+        return new RecordJsonAdapter<>(constructor, rawType.getSimpleName(), bindings).nullSafe();
+      };
+
+  private static record ComponentBinding<T>(


yeah I think the static is implicit but the IDE didn't give me a warning about it being redundant

swankjesse · 2021-12-04T14:48:48Z

moshi/src/main/java16/com/squareup/moshi/RecordJsonAdapter.java

+  private final String targetClass;
+  private final MethodHandle constructor;
+  private final ComponentBinding<Object>[] componentBindingsArray;
+  private final JsonReader.Options options;


Yay options

swankjesse

I think I’d like to do a follow-up to this to test its internal components, though that shouldn’t block release.

ZacSweers added 10 commits August 20, 2021 16:17

Standardize around JDK 8

fba0e63

Update GJF to support newer JDKs

8ea1047

Fix misc java 8 issues in tests

2e21d08

Prepare java 16/records checking at runtime

261c263

Implement real RecordJsonAdapter

25c4c1c

Spotless

2c4f98f

Prepare build for JDK 16+

055257d

Fix property name for kapt

5f30573

Small cleanup

10e5d0f

Make FallbackEnum java-8 happy

7d1c602

ZacSweers requested review from swankjesse and rharter August 20, 2021 20:36

melix reviewed Aug 20, 2021

View reviewed changes

moshi/build.gradle.kts Outdated Show resolved Hide resolved

ZacSweers added 14 commits August 20, 2021 17:44

Remove animalsniffer

cc6d1b6

Fix format

5f851ea

Add opens for ExtendsPlatformClassWithProtectedFields

5841cfc

Return null every time in shim for main tests

f8391bd

Use JDK 16 + release 8 to replace animalsniffer

bd0a70f

Simplify accessor accessible handling

0f94869

Remove manifest attrs

3eb768f

Fix typo

9913e17

Fix KCT tests + upgrade it

76a6bb8

Cover another

6668795

Try explicit kotlin daemon args for java 17?

c49b119

Disable 17-ea for now until kotlin 1.5.30

360179f

Add JsonQualifier and Json(name) tests + fix qualifiers

ef41c63

Ensure constructor is accessible

68fcc68

JakeWharton reviewed Aug 21, 2021

View reviewed changes

ZacSweers added 3 commits August 20, 2021 23:16

Unwrap InvocationTargetException

765a642

Use MethodHandle for constructor

355422c

Use MethodHandle for accessor too

4700a63

ZacSweers added 2 commits August 21, 2021 13:19

Revert "Remove manifest attrs"

b89f722

This reverts commit 3eb768f.

Proper MR jar

5906d57

ZacSweers added 2 commits August 21, 2021 13:40

*actually* fix GJF, which wasn't getting applied before

d025c15

We can just enable this everywhere now since we require JDK 16 anyway

Make IDE happy about modules access

3863c9b

ZacSweers added 3 commits August 21, 2021 13:58

Fixup records tests to play nice with modules

5d74681

Gotta be public

Add complex smoke test

82d22e9

Remove comment

c68005f

Not a regression test in this case

JakeWharton approved these changes Aug 23, 2021

View reviewed changes

ZacSweers merged commit 95250b0 into square:master Aug 23, 2021

ZacSweers deleted the z/records branch August 23, 2021 16:00

swankjesse reviewed Dec 4, 2021

View reviewed changes

moshi/build.gradle.kts

}

val mainSourceSet by sourceSets.named("main")

val java16 by sourceSets.creating {

Copy link

Member

swankjesse Dec 4, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

neat

swankjesse reviewed Dec 4, 2021

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement reflective support for Java Records #1381

Implement reflective support for Java Records #1381

ZacSweers commented Aug 20, 2021 •

edited

Loading

ZacSweers commented Aug 20, 2021 •

edited

Loading

JakeWharton left a comment

JakeWharton Aug 21, 2021

ZacSweers Aug 21, 2021

ZacSweers Aug 21, 2021

ZacSweers Aug 21, 2021

ZacSweers commented Aug 21, 2021 •

edited

Loading

JakeWharton commented Aug 21, 2021

ZacSweers commented Aug 21, 2021

ZacSweers commented Aug 21, 2021

ZacSweers commented Aug 21, 2021

swankjesse Dec 4, 2021

ZacSweers Dec 4, 2021

swankjesse Dec 4, 2021

swankjesse Dec 4, 2021

ZacSweers Dec 4, 2021

swankjesse Dec 4, 2021

swankjesse Dec 4, 2021

swankjesse Dec 4, 2021

swankjesse Dec 4, 2021

swankjesse Dec 4, 2021

swankjesse Dec 4, 2021

swankjesse Dec 4, 2021

swankjesse Dec 4, 2021

swankjesse Dec 4, 2021

swankjesse Dec 4, 2021

ZacSweers Dec 4, 2021

swankjesse Dec 4, 2021

swankjesse left a comment

Implement reflective support for Java Records #1381

Implement reflective support for Java Records #1381

Conversation

ZacSweers commented Aug 20, 2021 • edited Loading

ZacSweers commented Aug 20, 2021 • edited Loading

JakeWharton left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ZacSweers commented Aug 21, 2021 • edited Loading

JakeWharton commented Aug 21, 2021

ZacSweers commented Aug 21, 2021

ZacSweers commented Aug 21, 2021

ZacSweers commented Aug 21, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

swankjesse left a comment

Choose a reason for hiding this comment

ZacSweers commented Aug 20, 2021 •

edited

Loading

ZacSweers commented Aug 20, 2021 •

edited

Loading

ZacSweers commented Aug 21, 2021 •

edited

Loading