Opinions requested -- nullability analysis in Clang

Hello friends!

I’ve been evaluating the state of null analysis/etc. in clang recently, and it looks like clang’s story for static nullness analysis has been getting quite a bit better over time. With the help of others, I’ve identified a few areas where we may be able to improve, but I’d really like opinions on whether we think these changes would actually be a good thing.

Specifically, I have four distinct changes in mind:

  1. Turn some amount of nullability analysis on by default (with -Wall) in clang. This would be conceptually very similar to uninitialized value checking, and would be able to catch simple cases like

Foo *p = nullptr;
if (p = getPtr())
p->oneThing();
else
p->anotherThing(); // warning: p is null.

…But no promises for any nontrivial cases (without heavily annotated locals/function signatures :wink: ), because there’s currently no planned way to silence the warning if we’re somehow wrong.

  1. Speaking of nullness annotations, clang supports a lot of them. Migrating old code to use them could be painful, so having a tool that annotates obvious things for us may be nice to have (https://docs.google.com/document/d/1vXuhRTQsbf4F9PbFtCoapuAhCU4RrD-IAiaUfTwp4uA/edit?usp=sharing).

  2. Add a CXXRecordDecl-level attribute that instructs nullness analysis to treat instances of the attributed type as a pointer for the sake of nullness analysis. This would enable nullness analysis of things like unique_ptr/shared_ptr/… Doc is available here (https://docs.google.com/document/d/1Zyb8o210EqkAXxrnrv4XtRu4w_i0yXO04p4KTuTde4M/edit?usp=sharing).

  3. Add clang_tidy checks for missing nullness annotations on function signatures/global variable decls/member variable decls/…

Like said, any feedback on how {useful,useless} we think these things would be (and feedback on on the designs themselves) is highly appreciated. :slight_smile:

Thanks for your time!
George

I like static analysis, but I do not think the static analysis available in clang today for null checking is suitable for -Wall. One of the reasons is because it is difficult to silence false positives (as you mentioned). More importantly though, the impact to build time is quite substantial. Nullability checks are path sensitive, and path sensitive checks are super-exponential. If a file takes seconds to compile, it is fairly common for it to take minutes to analyze.

I’m fine with the check being under a different flag, but lumping it in with -Wall would cause a lot of developer pain. In fact, I would very much like to be able to run the analyzer at the same time that I do a compile. That idea has been discussed before though, and there was resistance:
For item 3., I was under the impression that most smart pointer classes inlined well enough that the static analyzer was still effective at finding null issues. Do you have a small example that causes a null pointer warning with a raw pointer, but doesn’t cause a null pointer warning with a smart pointer? 2. and 4. seem like reasonable ideas to me, assuming they are implementable.

I like static analysis, but I do not think the static analysis available in clang today for null checking is suitable for -Wall. One of the reasons is because it is difficult to silence false positives (as you mentioned). More importantly though, the impact to build time is quite substantial. Nullability checks are path sensitive, and path sensitive checks are super-exponential. If a file takes seconds to compile, it is fairly common for it to take minutes to analyze.

Sorry, I didn’t express myself clearly. The idea was more to make a new pass check like the uninitialized value check, but checking for nulls rather than “has this been assigned?” – it’s going to be a low-quality analysis, but I’d think it should be able to catch trivial cases without eating too many cycles.

For item 3., I was under the impression that most smart pointer classes inlined well enough that the static analyzer was still effective at finding null issues. Do you have a small example that causes a null pointer warning with a raw pointer, but doesn’t cause a null pointer warning with a smart pointer?

Looks like you’re right! :slight_smile: I assumed that because we didn’t give warnings on code like this:

static int *f() { return nullptr; }
int g() { return *f(); }

…We wouldn’t do much to look through function calls/etc. After playing around a bit, I’ve found that this was not a good assumption, and our handling for smart pointers/pointer types where we use the lower N bits for flags/etc. is actually quite good. Thanks for correcting me :slight_smile:

If you can keep the number of false positives low and the speed quick, then this seems reasonable and useful to me.