This is an archive of the discontinued LLVM Phabricator instance.

[analyzer] Nullability: Enable analysis of non-inlined nullable objc properties.
ClosedPublic

Authored by NoQ on Aug 11 2022, 12:30 AM.

Details

Summary

Patch by Paul Peltz! I'm going to be addressing comments. The overall idea sounds great and the code seems to have all the necessary parts in place but I haven't looked at it very carefully so please comment.

Author's original description:

The NullabilityChecker has a very early policy decision that non-inlined property accesses will be inferred as returning nonnull, despite nullability annotations to the contrary. This decision eliminates false positives related to very common code patterns that look like this:

if (foo.prop) {
    [bar doStuffWithNonnull:foo.prop];
}

While this probably represents a correct nil-check, the analyzer can't determine correctness without gaining visibility into the property implementation.

Unfortunately, inferring nullable properties as nonnull comes at the cost of significantly reduced code coverage. My goal here is to enable detection of many property-related nullability violations without a large increase in false positives.

The approach is to introduce a heuristic: after accessing the value of a property, if the analyzer at any time proves that the property value is nonnull (which would happen in particular due to a nil-check conditional), then subsequent property accesses on that code path will be *inferred* as nonnull. This captures the pattern described above, which I believe to be the dominant source of false positives in real code.

Diff Detail

Event Timeline

NoQ created this revision.Aug 11 2022, 12:30 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 11 2022, 12:30 AM
NoQ requested review of this revision.Aug 11 2022, 12:30 AM
NoQ added inline comments.Aug 11 2022, 3:28 PM
clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
85–86

Ooops that wasn't part of the patch I should remove that.

NoQ updated this revision to Diff 452887.Aug 15 2022, 9:29 PM

Get rid of unrelated code (whoops).

NoQ updated this revision to Diff 452893.Aug 15 2022, 10:26 PM

Remove a redundant C.addTransition(State). This caused us to run both the old mode and the new mode by producing a state split on an otherwise linear path.

NoQ added a comment.Dec 12 2022, 2:04 PM

Guess I'll commit?

NoQ added a comment.Dec 12 2022, 2:07 PM

(thanks @usama54321 for having a look and talking to me about this offline!)

NoQ accepted this revision.Dec 12 2022, 2:20 PM
This revision is now accepted and ready to land.Dec 12 2022, 2:20 PM
NoQ closed this revision.Dec 12 2022, 2:20 PM