google / guava

Google core libraries for Java
Apache License 2.0
49.99k stars 10.85k forks source link

Migrate off of jsr305 #2960

Open zyxist opened 6 years ago

zyxist commented 6 years ago

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:

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:

https://github.com/spotbugs/spotbugs/pull/180

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

ronshapiro commented 6 years ago

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

@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 commented 6 years ago

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:

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

ronshapiro commented 6 years ago

JSR-250 contains other annotations, too

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

cpovirk commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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

kashike commented 6 years ago

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 commented 6 years ago

@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 commented 6 years ago

...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 commented 6 years ago

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 commented 6 years ago

... 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 commented 6 years ago

@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 commented 6 years ago

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.

cpovirk commented 6 years ago

Progress update + summary for people new to this:

cgruber commented 6 years ago

Will there be a version of guava that uses both old and new in the interim, or will this be a hard cut off between versions?

On Mon, Dec 11, 2017, 5:16 AM Chris Povirk notifications@github.com wrote:

Progress update + summary for people new to this:

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/google/guava/issues/2960#issuecomment-350721044, or mute the thread https://github.com/notifications/unsubscribe-auth/AAUN4khif1bh5T6FtA7pnt9PQdEyyEdYks5s_SsAgaJpZM4PxSdM .

cpovirk commented 6 years ago

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.

orionll commented 6 years ago

FYI: it is impossible to use guava and java.xml.ws modules simultaneously:

module foo {
    requires com.google.common;
    requires java.xml.ws;
}

This code will not compile because there is a split package in jsr305 and javax.xml.ws.annotation modules.

orionll commented 6 years ago

Update to my previous comment. It is not impossible, however, this requires exclusion of jsr305 dependency:

<dependency>
    <groupId>com.google.guava</groupId>
    <artifactId>guava</artifactId>
    <version>25.0-jre</version>
    <exclusions>
        <exclusion>
            <groupId>com.google.code.findbugs</groupId>
            <artifactId>jsr305</artifactId>
        </exclusion>
    </exclusions>
</dependency>
garretwilson commented 5 years ago

I was part of the related discussion at #1018 about whether the JSR-305 should be "optional" or "provided", and thankfully (if I remember correctly) we all settled on "optional". But I remembered that the larger question came up of whether JSR-305 should be replaced altogether to accommodate Java 9, so I've been worrying about what to replace JSR-305 with in our own code at some point.

So today I did a little search and, lo and behold, it would appear from https://stackoverflow.com/a/37911663/421049 that Java changed the modules so that we can use JSR-305 in Java 9+. Does this mean that this is no longer an issue, and that we might as well stick with JSR-305? Sorry if I'm not up-to-date; maybe a little summary would help us all.

orionll commented 5 years ago

JSR-305 has a common package with Common Annotations which is a platform module in Java 9-10 (java.xml.ws.annotation). In Java 11 it is not a platform module anymore, but it still shares the same package with JSR-305. So, I think, we still need to get rid of JSR-305 to avoid split packages when someone is using both guava and Common Annotations.

cushon commented 5 years ago

(on preview, I'm mostly agreeing with @orionll)

It's true that the module that includes javax.annotation isn't included in the "default set of root modules", but it's still part of the distribution for JDK 9 and 10 and can be enabled with --add-modules=java.xml.ws.annotation. (I think java.annotations.common was renamed after the stack overflow answer.)

So given:

class T {
  public static void main(String[] args) throws Exception {
    for (String c : args) {
      System.err.println(Class.forName(c));
    }
  }
}

This works:

$ java -cp .:jsr305.jar T javax.annotation.Nullable
interface javax.annotation.Nullable

... but this doesn't work with JDK 9 or 10:

java -cp .:jsr305.jar --add-modules=java.xml.ws.annotation T javax.annotation.Nullable
Exception in thread "main" java.lang.ClassNotFoundException: javax.annotation.Nullable
garretwilson commented 5 years ago

So is there a consensus on what the replacement will be? Do you have specific Maven coordinates on what you'd replace it with? (Excuse the question. I know I could peruse all the threads to try to figure that out, but even that answer might not be up-to-date. So I thought maybe it would be useful just to summarize.)

garretwilson commented 5 years ago

So is there a consensus on what the replacement will be?

You guys don't know yet, I take it?

orionll commented 5 years ago

@garretwilson Please see this thread. The only remaining annotation is javax.annotation.ParametersAreNonnullByDefault which is used only in package-info.java. Any ideas how to get rid of it without breaking static analysis in IDEA?

garretwilson commented 5 years ago

So I guess the implicit answer to my question is "Guava has decided to migrate to the Checker Framework".

I can't answer the question about javax.annotation.ParametersAreNonnullByDefault. We're mostly only using JSR 305's @Nonnull and @Nullable annotations as a means of documentation (as it is much more concise, clearer, and certainly easier than adding @throws NullPointerException … everywhere) and to encourage the developer to at least think about this when creating an API.

For our purposes it's not clear to me the Checker Framework would be an improvement; I'm not even clear on which Maven dependency to use to get just the annotations. (Update: looking inside the JARs, it appears that org.checkerframework:checker-qual is the dependency with the annotations, but wow, that includes a lot of other things as well.) Adding null-checking tools to the build might be useful, but in our projects we're not there yet.

In short, to me there doesn't seem to be as big a motivation to switch, now that JSR 305 causes fewer module problems and there still seems to be lacking a consensus on a successor. For now it seems this small, optional dependency works for us. But I'm very interested in watching how successful Guava is in switching. (The fact that it's taking so long for Guava to migrate and that there's so much confusion regarding the subject is another warning to me that it might be better for our projects to stay with JSR 305 for now.)

bbottema commented 5 years ago

Any new insights regarding this issue and what is a good alternative to avoid the split package problem (regardless whether it's Java 9 or 11)? I'm still searching for a proper replacement candidate (a @Nullable and @NonNull / @NotNull with runtime retention et all). My library is stuck on JDK 7, due to the target audience..

kashike commented 5 years ago

checker-qual

bmarwell commented 5 years ago

Agreed. checker-qual is the single-best alternative available and can be set as optional dependency. Sadly, the checker framework itself still struggles with java 8 compatiblity (not all tests are working atm), java 9-11 is a whole new story.

bmarwell commented 5 years ago

This doesn't mean that guava will be unable to support newer versions.

@bbottema take a look at https://github.com/glowroot/glowroot/tree/master/build/checker-jdk6 (sorry for off-topic).

tlinkowski commented 5 years ago

In case it interests anyone, 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.

bbottema commented 4 years ago

Just an FYI: with all the conflicting information and many discussions regarding nullability annotations, I never realized that the JSR-305 annotation classes actually should not be subject to the split package problem in JDK 9 and up, as Mark Reinhold indicates referring to JEP 261.

cushon commented 4 years ago

@bbottema the change Mark mentioned in the SO answer allows using either javax.annotation.Generated from java.xml.ws.annotation, or javax.annotation.Nullable from jsr305.jar, but not both. There's still a split package issue here.

Demo:

class T {
  public static void main(String[] args) throws Exception {
    for (String c : args) {
      Class<?> clazz = Class.forName(c);
      System.err.println(clazz + " " + clazz.getClassLoader());
    }
  }
}

With Java 9.0.4, loading javax.annotation.Nullable from jsr305.jar works:

$ java -cp jsr305-3.0.2.jar:. T javax.annotation.Nullable
interface javax.annotation.Nullable jdk.internal.loader.ClassLoaders$AppClassLoader@1b9e1916

Loading javax.annotation.Generated from java.xml.ws.annotation works:

$ java --add-modules=java.xml.ws.annotation T javax.annotation.Generated
interface javax.annotation.Generated jdk.internal.loader.ClassLoaders$PlatformClassLoader@16f65612

Loading both annotations doesn't work, because of the split package issue:

$ java -cp jsr305-3.0.2.jar:. --add-modules=java.xml.ws.annotation T javax.annotation.Generated javax.annotation.Nullable
interface javax.annotation.Generated jdk.internal.loader.ClassLoaders$PlatformClassLoader@16f65612
Exception in thread "main" java.lang.ClassNotFoundException: javax.annotation.Nullable
orionll commented 4 years ago

Today I found that I can't use both Guava and JAX-WS. The following configuration doesn't work:

pom.xml:

<!-- Guava -->
<dependency>
    <groupId>com.google.guava</groupId>
    <artifactId>guava</artifactId>
    <version>28.1-jre</version>
</dependency>

<!-- JAX-WS -->
<dependency>
    <groupId>jakarta.xml.ws</groupId>
    <artifactId>jakarta.xml.ws-api</artifactId>
    <version>2.3.2</version>
</dependency>

module-info.java:

module java11 {
    requires com.google.common;
    requires java.xml.ws;
}

Immediately after launching, I get this error:

Error occurred during initialization of boot layer
java.lang.module.ResolutionException: Module jsr305 contains package javax.annotation, module java.annotation exports package javax.annotation to jsr305
netdpb commented 4 years ago

@orionll Have you tried the patching workaround mentioned by @tlinkowski in https://github.com/google/guava/issues/2960#issuecomment-522446788?

ejona86 commented 4 years ago

Shouldn't javax.annotation.Generated be a simpler case since it uses RetentionPolicy.SOURCE?

kevinb9n commented 4 years ago

Everyone: I'm sorry the state of things re: nullness annotations is such a disaster. For better or worse, all I can tell you is that cpovirk and I (and many others) are working on the long term solution: a proper artifact that will be embraced by all major tools, with clear, strong specifications. Alas, this will take time to come to fruition and in the meantime things will continue to be a mess.

We don't have public materials you can review yet at this time.

garretwilson commented 4 years ago

… cpovirk and I (and many others) are working on the long term solution: a proper artifact that will be embraced by all major tools, with clear, strong specifications.

This is great news, and comforting just knowing that there is finally an effort to produce such a thing, as it is sorely needed. The community is grateful and probably most don't want to rush things, but it would great if there was some way we could track the process and know the status. There's always the fear that these sorts of efforts, especially if they are hidden, might stall, die, dry up, and blow away (and we may never know, thinking it was still ongoing). How can we follow the effort and provide moral support and encouragement?

We don't have public materials you can review yet at this time.

And that lends to the fears I mentioned. "If there's no picture, it didn't happen" sort of thing. Even a web page with a status, milestones, and a periodic blog entry would be a super start.

kevinb9n commented 4 years ago

Very glad to hear that response!

I can probably give people who contact me offline (kevinb at google) a peek at the slides we presented at JVMLS in July.

The subject is so absurdly complex that it's a struggle to manage the discussions, which struggle varies exponentially with the number of participants; but we know that we need to go public soon and are laboring to prepare ourselves for that.

soc commented 4 years ago

@kevinb9n What's your timeline?

I'm on the verge of shipping/pushing my own stuff under nu.llable.

Would be great to know who and how many cooks are involved in your efforts.

kevinb9n commented 4 years ago

The group includes Google, JetBrains, Eclipse, SonarSource, Facebook, Uber, Pivotal (Spring), people representing SpotBugs and Checker Framework, and a few others. [EDIT: also reviewing our plans periodically with Oracle.]

I hope very much that we're going public within a month and 1.0 by EOY, but it is hard to promise anything.

soc commented 4 years ago

@kevinb9n I guess I can wait that month, assuming it's truly vendor-neutral and doesn't ship in some atrocious namespace like edu.umd.cs.findbugs.annotations.

carlsagan21 commented 4 years ago

Waiting for the new "nullable" become public. Is there any channel to subscribe any updates about the project beside this Issue thread?

soc commented 4 years ago

Month is up, any news?

If you have trouble getting things done (too many cooks, etc.), I can also publish my library for public use instead.

cpovirk commented 4 years ago

Kevin is out this week, so a small update:

We've been sorting out things like licensing and naming (including trademark searches and setting up domains and a Github organization). It's been progressing fine, but "going public" is looking more like around the end of the month.

We're also starting to try to compile more a formal list of what we need for a 1.0 release. In theory, that will help us estimate when we'll have an actual spec and artifact. (I personally suspect not this year, but it's hard to say even with the list and harder to say without.)

cpovirk commented 4 years ago

(We should soon have a public mailing list for announcements. I'll post here once that happens.)

cpovirk commented 4 years ago

Nullability annotations update:

soc commented 4 years ago

How is your project is going along, Chris?


For what it's worth, I pushed the first sketch of @notnull, @nullable, @immutable and @mutable annotations to github.com/soc/annotation and registered the domain annotation.id (no content yet).

I'll probably publish the artifacts in a few days.

berry120 commented 4 years ago

@cpovirk @kevinb9n I hate to be "that guy" again asking for a status update, but... any status update? I'm someone else that's really excited by finally having the prospect of an accepted standard library for this, and I'm certainly prepared to wait for it. Just want to make sure the efforts are progressing still and it's not dried up.

Thanks very much for your efforts & work on this so far, whatever state we're at.

cpovirk commented 4 years ago

It is still the main priority for the two of us for this quarter and at least into next year.

We keep scheduling discussions about our public presence but not having time for them. Arguably that's good news: It means we've spent a bunch of time talking about nullness itself. But of course it's always discouraging when we're not getting to something that we planned to, so maybe it's bad news? In either case, the goal of going public in the near future is probably not happening, but work continues.