Follow-up for `java.time` adapter changes by Marcono1234 · Pull Request #2972 · google/gson

Marcono1234 · 2026-01-13T12:07:19Z

Follow-up for #2948

internal Javadoc additions and changes
moved java.time tests to separate test class
added additional Maven Surefire Plugin execution which runs java.time test with --add-opens
some small code changes

(see also my GitHub review comments in the changed code below)

If you think some of this is not needed or want something changes, please let me know.

eamonnmcmanus

Thanks, this is great! Just a few small things.

Marcono1234 · 2026-01-13T20:52:19Z

+  /**
+   * Adapter factory for {@code java.time} classes. Returns {@code null} if not supported by the
+   * current environment (e.g. too old Android version, without desugaring).
+   */


Actually, maybe this comment I added is incorrect? When java.time is unavailable, I am not sure if trying to load JavaTimeTypeAdapters and accessing the factory would really trigger reflection exceptions. Maybe those would only occur on usage of the factory (which would be bad) or for all non-java.time classes the getName().startsWith("java.time.") would not succeed and the factory returns null (which would be fine).

Do you have an internal test which covers this, and know how this behaves?
(I will see if I can also test this with a dummy Android project)

That's an excellent question. I don't see anything in JavaTimeTypeAdapters that would absolutely force an attempt to resolve the java.time classes when the class is loaded or instantiated. We could maybe do something like have an unused private static final field that is initialized to an Instant. That would be guaranteed to fail during class loading, if there is no java.time. On the other hand, it would also force Instant and anything it references to be loaded even in code that doesn't otherwise use that class. Maybe DateTimeException would be better, since it's just a plain exception class with no dependencies.

I ended up excluding JavaTimeTypeAdapters from the internal version of Gson that is used for Android, because of minSdk errors. So in that context, the Class.forName gets a straight-up ClassNotFoundException. However, before I did that I did not see test failures that would consistent with failing when the factory is used. To be honest I'm more afraid that people will get errors when trying to package applications that target an API level before 33.

Have been testing it a bit in Android Studio with emulated Android devices (API Levels 25 & 33), and with API desugaring enabled and disabled.

It seems the current implementation does not throw any unexpected exceptions in any of these cases.

However, when API desugaring is enabled (regardless of API Level on the device) the problem is that the package name check fails because the desugared classes are named for example j$.time.Instant, so it uses the reflection-based adapter. That was already @cpovirk's concern in #2948 (comment).

What seems to work is obtaining the package name from one of the classes though (it seems desugaring is also applied to the used libraries, in this case Gson). I have adjusted the implementation accordingly. Is that ok?

I am not that familiar with Android development though; hopefully I did not miss anything or my test setup was incorrect.

I guess I'm at least a little nervous about DateTimeException.class.getName(): We catch the resulting exception, but my recollection is that Android likes to noisily log missing classes in at last some cases (google/guava#5693), and I could imagine that users might see errors at build time that they have to suppress (though maybe we can avoid that with changes to our Proguard config?).

As a practical matter, I wonder how many users are affected, especially since many apps require API Level 26+ nowadays, and whether the ones that are affected could be handled well enough by hard-coding j$.time.

I'm not saying that we definitely shouldn't do this, just that I'm still finding it hard to get excited to think hard enough to be confident in, sorry :\

Disclaimer: I am not that familiar with Android, and hadn't experienced these noise logging so far if I remember correctly. So maybe the following assumptions are incorrect.

We catch the resulting exception, but my recollection is that Android likes to noisily log missing classes in at last some cases

Is this specific to DateTimeException.class.getName(), or would that be a general issue for the complete JavaTimeTypeAdapters class, since if java.time is unavailable it would be accessing at least one missing class either way before it fails?

and I could imagine that users might see errors at build time

But for issues at build time that would apply to all java.time access then, not just this specific DateTimeException.class.getName(), wouldn't it?

I am also fine with adjusting the check, probably to cover the hardcoded j$.time then? Because otherwise (based on my previous testing mentioned above) it would use the reflective adapter for those desugared j$.time classes.

Without the DateTimeException access I will probably have to verify though that if java.time is completely missing (not even desugared), that JavaTimeTypeAdapters still fails during construction and not uncaught during usage.

But even without the DateTimeException.class.getName(), won't there be some form of direct access anyway? JavaTimeTypeAdapters directly imports and uses the java.time classes (regardless of my changes here), and relies on the call in TypeAdapters to handle the ReflectiveOperationException | LinkageError.

Hmm, yes, thanks again. I think the most correct way forward is to first call something like Class.forName("java.time.DateTimeException") (and handwave prior(?) handling of j$.time) so that any missing-class-related failures happen for the specific class that we're looking up. I think that will avoid any logging, and it will avoid needing to catch Error, which is always a little icky to be catching. (I'm OK with still keeping the broad catch block when we subsequently look up JavaTimeTypeAdapters itself, even though I'd expect the catch to be unnecessary at that point except to satisfy the compiler.)

Checking for both packages names would certainly be doable; but installing adapters for both seems difficult. Currently JavaTimeTypeAdapters directly uses the java.time types; if we wanted to support both package names, we would have to completely rewrite JavaTimeTypeAdapters to only use reflection. Not sure if this is worth it (or even needed).
...

Sounds reasonable to me; D8 processes the complete app, including its dependencies, so I guess it will desugar everything in that case. Not sure though what happens when using Android API which returns java.time (outside the java.time package and non-desugared), or what happens when apps communicate with each other and send java.time types (in case that is possible).
...

We can 'prefer' it only in the sense that the packageName check recognizes both; but adjusting the adapters to somehow support java.time and j$.time at the same time would require large refactoring I think (and I am not sure if it is worth it).

Ah, right. Hopefully the right thing will happen with GSON and the rest of the app agreeing on whether to use java.time or j$.time. I guess that your point implies that it doesn't matter which type we check for reflectively first: The actual adapters will always use whichever version D8 decided upon, so all the reflection can do (and all that it needs to do) is to keep us from proceeding on to blow up when neither is present.

I mean depending on how JavaTimeTypeAdapters is refactored the LinkageError might not happen on creation of JavaTimeTypeAdapters anymore (which is currently expected) but instead later uncaught when JavaTimeTypeAdapters's factory is erroneously used despite java.time being unsupported. (Not sure also how the current main code behaves; maybe it just works because JavaTimeTypeAdapters by happenstance eagerly loads some of the java.time classes.)

Got it. My hope is that the eager Class.forName("java.time.DateTimeException") check in my new proposal will accomplish that. I'm not 100% confident, but I'd feel comfortable going with it.

Have been testing the Gson 2.14.0 implementation a bit with an emulated Android device with API Level 25:
That implementation does not cause a LinkageError in any case, not even the 'expected' one without desugaring when java.time classes are missing. The JavaTimeTypeAdapters.JAVA_TIME_FACTORY is created without issues, and its create returns fast due to the rawType.getName().startsWith("java.time.") check. You can trigger a LinkageError when you define a custom class in the java.time package (e.g. java.time.DummyClass) and then try to use that with Gson; then it will try to load the non-existent java.time classes. But I doubt that anyone would do that.

On a newer emulated Android device (with java.time) I was also able to obtain a java.time class using Class.forName, while the app (and the included Gson code) used the desugared j$.time. In that case Gson did not recognize the java.time class.
Though I guess if an app deals with both j$.time and java.time at the same time, then issues with Gson aren't its only (and biggest) problems.

For caught LinkageError I did not notice any logging in Logcat, but maybe I did not look in the right places, or it is device dependent?

My hope is that the eager Class.forName("java.time.DateTimeException") check in my new proposal will accomplish that.

It will need an additional / fallback check for "j$.time.DateTimeException" as well then though, because D8 doesn't seem to adjust these string literals (even when directly used for Class.forName).

It might also be a bit risky, because Gson doesn't directly refer to DateTimeException then, and I am not sure if D8 could omit the desugared class then. (Though currently Gson calls methods which have throws DateTimeException, so hopefully that ensures the class is included as well.)

Maybe a better approach, without all of this class checking and exception catching would be:

move all the actual implementation and all access to java.time classes into a static nested class JavaTimeTypeAdapters.Impl or similar

have JavaTimeTypeAdapters always provide a successful factory, whose create method looks like this:
@Override public <T> TypeAdapter<T> create(Gson gson, TypeToken<T> typeToken) { Class<? super T> rawType = typeToken.getRawType(); String className = rawType.getName(); if (!className.startsWith("java.time.") && !className.startsWith("j$.time.")) { return null; } return Impl.create(rawType); }

I hope (?) Impl is then really only loaded if the class is from java.time or j$.time.

What do you think?

Have implemented this nested Impl class approach now in b246b10 (disable whitespace diff or use a local diff viewer to better see the changes), please let me know what you think. If you don't think this is a good approach, I can revert it again.

I like the new approach!

I had overlooked that the if protects us even in the existing approach. Thanks for explaining that.

GitHub doesn't present a good diff regardless because of the change in file name. (Maybe it would do better if I were to opt in to the new review UI? Presumably Git itself will have its move detection kick in, helping tools (including GitHub) after the fact.) I was able to review it easily enough from the command line.

I belatedly looked into the logcat output that I'd been referring to. It looks like it might happen only on attempts after the first. The repro that I put together (on an API Level 24 emulator) produces a message of "Rejecting re-init on previously-failed class java.lang.Class<com.google.common.base.AsciiTest$1>: java.lang.NoClassDefFoundError: Failed resolution of: Ljava/lang/ClassValue;" from this ART code, which is still present at head. But none of that should matter, given your efforts to avoid triggering initialization of java.time classes at all.

I had overlooked that the if protects us even in the existing approach. Thanks for explaining that.

(Assuming you mean the existing if (!rawType.getName().startsWith("java.time.")))

It is not really an explanation though, rather an observation 😅
Potentially this class loading behavior differs also between Android versions or devices?
But either way, it seems to differ from JVM behavior as mentioned in #2972 (comment); otherwise I assume the (expected) LinkageError would have occurred directly on Android when JavaTimeTypeAdapters was loaded.

I belatedly looked into the logcat output that I'd been referring to. It looks like it might happen only on attempts after the first.

Ah, thanks for testing that. During my manual testing I only triggered the LinkageError once I think.

Marcono1234 · 2026-01-18T02:46:05Z

+      } else if (rawType == ZoneId.class || rawType == ZoneOffset.class) {
+        // We don't check ZoneId.class.isAssignableFrom(rawType) because we don't want to match
+        // the non-public class ZoneRegion in the runtime type check in
+        // TypeAdapterRuntimeTypeWrapper.write. If we did, then our ZONE_ID would take
+        // precedence over a ZoneId adapter that the user might have registered. (This exact
+        // situation showed up in a Google-internal test.)
+        adapter = ZONE_ID;


This behavior seems to be a bit problematic:

it seems this use case only works if reflective access is permitted to java.time
For example, I added the test testZoneRegionCustomAdapter which tries to replicate this use case, but TypeAdapterRuntimeTypeWrapper tries to retrieve the adapter for the runtime type ZoneRegion. And that is the reflection-based one, for which construction fails if the fields of the class are inaccessible, even if TypeAdapterRuntimeTypeWrapper would discard that one in the end and prefer the adapter for the compile-time type.

when the base class ZoneId is not involved at all, e.g. when doing gson.toJson(obj) or when serializing a List<Object>, then it will not use the built-in adapter at all

Maybe one solution could be to add an additional ZoneId.class.isAssignableFrom(rawType) branch here and in that case check gson.getAdapter(ZoneId.class) == ZONE_ID¹ (that is, the user has not registered a custom adapter for ZoneId) and only if that is true return the ZONE_ID adapter.
(might only solve point 2 though)

What do you think? Or should that be deferred until we get feedback from users?

Footnotes

And probably cache the result if that is possible and not too risky. If there is only a single JavaTimeTypeAdapters.AdapterFactory for all Gson instances (or if that might be the case in the future after further refactoring), then this is not possible. ↩

I remember having quite a lot of trouble with ZoneId and ZoneOffset, though I don't remember the exact details. If the tests still pass then this change preserves the behaviour I settled on. I can well believe that that can be improved, but I'd suggest doing that in a separate change.

Marcono1234 · 2026-01-18T02:55:13Z

+      // Use arbitrary java.time.* class here, one which is quite simple and does not refer to many
+      // other classes
+      String className = DateTimeException.class.getName();
+      int packageEnd = className.lastIndexOf('.');
+      return className.substring(0, packageEnd + 1);


Maybe one risk here could be that R8 additionally renames the desugared class names, but it seems there is built-in configuration to preserve the desugared class names.

And even if R8 renamed the classes, maybe at worst this factory would not recognize java.time classes again and use the reflection-based adapter I assume.

I assume R8 also cannot just omit DateTimeException (which is used above to look up the package name) from the app, causing a LinkageError here, if DateTimeException.class is referenced here within the code.

Marcono1234 · 2026-01-18T13:20:12Z

-  // if we have a ZoneOffset. When reading, we need to construct the the appropriate thing depending
+  // if we have a ZoneOffset. When reading, we need to construct the appropriate thing depending
  // on which of those two fields we see.
  private static final TypeAdapter<ZoneId> ZONE_ID =


When running java -verbose:class ... respectively setting breakpoints in the JDK class loaders, it seems this constant here causes the classes ZoneId and ZoneOffset (and its superinterfaces TemporalAccessor and TemporalAdjuster) to be loaded (but not initialized), when JavaTimeTypeAdapters is initialized, even if no java.time. class is used afterwards.

If this redundant class loading is an issue, then maybe we should move all these adapter constants into a dedicated nested Adapters class or similar, to delay their initialization?

On the other hand, if generally the loading of all the referenced java.time. classes here is not considered an issue, then we could consider completely removing the package name check. This might make the implementation more robust in the context of Android API desugaring.

(Not sure why this only affects this ZONE_ID adapter and not any of the others, such as MONTH_DAY.)

Thank you for all the testing!

I haven't taken a real look at this, but from what I've read in your messages, I have a suspicion for why the loading happens only for the zone: It's likely related to class verification. If our code uses a ZoneOffset as a ZoneId somewhere, then the JVM's verifier needs to load both classes to check whether that's valid. Contrast that to types like Duration, where subtyping doesn't enter into the picture. I'd have to actually look, though, and I haven't thought about the larger picture.

eamonnmcmanus

I'm testing this against Google's internal tests now.

eamonnmcmanus · 2026-02-14T18:39:33Z

+import java.time.Instant;
+import java.time.LocalDate;
+import java.time.ZoneId;
+import org.junit.jupiter.api.Test;


Can we stick to JUnit 4 here? Sadly Google's internal infrastructure does not support JUnit 5, for Reasons.

This is part of the Native Image test which has to use JUnit 5/6 (and is consistently doing so already), due to the used tooling for Native Image testing.

Marcono1234 · 2026-08-05T18:19:59Z

@eamonnmcmanus and @cpovirk, are you still interested in this PR? If so I can try resolving the conflicts.

eamonnmcmanus · 2026-08-05T18:27:23Z

@eamonnmcmanus and @cpovirk, are you still interested in this PR? If so I can try resolving the conflicts.

Sorry, I lost track of this. I promised to run it against Google's internal tests, which I did, back in February, and did not find any failures. So if you don't mind bringing it up to date, we can merge it.

eamonnmcmanus · 2026-08-05T18:24:38Z

+      } else if (rawType == ZoneId.class || rawType == ZoneOffset.class) {
+        // We don't check ZoneId.class.isAssignableFrom(rawType) because we don't want to match
+        // the non-public class ZoneRegion in the runtime type check in
+        // TypeAdapterRuntimeTypeWrapper.write. If we did, then our ZONE_ID would take
+        // precedence over a ZoneId adapter that the user might have registered. (This exact
+        // situation showed up in a Google-internal test.)
+        adapter = ZONE_ID;


I remember having quite a lot of trouble with ZoneId and ZoneOffset, though I don't remember the exact details. If the tests still pass then this change preserves the behaviour I settled on. I can well believe that that can be improved, but I'd suggest doing that in a separate change.

Marcono1234 · 2026-09-04T11:47:02Z

Thanks for the follow-up! I have now updated the pull request, hopefully it is ok like this. Please let me know if you want anything changed.

This makes the Git diff for the next commit easier to read

cpovirk · 2026-09-21T15:03:10Z

+    return null;
+  }
+
+  // Separate method to really only load the `Impl` class when dealing with `java.time` types,


I'm not sure that a separate method buys us anything beyond what we already get from the if above (as you pointed out and as I note again in my other comment) and separate class below (for the ZoneId/ZoneOffset issue?). Not that I object to having a separate method, especially when we're arguably playing with fire here. But if you have acquired more arcane knowledge about incomplete classpaths, then please do share! :)

But if you have acquired more arcane knowledge about incomplete classpaths, then please do share! :)

No, I don't think I am more knowledgeable in that area than you. Most likely you can indeed put the Impl access directly inside the create method, and possible prove based on the Java Virtual Machine Specification that this will only load the class when that code is actually executed.
But I wanted to be on the safe side here with the separate method.

And Android class loading seems to differ anyway from the JVM one, so expected class loading behavior for the JVM might not directly apply to Android.

cpovirk · 2026-09-21T15:12:02Z

+  /**
+   * Adapter factory for {@code java.time} classes. Returns {@code null} if not supported by the
+   * current environment (e.g. too old Android version, without desugaring).
+   */


I like the new approach!

I had overlooked that the if protects us even in the existing approach. Thanks for explaining that.

GitHub doesn't present a good diff regardless because of the change in file name. (Maybe it would do better if I were to opt in to the new review UI? Presumably Git itself will have its move detection kick in, helping tools (including GitHub) after the fact.) I was able to review it easily enough from the command line.

I belatedly looked into the logcat output that I'd been referring to. It looks like it might happen only on attempts after the first. The repro that I put together (on an API Level 24 emulator) produces a message of "Rejecting re-init on previously-failed class java.lang.Class<com.google.common.base.AsciiTest$1>: java.lang.NoClassDefFoundError: Failed resolution of: Ljava/lang/ClassValue;" from this ART code, which is still present at head. But none of that should matter, given your efforts to avoid triggering initialization of java.time classes at all.

Follow-up for java.time adapter changes

9489c93

Marcono1234 commented Jan 13, 2026

View reviewed changes

Fix Gson subset build

2ce5629

eamonnmcmanus requested changes Jan 13, 2026

View reviewed changes

Marcono1234 commented Jan 13, 2026

View reviewed changes

Marcono1234 added 7 commits January 18, 2026 01:04

Account for desugared java.time package name

63dd844

Address review feedback

b0dfcba

Add shrinker test

738c4b6

Add tests for java.time enums

ae58851

Add test for custom ZoneId adapter

4fbcdb4

Remove test failing on JDK 11

fc97473

Adjust retrieval of java.time package name

c2f4868

Marcono1234 commented Jan 18, 2026

View reviewed changes

Merge branch 'main' into marcono1234/java-time-follow-up

e196a2d

eamonnmcmanus requested changes Feb 14, 2026

View reviewed changes

Marcono1234 commented Feb 15, 2026

View reviewed changes

Comment thread gson/src/main/java/com/google/gson/internal/bind/TypeAdapters.java Outdated

eamonnmcmanus approved these changes Aug 5, 2026

View reviewed changes

Marcono1234 added 2 commits September 4, 2026 12:10

Merge branch 'main' into marcono1234/java-time-follow-up

edefa2a

Tweak Javadoc & tests

31a1f2e

Marcono1234 added 4 commits September 15, 2026 12:17

Merge branch 'main' into marcono1234/java-time-follow-up

3b1e642

Rename class

9e0059b

This makes the Git diff for the next commit easier to read

Refactor java.time adapter factory creation

b246b10

Rename animal-sniffer ignore annotation

1c02db1

cpovirk approved these changes Sep 21, 2026

View reviewed changes

eamonnmcmanus assigned cpovirk Sep 21, 2026

Conversation

Marcono1234 commented Jan 13, 2026

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

eamonnmcmanus left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Marcono1234 Jan 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Marcono1234 Jan 18, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Marcono1234 Sep 16, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cpovirk Sep 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Marcono1234 Sep 19, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Marcono1234 Jan 18, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Footnotes

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Marcono1234 Jan 18, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eamonnmcmanus left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Marcono1234 commented Aug 5, 2026

Uh oh!

eamonnmcmanus commented Aug 5, 2026

Uh oh!

Marcono1234 Jan 13, 2026 •

edited

Loading

Marcono1234 Jan 18, 2026 •

edited

Loading

Marcono1234 Sep 16, 2026 •

edited

Loading

cpovirk Sep 17, 2026 •

edited

Loading

Marcono1234 Sep 19, 2026 •

edited

Loading

Marcono1234 Jan 18, 2026 •

edited

Loading

Marcono1234 Jan 18, 2026 •

edited

Loading