Feature: integrate Jsr305CheckstylePlugin plugin

[mbert]

The Jsr305Annotations plugin has so far been hosted as an independent project. It allows enforcing the use of the "nullness" annotations in code in order to both support static code analysis and understandability of code.

In order to simplify its integration into IDEs I would like to see it integrated in the sevntu project. The authors have already changed the project license to LGPL 2.1 thus allowing to do so.

I have prepared a pull request to implement this feature.

[romani]

@mbert, please explain me what your Check is doing in examples please as I think good to know for users.

Before you pass our strict CI, let's work on clear explanation of your Check reason/targets.

[mbert]

OK, no problem. The check is an already existing project that was developed by colleagues in our then company and open-sourced thereafter. Here's the github repo for it. Because it's a standalone CheckStyle plugin it needs to be integrated in projects manually, which can be inconvenient in particular for IDEs. My contribution has only consisted of integrating it here

I realise that the original project does not contain any user documentation. Let me summarise it here:

The jsr305 annotations, like @Nonnull or @CheckForNull etc. you can specify your intention on how your code will behave. This may look like:

@Nonnull
public String foo(@Nonnull final Long bar, @Nullable final String baz) { ... }

You can also define class-wide defaults:

@ParametersAreNonnullByDefault
class Foo {
  @Nonnull
  public String foo(final Long bar, @Nullable final String baz) { ... }
}

Annotations like these can assist static code analysis (some tools evaluate them if they are present), and they also document to others, how your code (API, ...) is supposed to be used.

The Jsr305AnnotationsCheck allows setting up CheckStyle checks to enforce that all code in particular packages has been annotated that way, i.e. that nothing has been forgotten.

I use the existing plugin in some projects where the annotations are considered compulsory as part of the coding conventions.

Since the existing plugin always needs to integrated by hand (in particular in IDEs this can be error-prone), and also because it is rather unknown the way it is now, I hope by integrating it into the sevntu checks I can make it easier to integrate and encourage more people to use it.

Please let me know whether you have any more questions, I shall happily add more examples if necessary.

[romani]

Yes, please continue with examples, checkstyle is not type aware tool https://checkstyle.org/#Limitations , I still do not understand what this Check is for. Please give few real problems in code and how to configure Check and what if will demand to put in code and .... All such examples will be in javadoc of Check.

[mbert]

The plugin does not need to fully understand types, because they are all treated the same if they are not primitive types (void, int, ...) which can be identified by a positive-list. Hence whenever there is a method definition then a jsr305 annotation for the return type is either illegal if the return type is a primitive, or it is required, either at this position or by a class-default or by a parent class (if any).

That's pretty much how it's being done technically. I'll try not to mix up things and continue with some usage examples in the next comment.

[mbert]

OK, here's a few more examples with screenshots:

A return value needs to have an annotation indicating whether it can ever be null. This one here cannot:

If I leave out the annotation, I will get a warning:

I can set class-wide defaults, e.g. here for parameters:

A redundant annotation will lead to a warning:

If a return value can be null, it needs to be checked when calling the respective method. Hence you annotate it with @CheckForNull rather than @Nullable:

The whole idea behind the plugin is to help enforcing consistent use of these annotations. If you have your static code analysis set up properly to use the annotations, your code also documents by these annotations how it should be used. In an organisation you will most likely use libraries created by colleagues. They come with source-jars. If within your organisation the chain of annotations and static checks works all the way, then you will know from looking at e.g. an interface from a library you use whether you need to null-check or not.

I hope that helped a bit? If not, please let me know!

[mbert]

Hoping to convince you of course, I am gathering tasks that I need to do in order to get the PR through later. From the travis-builds I've seen that only the first is related to my PR's code while number 2 and 3 seem to be related to a problem in the environment, and 4 of course represents this issue's outcome.

Hence I would have to increase code coverage to 100%, that should be fine.

From your comments here I gather that there is some documentation missing. If you've got a good example from which I can get some inspiration, I'd be happy to use that for orientation.

[rnveach]

This issue sounds very similar to #606 and kinda like #622 .
Please see how they give examples of check's logic and violations so we can get an idea of what you are aiming for.

A redundant annotation will lead to a warning:

You may want to lean back on your check doing too much in 1. Is it possible this redundant annotation check be in a separate check an configured to any general annotation user wants or is this a very rare use case?
Also are you just checking the method or will you be checking the parameters too?

[mbert]

Yes, that's similar indeed - I was not expecting that others had expressed similar ideas.

The good news is: it's already done. My PR contains a port of a checkstyle plugin that has already been available for several years and is well-tested by users.

The plugin checks both return values and parameters. In its current form there is no option for disabling warnings on redundant annotations, but that should be fairly easy to integrate. In its current form the plugin is designed to support a consistent use of these annotations. If somebody wants to allow "inconsistent" annotations one might debate on whether this should be supported, too. It should nevertheless be implementable.

[romani]

@mbert , thanks a lot, now I become to understand your ideas.

Hence you annotate it with @CheckForNull rather than @Nullable

please extend examples on such validation, It is still not clear what Check i going to do in such situation.
Please try to describe Check idea in format like checkstyle/checkstyle#5899
Tests examples it is better to format as https://github.com/checkstyle/checkstyle/blob/master/src/test/resources/com/puppycrawl/tools/checkstyle/checks/annotation/annotationlocation/InputAnnotationLocationEnum.java to make it absolutely clear on expected behavior for certain configuration.

[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. Also, an inherited return value cannot become more "nullable", while an inherited parameter cannot become more "nonnull".

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: Redefining inherited parameter nullness is illegal
    public Object method3(@Nullable 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]

Hi, any update?

[romani]

Sorry for delay, too much acticity in main project, I will reply.

[romani]

@mbert ,
sorry for delay in reply.

The annotation can be made directly or applied through either inheritance from an already annotated class or a annotation for class-wide defaults.

class Class3 implements Interface1 {
    @Override // Error: Redefining inherited parameter nullness is illegal
    public Object method3(@Nullable Object obj1, Object obj2) { ... }

This will not work, as interface and class might be in different files, please recheck whole list of limitations - https://checkstyle.org/writingchecks.html#Limitations .
Checkstyle is single file validation tool.

Please explain who you workaround such limitations.
Without inheritance validation of annotations looks like good task for checkstyle.

[mbert]

[edit] Please ignore, see my comment below.

I would not call this a workaround. It's all possible using the CheckStyle API.

Starting from the DetailAST to check you use getParent() to access the parent element. If it has a type (a class or interface definition, a method, constructor or enum definition) that can have a "nullness annotation" you collect them if there are any and then check for possible conflicts.

To get an idea you may look into the file Jsr305AnnotationsCheck in my PR, at line 905 you can see how it is implemented.

[romani]

Presence of parent class in same file is very rare case in real life, classes usually located in separate files.
So Check will produce a lot of false positives. It can be strict only if all hierarchy in present in one file. In other cases Check should skip reporting violations.
But in such case, half of functionality become disabled.

Please explain why you keep interfaces and classes in the same file. Am I missing something ?

[mbert]

I've dug through the code again (I need to remind here that I am not the original author). The plugin does not actually analyse the parent class. It is enough to know that a method is overridden; in that case the following rules apply:

• 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.

This constraint can be relaxed by setting the allowOverridingParameter and/or allowOverridingReturnValue parameters to true.

Sorry for the confusion. I should have looked things up before replying yesterday, I, too, am a bit tied up in other work atm.

[romani]

Thanks a lot for explanation, now it makes sense.
Let's close this issue and create new issue "new check: Jsr305Annotations" and state in description whole summary of Check: config with properties explanation and few examples of validation. You already have most of content in this issue, so you just need to organize it. The same will be used in javadoc.

We will do final review to make sure all is fine and this will let you avoid bunch of refactoring in PR.

[mbert]

OK, new issue is #734.