Page MenuHomePhabricator

[analyzer] Do not report NSError null dereference for _Nonnull params
ClosedPublic

Authored by vsavchenko on Apr 8 2020, 5:20 AM.

Details

Summary

We want to trust user type annotations and stop assuming pointers declared
as _Nonnull still can be null. This functionality is implemented as part
of NullabilityChecker as it tracks non-null types already. It could
be easily implemented as a part of NSError checker, but it would look like
a simple "shut up" plug as opposed to a more generic solution.

Diff Detail

Event Timeline

vsavchenko created this revision.Apr 8 2020, 5:20 AM

Hi everyone!
I'm thinking about adding support for __attribute((nonnull))__ as well, but it should be handled a bit differently. That annotation is for parameters and not for types, so the corresponding constraints should be generated only when entering the function (as opposed to each load). I'm not sure that it should be a part of this commit though because it will have a completely standalone solution. Another question about __attribute((nonnull))__ is "Where should we put it?". It is quite reasonable to put it into NullabilityChecker, but NonnullParamChecker seems like a good candidate as well.

vsavchenko updated this revision to Diff 255993.Apr 8 2020, 5:55 AM

Fix formatting issues in the test file

NoQ accepted this revision.Apr 8 2020, 6:13 AM

NonnullParamChecker

Yup, looks like that's actually the one that deals with __attribute__((nonnull)), as opposed to _Nonnull.

clang/lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp
521
527
537

Ok, so we're continuing normally if the value is already known to have been assigned to null. We could sink the analysis instead but presumably it's not our job as another checker must have warned before we get there (let's comment about this maybe).

This revision is now accepted and ready to land.Apr 8 2020, 6:13 AM
vsavchenko marked 2 inline comments as done.Apr 8 2020, 6:26 AM
vsavchenko added inline comments.
clang/lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp
521

But this auto is also fine IMO as you can clearly see the actual type in the RHS. Mb const auto *Region at least?

537

Sure!

NoQ added inline comments.Apr 8 2020, 6:28 AM
clang/lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp
521

Yup, this one should be const auto *.

Hi everyone!
I'm thinking about adding support for __attribute((nonnull))__ as well, but it should be handled a bit differently. That annotation is for parameters and not for types, so the corresponding constraints should be generated only when entering the function (as opposed to each load). I'm not sure that it should be a part of this commit though because it will have a completely standalone solution. Another question about __attribute((nonnull))__ is "Where should we put it?". It is quite reasonable to put it into NullabilityChecker, but NonnullParamChecker seems like a good candidate as well.

Wait, we don't already assume nonnull attributed parameters as, well, not null? That's crazy.

Hi everyone!
I'm thinking about adding support for __attribute((nonnull))__ as well, but it should be handled a bit differently. That annotation is for parameters and not for types, so the corresponding constraints should be generated only when entering the function (as opposed to each load). I'm not sure that it should be a part of this commit though because it will have a completely standalone solution. Another question about __attribute((nonnull))__ is "Where should we put it?". It is quite reasonable to put it into NullabilityChecker, but NonnullParamChecker seems like a good candidate as well.

Wait, we don't already assume nonnull attributed parameters as, well, not null? That's crazy.

Yep, I was shocked by it as well. Maybe for some warnings it is checked in the end and they are truncated before reporting. However, DereferenceChecker definitely dispatches ImplicitNullDerefEvent for params marked with __attribute__((nonnull)).

vsavchenko updated this revision to Diff 256009.Apr 8 2020, 6:53 AM

Fix review remarks

vsavchenko marked 4 inline comments as done.Apr 8 2020, 6:53 AM
NoQ added a comment.Apr 8 2020, 7:07 AM

Wait, we don't already assume nonnull attributed parameters as, well, not null? That's crazy.

That's only for the top frame. For nested stack frames we're not only assuming they're not null, we're also emitting a warning when they're null. But, yeah, it's clearly an unfortunate omission.

NoQ added inline comments.Apr 8 2020, 7:10 AM
clang/lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp
536

That's still too much auto.

vsavchenko updated this revision to Diff 256017.Apr 8 2020, 7:19 AM

Get rid of nonessential 'auto' in variable declaration

vsavchenko marked an inline comment as done.Apr 8 2020, 7:20 AM
vsavchenko updated this revision to Diff 256020.Apr 8 2020, 7:41 AM

Remove more auto's

NoQ accepted this revision.Apr 8 2020, 7:44 AM

Thanks!

I'll commit.

In D77722#1969551, @NoQ wrote:

Thanks!

I'll commit.

Awesome! Thanks!

This revision was automatically updated to reflect the committed changes.