Eliminate TypeMarkers for simple and one level parameterized types - #3035
Eliminate TypeMarkers for simple and one level parameterized types#3035atrocities wants to merge 1 commit into
Conversation
Add 5 factory methods to TypeMarker and an implementation of ParameterizedType for simple types and the 4 base parameterized types that conjure generates. This reduces the need for most TypeMarkers, while also being reasonably readable, and type safe, so that no casting of TypeMarkers is needed by the caller. TypeMarkers are still produced for parameterized types that have more than one level of parameterization. In practice, this comprises a vanishingly small number of types.
Generate changelog in
|
atrocities
changed the title
kunalrkak
left a comment
kunalrkak
left a comment
There was a problem hiding this comment.
overall lgtm, few nits
| }; | ||
| } | ||
|
|
||
| private static final ClassName MAP_NAME = ClassName.get(Map.class); |
There was a problem hiding this comment.
is there no shared location we can put all the below code to avoid the code duplication?
| return new WrappingTypeMarker<>(type); | ||
| } | ||
|
|
||
| public static <T> TypeMarker<List<T>> listOf(Class<T> elementType) { |
There was a problem hiding this comment.
could we add some simple equality tests for the new parameterized wrapped type factories? i.e.
assertThat(TypeMarker.listOf(String.class)).isEqualTo(new TypeMarker<List<String>>() {});
would be good to also confirm equal hashcodes and equivalent values from getType() so we know there won't be any issues replacing the existing anon TypeMarkers
| return CodeBlock.of("new $T<$T>() {}", TypeMarker.class, type); | ||
| } | ||
|
|
||
| private static boolean hasExpectedSimpleTypeArguments(ParameterizedTypeName type) { |
There was a problem hiding this comment.
I couldn't find one while poking, can we also add a case confirming this behavior leaves "complex" wrapped types as is? I see some examples use ImmutableList which would presumably fail at TYPE_MARKER_FACTORIES#containsKey.
2 participants
Before this PR
Anon TypeMarker implementations were created for all types that need to be serialized/deserialized. This results in pollution of jars at rest, and meta/symbol/class space at runtime.
The reason for this is because of the erasure of the parameterized type arguments for generic types, which makes it not possible to express something like List.class as an argument to a factory method. The workaround was to capture this type information in a subclass instead. While this approach was not strictly required for simple, non parameterized types, it was chosen due to uniformity.
After this PR
Add 5 factory methods to TypeMarker and an implementation of ParameterizedType for simple types and the 4 base parameterized types that conjure generates. This reduces the need for most TypeMarkers, while also being reasonably readable, and type safe, so that no casting of TypeMarkers is needed by the caller.
TypeMarkers are still produced for parameterized types that have more than one level of parameterization. In practice, this comprises a vanishingly small number of types. While conjure-java does not generate any parameterized classes outside of Set, List, Map, and Optional, it's possible that more types may be added in the future, for which anon TypeMarkers will be generated for those types unless this collection of factory methods is also updated.
Possible downsides?
This only applies to the Undertow side. Changes to Dialogue will need to be made separately.