Page MenuHomePhabricator

[analyzer] Trust _Nonnull annotations for system framework

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



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.

Diff Detail


Event Timeline

dcoughlin added inline comments.Mar 14 2018, 11:25 AM
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.

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.
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.