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
@@ -6,6 +6,10 @@ Currently the versioning policy of this project follows [Semantic Versioning v2.

## Unreleased

### Changed

* SpotBugs annotation is recommended instead of JSR305 annotation ([#130](https://github.com/spotbugs/spotbugs/pull/130))

### Fixed

* Wrong Class-Path in MANIFEST.MF ([#407](https://github.com/spotbugs/spotbugs/pull/407))
@@ -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.

}

def manifestSpec = manifest {
@@ -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.

@TypeQualifierNickname
@Deprecated
public @interface CheckForNull {

}
@@ -29,13 +29,10 @@
* be checked when invoking the method.
*
* The checker treats this annotation as inherited by overriding methods.
*
* @deprecated - use {@link javax.annotation.CheckReturnValue} instead.
*/
@Documented
@Target({ ElementType.METHOD, ElementType.CONSTRUCTOR })
@Retention(RetentionPolicy.CLASS)
@Deprecated
public @interface CheckReturnValue {

@Deprecated
@@ -19,8 +19,6 @@

package edu.umd.cs.findbugs.annotations;

import javax.annotation.Nonnull;

/**
* Describes the confidence with which FindBugs reports a bug instance.
*/
@@ -33,7 +31,7 @@
private final int confidenceValue;

/** Given a numeric confidence value, report the corresponding confidence enum value */
@Nonnull
@NonNull
static public Confidence getConfidence(int prio) {
for(Confidence c : values()) {
if (prio <= c.confidenceValue) {
@@ -41,28 +41,12 @@
* package, and then use @Nullable only on those parameters, methods or fields
* that you want to allow to be null.
*
* @deprecated - Use the JSR305 annotations instead.
* For example, you can use {@link javax.annotation.ParametersAreNonnullByDefault} instead
* of @DefaultAnnotation(NonNull.class) so that method parameters are nonnull by default in the annotated
* element. You can also use {@link javax.annotation.meta.TypeQualifierDefault}
* in general to define your own annotation that specifies a default type qualifier. For example,
* <br><pre><code>
* {@link Nonnegative}
* {@link TypeQualifierDefault}({@link ElementType#PARAMETER})
* public @interface ParametersAreNonnegativeByDefault {}
* </code></pre>
*
* <br>The JSR305 {@link javax.annotation.CheckReturnValue}
* annotation can be applied to a type or package, and it will act as a default for all methods
* in that class or package unless otherwise overridden.
*
* @author William Pugh
*/

@Documented
@Target({ ElementType.TYPE, ElementType.PACKAGE })
@Retention(RetentionPolicy.CLASS)
@Deprecated
public @interface DefaultAnnotation {
Class<? extends Annotation>[] value();

@@ -43,7 +43,6 @@
@Documented
@Target({ ElementType.TYPE, ElementType.PACKAGE })
@Retention(RetentionPolicy.CLASS)
@Deprecated
public @interface DefaultAnnotationForFields {
Class<? extends Annotation>[] value();

@@ -43,7 +43,6 @@
@Documented
@Target({ ElementType.TYPE, ElementType.PACKAGE })
@Retention(RetentionPolicy.CLASS)
@Deprecated
public @interface DefaultAnnotationForMethods {
Class<? extends Annotation>[] value();

@@ -37,20 +37,12 @@
* package, and then use @Nullable only on those parameters, methods or fields
* that you want to allow to be null.
*
* @deprecated - use the JSR305 annotations instead,
* For example, you can use {@link javax.annotation.ParametersAreNonnullByDefault} instead
* of @DefaultAnnotation(NonNull.class), and {@link javax.annotation.meta.TypeQualifierDefault}
* in general to define a type qualifier default. The JSR305 {@link javax.annotation.CheckReturnValue}
* annotation can be applied to a type or package, and it will act as a default for all methods
* in that class or package unless otherwise overridden.
*
* @author William Pugh
*/

@Documented
@Target({ ElementType.TYPE, ElementType.PACKAGE })
@Retention(RetentionPolicy.CLASS)
@Deprecated
public @interface DefaultAnnotationForParameters {
Class<? extends Annotation>[] value();

@@ -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

@TypeQualifierNickname
@Deprecated
public @interface NonNull {

}
@@ -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

@TypeQualifierNickname
@Deprecated
public @interface Nullable {

}
@@ -35,11 +35,8 @@
* overriding method.
*
* @see edu.umd.cs.findbugs.annotations.When
*
* @deprecated - Use {@link javax.annotation.OverridingMethodsMustInvokeSuper} instead
**/
@Documented
@Deprecated
@Target({ ElementType.METHOD })
@Retention(RetentionPolicy.CLASS)
public @interface OverrideMustInvoke {
@@ -37,12 +37,10 @@
* annotation of the corresponding parameter in the superclass applies)
* <li>there is a default annotation applied to a more tightly nested element.
* </ul>
*
*/
@Documented
@Nonnull
@TypeQualifierDefault(ElementType.METHOD)
@Retention(RetentionPolicy.RUNTIME)
@Deprecated
public @interface ReturnValuesAreNonnullByDefault {
}
@@ -36,7 +36,6 @@
@Retention(RetentionPolicy.CLASS)
@javax.annotation.Nonnull(when = When.UNKNOWN)
@TypeQualifierNickname
@Deprecated
public @interface UnknownNullness {

}
@@ -1,18 +1,6 @@
/**
* Annotations for FindBugs (mostly deprecated except for {@link edu.umd.cs.findbugs.annotations.SuppressFBWarnings}).
*
* This annotations are mostly deprecated and replaced by JSR 305 annotations
* defined in javax.annotation. The annotations still actively supported are:
* <ul>
* <li> {@link edu.umd.cs.findbugs.annotations.SuppressFBWarnings} for suppressing FindBugs warnings
* <li> Annotations about expected/unexpected warnings in FindBugs regression tests
* <ul>
* <li> {@link edu.umd.cs.findbugs.annotations.ExpectWarning} Warnings expected to be generated
* <li> {@link edu.umd.cs.findbugs.annotations.NoWarning} Warnings that should not be generated
* <li> {@link edu.umd.cs.findbugs.annotations.DesireWarning} Warnings we wish to generated
* <li> {@link edu.umd.cs.findbugs.annotations.DesireNoWarning} Warnings we wish to not generate generated
* </ul></ul>
*
* There are another set of annotations used by an experimental detector for unclosed resources:
* <ul>
* <li>{@link edu.umd.cs.findbugs.annotations.CleanupObligation}
@@ -22,4 +10,3 @@
*/
package edu.umd.cs.findbugs.annotations;

@@ -295,7 +295,6 @@ uploadArchives {
ext.moduleName = 'com.github.spotbugs.spotbugs'
apply from: "$rootDir/gradle/jigsaw.gradle"

// TODO : jsr305.jar (really?)
// TODO : generatemanual (we should decide what to do with the manual)
// TODO : generatepdfmanual
// TODO : bugdesc
ProTip! Use n and p to navigate between commits in a pull request.
You can’t perform that action at this time.