Closed cmelchior closed 7 years ago
While reviewing null checks and possibly eliminating some, seems like a great idea, it is important to realize that the annotations are not a general replacement for checks. There are boat-loads of ways that a parameter can be null, that static analysis won't catch...
On May 15, 2017 1:16 AM, "Christian Melchior" notifications@github.com wrote:
Square has started pushing JSR305 annotations as the way to express nullability in their API's.
- square/okio#307 https://github.com/square/okio/pull/307
- https://medium.com/square-corner-blog/rolling-out- nullable-42dd823fbd89
- https://medium.com/@jakewharton/an-optionals- place-in-kotlin-17d7b271eefe
- https://youtrack.jetbrains.com/issue/KT-10942
And this is starting a push on other libraries as well:
- ReactiveX/RxJava#5341 https://github.com/ReactiveX/RxJava/issues/5341
When we originally added the @Required annotation it was precisely because there wasn't one single standard for @Nullable/@NonNull being used. At the time we found at least 7 different versions I believe. JSR305, Android, Jetbrains, Findbugs, Eclipse and a few others I forgot.
However, I wouldn't mind at all help move the Android eco-system towards a default standard for this. It would also improve tool support with Android Studio as well as improve our API compatibility with Kotlin.
Suggested roadmap:
- Add JSR305 as a provided dependency.
- Add Nullability annotation to our API: @ParametersAreNonnullByDefault is much preferred but not supported by Kotlin yet.
- Delete null-checks in code.
- Add support for @NonNull as a replacement for @Required in our annotation processor.
- Deprecate @Required and (possibly) remove it at later major release. Although keeping it around doesn't carry a lot of overhead.
Thoughts @realm/java https://github.com/orgs/realm/teams/java?
The only downside I can see is that we will add a (provided) dependency to Realm, which so far we have been very hesitant in doing.
— You are receiving this because you are on a team that was mentioned. Reply to this email directly, view it on GitHub https://github.com/realm/realm-java/issues/4643, or mute the thread https://github.com/notifications/unsubscribe-auth/AAQ9WEsexiwiiUQpyfTTsWZ-KuyezCyQks5r6AnUgaJpZM4NazXG .
@bmeike this may be true for Java, but Kotlin will enforce these nullability contracts at compile time, which is a huge benefit for those users.
@bmeike With nullability return/param annotation in java, null checks enforcement in Kotlin will be forced upon the coder/caller which is really good.
By default, if no annotation is used, the below dangerous code would compile in Kotlin:
fun Take(c: RealmCategory) {
//do stuff
}
Take(realm.where(Category.class).findFirst())
Take() will crash if findFirst() returns null since Kotlin injects null checks at Take().
By adding @nullable annotation to return of RealmQuery.fiindFirst(), Kotlin would not even compile the above code and forces coder to make modifications to handle the null protection
fun Take(c: RealmCategory?) {
val v = c ?: return
//do stuff with v
}
Nullability annotations were added in https://github.com/realm/realm-java/pull/5044. Closing
Square has started pushing JSR305 annotations as the way to express nullability in their API's.
And this is starting a push on other libraries as well:
When we originally added the
@Required
annotation it was precisely because there wasn't one single standard for@Nullable/@NonNull
being used. At the time we found at least 7 different versions I believe. JSR305, Android, Jetbrains, Findbugs, Eclipse and a few others I forgot.However, I wouldn't mind at all help move the Android eco-system towards a default standard for this. It would also improve tool support with Android Studio as well as improve our API compatibility with Kotlin.
Suggested roadmap: 1) Add JSR305 as a provided dependency. 2) Add Nullability annotation to our API:
@ParametersAreNonnullByDefault
is much preferred but not supported by Kotlin yet. 3) (Delete null-checks in code). 4) Add support for@NonNull
as a replacement for@Required
in our annotation processor. 5) Deprecate@Required
and (possibly) remove it at later major release. Although keeping it around doesn't carry a lot of overhead.Thoughts @realm/java?
The only downside I can see is that we will add a (provided) dependency to Realm, which so far we have been very hesitant in doing.