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

Fix issue with recursive type variable protections to fix #1390 #1391

Merged
merged 2 commits into from Aug 2, 2021

Conversation

mcumings
Copy link
Contributor

@mcumings mcumings commented Sep 25, 2018

When a type variable is referenced multiple times it needs to resolve to the same value. Previously, the second attempt would abort resolution early in order to protect against infinite recursion.

NOTE: I could use some scrutiny on this as I don't fully understand the implications of all the code branches. This commit does resolve the issue but stylistically I'm not really sold on breaking out of the while loop in order to capture the final result for subsequent resolution attempts.

Fixes #1390

When a type variable is referenced multiple times it needs to resolve
to the same value.  Previously, the second attempt would abort
resolution early in order to protect against infinite recursion.
@mcumings
Copy link
Contributor Author

The Codacy check is flagging existing code with a legitimate == check. Not sure how I should respond to that...

@lyubomyr-shaydariv
Copy link
Contributor

lyubomyr-shaydariv commented Sep 26, 2018

The Codacy check is flagging existing code with a legitimate == check. Not sure how I should respond to that...

Neither componentType nor newComponentType are declared as java.lang.Class instances, hence == is not guaranteed to work properly. My IntelliJ IDEA code analisys reports a lot of ==/equals issues for $Gson$Type too, and I think you should simply rewrite it using equals() (the old code is dated 2010, most likely prior to Codacy). Codacy seems to analyze the modified code, so that's probably why it reports your change only.

@mcumings
Copy link
Contributor Author

Fair point. There were a couple points in the resolve method where instance equality checks were sufficient (i.e., it was checking to see if the original value was being returned, etc.). I updated the others to use the equal method. I didn't want to spread this too broadly however, so I stopped there.

Cadacy is now happy. 😃

@Marcono1234
Copy link
Collaborator

Could you please edit the description and include:

Fixes #1390

It appears otherwise GitHub does not understand that this pull request and the issue are related.

@mcumings
Copy link
Contributor Author

Done. Hopefully this PR isn't too old as to be useless at this point.

@Marcono1234
Copy link
Collaborator

Thanks! There haven't been any other changes to $Gson$Types since then so this PR is still useful.
Hopefully the maintainers have a look at all the open pull requests soon.

@eamonnmcmanus eamonnmcmanus merged commit d65960b into google:master Aug 2, 2021
eamonnmcmanus added a commit to eamonnmcmanus/gson that referenced this pull request Aug 3, 2021
Use two-space indentation for the new test.
Use standard Google import style.
Supply missing type argument for `TypeVariable`.
eamonnmcmanus added a commit that referenced this pull request Aug 3, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Recursive TypeVariable resolution results in ClassCastException when type var is referenced multiple times
5 participants