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

Add option to allow returning nullable from methods in Jsr305AnnotationCheck #887

Open
vdaniloff opened this issue Apr 11, 2022 · 2 comments

Comments

@vdaniloff
Copy link

vdaniloff commented Apr 11, 2022

http://sevntu-checkstyle.github.io/sevntu.checkstyle/apidocs/com/github/sevntu/checkstyle/checks/coding/Jsr305AnnotationsCheck.html

/var/tmp $ javac Test.java

/var/tmp $ cat Test.java

package org.example;

import javax.annotation.Nullable;

public class Test {
    @Nullable // violation
    Object returnNullable() {
        return null;
    }
}

/var/tmp $ cat config.xml

<!DOCTYPE module PUBLIC
        "-//Puppy Crawl//DTD Check Configuration 1.3//EN"
        "http://www.puppycrawl.com/dtds/configuration_1_3.dtd">
<module name="Checker">
	<module name="TreeWalker">
		<module name="Jsr305Annotations">
			<property name="packages" value="org.example"/>
		</module>
	</module>
</module>

For Linux users:

/var/tmp $ java -classpath checkstyle-10.1-all.jar:sevntu-checks-1.41.0.jar com.puppycrawl.tools.checkstyle.Main -c config.xml Test.java

For Windows users:

C:\tmp> java -classpath checkstyle-10.1-all.jar;sevntu-checks-1.41.0.jar com.puppycrawl.tools.checkstyle.Main -c config.xml Test.java

Starting audit...
[ERROR] C:\work\checkstyle\Test.java:6:5: @Nullable is not allowed on method return values. [Jsr305Annotations]
Audit done. 
Checkstyle ends with 1 errors.                                                                                    

Describe what you expect in detail.

Expected:
A configuration option allowNullableReturnValue can be set, so that the following configuration could be used

<!DOCTYPE module PUBLIC
        "-//Puppy Crawl//DTD Check Configuration 1.3//EN"
        "http://www.puppycrawl.com/dtds/configuration_1_3.dtd">
<module name="Checker">
	<module name="TreeWalker">
		<module name="Jsr305Annotations">
			<property name="packages" value="org.example"/>
                        <property name="allowNullableReturnValue" value="true"/>
		</module>
	</module>
</module>

And the result would be

Starting audit...            
Audit done. 

Motivation is in fact using @Nullable is quite common practice, though I realize @CheckForNull is recommended by most tools (including SonarQube)

@romani
Copy link
Member

romani commented Apr 12, 2022

@mbert, please review this request.

As for me we do not need property, it is just a defect.
@vdaniloff, can explain in what cases it is valuable to have violation on such code?

@vdaniloff
Copy link
Author

vdaniloff commented Apr 12, 2022

I think this all comes from original recommendations from JSR305: e. g. javadoc for Nullable

This annotation is useful mostly for overriding a Nonnull annotation. Static analysis tools should generally treat the annotated items as though they had no annotation, unless they are configured to minimize false negatives. Use CheckForNull to indicate that the element value should always be checked for a null value.

But in fact I don't know. Personally for me it would work even without a property, but it would be a breaking change IMO. I can imagine a codestyle that requires CheckForNull though. Sonar did not support Nullable in static analysis, as I understand (probably this is outdated) https://community.sonarsource.com/t/support-for-nullable-annotation-for-method-return-values/35555/4

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants