New check: Jsr305Annotations

[mbert]

The Jsr305 annotations (annotations for software defect detection) contain a subset of "nullness" annotations that can be used to mark parameters as possibly null (@Nullable) or always non-null (@Nonnull), function return values as to be checked for null (@CheckForNull) or always non-null (@Nonnull) including defaults on class level (@ParametersAreNonnullByDefault, @ParametersAreNullableByDefault, @ReturnValuesAreNonnullByDefault).

Using these annotations a programmer can declare how the code is meant to behave, and static code analysis (like e.g. FindBugs) can be used to verify that this is actually true. Also these annotations help others understanding code more easily, e.g. if confrontend with an annotated interface the necessity for null checks can easily be deducted from the annotations.

The Jsr305AnnotationsCheck supports enforcing the following code style:

1. Every method declaration, implementation or lambda requires nullness annotations for all parameters and return values except for primitive types (because a void or an int can never be null anyway).
2. The annotation can be made directly or applied through either inheritance from an already annotated class or a annotation for class-wide defaults.
3. Annotations need to make sense. For instance, a class-scope annotation cannot be used for a method, and a method annotation cannot be used for a class.
4. In overridden methods, the following rule applies (regardless of what was annotated in the parent method): For parameter definitions @Nonnull annotation is always illegal because being less "nullable" cannot be assumed for a parameter in an inherited method. Conversely @Nullable is always allowed. For return values it is the other way round: @CheckForNull is always illegal while @Nonnull is always legal.

Here are a few examples:

// no class-level annotation
class Class1 {
    @CheckForNull // Error: obj2 not annotated!
    String method1(@Nullable Object obj1, Object obj2) { ... }
    // Error: return value not annotated
    String method2() { ... }
}

// with class-level defaults
@ParametersAreNonnullByDefault
class Class2 {
    @CheckForNull // Legal
    String method1(Object obj1, Object obj2) { ... }
    @Nonnull // Legal
    String method2(Object obj1, @Nullable Object obj2) { ... }
    @Nonnull // Error, redefinition of obj2's nullness
    String method3(Object obj1, @Nonnull Object obj2) { ... }
    // Error: return value not annotated
    String method4() { ... }
}

// annotations apply across inheritance
@ParametersAreNonnullByDefault
interface Interface1 {
    @Nonnull
    Object method1(Object obj1, Object obj2);
    @CheckForNull
    Object method2(@Nullable Object obj1, Object obj2);
    @Nonnull
    Object method3(@Nullable Object obj1, Object obj2);
    @Nonnull
    Object method4(@Nullable Object obj1, Object obj2);
    @Nonnull
    Object method5();
}
class Class3 implements Interface1 {
    @Override // Legal
    public Object method1(Object obj1, Object obj2) { ... }
    @Override 
    @Nonnull // Legal, return value becomes less "nullable"
    public Object method2(Object obj1, Object obj2) { ... }
    @Override // Error: Setting a parameter to non-null in an overridden method is illegal
    public Object method3(@Nonnull Object obj1, Object obj2) { ... }
    @Override // Legal: parameter obj2 becomes more "nullable"
    public Object method4(Object obj1, @Nullable Object obj2) { ... }
    @Override 
    @CheckForNull // Error: return value becomes more "nullable"
    public Object method5() { ... }
}

The rules for treating inherited methods' return values and parameters can be relaxed, which can be useful for transitional periods when introducing this check to existing code. Having set the properties allowOverridingReturnValue and allowOverridingParameter to true, in the example above all method definititions for Class3 will be legal.

To get a fuller picture of the possible warnings and the corresponding code, I have appended the test classes from the check's unit test (they are also in the PR). This is the output the check generates for them:

ParameterTestObject.java:35:44: No nullness Annotation for parameter definition found!
ParameterTestObject.java:40:45: No nullness Annotation for parameter definition found!
ParameterTestObject.java:40:64: No nullness Annotation for parameter definition found!
ParameterTestObject.java:73:41: Parameter definitions dont need checking, use @Nullable or @Nonnull.
ParameterTestObject.java:79:27: It is not allowed to increase nullness constraint for overriden method parameter definitions!
ParameterTestObject.java:97:35: @Nonnull and @Nullable are not allowed together!
PrimitivesTestObject.java:29:37: Primitives must not have any nullness annotations!
PrimitivesTestObject.java:34:38: Primitives must not have any nullness annotations!
PrimitivesTestObject.java:39:42: Parameter definitions dont need checking, use @Nullable or @Nonnull.
PrimitivesTestObject.java:44:5: Primitives must not have any nullness annotations!
PrimitivesTestObject.java:50:5: Primitives must not have any nullness annotations!
PrimitivesTestObject.java:56:5: @Nullable is not allowed on method return values!
PrimitivesTestObject.java:62:5: Primitives must not have any nullness annotations!
PrimitivesTestObject.java:69:5: Primitives must not have any nullness annotations!
PrimitivesTestObject.java:116:34: Primitives must not have any nullness annotations!
ClassTestObject.java:33:5: @ParametersAreNullableByDefault and @ParametersAreNonnullByDefault are not allowed together!
ClassTestObject.java:47:26: It is not necessary to annotate @Nullable if you annoted the method or class with @ParametersAreNullableByDefault.
ClassTestObject.java:57:29: No nullness Annotation for parameter definition found!
ClassTestObject.java:79:32: It is not necessary to annotate @Nonnull if you annotated the method or class with @ParametersAreNonnullByDefault.
ClassTestObject.java:87:36: No nullness Annotation for parameter definition found!
ClassTestObject.java:141:43: No nullness Annotation for parameter definition found!
ClassTestObject.java:148:9: It is not necessary to annotate @Nonnull if you annoted the class with @ReturnValuesAreNonnullByDefault.
ClassTestObject.java:152:9: @ReturnValuesAreNonnullByDefault is not allowed on method return values!
ClassTestObject.java:163:9: It is not necessary to annotate @Nonnull if you annoted the class with @ReturnValuesAreNonnullByDefault.
ClassTestObject.java:169:9: @ReturnValuesAreNonnullByDefault is not allowed on method return values!
ClassTestObject.java:187:9: @ReturnValuesAreNonnullByDefault is not allowed on method return values!
EnumTestObject.java:45:32: It is not necessary to annotate @Nonnull if you annotated the method or class with @ParametersAreNonnullByDefault.
EnumTestObject.java:54:36: No nullness Annotation for parameter definition found!
EnumTestObject.java:71:9: It is not necessary to annotate @Nonnull if you annoted the class with @ReturnValuesAreNonnullByDefault.
EnumTestObject.java:77:9: @ReturnValuesAreNonnullByDefault is not allowed on method return values!
EnumTestObject.java:90:5: @ParametersAreNullableByDefault and @ParametersAreNonnullByDefault are not allowed together!
InheritanceTestObject.java:47:47: No nullness Annotation for parameter definition found!
InheritanceTestObject.java:76:43: No nullness Annotation for parameter definition found!
DefaultParameterTestObject.java:37:5: It is not necessary to annotate @Nonnull if you annoted the class with @ReturnValuesAreNonnullByDefault.
LambdaTestObject.java:35:10: It is not allowed to increase nullness constraint for overriden method parameter definitions!

jsr305-test-files.zip

[mbert]

Existing PR is here: #728
I may have to re-upload it (don't know the exact workflow here).

[romani]

@rnveach, I am ok to host such Check in our project.
I am ok even with names.

Please review and approve issue, to start PR acceptance process.

[rnveach]

My only concern is if this check might be doing too much in 1 place and if it could be broken up somehow. PR code is 1000 lines.

@romani
Please confirm if you want these changes applied to sevntu and checkstyle code. My current thought is that since we don't use these annotations anywhere we might want to disable it for now unless you think it provides a benefit for reading the code.

Also what package should this check be in? PR currently has it in it's own custom package and not one of our standard locations.

[romani]

Please confirm if you want these changes applied to sevntu and checkstyle code.

We should run it with ignore severity. We are not ready to use that approach in design.

Also what package should this check be in?

I would avoid extra packages. I think better place is coding package

if this check might be doing too much in 1 place and if it could be broken up somehow.

We will keep it's name, so it far from generic, so let it do what it does now. While it is in sevntu, I am a bit relaxed on demands. I have no time to make deep analysis of that Check and propose better design. So I am ok to keep it as is, with demand to follow our quality gate.

[mbert]

I'm fine with that. I will prepare a new PR with the code living in the coding package. Since I already have feedback from the CI I'll also look into the test coverage wich was still a bit too low.

[romani]

thanks a lot, please also run your Check by https://github.com/checkstyle/contribution/tree/master/checkstyle-tester to generate report over all projects and make sure your Check does not have exception at least.
We will need execution of your Check on some projects that use Jsr305 annotations, to make sure your Check does not have false-positives on few projects at least.

[mbert]

OK. Bear with me. I'm currently working on the test coverage which is going to take a while (you know, those damn last 5%).

[romani]

Oh, yes, we know, in main project we demand even mutation 100%, so in sevntu we are relaxed a bit.
Note for you: coverage should be done by Input files. Only most exceptional cases should be covered by mocks and reflection. We allow reflection to avoid damage of design but it last resort.
Easy way to find all required input file is to substitute expression with some value and run our regression diff tool on a lot of sourses, found diff copy to inputs

[rnveach]

Since this is sevntu, he has limited regression support since groovy doesn't support sevntu.
See https://github.com/checkstyle/contribution/tree/master/checkstyle-tester#testing-sevntu-checks and checkstyle/contribution#340

If your feeling adventurous you can try using the shell scripts: https://github.com/checkstyle/contribution/blob/master/checkstyle-tester/sevntu_launch_diff.sh and https://github.com/checkstyle/contribution/blob/master/checkstyle-tester/launch_diff_variables.sh

[mbert]

Thank you. I have finished pimping the tests. The vast majority is on input files, but I have also implemented some classic unit tests.

[mbert]

Yes, I've read that part about checkstyle-tester. I haven't really started yet. If you've got any additional information that could speed up the process this would help a lot.

[rnveach]

@mbert If you need additional help, just point out what you need in the PR and we will try to help as best we can.

[mbert]

I will try to get a run with the checkstyle-tester today or tomorrow (depends on project load here).
If I run into problems, I'll ask here.

I have also created an example project that I would upload to this issue. It is annotated the way we want to enforce it with the plugin, also it uses the sevntu maven plugin from a preceding "mvn clean install" with the fresh code.

Once I have finished with the checkstyle-tester and found that no more amendmends to the code seem necessary from my side I'll upload the PR and the sample project. Sounds OK?

[rnveach]

sounds fine to me.

[mbert]

I don't seem to understand how to run the checkstyle-tester correctly. I'm running it like this:

groovy diff.groovy --localGitRepo /home/md/src/checkstyle --patchBranch master --config my_check.xml --listOfProjects projects-to-test-on.properties

I edited my_check.xml and added the new check's configuration. The script starts up fine, but after a while emits an error message:

Starting diff report generation ...
patch-diff-report-tool execution started.
Exception in thread "main" java.lang.IllegalArgumentException: Both Base and Patch XML report files have the same path.

According to the docs ("Sevntu can only be run with patch only branch and config.") I am using only a patch branch and a config argument - which seems to make sense because there's nothing to diff against when there are no changes in the checkstyle base project.

Can you give me a quick hint how to run this properly?

[mbert]

OK, I've (kind of) helped myself now by creating a branch in the checkstyle repo which is identical to master. Here's the results I obtained: report.zip

In the index.html file there are lots of warnings emitted by the new check which is to be expected of course. So that result seems fine, right?

[mbert]

Here's an annotated example project. To try out the new check, the following steps are required:

1. Cherry-pick the commit from my PR Issue #734: New check 'Jsr305AnnotationsCheck' #742
2. In the sevntu-checkstyle workspace, run mvn clean install in both, sevntu-checks and sevntu-checkstyle-maven-plugin
3. Unzip the attached archive example-webservice.zip with the example project
4. Run mvn clean verify to build the project, this should generate one checkstyle warning in PingServiceImpl
5. Play around with some of the sources (e.g. remove, add or change existing nullness annotations) and run mvn checkstyle:check to see what changes

[mbert]

Now I've got the first CI feedback. That was a new one to me.

There's a number of PMD failures, most of them however on the 'test object' java files used by the unit test (i.e. code that is never executed). Would excluding them from the check be acceptable? If yes, how?

Also the test class Jsr305AnnotationsCheckTest, uses a Jsr305AnnotionsTestUtil which contains the assert statements which PMD wants to see in the main test class. Not sure how to deal with that one.

I have fixed the PMD warnings from the check's main class. Waiting for feedback on my above comments for the test code.

[rnveach]

@mbert Anything related to the implementation, lets discuss in the PR so we don't have 2 split conversations going on.

of them however on the 'test object' java files used by the unit test (i.e. code that is never executed). Would excluding them from the check be acceptable?

If your files are input files to tests, they should go under test/resources and be prefixed with the words Input. We expect violations on pure tests or main code to be resolved or suppressed with a reason.

I'm running it like this:
groovy diff.groovy --localGitRepo /home/md/src/checkstyle --patchBranch master --config my_check.xml --listOfProjects projects-to-test-on.properties

It must be --patchConfig and not --config. See examples in readme.

[mbert]

If your files are input files to tests, they should go under test/resources and be prefixed with the words Input. We expect violations on pure tests or main code to be resolved or suppressed with a reason.

Ah, good to know. Will fix that next Monday. And then let's see what remains.

It must be --patchConfig and not --config. See examples in readme.

Also good to know. Is the result I have produced using my little workaround relevant, or should I rerun it?

[rnveach]

Here's the results I obtained: report.zip

It looks you just gave us the results of what is in the patch directory. When groovy is done, there is a directory named diff that should have the projects and HTML files which are more user friendly to view.

[mbert]

OK, here's what's in the diff folder: diff.zip

[romani]

fix is merged