This is an archive of the discontinued LLVM Phabricator instance.

[analyzer] Trust _Nonnull annotations for system framework
ClosedPublic

Authored by george.karpenkov on Mar 9 2018, 5:05 PM.

Details

Summary

Changes the analyzer to believe that methods annotated with _Nonnull from system frameworks indeed return non null objects.
Local methods with such annotation are still distrusted.
rdar://24291919

Diff Detail

Repository
rL LLVM

Event Timeline

dcoughlin added inline comments.Mar 14 2018, 11:25 AM
include/clang/StaticAnalyzer/Checkers/Checkers.td
219 ↗(On Diff #137876)

I think this should probably go in either 'core' or 'apiModeling' (I have a preference for the second). The "nullability" package is for nullability diagnostics. We want this modeling to still take place even when the nullability diagnostics are turned off.

lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp
1216 ↗(On Diff #137876)

I don't think the "NS" namespace is the right indicator here. There are a lot of classes in other namespaces ("CI" for CoreImage, "UI" for UIKit, etc.).

1219 ↗(On Diff #137876)

We should do this for C function calls as well. There is a large C function API surface area. We shouldn't do it for function pointers or blocks, since those may be client input.

1230 ↗(On Diff #137876)

We should not do this for methods on protocols even if they are in the system header. Returns values from protocol represent client inputs, so we don't want to lose coverage in frameworks on defensive code paths that handle when a client returns nil for a method that is declared nonnull.

george.karpenkov marked an inline comment as done.Mar 16 2018, 2:48 PM
george.karpenkov added inline comments.
lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp
1216 ↗(On Diff #137876)

That's actually an outdated comment, it currently checks isInSystemHeader()

1230 ↗(On Diff #137876)

Seems like it is not possible to get a decl here which would correspond to a protocol.
A function would be called with id<MyProtocol>, not with just MyProtocol.

dcoughlin accepted this revision.Mar 21 2018, 9:39 PM

Looks good to me.

This revision is now accepted and ready to land.Mar 21 2018, 9:39 PM
This revision was automatically updated to reflect the committed changes.