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

Migrate away from JSR-305 dependency #242

Closed
kashike opened this issue May 9, 2018 · 21 comments

Comments

@kashike
Copy link

commented May 9, 2018

See google/guava#2960 for context.

Guava migrated to annotations from the Checker Framework: google/guava@dc32104 google/guava@e8d744e

@ben-manes

This comment has been minimized.

Copy link
Owner

commented May 9, 2018

Thanks. I hadn't noticed that the missing annotations were added to ErrorProne, so I don't think there are any blockers. In semver, would you consider this a patch or minor revision?

@kashike

This comment has been minimized.

Copy link
Author

commented May 9, 2018

I'm not sure what I'd consider it, but it might be wiser to stick it under a minor revision - it isn't what I'd consider a "patch".

@ben-manes

This comment has been minimized.

Copy link
Owner

commented May 9, 2018

Thanks. That's what I was thinking too and a second opinion is helpful.

@ben-manes

This comment has been minimized.

Copy link
Owner

commented May 28, 2018

I'm not entirely sure where all of the new mappings are, any suggestions?

JSR 305 New
javax.annotation.CheckForNull Perhaps Nullable?
javax.annotation.Nonnegative ???
javax.annotation.Nonnull org.checkerframework.checker.nullness.qual.NonNull
javax.annotation.Nullable org.checkerframework.checker.nullness.qual.Nullable
javax.annotation.concurrent.GuardedBy com.google.errorprone.annotations.GuardedBy
javax.annotation.concurrent.Immutable com.google.errorprone.annotations.Immutable
javax.annotation.concurrent.NotThreadSafe ???
javax.annotation.concurrent.ThreadSafe ???
@ben-manes

This comment has been minimized.

Copy link
Owner

commented May 28, 2018

The 350kb checker jar is also a bit obnoxiously bloated. I don’t see a good alternative, but it feels excessive for just nullability annotations.

@kashike

This comment has been minimized.

Copy link
Author

commented May 29, 2018

JSR 305 New
javax.annotation.Nonnegative org.checkerframework.checker.index.qual.NonNegative

javax.annotation.concurrent.NotThreadSafe and javax.annotation.concurrent.ThreadSafe have no replacements, as far as I know.

It's also worth noting that annotations such as org.checkerframework.checker.nullness.qual.NonNull and org.checkerframework.checker.nullness.qual.Nullable are type annotations - see google/guava#3145.

@ben-manes

This comment has been minimized.

Copy link
Owner

commented May 29, 2018

I double checked and I must have downloaded the wrong jar. The checker-qual jar is 200kb, so a bit better. The checker-compat-qual is 6kb and doesn't contain type annotations so we'd use NullableDecl and NonNullDecl if we went that direction. I'm not too picky either way, I guess.

I have a branch, jsr305, with the migrations so far. Do you have any more recommendations for it?

@ben-manes

This comment has been minimized.

Copy link
Owner

commented May 29, 2018

Also, I should note that I didn't use the type annotations' ability to be put within the generic argument, e.g. @NonNull Map<@NonNull K, @NonNull V>, because I didn't think that would be helpful. But I'm willing to be convinced otherwise.

@ben-manes

This comment has been minimized.

Copy link
Owner

commented May 30, 2018

Oh! I forgot to use your non negative mapping. I’ll go back and add them in tomorrow.

@ben-manes ben-manes reopened this May 30, 2018

@jbduncan

This comment has been minimized.

Copy link

commented May 30, 2018

Also, I should note that I didn't use the type annotations' ability to be put within the generic argument, e.g. @nonnull Map<@nonnull K, @nonnull V>, because I didn't think that would be helpful. But I'm willing to be convinced otherwise.

@ben-manes, may I ask why you think it wouldn't be helpful to label the generic arguments with @NonNull as well? :)

@ben-manes

This comment has been minimized.

Copy link
Owner

commented May 30, 2018

Oh, I guess because I’ve used the annotations as documentary rather than assumed a static analyzer would be run by users. As an easy way to glance without reading the JavaDoc, it answers basic questions. Since developers are accustomed to assuming collections don’t have null elements, that is noise and not useful.

Perhaps now that view is outdated so I’d be fine adding if you think it’s worthwhile.

@jbduncan

This comment has been minimized.

Copy link

commented May 30, 2018

@ben-manes In my humble opinion, that is a perfectly good view on the use of nullness annotations but also a bit of an outdated one now.

AFAICT, nullness-checking static analysis tools like Checker Framework and Uber's NullAway are becoming more and more popular, so having consistent nullness annotations would help there.

Also, AFAIK Kotlin takes nullness annotations into account when exercising the null-checking part of its type system.

Having said all this, though, I don't know if Kotlin assumes things are nullable or non-null by default, and I think Checker Framework assumes things are non-null by default, so I don't know if adding @NonNull to generic arguments will help Kotlin and Checker Framework users much.

But regardless, my gut reaction is it would be very sensible to be explicit and label both types and generic arguments as @NonNull where applicable. :)

@ben-manes

This comment has been minimized.

Copy link
Owner

commented May 30, 2018

My recollection is that Checker assumes null by default, which makes using it painful. I tried setting it up once and felt it was just too invasive, among other issues. I think Kotlin switched from null by default to non-null due to that, which masks noise but makes the type system silent about potential bugs. I do think that's a fair choice though and libraries should annotate their public interfaces for documentation and tooling.

In my next round of changes, I'll add the annotations to the type arguments and restore the non-negative. Thanks!

@ben-manes ben-manes closed this in 2e6d328 May 31, 2018

@tbroyer

This comment has been minimized.

Copy link

commented Jun 19, 2018

(context: currently migrating a project away from JSR305 and looking around for what people used in replacement)

Fwiw, there are ThreadSafe and NotThreadSafe annotations in net.jcip.annotations, which you can find under Apache 2.0 license at com.github.stephenc.jcip:jcip-annotations (rather than the CC BY 2.5 of the original net.jcip:jcip-annotations)

@ben-manes

This comment has been minimized.

Copy link
Owner

commented Jun 19, 2018

Thanks, that's a good point! I had forgotten about the JCIP originals.

Do you think adding them back in would be helpful? I did like it as another hint of expected behavior, but I'd also guess that most users assumed the behavior already when chosing this library.

@tbroyer

This comment has been minimized.

Copy link

commented Jun 19, 2018

I'm not (yet?) a caffeine user so 🤷‍♂ but I see you have a couple public classes that used to be annotated with @NotThreadSafe, so it might be useful to add the annotations back, if only for documentation (it also looks like Google might have a non-opensourced Error Prone checker for @ThreadSafe, and maybe one day they'll opensource it; you'll have the checks "for free" then 🎉)

@ben-manes

This comment has been minimized.

Copy link
Owner

commented Jun 19, 2018

I'll reopen so that I don't forget :)

@ben-manes ben-manes reopened this Jun 19, 2018

@ben-manes

This comment has been minimized.

Copy link
Owner

commented Jun 19, 2018

I took a quick look and I think they can be ignored. All the public apis are thread-safe so it is only internal documentation for non-threadsafe data structures. That's nice for anyone brave enough to dig into the code, but otherwise not critical. I think users can assume all public apis are threadsafe and act accordingly.

The only semi-public api is in the simulator, which is used for researchy purposes. Its Policy api had the annotation to communicate that assumption. The simulator uses Akka and proxies to each policy with a dedicated actor, which allows for parallel execution with a back-pressure by bounded mailboxes. I think anyone adding a new policy would skim the code for examples, so this wouldn't be too confusing.

@ben-manes ben-manes closed this Jun 19, 2018

@tbroyer

This comment has been minimized.

Copy link

commented Jun 19, 2018

Fwiw, one other use was BloomFilter, in the simulator projector.

@ben-manes

This comment has been minimized.

Copy link
Owner

commented Jun 19, 2018

yep, though for now that's used via enabling simulator flags (vs by a policy directly).

Someday I'd like to bring that into the library for the "doorkeeper" optimization. However, it biases the policy more towards frequency so it hurts LRU-friendly workloads. We have a pre-published paper on an adaptive scheme to correct for this (automatically tuning based on sampling the workload's characteristics). When that's passed peer review I'll work with @ohadeytan and @gilga1983 on implementing it.

@ben-manes

This comment has been minimized.

Copy link
Owner

commented Feb 24, 2019

Released 2.7

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