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 off of jsr305 #2960

Open
zyxist opened this issue Oct 7, 2017 · 75 comments
Open

Migrate off of jsr305 #2960

zyxist opened this issue Oct 7, 2017 · 75 comments

Comments

@zyxist
Copy link

@zyxist zyxist commented Oct 7, 2017

There are several issues with using jsr305.jar by Guava.

JSR-305 is dormant, has been for a long while and shows no hope of ever producing an agreed set of annotations in our lifetime. Further more these annotations use javax. packages which it is not possible to use according to the Oracle Java binary licence, so applications can not use and ship these dependencies along with a JRE without violating the Oracle licence agreement.

F. JAVA TECHNOLOGY RESTRICTIONS. You may not create, modify, or change the behavior of, or authorize your licensees to create, modify, or change the behavior of, classes, interfaces, or subpackages that are in any way identified as "java", "javax", "sun", “oracle” or similar convention as specified by Oracle in any naming convention designation.

The JSR-305 group has not defined any official releases according to its jsr page so the only implementations is a seemingly random implementation provided by the FindBugs team. Even if the team where experts on the JSR (which some where) they are not official as there has been no vote and are not available from the JSR hompage - so the javax package name restriction still applies.

Using jsr305 causes additional issues, if Guava is used in a modular JDK9 applications, because it puts the annotations into javax.annotation package, which is also used by a couple of other JAR-s and a legacy JDK module java.xml.ws.annotation. If one wants to create a modular JDK9 application with two dependencies to conflicting JAR-s, Java refuses to compile and run it because of a package split. Example:

  • Guava -> forces us to require jsr305 automatic module,
  • Dagger -> forces us to require either java.xml.ws.annotation or jsr250 automatic module.

All of the modules use javax.annotation.

Findbugs has been rebooted as Spotbugs and they are going to make a switch from JSR-305 to their own internal annotations in version 4.0.0 that do not break anything:

spotbugs/spotbugs#180

I think Guava should consider switching to them in order not to pollute application dependencies with jsr305 JAR.

@ronshapiro
Copy link
Contributor

@ronshapiro ronshapiro commented Oct 15, 2017

AFAICT these are the annotations from that package that we use:

  • @Nullable
  • @CheckForNull
  • @CheckReturnValue
  • @ParametersAreNonnullByDefault
  • @concurrent.GuardedBy
  • @concurrent.Immutable
  • @concurrent.NotThreadSafe
  • @concurrent.ThreadSafe

@Nullable is probably the one that we have the most options, since almost everyone treats any annotation with the simple name of Nullable as the same.

The others are less clear to me - they're technically just for static analysis, but they all have RUNTIME retention. It's not abundantly clear how we could get around using them without having ErrorProne, IDEs, and all other static analysis pipelines adopting a new set of APIs.

It sounds like we should maybe be avoiding javax.annotation.Generated entirely until we can figure this out. This will be a large project (and span beyond just Google products). Would that solve the module issue?

@zyxist
Copy link
Author

@zyxist zyxist commented Oct 16, 2017

I agree that this is more a long-term action. I created this issue more to raise the awareness of the problems with jsr305 and new Java. Spotbugs 4.0 is not released yet, and other tools would have to agree for a new set of annotations, and provide the sufficient support. But on the other hand, Guava is in the right position to influence a decision, which way to go.

Regarding javax.annotation.Generated:

  • avoiding it will only reduce the problem, not remove it (JSR-250 contains other annotations, too - if any of them is in use by any dependency or main project, you need to patch the modules).
  • Java 9 provides a new, unambiguous variant: javax.annotation.processing.Generated,
  • you can detect which one of them is available by the classloader, and generate the code either without any annotation, with javax.annotation.Generated, or with javax.annotation.processing.Generated. For JDK8, it would work "as usual", and for JDK9, it would depend on the required JDK module in the application module descriptor, so that the developer has a choice, which way to go.

I appreciate that you didn't forget about Generated :).

@ronshapiro
Copy link
Contributor

@ronshapiro ronshapiro commented Oct 16, 2017

JSR-250 contains other annotations, too

Seems to me that the others are significantly less heavily used.

@cpovirk
Copy link
Member

@cpovirk cpovirk commented Oct 17, 2017

Migrating off the jsr305 annotations makes sense to me.

Hopefully most tools care only about the simple class name of the annotations, not their packages. For starters, I checked Error Prone's @CheckReturnValue and @GuardedBy checkers. They care only about the simple name. But I'm sure that some tools (inside and outside Google) will care about the package.

If we're moving off the "standard" annotations, I wonder what it makes the most sense for us to migrate to. Spotbugs is a natural choice, since we know it has all the annotations that we want. But of course there are many nullability annotations. And arguably we aren't targeting Spotbugs nowadays so much as we're targeting Error Prone. Maybe Error Prone would be interested in adding some annotations of their own. Or maybe, for some of the annotations we use less frequently, we could have our own (package-private?) versions to avoid committing to any tool.

(One other note about @Nullable: We can't use type annotations yet because we currently make our "Android" branch available to Java 7 users. This may rule out some libraries' nullness annotations.)

@jbduncan
Copy link
Contributor

@jbduncan jbduncan commented Oct 17, 2017

If we're moving off the "standard" annotations, I wonder what it makes the most sense for us to migrate to.

Adopting the Checker Framework's annotations may make sense here. Considering that, AFAIK, its nullability annotations are the most plentiful and advanced out there, it may encourage more people to try the framework out.

But if its annotations count as type annotations, then we'd be unable to use them...

@JakeWharton
Copy link

@JakeWharton JakeWharton commented Oct 17, 2017

Hopefully most tools care only about the simple class name of the annotations, not their packages.

This is not true of Kotlin, although they'll be receptive to adding your annotations to their list.

@kashike
Copy link

@kashike kashike commented Oct 19, 2017

Looking at the options, it seems to me that Checker Framework would be a good solution. Thanks for pointing me to that, @jbduncan.

@kashike
Copy link

@kashike kashike commented Oct 25, 2017

I'm curious what the replacement for javax.annotation.concurrent.Immutable is with Checker Framework, if there is one - do you happen to know, @jbduncan?

@jbduncan
Copy link
Contributor

@jbduncan jbduncan commented Oct 25, 2017

@kashike I admit that I don't know if the Checker Framework has an Immutable annotation, but I do know that error-prone's annotations project has one. I've never tried the Checker Framework personally, so I don't know how it behaves in combination with error-prone (which, by comparison, I have used), but theoretically if one can get them to work nicely together, then we'd have a superior, statically analysed solution to javax.annotation.concurrent.Immutable.

@jbduncan
Copy link
Contributor

@jbduncan jbduncan commented Oct 26, 2017

...but that's not even considering the other sorts of annotations that we're already using and will probably still want to use, like @Nullable...

This may get tricky.

@jbduncan
Copy link
Contributor

@jbduncan jbduncan commented Oct 26, 2017

The Checker Framework authors themselves suggest that their own type annotations are backwards-compatible with Java 6, but only if they're written inside comments.

That's not exactly ideal...

@kashike
Copy link

@kashike kashike commented Oct 26, 2017

... Spotbugs is a natural choice, since we know it has all the annotations that we want. ...

Does it? I don't see everything that was listed above - it appears as if some annotations that are used by Guava are not in spotbugs.

As for Checker Framework: it appears to have most things, except @Immutable: https://gist.github.com/kashike/d8fb2007ab01041a08a2d5cf20bc2d17

...create a custom @Immutable annotation? It seems as if errorprone has one already, actually.

@jbduncan
Copy link
Contributor

@jbduncan jbduncan commented Oct 26, 2017

@kashike Apologies if I've not explained myself very well before, but I just want to make sure we're both on the same page with regards to the Checker Framework's annotations.

It seems to me from what @cpovirk's said already and what I personally know of the Checker Framework's annotations that, sadly, since they are type annotations, they can only be used in Java 8+ projects - JSR308, which is what type annotations are derived from, is only implemented in compilers that understand Java 8.

This is a problem because guava-android is compiled with a Android toolchain, and modern Android toolchains only understand Java 7 syntax and a small subset of Java 8 syntax - not enough to understand type annotations as legal constructs.

The only workaround I've found so far (suggested by the Checker Framework authors here) is to write type annotations in comments. So, for example, instead of:

List<@Nullable String> listOfNullsAndStrings = ...;

we'd have:

List</*@Nullable*/ String> listOfNullsAndStrings = ...;

The Checker Framework understands type annotations written like this, but I somewhat doubt other tools like IntelliJ IDEA, {Find,Spot}Bugs, and Google's internal tools understand them written like this too. Thus, we may not be able to use the Checker Framework's annotations in Guava until Android catches up.

If you did understand this already, apologies! Otherwise, I hope that this has cleared things up a bit. :)

@kashike
Copy link

@kashike kashike commented Oct 26, 2017

Thanks for the reply, @jbduncan. I'm also looking at what to use in my own projects (which are Java 8) - sorry for not stating this.

I've been using com.google.code.findbugs:jsr305 for a long time now, but this issue has made me want to switch to something new (which, in this case, is currently going towards org.checkerframework:checker-qual) for annotations on things (I annotate pretty much everything that can be annotated with @Nonnull or @Nullable, as well as @Immutable). checker-qual seems to have everything except @Immutable.

I'm trying to find a replacement that has everything I need, but I haven't been able to yet. Perhaps I'll delay switching until the Guava team comes to a solution for this issue in Guava.

PhilippWendler added a commit to sosy-lab/java-common-lib that referenced this issue Oct 27, 2017
The first release of SpotBugs is backwards compatible, so almost nothing
changes, but you need to run "ant spotbugs" now.

Furthermore, we need to add a dependency on jsr305 for the
javax.annotations package, which was previously included in the
findbugs-annotations package, but this was actually bad because Guava
also has a dependency on jsr305, so we ended up with the annotations
twice on the class path.
In the future, we will probably need to migrate away from these
annotations, as both SpotBugs and Guava consider:
spotbugs/spotbugs#130
spotbugs/spotbugs#180
google/guava#2960
@cpovirk
Copy link
Member

@cpovirk cpovirk commented Dec 11, 2017

Progress update + summary for people new to this:

@cgruber
Copy link
Contributor

@cgruber cgruber commented Dec 11, 2017

@cpovirk
Copy link
Member

@cpovirk cpovirk commented Dec 11, 2017

Hmm, good question. I have been hoping to just cut from one to the other, on the theory that tools can be updated to recognize both ahead of time. But if anyone knows of problems that we could avoid by having both present, let me know.

@garretwilson
Copy link

@garretwilson garretwilson commented May 15, 2020

If anyone reading this has a talent for participating constructively in
amorphous design decisions and likes to think deeply about nullness and
type systems, please reach out to me offline. My work address doesn't
include the "9n" part. …

@kevinb9n I just wanted to note that a week ago, in response to your call for participation, I reached out to you at what I believe to be your email address based upon your description, but I haven't heard back. If you would like to clarify how I can contact you, or if you would like to contact me, I stand ready to contribute to your project.

@soc
Copy link

@soc soc commented May 15, 2020

@garretwilson I think they just went ahead with their stuff at https://github.com/jspecify/jspecify.

@kevinb9n
Copy link
Contributor

@kevinb9n kevinb9n commented May 15, 2020

Yes, we've been working there for a while, and not much good will come of public attention yet at this point.

@soc
Copy link

@soc soc commented May 15, 2020

@kevinb9n Perhaps replying to that person's mail would have been a good way to achieve that? ;-)

@kevinb9n
Copy link
Contributor

@kevinb9n kevinb9n commented May 15, 2020

A very observant point.

Janmm14 added a commit to Janmm14/BungeeCord that referenced this issue Jul 18, 2020
… @UnmodifiableView

Mostly done only for public methods in the API and then used inspections to also mark methods of implementing classes.

Problem:
- Development of BungeeCord plugins in Kotlin makes nullability as uncertain as in java
- Certain methods in Configuration never return null although some might expect it
- While some IDEs like IntelliJ can infer a couple of these annotations (especially a couple Nullable/NotNull ones), it is not able to infer all annotations added in this commit

Solution:
Adding jetbrains-annotations dependency (apache license 2.0). It provides a couple useful annotations like @NotNull, @nullable and @contract
I've chosen to not use @lombok.NonNull as it'd add additional, unwanted null checks when added to parameters.
I've also seen that usage of javax annotations could lead to licensing problems ( google/guava#2960 ), also javax does not have @contract
@contract provides the ability to say which parameters affect nullability and failure of the method in some cases where it is clear. Also it allows to mark methods without side effects (pure), those calls can be removed if its return value is not used.
Janmm14 added a commit to Janmm14/BungeeCord that referenced this issue Jul 18, 2020
… @UnmodifiableView

Mostly done only for public methods in the API and then used inspections to also mark methods of implementing classes.

Problem:
- Development of BungeeCord plugins in Kotlin makes nullability as uncertain as in java
- Certain methods in Configuration never return null although some might expect it
- While some IDEs like IntelliJ can infer a couple of these annotations (especially a couple Nullable/NotNull ones), it is not able to infer all annotations added in this commit

Solution:
Adding jetbrains-annotations dependency (apache license 2.0). It provides a couple useful annotations like @NotNull, @nullable and @contract
I've chosen to not use @lombok.NonNull as it'd add additional, unwanted null checks when added to parameters.
I've also seen that usage of javax annotations could lead to licensing problems ( google/guava#2960 ), also javax does not have @contract
@contract provides the ability to say which parameters affect nullability and failure of the method in some cases where it is clear. Also it allows to mark methods without side effects (pure), those calls can be removed if its return value is not used.
Janmm14 added a commit to Janmm14/BungeeCord that referenced this issue Jul 18, 2020
… @UnmodifiableView

Mostly done only for public methods in the API and then used inspections to also mark methods of implementing classes.

Problem:
- Development of BungeeCord plugins in Kotlin makes nullability as uncertain as in java
- Certain methods in Configuration never return null although some might expect it
- While some IDEs like IntelliJ can infer a couple of these annotations (especially a couple Nullable/NotNull ones), it is not able to infer all annotations added in this commit

Solution:
Adding jetbrains-annotations dependency (apache license 2.0). It provides a couple useful annotations like @NotNull, @nullable and @contract
I've chosen to not use @lombok.NonNull as it'd add additional, unwanted null checks when added to parameters.
I've also seen that usage of javax annotations could lead to licensing problems ( google/guava#2960 ), also javax does not have @contract
@contract provides the ability to say which parameters affect nullability and failure of the method in some cases where it is clear. Also it allows to mark methods without side effects (pure), those calls can be removed if its return value is not used.
sciwhiz12 added a commit to ParchmentMC/Feather that referenced this issue May 12, 2021
google/guava#2960 describes the possible issues
with using JSR305. To avoid these entirely, we're now using Checker Framework's
annotations; specifically using the `checker-qual` artifact to only have the
annotations and not the checkers (compiler plugins).
@rossbu
Copy link

@rossbu rossbu commented Oct 27, 2021

it looks this issue has been sitting for a decade, and it surely will be another decade just to JSR Nullable and NotNull ?? what's going on? just because One guy William left Oracle, the whole industry can not create a unified standard ?. what a story!

@soc
Copy link

@soc soc commented Oct 27, 2021

@rossbu The approach and scope in their new project hasn't changed from the previous failed attempts. This architecture astronaut style of complexity has been the main reason for the lack of success of these annotations (despite tons of goodwill from basically everyone) til now. Until this is addressed, I wouldn't expect a different outcome.

@kevinb9n
Copy link
Contributor

@kevinb9n kevinb9n commented Oct 27, 2021

In any case, we're hard at work.

@cpovirk
Copy link
Member

@cpovirk cpovirk commented Oct 27, 2021

I've owed an update here since the 31.0 release, but it's been blocked behind some work to test out some improvement's in Kotlin's null-handling. For the moment, there's an update on our usage of nullness annotations at the bottom of the release notes for that version. We're trying to make changes gradually and without regressing the existing nullness information available to the various tools that consume it.

@jmott
Copy link

@jmott jmott commented Jan 7, 2022

My vulnerability scanners are now all complaining about Guava because the com.google.code.findbugs:jsr305:jar:3.0.2 dependency it pulls in is flagged for the recent log4j remote code execution vulnerabilities. Any update on this project to move away from jsr305? Thanks!

@ben-manes
Copy link
Contributor

@ben-manes ben-manes commented Jan 7, 2022

The poms for jsr305 and guava don't show a log4j dependency (at any scope). It sounds like a false positive.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet