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

Proposal: Switch to a provided JSR305 artifact for nullability + package annotations #5341

Closed
ZacSweers opened this issue May 14, 2017 · 16 comments

Comments

@ZacSweers
Copy link
Contributor

commented May 14, 2017

I'd like to propose switching away from the current custom @Nullable and @NonNull annotations and using a combination of a provided JSR305 dependency (that is, only compileOnly and not actually packaged, but still visible to static analysis tools).

This would provide two main benefits:

  • Use of a standard @Nullable annotation (Guava uses the same one). Currently anyone that uses other annotations gets burdened with fully-qualified copies of RxJava's due to IDE autocomplete, and it's a nuisance to clean it up.
  • Use of package-level annotations to make the default for everything @NonNull. You could use @ParametersAreNonnullByDefault for just parameters, or also have a separate artifact with a custom @EverythingIsNonNullByDefault annotation like here that would also cover fields and return types. This would obviate the need for using @NonNull entirely, and is respected by most conventional static analysis tools.
  • Potential improvements for interoperability with (Rx)Kotlin - https://youtrack.jetbrains.com/issue/KT-10942

I know RxJava prefers to have no dependencies, but I think that having this as a provided dependency is a good middle ground for the value it provides.

Other prior reading:

@vanniktech

This comment has been minimized.

Copy link
Contributor

commented May 14, 2017

I'd also prefer if RxJava would use the JSR305 @NonNull & @Nullable, especially with @ParametersAreNonnullByDefault. Although I think RxJavas @NonNull and @Nullable are now considered stable with the 2.1.0 release and hence not possible to remove if we'd stick to the plan, right?

@ZacSweers

This comment has been minimized.

Copy link
Contributor Author

commented May 14, 2017

stable, but not a breaking API change technically (from a compile perspective) since they're just annotations and the new annotations would be a superset of their functionality. They could be left in if the idea was to allow other RxJava-dependent libraries to use them if they want, while RxJava itself could use JSR305?

@akarnokd

This comment has been minimized.

Copy link
Member

commented May 14, 2017

If you specify @ParametersAreNonnullByDefault, how do you override it for BiConsumer to be not defined?

I suppose provided is an acceptable option but can't delete our own annotation files; at most we strip them from all those paces which should still count as binary compatible.

@vanniktech

This comment has been minimized.

Copy link
Contributor

commented May 14, 2017

You'd mark it as @Nullable

@akarnokd

This comment has been minimized.

Copy link
Member

commented May 14, 2017

That's not the same as not defining it. Wouldn't an IDE complain about not checking for null for both parameters when you know only one of them can be null with that specific subscribe?

@vanniktech

This comment has been minimized.

Copy link
Contributor

commented May 14, 2017

Yup that will be a drawback. Once @Nullable is applied you'll need to check even if a specific version of subscribe guarantees it to be @NonNull.

@ZacSweers

This comment has been minimized.

Copy link
Contributor Author

commented May 15, 2017

Are there many other cases like that?

@akarnokd

This comment has been minimized.

Copy link
Member

commented May 17, 2017

Any defaultItem operator where the value itself doesn't enter the stream (such as Observable.blockingFirst but gets returned where we don't check for null.

@akarnokd akarnokd added the PR welcome label Jun 15, 2017

@akarnokd

This comment has been minimized.

Copy link
Member

commented Jun 15, 2017

Let's see the implications in a PR. Remember, don't delete the original RxJava annotation files.

@akarnokd

This comment has been minimized.

Copy link
Member

commented Feb 8, 2018

Anybody still interested in adding the annotations? Note though that due to the bad interaction of JSR305 and Java 9, we should stick to our own annotations which are supposedly understood by IntelliJ.

@realdadfish

This comment has been minimized.

Copy link

commented Apr 16, 2018

I just regularly review code where nullable things are accidentially returned from map {} calls and the like in Kotlin that potentially crash on runtime. I'd therefor really like see @NonNull annotations (however implemented) to let the IDE issue out errors.

@JakeWharton

This comment has been minimized.

Copy link
Member

commented Apr 16, 2018

JSR 305 is dead and actively prevents libraries from being used on the module path with Java 9 and newer. We should be tearing it out of every library which made the mistake of adding it.

@realdadfish

This comment has been minimized.

Copy link

commented Apr 16, 2018

I see, just found some context: https://blog.codefx.org/java/jsr-305-java-9/

@JakeWharton

This comment has been minimized.

Copy link
Member

commented Apr 16, 2018

And I'm not specifically against what you proposed, just JSR 305 in general. Your comment just reminded me of this issue being a thing.

@akarnokd

This comment has been minimized.

Copy link
Member

commented May 26, 2018

Closing because of JSR 305's problem with modules in 9. We'll keep using our own annotations and apply it wherever necessary.

@tlinkowski

This comment has been minimized.

Copy link

commented Aug 19, 2019

In case it interests you, I wrote a blog post about when it still makes sense to use JSR 305 instead of e.g. Checker Framework, provided that you have the requirements I had. In short, JSR 305 is the only library whose package-scope annotations are honored by both IntelliJ and Kotlin.

Also, note that JSR 305 can be used with JPMS - it just needs to be patched if there's a conflict.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.