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

New check: Jsr305Annotations #734

Closed
mbert opened this issue Apr 2, 2019 · 35 comments

Comments

@mbert
Copy link
Contributor

commented Apr 2, 2019

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

This comment has been minimized.

Copy link
Contributor Author

commented Apr 2, 2019

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

@romani

This comment has been minimized.

Copy link
Member

commented Apr 7, 2019

@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 rnveach added the approved label Apr 7, 2019

@rnveach

This comment has been minimized.

Copy link
Contributor

commented Apr 7, 2019

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

This comment has been minimized.

Copy link
Member

commented Apr 8, 2019

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

This comment has been minimized.

Copy link
Contributor Author

commented Apr 8, 2019

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

This comment has been minimized.

Copy link
Member

commented Apr 9, 2019

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

This comment has been minimized.

Copy link
Contributor Author

commented Apr 9, 2019

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

This comment has been minimized.

Copy link
Member

commented Apr 11, 2019

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

This comment has been minimized.

Copy link
Contributor

commented Apr 11, 2019

@mbert

This comment has been minimized.

Copy link
Contributor Author

commented Apr 11, 2019

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

This comment has been minimized.

Copy link
Contributor Author

commented Apr 11, 2019

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

This comment has been minimized.

Copy link
Contributor

commented Apr 11, 2019

@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

This comment has been minimized.

Copy link
Contributor Author

commented Apr 11, 2019

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

This comment has been minimized.

Copy link
Contributor

commented Apr 11, 2019

sounds fine to me.

@mbert

This comment has been minimized.

Copy link
Contributor Author

commented Apr 12, 2019

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

This comment has been minimized.

Copy link
Contributor Author

commented Apr 12, 2019

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 added a commit to mbert/sevntu.checkstyle that referenced this issue Apr 12, 2019

mbert added a commit to mbert/sevntu.checkstyle that referenced this issue Apr 12, 2019

@mbert

This comment has been minimized.

Copy link
Contributor Author

commented Apr 12, 2019

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

This comment has been minimized.

Copy link
Contributor Author

commented Apr 12, 2019

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.

mbert added a commit to mbert/sevntu.checkstyle that referenced this issue Apr 12, 2019

Issue sevntu-checkstyle#734: Fixed PMD violations from main class (PM…
…D use in tests needs to be discussed in sevntu-checkstyle#734 and/or elsewhere)
@rnveach

This comment has been minimized.

Copy link
Contributor

commented Apr 12, 2019

@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

This comment has been minimized.

Copy link
Contributor Author

commented Apr 12, 2019

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

This comment has been minimized.

Copy link
Contributor

commented Apr 12, 2019

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

This comment has been minimized.

Copy link
Contributor Author

commented Apr 15, 2019

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

mbert added a commit to mbert/sevntu.checkstyle that referenced this issue Apr 15, 2019

Issue sevntu-checkstyle#734: Moved test input classes into src/test/r…
…esources as required, fixed more PMD issues

mbert added a commit to mbert/sevntu.checkstyle that referenced this issue Apr 18, 2019

mbert added a commit to mbert/sevntu.checkstyle that referenced this issue Apr 18, 2019

Issue sevntu-checkstyle#734: required renaming of inner classes, comm…
…ent in pom.xml, examples in Javadoc, contributors' list in README.textile

mbert added a commit to mbert/sevntu.checkstyle that referenced this issue May 15, 2019

mbert added a commit to mbert/sevntu.checkstyle that referenced this issue May 15, 2019

Issue sevntu-checkstyle#734: Fixed PMD violations from main class (PM…
…D use in tests needs to be discussed in sevntu-checkstyle#734 and/or elsewhere)

mbert added a commit to mbert/sevntu.checkstyle that referenced this issue May 15, 2019

Issue sevntu-checkstyle#734: Moved test input classes into src/test/r…
…esources as required, fixed more PMD issues

mbert added a commit to mbert/sevntu.checkstyle that referenced this issue May 15, 2019

mbert added a commit to mbert/sevntu.checkstyle that referenced this issue May 15, 2019

mbert added a commit to mbert/sevntu.checkstyle that referenced this issue May 15, 2019

mbert added a commit to mbert/sevntu.checkstyle that referenced this issue May 15, 2019

mbert added a commit to mbert/sevntu.checkstyle that referenced this issue May 15, 2019

mbert added a commit to mbert/sevntu.checkstyle that referenced this issue May 15, 2019

mbert added a commit to mbert/sevntu.checkstyle that referenced this issue May 15, 2019

Issue sevntu-checkstyle#734: required renaming of inner classes, comm…
…ent in pom.xml, examples in Javadoc, contributors' list in README.textile

mbert added a commit to mbert/sevntu.checkstyle that referenced this issue May 15, 2019

mbert added a commit to mbert/sevntu.checkstyle that referenced this issue May 16, 2019

mbert added a commit to mbert/sevntu.checkstyle that referenced this issue May 20, 2019

mbert added a commit to mbert/sevntu.checkstyle that referenced this issue May 20, 2019

mbert added a commit to mbert/sevntu.checkstyle that referenced this issue May 27, 2019

mbert added a commit to mbert/sevntu.checkstyle that referenced this issue May 27, 2019

mbert added a commit to mbert/sevntu.checkstyle that referenced this issue May 28, 2019

mbert added a commit to mbert/sevntu.checkstyle that referenced this issue May 28, 2019

mbert added a commit to mbert/sevntu.checkstyle that referenced this issue Jun 3, 2019

mbert added a commit to mbert/sevntu.checkstyle that referenced this issue Jun 3, 2019

mbert added a commit to mbert/sevntu.checkstyle that referenced this issue Jun 6, 2019

mbert added a commit to mbert/sevntu.checkstyle that referenced this issue Jun 17, 2019

romani added a commit that referenced this issue Jun 19, 2019

@romani romani added this to the 1.34.0 milestone Jun 19, 2019

@romani romani added the new module label Jun 19, 2019

@romani

This comment has been minimized.

Copy link
Member

commented Jun 19, 2019

fix is merged

@romani romani closed this Jun 19, 2019

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