Page MenuHomePhabricator

[analyzer] Nullability: fix a crash when adding notes inside a synthesized property accessor.
ClosedPublic

Authored by NoQ on Apr 24 2017, 8:45 AM.

Details

Summary

In this test case, the body of a property is autosynthesized, and its source locations are invalid. Path diagnostic locations must refer to valid source locations, hence we're hitting an assertion. This patch disables the bugvisitor's note, however we'd need to figure out where to stick it eventually. These new "extra notes" of mine might be useful (we could put them at property declaration), i could add them if everybody likes this idea.

Diff Detail

Repository
rL LLVM

Event Timeline

NoQ created this revision.Apr 24 2017, 8:45 AM
zaks.anna accepted this revision.May 9 2017, 11:22 PM

These new "extra notes" of mine might be useful (we could put them at property declaration), i could add them if everybody likes this idea.

What are these? Is there a PR?

It would be great if we could place a note on a parent that has location. For example, can we walk up the location context?
If you do not fix it in this PR, it's definitely worth a FIXME.

This revision is now accepted and ready to land.May 9 2017, 11:22 PM
NoQ added a comment.May 9 2017, 11:40 PM

What are these? Is there a PR?

The best explanation we currently have, with examples, is D24278

It would be great if we could place a note on a parent that has location.

That's actually the best approach in my opinion, if achievable.

For example, can we walk up the location context?

That wouldn't work this way because we'd have the completely redundant "calling property accessor" piece before that, and "returning..." after that. Instead, i guess i'd try to actually report a note either before entering the call or after returning from the call, so that call enter notes get pruned for having nothing in between.

That wouldn't work this way because we'd have the completely redundant "calling property accessor" piece before that, and "returning..." after that.

I think we should not print "calling" and "returning" for calling into and returning from autogenerated code, If we do, we should fix that as well. Wouldn't it be simpler to just not emit those nodes if we see that they are calling autogenerated code? (Do we have a valid source location in autogenerated context? If not, we could just not print "calling" and "returning" for calls into a body that does not have proper location info.) If you looked into this already and these are not useful suggestions, please, disregard:)

NoQ added a comment.May 9 2017, 11:48 PM

That wouldn't work this way because we'd have the completely redundant "calling property accessor" piece before that, and "returning..." after that.

I think we should not print "calling" and "returning" for calling into and returning from autogenerated code,

This sounds logical, yeah, i could do that as well (though right now it doesn't work that way, and i suspect nobody tried).

NoQ updated this revision to Diff 99300.May 17 2017, 7:35 AM

Automatically pop up from bodyfarm-originated code in PathDiagnosticLocation::getStmt().

Avoid putting "Calling..." and "Returning..." notes on Objective-C auto-synthesized property calls, because they would never call visible code later.

Test the newly added path notes.

Thanks! A couple of minor comments below.

lib/StaticAnalyzer/Core/PathDiagnostic.cpp
699 ↗(On Diff #99300)

Nit: You could probably factor this into a nicely named helper function.

704 ↗(On Diff #99300)

Is ParentLC guaranteed never to be null? I would prefer checking for it inside the while. Even if it is not likely to happen now, the code could evolve to handle more cases..

932 ↗(On Diff #99300)

There was a case where we wanted to produce events for auto synthesized code, could you add a comment on what they were? (Otherwise, it's not clear why we need the isPropertyAccessor check.)

NoQ updated this revision to Diff 100673.May 30 2017, 2:18 AM
NoQ marked 3 inline comments as done.

Fix comments.

lib/StaticAnalyzer/Core/PathDiagnostic.cpp
704 ↗(On Diff #99300)

We're unlikely to begin analysis at top-frames with auto-synthesized code bodies in the future. Added an assertion.

This revision was automatically updated to reflect the committed changes.