`_Nullable` annotations don't trigger `clang-analyzer-core.CallAndMessage` diagnostics

[jmcclellan-figma]

In the following file

struct A {
    void doSomething();
};

struct B {
    A* _Nullable a();
};

int main() {
    const auto a = B{}.a();
    // un-comment this line to get the clang-analyzer-core.CallAndMessage lint.
    // I'm wondering why this line is needed, since we mark it as nullable already!
    // [[maybe_unused]] int x = a ? 1 : 0;
    a->doSomething();
    return 0;
}

If you run clang-tidy --checks="clang-analyzer-core*" file.cpp, you won't get a clang-analyzer-core.CallAndMessage diagnostic until you comment out the line that checks a against null. I think it would be useful and in the spirit of the nullability annotations to instead issue a diagnostic here, because the return value of a() is explicitly marked nullable. godbolt short link for repro

[llvmbot]

@llvm/issue-subscribers-clang-static-analyzer

Author: Russell McClellan (jmcclellan-figma)

In the following file

struct A {
    void doSomething();
};

struct B {
    A* _Nullable a();
};

int main() {
    const auto a = B{}.a();
    // un-comment this line to get the clang-analyzer-core.CallAndMessage lint.
    // I'm wondering why this line is needed, since we mark it as nullable already!
    // [[maybe_unused]] int x = a ? 1 : 0;
    a->doSomething();
    return 0;
}```

If you run `clang-tidy --checks="clang-analyzer-core*" file.cpp`, you won't get a `clang-analyzer-core.CallAndMessage` diagnostic until you comment out the line that checks `a` against null.  I think it would be useful and in the spirit of the [nullability annotations](https://clang.llvm.org/docs/analyzer/developer-docs/nullability.html) to instead issue a diagnostic here, because the return value of `a()` is explicitly marked nullable.  [godbolt short link for repro](https://godbolt.org/z/Ps165aKa7)
</details>

[steakhal]

Thanks for bringing it here from Discord.
I think its an important feature to support. I can recall some earlier attempts but no references ATM.
This should be almost trivial to implement, so I wonder why we have this gap in our feature set. Maybe @haoNoQ you have some context.

[haoNoQ]

Hi!

First of all, it is by design that the core.CallAndMessage checker doesn't respond to _Nullable at all. The checkers that respond to _Nullable/_Nonnull annotations live in the nullability.* package. You can turn them on and off separately. There are also some off-by-default clang warnings that you may want to enable, such as -Wnullable-to-nonnull-conversion. For example: https://godbolt.org/z/31n7dzsra

And then, yeah, it looks like you're right: They must have forgot to emit a warning specifically when a C++ method is called. If you do a A aa = *a; instead of a->doSomething() you do actually get a warning: https://godbolt.org/z/YP6v6Gf86

<source>:14:12: warning: Nullable pointer is dereferenced [clang-analyzer-nullability.NullableDereferenced]
   14 |     A aa = *a;
      |            ^~
<source>:10:20: note: Nullability 'nullable' is inferred
   10 |     const auto a = B{}.a();
      |                    ^~~~~~~

This is probably an accidental omission. Could be an easy fix!

Also yeah it's indeed intentional that the core.CallAndMessage checker responds to the ?: operator. (But more on that later.)

---

Now, I want you to consider whether _Nullable is actually the right tool for the job, because the fundamentals are a bit flimsy there. The checker probably does exactly what you want, but it does so somewhat illegally. And a lot of people who thought they wanted it to do that, actually didn't. So the checkers ended up unfinished and somewhat unmaintained, leading to bugs like this one.

Based on your question, you use the attribute _Nullable in your code as if it means "you must check this pointer for null". This could be because the null value shows up unpredictably. This could also be because you're so worried about software quality that you force yourself into a strict coding convention that requires null checks everywhere.

However, that's not what _Nullable actually means. What it really means is "you may need to check this pointer for null". And it doesn't tell you when, under which specific conditions, do you actually need to have a null check. Could be always. Could be sometimes. Could be just one little place across your entire codebase.

FWIW, to the best of my knowledge, there is no better attribute in Clang that describes the "you must check this pointer for null" situation. There were a few suggestions of that nature but none of them have materialized. Since you're already in C++, you may want to simply use optionals as a largely analogous thing. With the usual caveat that std::optional<T &> isn't a thing so you may need to improvise. Or you could use smart pointers to combine ownership safety with null safety. Not the standard ones, of course, those don't know anything about null safety. But a tiny wrapper type could help. And then, well, you'll probably have some ABI troubles with this approach.

(Historically speaking, the role of _Nullable was mostly about the fact that the compiler needed to leave some room for a null value inside the bit representation of the type. Which is meaningless in C and C++ but it does have a lot to do with the Objective-C/Swift interoperation, the main historical application of nullability annotations. And, yeah, of course in Swift you must check optionals for emptiness before every use too. But they've deliberately refused to carry over the same convention to the C side of things. They had to add the attribute so that Swift was prepared to consume a null value. But that doesn't mean they wanted to add a bunch of new null checks in their existing Objective-C code.)

But either way, yeah, when the checker nullability.NullableDereferenced emits the above warning, it oversteps its authority granted to it by the _Nullable attribute. By a pretty large margin. It warns you about something that isn't illegal in the first place. In practice, very few warnings produced by this checker translate to realistic null-pointer-dereference crashes at runtime. A lot of them lead you to places in which the null check is entirely unnecessary.

So the checker does what you want (and so do a few other related checkers), but it may also be a bad thing. A lot of people have given up on it because the amount of unnecessary null checks they had to add to shut the checker up was insurmountable.

The related checkers nullability.NullablePassedToNonnull and nullability.NullableReturnedFromNonnull suffer from the same problem.

The rest of the nullability checkers, namely nullability.NullPassedToNonnull and nullability.NullReturnedFromNonnull, are focused entirely on the _Nonnull attribute, and they are completely fine. Unlike the flimsy attribute _Nullable, the attribute _Nonnull is perfectly actionable from compiler's perspective. A literal null value clearly shouldn't be put into a _Nonnull storage. So these are usually quite actionable and useful and the signal-to-noise ratio is much lower.

Now, the compiler warning -Wnullable-to-nonnull-conversion also oversteps its authority granted to it by the _Nullable attribute, but by a smaller margin. It may still be unnecessarily noisy, but less so than the static analyzer. More specifically, the compiler warning is emitted when the null check on a _Nullable pointer is clearly missing entirely. Whereas the static analyzer also emits a warning when it's missing on a given execution path, even if there are other execution paths on which the check is performed correctly. So the static analyzer warning has a much higher chances of running into an intentionally omitted null check.

---

FWIW, normally, for most purposes, the static analyzer's approach is actually the right approach. It's just that the _Nullable attribute is so vague and flimsy that it rubs it the wrong way.

Like, for example, consider your example. Yeah, the only reason you have a core.CallAndMessage ever displays a warning is because of the explicit a ? ... : ... check right above it. The checker extensively explains why it's emitting the warning in its note messages:

<source>:14:5: warning: Called C++ object pointer is null [clang-analyzer-core.CallAndMessage]
   14 |     a->doSomething();
      |     ^
<source>:13:30: note: Assuming 'a' is null
   13 |     [[maybe_unused]] int x = a ? 1 : 0;
      |                              ^

In other words, it doesn't care that the pointer is "nullable" or "can be null" or anything of that nature. It simply tells you that if the pointer a was null on line 13, then it remains null on line 14 as well. (It's not like it was reassigned or anything.)

This is how static analyzer checkers typically work. They "lock in" on individual execution paths and try to predict what's going to happen on them.

In your example the branch causes the static analyzer to consider two distinct execution paths: the one that goes through the ?-branch and the one that goes through the :-branch. Once the static analyzer reaches the operator ?: it effectively forks itself and simulates the rest of the program twice, independently. In one universe the pointer was non-null so the variable x became 1 and the method call was legal and the program has exited successfully. In the other universe the pointer was null so x became 0 and the method call has crashed. It found a path on which the program crashes, so it tells you about it. And in the notes it describes the path by listing the assumptions under which this execution path is executed, such as "Assuming 'a' is null".

When you comment out the operator ?:, the static analyzer no longer has a reason to "fork" itself. Instead, it considers a single execution path on which absolutely nothing is known about a. And once it reaches the method call, it doesn't have any reasons to believe that the pointer a actually is null on the current path. So it doesn't emit any warnings.

In other words, the null check a ? ... : ... is part of the proof that something's wrong about your example code. The static analyzer doesn't tell you that this code may crash. Instead, it tells you that either your code may crash, or the check is dead code. If the pointer cannot be null, why check? If the pointer can be null, why use without check later?

For most purposes this is exactly what you want: find such crappy code paths. Find code that makes no sense no matter how you look at it. You're happy that the tool carefully considers all these possibilities for you. It can find these paths even when they're not covered by your runtime tests, which is why people love static analysis in the first place.

But now you probably see what the problem with _Nullable is. Yeah, _Nullable is completely meaningless, but that's not even the biggest problem. We're already naturally restricting ourselves to an individual execution path. By the time we learn about something being _Nullable, we're already "locked in" on everything that happened before that point. Even if something is nullable at that line of code - that doesn't mean it's nullable in that specific universe in which we're already permanently trapped by the time we reach that line of code. Technically we don't even know whether other parallel universes exist anymore. From this position we have very little opportunity to consider the program as a whole, to assess quantitatively how much the _Nullable contract was neglected.

So, long story short, this is why -Wnullable-to-nonnull-conversion is often significantly more practical than the static analyzer. It's still weird but it's at least dealing with situations where the attribute was neglected entirely. Which mostly boils down to code such as foo(bar()) where bar() returns a _Nullable and foo() accepts a _Nonnull. So it doesn't catch much but it's honest work. (Somewhat.) The static analyzer, on the other hand, tries to force you to make your code completely airtight. It takes a lot of free time to satisfy the contract that it implements, which historically wasn't exactly intended.

[jmcclellan-figma]

Thanks for the comprehensive response, so sounds like the behavior of clang-analyzer-core.CallAndMessage is intentional, this makes sense to me!

Sounds like there's an open question about clang-analyzer-nullability.NullableDereferenced though - should this also fire on C++ member calls?

[haoNoQ]

Yes you're absolutely right about that. This was probably an accidental omission. They were probably too worried about Objective-C to remember that C++ exists. This should be an easy fix.