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

Spotbugs: CheckForNull does not override ParametersAreNonnullByDefault #8974

Closed
danielb987 opened this issue Aug 27, 2020 · 5 comments
Closed
Assignees
Labels
Pending closure This issue or PR has not been updated for a while

Comments

@danielb987
Copy link
Contributor

danielb987 commented Aug 27, 2020

While trying to understand the problem in #6510 (comment), I found that @CheckForNull for a parameter does not overrides @ParametersAreNonnullByDefault for the class.

package jmri;

import javax.annotation.*;

@ParametersAreNonnullByDefault
class ClassA {
    
    public void testA(@CheckForNull Object param) {
        // Do nothing
    }
    
    public void testB(@Nullable Object param) {
        // Do nothing
    }
}

This means that Spotbugs will give an error on new ClassA().testA(null); but not on new ClassA().testB(null);, since the later is annotated with @Nullable.

The JMRI documentation about Spotbugs say that @CheckForNull can be used for parameters, but that's not true if the class is annotated with @ParametersAreNonnullByDefault.

The JMRI documentation says that @Nullable shouldn't be used and that @CheckForNull should be used instead. But if the class is annotated with @ParametersAreNonnullByDefault, @Nullable works but @CheckForNull doesn't.

My branch nonnull_4_test has a working example of the above.

@danielb987 danielb987 self-assigned this Aug 27, 2020
@danielb987
Copy link
Contributor Author

@bobjacobsen @pabender
Any comments on this issue?

The JMRI documentation says that @Nullable shouldn't be used since SpotBugs doesn't really check anything when this is used. But I don't really understand what Spotbugs should check when @Nullable is used, so I don't see the problem with using it.

@danielb987
Copy link
Contributor Author

I have done additional testing of this class:

package jmri;

import javax.annotation.*;

@ParametersAreNonnullByDefault
class ClassA {
    
    public void testA(@CheckForNull Object param) {
        testC(param);
    }
    
    public void testB(@Nullable Object param) {
        testC(param);
    }
    
    private void testC(Object param) {
        // Do nothing
    }
    
}

The annotation @CheckForNull doesn't seem to do anything. Spotbugs complains if I call testA(null) and Spotbugs does not complains about the call testC(param) from the method testA. So Spotbugs does not treat the param in the method declaration for testA as a parameter that can be null.

The annotation @Nullable makes Spotbugs be happy with the call testB(null) but Spotbugs complains about the call testC(param) from the method testB. So Spotbugs clearly treats the param in the method declaration for testB as a parameter that can be null.

So my testing shows that @Nullable works fine but @CheckForNull does not work at all when the class is annotated with @ParametersAreNonnullByDefault.

The comment in the JMRI documentation about spotbugs which tells that @CheckForNull should be used instead of @Nullable was written for FindBugs before we switched over to Spotbugs, so it may be a difference between Spotbugs and Findbugs. But the comment is not valid today.

@bobjacobsen
Copy link
Member

bobjacobsen commented Aug 28, 2020 via email

@stale
Copy link

stale bot commented Mar 30, 2021

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. JMRI is governed by a small group of maintainers which means not all opened issues may receive direct feedback.

@stale stale bot added the Pending closure This issue or PR has not been updated for a while label Mar 30, 2021
@stale
Copy link

stale bot commented Apr 14, 2021

This issue has been automatically closed due to lack of activity. In an effort to reduce noise, please do not comment any further. Note that the maintainers may elect to reopen this issue at a later date if deemed necessary.

@stale stale bot closed this as completed Apr 14, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Pending closure This issue or PR has not been updated for a while
Projects
None yet
Development

No branches or pull requests

2 participants