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

[NoQ]

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.

[zaks.anna]

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.

[NoQ]

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.

[zaks.anna]

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]

In D32437#750622, @zaks.anna wrote:

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]

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.

[zaks.anna]

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]

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.

cfe/trunk/include/clang/StaticAnalyzer/Core/BugReporter/PathDiagnostic.h

public:
   static inline bool classof(const PathDiagnosticPiece *P) {
     return P->getKind() == Event;
   }
 };
 
 class PathDiagnosticCallPiece : public PathDiagnosticPiece {
   PathDiagnosticCallPiece(const Decl *callerD,
                           const PathDiagnosticLocation &callReturnPos)
       : PathDiagnosticPiece(Call), Caller(callerD), Callee(nullptr),
-      NoExit(false), callReturn(callReturnPos) {}
+        NoExit(false), IsCalleeAnAutosynthesizedPropertyAccessor(false),
+        callReturn(callReturnPos) {}
 
   PathDiagnosticCallPiece(PathPieces &oldPath, const Decl *caller)
       : PathDiagnosticPiece(Call), Caller(caller), Callee(nullptr),
-      NoExit(true), path(oldPath) {}
-  
+        NoExit(true), IsCalleeAnAutosynthesizedPropertyAccessor(false),
+        path(oldPath) {}
   const Decl *Caller;
   const Decl *Callee;
 
   // Flag signifying that this diagnostic has only call enter and no matching
   // call exit.
   bool NoExit;
 
+  // Flag signifying that the callee function is an Objective-C autosynthesized
+  // property getter or setter.
+  bool IsCalleeAnAutosynthesizedPropertyAccessor;
+
   // The custom string, which should appear after the call Return Diagnostic.
   // TODO: Should we allow multiple diagnostics?
   std::string CallStackMessage;
 
 public:
   PathDiagnosticLocation callEnter;
   PathDiagnosticLocation callEnterWithin;
   PathDiagnosticLocation callReturn;  

cfe/trunk/lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp

if (!TrackedNullab)
     return nullptr;
 
   if (TrackedNullabPrev &&
       TrackedNullabPrev->getValue() == TrackedNullab->getValue())
     return nullptr;
 
   // Retrieve the associated statement.
   const Stmt *S = TrackedNullab->getNullabilitySource();
-  if (!S) {
+  if (!S || S->getLocStart().isInvalid()) {
     S = PathDiagnosticLocation::getStmt(N);
   }
 
   if (!S)
     return nullptr;
 
   std::string InfoText =
       (llvm::Twine("Nullability '") +

cfe/trunk/lib/StaticAnalyzer/Core/PathDiagnostic.cpp

return getLocationForCaller(CEE->getCalleeContext(),
                                 SMng);
   } else {
     llvm_unreachable("Unexpected ProgramPoint");
   }
 
   return PathDiagnosticLocation(S, SMng, P.getLocationContext());
 }
 
+static const LocationContext *
+findTopAutosynthesizedParentContext(const LocationContext *LC) {
+  assert(LC->getAnalysisDeclContext()->isBodyAutosynthesized());
+  const LocationContext *ParentLC = LC->getParent();
+  assert(ParentLC && "We don't start analysis from autosynthesized code");
+  while (ParentLC->getAnalysisDeclContext()->isBodyAutosynthesized()) {
+    LC = ParentLC;
+    ParentLC = LC->getParent();
+    assert(ParentLC && "We don't start analysis from autosynthesized code");
+  }
+  return LC;
+}
+
 const Stmt *PathDiagnosticLocation::getStmt(const ExplodedNode *N) {
+  // We cannot place diagnostics on autosynthesized code.
+  // Put them onto the call site through which we jumped into autosynthesized
+  // code for the first time.
+  const LocationContext *LC = N->getLocationContext();
+  if (LC->getAnalysisDeclContext()->isBodyAutosynthesized()) {
+    // It must be a stack frame because we only autosynthesize functions.
+    return cast<StackFrameContext>(findTopAutosynthesizedParentContext(LC))
+        ->getCallSite();
+  }
+  // Otherwise, see if the node's program point directly points to a statement.
   ProgramPoint P = N->getLocation();
   if (Optional<StmtPoint> SP = P.getAs<StmtPoint>())
     return SP->getStmt();
   if (Optional<BlockEdge> BE = P.getAs<BlockEdge>())
     return BE->getSrc()->getTerminator();
   if (Optional<CallEnter> CE = P.getAs<CallEnter>())
     return CE->getCallExpr();
   if (Optional<CallExitEnd> CEE = P.getAs<CallExitEnd>())

 
 void PathDiagnosticCallPiece::setCallee(const CallEnter &CE,
                                         const SourceManager &SM) {
   const StackFrameContext *CalleeCtx = CE.getCalleeContext();
   Callee = CalleeCtx->getDecl();
 
   callEnterWithin = PathDiagnosticLocation::createBegin(Callee, SM);
   callEnter = getLocationForCaller(CalleeCtx, CE.getLocationContext(), SM);
+
+  // Autosynthesized property accessors are special because we'd never
+  // pop back up to non-autosynthesized code until we leave them.
+  // This is not generally true for autosynthesized callees, which may call
+  // non-autosynthesized callbacks.
+  // Unless set here, the IsCalleeAnAutosynthesizedPropertyAccessor flag
+  // defaults to false.
+  if (const ObjCMethodDecl *MD = dyn_cast<ObjCMethodDecl>(Callee))
+    IsCalleeAnAutosynthesizedPropertyAccessor = (
+        MD->isPropertyAccessor() &&
+        CalleeCtx->getAnalysisDeclContext()->isBodyAutosynthesized());
 }
 
 static inline void describeClass(raw_ostream &Out, const CXXRecordDecl *D,
                                  StringRef Prefix = StringRef()) {
   if (!D->getIdentifier())
     return;
   Out << Prefix << '\'' << *D << '\'';
 }
static bool describeCodeDecl(raw_ostream &Out, const Decl *D,
   }
 
   Out << Prefix << '\'' << cast<NamedDecl>(*D) << '\'';
   return true;
 }
 
 std::shared_ptr<PathDiagnosticEventPiece>
 PathDiagnosticCallPiece::getCallEnterEvent() const {
-  if (!Callee)
+  // We do not produce call enters and call exits for autosynthesized property
+  // accessors. We do generally produce them for other functions coming from
+  // the body farm because they may call callbacks that bring us back into
+  // visible code.
+  if (!Callee || IsCalleeAnAutosynthesizedPropertyAccessor)
     return nullptr;
 
   SmallString<256> buf;
   llvm::raw_svector_ostream Out(buf);
 
   Out << "Calling ";
   describeCodeDecl(Out, Callee, /*ExtendedDescription=*/true);
 
PathDiagnosticCallPiece::getCallEnterWithinCallerEvent() const {
   Out << "Entered call";
   describeCodeDecl(Out, Caller, /*ExtendedDescription=*/false, " from ");
 
   return std::make_shared<PathDiagnosticEventPiece>(callEnterWithin, Out.str());
 }
 
 std::shared_ptr<PathDiagnosticEventPiece>
 PathDiagnosticCallPiece::getCallExitEvent() const {
-  if (NoExit)
+  // We do not produce call enters and call exits for autosynthesized property
+  // accessors. We do generally produce them for other functions coming from
+  // the body farm because they may call callbacks that bring us back into
+  // visible code.
+  if (NoExit || IsCalleeAnAutosynthesizedPropertyAccessor)
     return nullptr;
 
   SmallString<256> buf;
   llvm::raw_svector_ostream Out(buf);
 
   if (!CallStackMessage.empty()) {
     Out << CallStackMessage;
   } else {

cfe/trunk/test/Analysis/nullability-notes.m

+// RUN: %clang_analyze_cc1 -fblocks -analyzer-checker=core,nullability.NullPassedToNonnull,nullability.NullReturnedFromNonnull,nullability.NullablePassedToNonnull,nullability.NullableReturnedFromNonnull,nullability.NullableDereferenced -analyzer-output=text -verify %s
+
+#include "Inputs/system-header-simulator-for-nullability.h"
+
+void takesNonnull(NSObject *_Nonnull y);
+
+@interface ClassWithProperties: NSObject
+@property(copy, nullable) NSObject *x;
+-(void) method;
+@end;
+@implementation ClassWithProperties
+-(void) method {
+  // no-crash
+  NSObject *x = self.x; // expected-note{{Nullability 'nullable' is inferred}}
+  takesNonnull(x); // expected-warning{{Nullable pointer is passed to a callee that requires a non-null 1st parameter}}
+                   // expected-note@-1{{Nullable pointer is passed to a callee that requires a non-null 1st parameter}}
+}
+@end