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

Having reflective helpers in Moshi.kt has some tradeoffs for source builds #1735

Open
cpovirk opened this issue Sep 5, 2023 · 0 comments
Open
Labels

Comments

@cpovirk
Copy link
Contributor

cpovirk commented Sep 5, 2023

I just pulled in Moshi 1.15.0 and then the subsequent unreleased changes as part of my mission to pick up #1731. That meant that I also picked up #1496, which copied the reflection-related extension functions to make them instance functions in Moshi.kt.

I see the appeal of this on the API level, but it led to a couple downsides for us, since we build Moshi from source:

  • We run a tweaked version of JvmReflectionAPICallChecker, which produces a compile error when building code that uses reflection but doesn't depend on kotlin_reflect. Previously, we could include the kotlin_reflect dep only for users who opted in to using the extensions (by having the extensions in a separate build target / kotlinc invocation). Because we don't want to add that dep everywhere (since the mere inclusion of the dependency at compile time apparently tells kotlinc to output lots of information about downstream classes, increasing apk size), we end up having to omit it, suppress the error, and rely on users to add the dep when they need to. This isn't catastrophic (and in fact we appear to have no users of reflection nowadays, though I think we used to have at least one that migrated off?), but it's a tradeoff.
  • The functions call javaType, which is an experimental API. This produces a different kind of compile error, since we ban usages of experimental APIs by default. (Incidentally, I also had to address a similar error with the new usages of contract, but I found it straightforward to rewrite calls of those functions to use checkNotNull instead.) We do have a way to opt in, but we allow it only at the granularity of a kotlinc invocation, so we'd prefer not to opt in the whole of Moshi core in case it begins using additional experimental APIs in the future, ones that might be more concerning that this reflective API. Of course, I imagine that your plans do not including using concerning experimental APIs :)

(What I actually did was just comment out the new functions (and remove the old extension functions, since they are unused) in our local copy of Moshi. That might lead to a merge conflict or a little confusion down the line, but it will probably work out OK, and any problems that it does cause should be minor.)

Anyway, this is all highly survivable for us, but I figured I'd pass it along, especially seeing that the changes haven't been released yet, in case any of this is surprising or implies anything worse than what I've personally encountered.

@cpovirk cpovirk added the bug label Sep 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

1 participant