-
Notifications
You must be signed in to change notification settings - Fork 761
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
Overhaul internals to use KType
#1549
Draft
ZacSweers
wants to merge
27
commits into
master
Choose a base branch
from
z/ktype
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Draft
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This is handled internally now
Mostly involves removing needless null-checks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Overview
This overhauls Moshi to use
KType
as its source of truth rather thanType
. This puts us one step closer toward making all of Moshi's internals purely Kotlin while maintaining (backward) compatibility with Java.KFactory
The core of this is
JsonAdapter.KFactory
, which is analogous to the previousJsonAdapter.Factory
but changed to accept aKType
instead of aType
. We'll want to change internal standard adapters to implement this as well, but I'll leave those to later PRs to reduce churn in this one.With it I've added a
fun JsonAdapter.Factory.asKFactory(): JsonAdapter.KFactory
to ease interop (implemented asInteropKFactory
). This does not add the reverse, but open to it.This new factory also still uses
java.lang.Annotation
for now, also saving for a later PR to reduce churn.JsonAdapter<T>
Up to this point,
JsonAdapter
has always used nullable values forfromJson
andtoJson
, regardless of the nullability on the generic. Now nullability travels from the generic type itself, allowingJsonAdapter<T?>
andJsonAdapter<T>
to indicate nullable and non-nullable respectively. We also leverage the newT & Any
syntax in Kotlin 1.7 inJsonAdapter.nonNull()
.typeOf()
usagetypeOf()
went stable in 1.6, but we still had to guard it with experimental annotations because we always had to immediately defer back toKType.javaType
(which remains experimental). By moving to KType internally, we can mostly lift these experimental annotations. We can also potentially implementKType.javaType
ourselves internally to completely remove them, since that API isn't a compiler intrinsic.Behavior change -
WildcardType
Kotlin's
KType
cannot represent a JavaWildcardType
. Instead, these are only available asKTypeProjection
. Talked with @swankjesse and we're ok with dropping support for wildcard types as standalone types. Instead these will now get stripped when canonicalizing types.Behavior change - null-checks
Now that
nonNull()
actually checks null values, we get slightly different null error messages. They still indicate the path, but in some cases may be less informative than before (i.e. only saying the path was null, but not the type).Code gen
Code gen's been updated to rely on KType directly now since we can use
typeOf()
freely, simplifying things quite a bit. The new nullability enforcement inJsonAdapter
also allows us to functionally removeUtil.unexpectedNull()
Example generated adapter