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

No more JSR305, use SpotBugs internal annotation instead #180

Merged
merged 7 commits into from Oct 5, 2017

Conversation

Projects
None yet
7 participants
@KengoTODA
Copy link
Member

commented May 25, 2017

This PR will close #130 and cancel deprecation on SpotBugs annotations.
Let's have discussion about we should cancel this deprecation or not.

I think we don't need to recommend jsr305 in current situation, but this change will affect public interface so I'm marking this PR for SpotBugs 4.0.0 release. Please do not merge this PR for now.

@KengoTODA KengoTODA added this to the SpotBugs 4.0.0 milestone May 25, 2017

@KengoTODA KengoTODA self-assigned this May 25, 2017

@jsotuyod

This comment has been minimized.

Copy link
Member

commented May 27, 2017

I'm ok with ceasing to publish jsr305, and removing those from our codebase.

As for reintroducing the old annotations, I'm ok with it, but why not simply telling users to adopt the annotations from JSR-308 (checker framework), which are an approved and published JSR that includes both @Nullable and @NonNull annotations? Using SpotBugs is still easier than using the checker framework for compile-time validations.

@iloveeclipse

This comment has been minimized.

Copy link
Member

commented Jul 1, 2017

@stephenc

This comment has been minimized.

Copy link

commented Jul 4, 2017

why not simply telling users to adopt the annotations from JSR-308 (checker framework), which are an approved and published JSR that includes both @nullable and @nonnull annotations?

One thing is that the checker framework actually includes 10 different @NonNull annotations (https://checkerframework.org/api/) so that is not going to help user confusion.

Another thing is that presumably spotbugs will want to pick up some enhanced checks that leverage JSR-308 in which case - presumably - spotbugs would catch any of the bugs in checker and more besides.

The final thing is that, at least for these type of annotations, the fewer dependencies in the annotation JAR the better. Zero dependencies is ideal, so pushing people to checker for the Nullable / NonNull / CheckForNull annotations but keeping them with spotbugs for the OverrideMustInvoke, etc is not a good plan IMHO

@@ -33,15 +33,12 @@
*
* When this annotation is applied to a method it applies to the method return
* value.
*
* @deprecated - use {@link javax.annotation.CheckForNull} instead.
**/
@Documented
@Target({ ElementType.FIELD, ElementType.METHOD, ElementType.PARAMETER, ElementType.LOCAL_VARIABLE })
@Retention(RetentionPolicy.CLASS)
@javax.annotation.Nonnull(when = When.MAYBE)

This comment has been minimized.

Copy link
@stephenc

stephenc Jul 19, 2017

I think these should probably be removed if removing "JSR-305" stuff

This comment has been minimized.

Copy link
@KengoTODA

KengoTODA Sep 27, 2017

Author Member

It seems really hard to delete dependency on jsr-305, or we cannot support feature depending on @TypeQualifierDefault such as ReturnValuesAreNonnullByDefault.

This comment has been minimized.

Copy link
@stephenc

stephenc Sep 27, 2017

If you need defaults, you need your own defaults annotation. There is no JSR 305 and this there are no annotations from it until the JSR page publishes at least a draft javadocs.

@@ -32,15 +32,12 @@
*
* Annotated Fields must only not be null after construction has completed.
* Annotated methods must have non-null return values.
*
* @deprecated - use {@link javax.annotation.Nonnull} instead.
**/
@Documented
@Target({ ElementType.FIELD, ElementType.METHOD, ElementType.PARAMETER, ElementType.LOCAL_VARIABLE })
@Retention(RetentionPolicy.CLASS)
@javax.annotation.Nonnull(when = When.ALWAYS)

This comment has been minimized.

Copy link
@stephenc

stephenc Jul 19, 2017

more "JSR-305" stuff that should probably be removed

@@ -36,15 +36,12 @@
*
* When this annotation is applied to a method it applies to the method return
* value.
*
* @deprecated - use {@link javax.annotation.Nullable} instead.
**/
@Documented
@Target({ ElementType.FIELD, ElementType.METHOD, ElementType.PARAMETER, ElementType.LOCAL_VARIABLE })
@Retention(RetentionPolicy.CLASS)
@javax.annotation.Nonnull(when = When.UNKNOWN)

This comment has been minimized.

Copy link
@stephenc

stephenc Jul 19, 2017

more "JSR-305" stuff that should probably be removed

@KengoTODA

This comment has been minimized.

Copy link
Member Author

commented Jul 24, 2017

@iloveeclipse Does JDT annotations provide build tool integration or static analysis integration?
If not, I think Checkerframework Qualifier is better to choose.

@iloveeclipse

This comment has been minimized.

Copy link
Member

commented Jul 24, 2017

Does JDT annotations provide build tool integration or static analysis integration?

What do you mean by that? It is just plain java annotations in a library. One can use it as one likes, but they do not do anything except being annotations.

If not, I think Checkerframework Qualifier is better to choose.

Sorry, I've found no obvious links on their homepage (https://checkerframework.org/) pointing to the annotations library they use. Can you please provide a direct link?

@KengoTODA KengoTODA force-pushed the KengoTODA:fix-130 branch from 51e941f to 16aa35c Sep 27, 2017

@@ -7,7 +7,7 @@ apply from: "$rootDir/gradle/javadoc.gradle"
apply from: "$rootDir/gradle/maven.gradle"

dependencies {
compile 'com.google.code.findbugs:jsr305:3.0.1'
compile 'com.google.code.findbugs:jsr305:3.0.2'

This comment has been minimized.

This comment has been minimized.

Copy link
@stephenc

stephenc Sep 27, 2017

Any jar that has classes in java or javax packages violates the Oracle JVM redistribution license. By adding back such a dependency you restore this limitation

This comment has been minimized.

Copy link
@KengoTODA

KengoTODA Sep 27, 2017

Author Member

OK then delete ReturnValuesAreNonnullByDefault and ask users to migirate to DefaultAnnotation or others 👍

This comment has been minimized.

Copy link
@KengoTODA

KengoTODA Sep 27, 2017

Author Member

Not only ReturnValuesAreNonnullByDefault but also CheckForNull needs migration, because current spotbugs-annotation does not support When. and TypeQualifierNickname.

If the distribution is the problem, we can change this dependency to compileOnly, then our distribution package will not include jsr-305-3.0.1.jar.

This comment has been minimized.

Copy link
@KengoTODA

KengoTODA Sep 27, 2017

Author Member

Oops, TypeQualifierAnnotation depends on When in jsr-305...

This comment has been minimized.

Copy link
@KengoTODA

KengoTODA Sep 27, 2017

Author Member

Note: here is license http://www.oracle.com/technetwork/java/javase/terms/license/index.html

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.

@stephenc Who violates this is not us but jsr305.jar, is it also bad to depend on it? What do you mean by "you restore this limitation"?

This comment has been minimized.

Copy link
@stephenc

stephenc Sep 27, 2017

When a class is loaded, if the annotation is not present... no foul...

Now what happens if there is a reboot JSR that superseded JSR 305, and it adds javax.annotation.Nonnull, but decided to use value for the when clause so that it is @Nonnull(___) rather than forcing everyone to type in @Nonnull(when=___)... save 5 characters many times... that's a win...

In that case when a class is loaded... and that includes an annotation... you will have the entire class fail to load as the annotation is present but the signature is invalid.

You have to be exceedingly careful in how you evolve the fields in an annotation.

Oracle are aware of this issue, which is why they have that restriction, because if they do not keep the guard of the "must be published as part of a JSR process" then it would be their fault for releasing a JSR-xyz that had the alternative javax.annotation.Nonnull.

Right now, however, there is no publication from JSR-305, so it will be your fault if you have those class references in your class files.

Additionally, a really nit-picking lawyer told me that the classloading failure described above could be interpreted as "change the behavior of"... so if you are distributing an oracle JVM, then - in his overly pedantic view - you should not even have those annotations referenced in your class files...

now that last bit is not an argument that I personally put much strength in... but I do think that at some point in time there may be a JSR-305 reboot, and we do not know what they would produce from that, hence we should not be preparing to shoot ourselves in the foot, especially given that the IDE that redistribute a JVM in their installers have gone to great lengths not to include those annotations in their classfiles... indicating to me that they share somewhat similar concerns

This comment has been minimized.

Copy link
@KengoTODA

KengoTODA Sep 29, 2017

Author Member

In my personal opinion, we can keep current dependency for 3.1.0 release, to keep SpotBugs as much as same with FindBugs. Of course, we all know this dependency is negative legacy, we should have issue for 4.0.0 to delete them.

@KengoTODA KengoTODA force-pushed the KengoTODA:fix-130 branch from 16aa35c to e3f7f6a Sep 27, 2017

@KengoTODA

This comment has been minimized.

Copy link
Member Author

commented Sep 27, 2017

@iloveeclipse

Sorry, I've found no obvious links on their homepage (https://checkerframework.org/) pointing to the annotations library they use. Can you please provide a direct link?

https://mvnrepository.com/artifact/org.checkerframework/checker-qual

Especially for nullness handling: https://checkerframework.org/api/org/checkerframework/checker/nullness/qual/package-summary.html

@KengoTODA KengoTODA changed the title RFC: No more JSR305, use SpotBugs internal annotation instead No more JSR305, use SpotBugs internal annotation instead Sep 27, 2017

@KengoTODA KengoTODA removed the help wanted label Sep 27, 2017

@KengoTODA KengoTODA requested a review from iloveeclipse Sep 27, 2017

@seamlik

This comment has been minimized.

Copy link

commented Sep 29, 2017

Forgive me if I'm not familiar with the history of FindBugs, but I guess JSR-305 was (or still considered is) a sub-project (or partner project) of FindBugs. I don't know why JSR-305 was abandoned in the first place, but since SpotBugs is going to supersede FindBugs, why not also adopt JSR-305 and keep it maintained?

@KengoTODA

This comment has been minimized.

Copy link
Member Author

commented Sep 29, 2017

JSR305 is not sub-project nor partner project of FindBugs, but its spec lead is William Pugh the author of FindBugs. I'm personally not sure the historical background about this.

This spec is already marked as Dormant, so we have no motivation to keep it maintained.

refs #130: make ReturnValuesAreNonnullByDefault not deprecated
according to commit comment of d3b4194,
it is not reasonable to keep this annotation as deprecated.
We've made other annotations updated in that commit not deprecated,
then this annotation should be marked as non-deprecated too.
@henrik242
Copy link
Contributor

left a comment

I guess this looks good now

@henrik242 henrik242 merged commit 75326d3 into spotbugs:master Oct 5, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

PhilippWendler added a commit to sosy-lab/java-common-lib that referenced this pull request Oct 27, 2017

Replace FindBugs with its successor SpotBugs.
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

vorburger added a commit to vorburger/jetcd that referenced this pull request Nov 7, 2018

lburgazzoli added a commit to etcd-io/jetcd that referenced this pull request Nov 7, 2018

@buzz3791

This comment has been minimized.

Copy link

commented Nov 15, 2018

JSR305 is not sub-project nor partner project of FindBugs, but its spec lead is William Pugh the author of FindBugs. I'm personally not sure the historical background about this.

This spec is already marked as Dormant, so we have no motivation to keep it maintained.

A bit more info about JSR 305... on Jul 25, 2018, Mark Reinhold Chief Architect, Java Platform Group at Oracle wrote, "JSR 305, which would’ve standardized javax.annotation.NonNull, never completed because its spec lead [William Pugh] went AWOL. It had nothing to do with any decision by Oracle.", source: Mark commented on Ludwig Weinzierl's Stackoverflow answer to jaxzin's question "Which @NotNull Java annotation should I use?
", https://stackoverflow.com/a/42695253/110126.

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