Fix TypeVariable/WildcardType recursion causing stackoverflows #948
Conversation
| * limitations under the License. | ||
| */ | ||
|
|
||
| package com.squareup.moshi.internal; |
There was a problem hiding this comment.
I'm not entirely sure this should be in the internal package because it's testing changes made in Types, which is not in the internal package.
There was a problem hiding this comment.
Ahh good shout 37aaaae
| /** | ||
| * Test fixes for infinite recursion on {@link Util#resolve(java.lang.reflect.Type, Class, | ||
| * java.lang.reflect.Type)}, described at <a href="https://github.com/google/gson/issues/440">Issue #440</a> | ||
| * and similar issues. |
There was a problem hiding this comment.
I'm confused by this comment because the code that fixes it (in 7b73ce0) puts the fixes in Types, not Util.
There was a problem hiding this comment.
Gson's GsonTypes has a bunch of stuff that's split into Moshi's Types and Util. In Moshi - this method is in Util
| /** | ||
| * Test simplest case of recursion. | ||
| */ | ||
| @Test public void testRecursiveResolveSimple() { |
There was a problem hiding this comment.
The other Moshi tests do not use the "testBlah" naming structure; they just do "blah."
| assertThat(adapter).isNotNull(); | ||
| } | ||
|
|
||
| @Test public void testRecursiveTypeVariablesResolve1() { |
There was a problem hiding this comment.
The other Moshi tests do not use the "testBlah" naming structure; they just do "blah."
| assertThat(adapter).isNotNull(); | ||
| } | ||
|
|
||
| @Test public void testRecursiveTypeVariablesResolve12() { |
There was a problem hiding this comment.
This is the nittiest of nits, but might as well fix gson's typo here (should be "Resolve2", not "Resolve12").
| @@ -169,38 +171,52 @@ public static Type removeSubtypeWildcard(Type type) { | |||
| } | |||
|
|
|||
| public static Type resolve(Type context, Class<?> contextRawType, Type toResolve) { | |||
| return resolve(context, contextRawType, toResolve, new HashSet<TypeVariable>()); | |||
There was a problem hiding this comment.
There was a problem hiding this comment.
I don‘t love this solution because it allocates a collection always even though this case is quite rare.
There was a problem hiding this comment.
What if we initialize with an empty capacity? Still an instance but minimizes the footprint for non-typevar users
There was a problem hiding this comment.
lets do that, but use null as the sentinel for empty?
This fixes two related issues, one documented in #338. Both cases are solved borrowing the Gson solutions for them, since all the code/logic around wildcard types for them are borrowed from them as well.
The first is fixing recursion in
Util#sub/supertypeOfusing the solution from google/gson@a300148. This is a bugfix itself, but also necessary for the next issue.The second is fixing recursion in
Types#resolve()by tracking previously visited type variables using the solution from google/gson#1128Resolves #338