Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Don't exclude static local classes #1969

Merged

Conversation

@Marcono1234
Copy link
Contributor

@Marcono1234 Marcono1234 commented Sep 17, 2021

Java 16 added Records, which can be declared locally but are always static. It also allows interfaces to be declared locally.

This pull request makes makes Excluder allow such static local classes. Despite Gson not having a built-in adapter for Record types yet, it at least makes it easier for users to write their own, see #1794 (comment).
The reason why local classes were previously not permitted seems to be that they can capture enclosing references which cannot be serialized and deserialized properly, see #298. However for static classes this won't be an issue.

I have not added a test for this because Gson currently uses Java 6 to compile, so it would require adjustments to the Maven build to support compiling and running one test with Java 16. With Maven this is slightly more cumbersome than with Gradle because users need to manually set up a toolchain to specify a Java 16 JDK, whereas Gradle can handle that on its own.
But you can test this manually by running:

public void test() {
  record R(int i) {}
  record C(R r) {}
  // Without the change this prints "null"
  System.out.println(new Gson().toJson(new C(new R(1))));
}

Relates to #1510

@@ -199,7 +199,8 @@ private boolean excludeClassChecks(Class<?> clazz) {
return true;
}

if (isAnonymousOrLocal(clazz)) {
// Permit static local classes, such as Records (Java 16 feature)
if (!isStatic(clazz) && isAnonymousOrLocal(clazz)) {
Copy link
Member

@eamonnmcmanus eamonnmcmanus Sep 18, 2021

Choose a reason for hiding this comment

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

I am wondering if it makes sense for isAnonymousOrLocal to become isAnonymousOrLocalAndNotStatic. In the other case where we call the method (excludeField), would we want to accept static local classes as well?

Copy link
Contributor Author

@Marcono1234 Marcono1234 Sep 18, 2021

Choose a reason for hiding this comment

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

Ah, you are right excludeField should not exclude fields with static local classes as type either.
Moving it into isAnonymousOrLocal sounds good. What about isAnonymousOrNonStaticLocal as name instead?

Copy link
Member

@eamonnmcmanus eamonnmcmanus Sep 18, 2021

Choose a reason for hiding this comment

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

Sure, sounds good.

Copy link
Contributor Author

@Marcono1234 Marcono1234 Sep 18, 2021

Choose a reason for hiding this comment

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

Have force-pushed the change. It now in theory also allows static anonymous classes (to not make the boolean expression too nested), but I don't think such a thing is possible in Java at the moment.

@Marcono1234 Marcono1234 force-pushed the marcono1234/allow-static-local-class branch from fca73eb to 0408b31 Sep 18, 2021
@eamonnmcmanus eamonnmcmanus merged commit aa5554e into google:master Sep 18, 2021
2 checks passed
@Marcono1234 Marcono1234 deleted the marcono1234/allow-static-local-class branch Sep 18, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants