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

Improve creation of ParameterizedType #2375

Conversation

Marcono1234
Copy link
Collaborator

@Marcono1234 Marcono1234 commented Apr 15, 2023

Purpose

Improve creation and validation of ParameterizedType

Description

  • Reject non-generic raw types for TypeToken.getParameterized
  • Fix ParameterizedTypeImpl erroneously requiring owner type for types without owner (mainly / only local classes?)
  • Extend tests and use assertThrows

Checklist

  • New code follows the Google Java Style Guide
  • If necessary, new public API validates arguments, for example rejects null
  • New public API has Javadoc
    • Javadoc uses @since $next-version$
      ($next-version$ is a special placeholder which is automatically replaced during release)
  • If necessary, new unit tests have been added
    • Assertions in unit tests use Truth, see existing tests
    • No JUnit 3 features are used (such as extending class TestCase)
    • If this pull request fixes a bug, a new test was added for a situation which failed previously and is now fixed
  • mvn clean verify javadoc:jar passes without errors

- Reject non-generic raw types for TypeToken.getParameterized
- Fix ParameterizedTypeImpl erroneously requiring owner type for types
  without owner
@@ -351,6 +351,18 @@ public static TypeToken<?> getParameterized(Type rawType, Type... typeArguments)
Class<?> rawClass = (Class<?>) rawType;
TypeVariable<?>[] typeVariables = rawClass.getTypeParameters();

// Note: Does not check if owner type of rawType is generic because this factory method
// does not support specifying owner type
if (typeVariables.length == 0) {
Copy link
Member

Choose a reason for hiding this comment

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

I ran #2376 against all of Google's internal tests and found one failure where someone was calling this method with a non-generic type and an empty typeArguments array. Of course then it is just the same as calling get(NonGeneric.class), but I think this case should continue to work. It's something of a historical quirk that the reflection API represents classes with no type parameters so differently from classes with some.

There were no other failures, so I think the other changes are probably safe.

This change would imply updating the @throws text.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks for testing this!

found one failure where someone was calling this method with a non-generic type and an empty typeArguments array

Was this actually a bug in the code or is there some reason why the code was doing this? The only situation I can imagine is if the typeArguments is created dynamically and the direct caller of getParameterized does not know whether the raw class it provides is generic; but even then I think it is a bit questionable requesting creation of a ParameterizedType when it is not actually known (and not verified) that the given raw type is generic.
Are you able to share a bit more information about the code where this failed, maybe in some redacted or simplified way?

Of course then it is just the same as calling get(NonGeneric.class), but I think this case should continue to work.

Except that getParameterized actually gives you a TypeToken(ParameterizedType) where the ParameterizedType is bogus, instead of a TypeToken(Class). So I am really not sure if we should continue to support this.

We could change TypeToken.getParameterized to return a TypeToken(Class) in that case, but that could on the other hand also cause backward compatibility issues; though supporting TypeToken.getParameterized to create bogus TypeToken(ParameterizedType) is probably also not something we want to support.

Copy link
Member

Choose a reason for hiding this comment

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

The code I was talking about is in a test, and looks something like this:

    Type type = TypeToken.getParameterized(FooResponse.class).getType();
    return new GsonBuilder()
        .registerTypeAdapterFactory(AutoValueAdapterFactory.create())
        .registerTypeAdapter(ImmutableList.class, new ImmutableListDeserializer())
        .create()
        .fromJson(json, type);

I had not realized that TypeToken.getParameterized returns a ParameterizedType with no type arguments in this case. That is indeed a bizarre sort of beast, and I can't help feeling that this code works mostly by accident. Since it's test code, rather than production, and apparently nothing else in all of Google's code is doing this, I think it's probably OK to introduce this exception. We should make sure to mention it in the next release notes, though.

Copy link
Member

Choose a reason for hiding this comment

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

For the record, I did find another instance, in non-test code this time. I think the code in question happened to be broken for unrelated reasons when I did my earlier test, which is why I didn't see the failure then. I think it might be worth catching this case and returning TypeToken.get(rawType), assuming the type-argument list is also empty.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ok, have created #2447 for that. I hope that is what you had in mind.

@eamonnmcmanus eamonnmcmanus merged commit a589ef2 into google:main Jun 23, 2023
7 checks passed
@Marcono1234 Marcono1234 deleted the marcono1234/parameterized-type-improvements branch June 23, 2023 22:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants