SourceForge has been redesigned. Learn more.
Close

#1102 RCN_REDUNDANT_NULLCHECK_OF_NONNULL_VALUE false positive

closed-rejected
5
2017-10-22
2012-08-02
J. Stolze
No

FindBugs assumes that it has all code available during execution. This assumption can be dangerous, As shown in the following example.

The class GoodBehavingClass will always return a non null String object in the method isNullReturned which according to FindBugs should not be checked against null in the checkIfNullIsReturned method. When this class is part of a library which is distributed to third parties it can lead to extension of the GoodBehavingClass by BadBehavingClass which always returns null, which would lead to NullPointerExceptions when passed to this checkIfNullIsReturned method when this redundant null check is removed.

I think you can only state this redundant null check only when the method or class is declared as final.

Discussion

  • J. Stolze

    J. Stolze - 2012-08-02

    Class which illustrates the problem

     
  • William Pugh

    William Pugh - 2012-08-10

    What version of FindBugs are you using? I'm unable to replicate this.

     
  • William Pugh

    William Pugh - 2012-08-10

    Nevermind. Had to run it without including BadBehavingClass. Was able to reproduct.

     
  • William Pugh

    William Pugh - 2012-08-10
    • status: open --> open-accepted
     
  • Comment has been marked as spam. 
    Undo

    You can see all pending comments posted by this user  here

    Anonymous

    Anonymous - 2012-09-26

    I think this hypothesis is not that bad, since in most cases FindBugs will probably be right. However I think one should be able to make it disappear by annotating the method with @CheckForNull or similar but it does not seem to work (in Eclipse FindBugs 2.0.1 at least)

     
    Last edit: Anonymous 2017-06-19
  • J. Stolze

    J. Stolze - 2012-10-01

    When you apply the criteria I suggest, final method or final class, FB would be right in all cases. This is clearly better than assuming you're right. As the example shows.

     
  • Fred Toussi

    Fred Toussi - 2014-01-17

    With version 2.03, related false positives occur in contexts like this and vice versa with respect to assignment of null or not null Object to globalError in large classes where several calls separate the assignments and nullability checks:

    Object globalError;
    
    void myMethod() {
      // some code
    
      if (someCondition)
      globalError = someVal;
    
      // some code
    }
    
    void myProg() {
      globalError = null;
      myMethod();
    
      // FindBugs considers this check redundant
      if (globalError != null) {
        // do something
    
      }
    }
    
     
    Last edit: Fred Toussi 2014-01-18
  • William Pugh

    William Pugh - 2014-01-17

    On that code snippet, FindBugs 2.0.3 and 3.0.0-Snapshot produce no warnings.

     
  • Fred Toussi

    Fred Toussi - 2016-02-02

    HSQLDB version 2.3.3 jar is available. The SVN URL for the false positive is below:
    https://sourceforge.net/p/hsqldb/svn/HEAD/tree/base/tags/2.3.3/src/org/hsqldb/Scanner.java#l1894

    FindBugs considers the second check for if (token.dataType == null as redundant but each scanNext(errorCode) call may set token.dataType=null with a call to Token.reset().

        if (token.tokenType == Tokens.OPENBRACKET) {
            scanNext(errorCode);
    
            if (token.dataType == null
                    || token.dataType.typeCode != Types.SQL_INTEGER) {
                throw Error.error(errorCode);
            }
    
            precision = ((Number) this.token.tokenValue).intValue();
    
            scanNext(errorCode);
    
            if (token.tokenType == Tokens.COMMA) {
                if (startToken != Tokens.SECOND) {
                    throw Error.error(errorCode);
                }
    
                scanNext(errorCode);
    
                if (token.dataType == null
                        || token.dataType.typeCode != Types.SQL_INTEGER) {
                    throw Error.error(errorCode);
                }
    
                scale = ((Number) token.tokenValue).intValue();
    
                scanNext(errorCode);
            }
    
     
  • Nahuel Barrios

    Nahuel Barrios - 2016-11-14

    Hi everybody!

    I'm facing the same problem and because I've got a method which is protected and annotated as android.support.annotation.Nullable (I check its nullability before using its return value) so I think these kind of things should be checked too.

    Regards!

     
  • Andrey Loskutov

    Andrey Loskutov - 2017-10-22
    • Status: open-accepted --> closed-rejected
     

Log in to post a comment.