#269 Detect null checks that are guaranteed to never be true

closed-fixed
nobody
5
2012-07-02
2012-05-31
No

(to get it out of the way, thanks for such a great tool!)

I recently introduced a bug into our code with a silly mistake. It involved having logic checking whether the returned object from a method was null. The mistake I made was that I retained the null check, but changed the method to always return a newly constructed object (guaranteed to be non-null). It looked similar to this class:

public class SillyNullCheck {

public void doSomething\(\) \{
    if \(neverReturnsNull\(\) == null\) \{
        // can never happen
    \} else \{
        // will always invoke code here
    \}
\}

private List<Object> neverReturnsNull\(\) \{
    return new ArrayList<Object>\(\);
\}

}

It would be great if findbugs could tell that null check in doSomething could never evaluate to true. I recognise this is a simple case for a few reasons:

A) the implementation of the method has no conditions or data flows inside it, and returns using a new statement, rather than another method invocation, which is not typical
B) the 'neverReturnsNull' method is private, meaning that you can guarantee the exact implementation which will be called by 'doSomething', if the method is subclassable, there wouldn't be that guarantee

I don't know how common this would be, but it would have saved me from causing a production bug. Yep, I'm embracing my dumb mistakes :-)

Does this sound like something findbugs could catch?

Discussion

  • Graham Allan

    Graham Allan - 2012-05-31

    Argh. I forgot to mention, even if the 'neverReturnsNull' method were overrideable, or it had complex data flows or returns the result of another method call, you could check for the presence of @Nonnull on the method, and treat that case similar to a method that has returns a new object.

     
  • William Pugh

    William Pugh - 2012-07-02
    • status: open --> closed-fixed
     
  • William Pugh

    William Pugh - 2012-07-02

    We already check for this. However, it had been reported as low confidence. I've just committed a change to boost it to low confidence. In the test case you provided, it is reported as:

    Bug: Redundant nullcheck of sfBugs.RFE3531161.neverReturnsNull(), which is known to be non-null in sfBugs.RFE3531161.doSomething()
    This method contains a redundant check of a known non-null value against the constant null.

    Confidence: Normal, Rank: Of Concern (18)
    Pattern: RCN_REDUNDANT_NULLCHECK_OF_NONNULL_VALUE
    Type: RCN, Category: STYLE (Dodgy code)

     

Log in to post a comment.

Get latest updates about Open Source Projects, Conferences and News.

Sign up for the SourceForge newsletter:





No, thanks