[analyzer] Extend NoStoreFuncVisitor to insert a note on IVars

[george.karpenkov]

Summary

The note is added in the following situation:

• We are throwing a nullability-related warning on an IVar
• The path goes through a method which *could have* (syntactically determined) written into that IVar, but did not

rdar://42444460

[NoQ]

clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp

330 ↗

(On Diff #156849)

Shouldn't it say self->? Dot is kinda property syntax (?)

391–396 ↗

(On Diff #156849)

Wouldn't ObjCIvarRegion::getDecl() be sufficient?

409–410 ↗

(On Diff #156849)

I think we traditionally run a statement matcher on a Decl->getBody(), but i guess it doens't really matter. Just kinda nice to know that we're dealing with statements only and that we could have used a StmtVisitor (as compared to a much more complicated RecursiveASTVisitor) instead.

[george.karpenkov]

clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp

330 ↗

(On Diff #156849)

that's how you refer to ivars in Objective-C.

391–396 ↗

(On Diff #156849)

which line and which operation does this comment refer to?

409–410 ↗

(On Diff #156849)

yeah could be replaced with a StatementVisitor

[NoQ]

clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp

391–396 ↗

(On Diff #156849)

Replace the whole loop, and maybe more code around it, with some sort of dyn_cast<ObjCIvarRegion>(RegionOfInterest)->getDecl().

[NoQ]

clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp

321–323 ↗

(On Diff #157152)

I aim to eliminate this idiom, but i'm definitely not there yet.

456–457 ↗

(On Diff #157152)

That's a lot of parameters that would look better as member variables. I think we should just put an ASTContext reference into the visitor (not include it in Profile) instead of passing it around.

[george.karpenkov]

Reopening as had to revert due to uncommitted dependency.

clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp

330 ↗

(On Diff #156849)

ooops I was wrong.

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

 #include "clang/AST/Decl.h"
 #include "clang/AST/DeclBase.h"
 #include "clang/AST/DeclCXX.h"
 #include "clang/AST/Expr.h"
 #include "clang/AST/ExprCXX.h"
 #include "clang/AST/ExprObjC.h"
 #include "clang/AST/Stmt.h"
 #include "clang/AST/Type.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
 #include "clang/Analysis/AnalysisDeclContext.h"
 #include "clang/Analysis/CFG.h"
 #include "clang/Analysis/CFGStmtMap.h"
 #include "clang/Analysis/ProgramPoint.h"
 #include "clang/Basic/IdentifierTable.h"
 #include "clang/Basic/LLVM.h"
 #include "clang/Basic/SourceLocation.h"
 #include "clang/Basic/SourceManager.h"
std::shared_ptr<PathDiagnosticPiece> VisitNode(const ExplodedNode *N,
     auto CallExitLoc = N->getLocationAs<CallExitBegin>();
 
     // No diagnostic if region was modified inside the frame.
     if (!CallExitLoc)
       return nullptr;
 
     CallEventRef<> Call =
         BRC.getStateManager().getCallEventManager().getCaller(SCtx, State);
-
     const PrintingPolicy &PP = BRC.getASTContext().getPrintingPolicy();
     const SourceManager &SM = BRC.getSourceManager();
+
+    // Region of interest corresponds to an IVar, exiting a method
+    // which could have written into that IVar, but did not.
+    if (const auto *MC = dyn_cast<ObjCMethodCall>(Call))
+      if (const auto *IvarR = dyn_cast<ObjCIvarRegion>(RegionOfInterest))
+        if (potentiallyWritesIntoIvar(Call->getRuntimeDefinition().getDecl(),
+                                      IvarR->getDecl()) &&
+            !isRegionOfInterestModifiedInFrame(N))
+          return notModifiedMemberDiagnostics(
+              Ctx, SM, PP, *CallExitLoc, Call,
+              MC->getReceiverSVal().getAsRegion());
+
     if (const auto *CCall = dyn_cast<CXXConstructorCall>(Call)) {
       const MemRegion *ThisRegion = CCall->getCXXThisVal().getAsRegion();
       if (RegionOfInterest->isSubRegionOf(ThisRegion)
           && !CCall->getDecl()->isImplicit()
           && !isRegionOfInterestModifiedInFrame(N))
-        return notModifiedInConstructorDiagnostics(Ctx, SM, PP, *CallExitLoc,
+        return notModifiedMemberDiagnostics(Ctx, SM, PP, *CallExitLoc,
                                                    CCall, ThisRegion);
     }
 
     ArrayRef<ParmVarDecl *> parameters = getCallParameters(Call);
     for (unsigned I = 0; I < Call->getNumArgs() && I < parameters.size(); ++I) {
       const ParmVarDecl *PVD = parameters[I];
       SVal S = Call->getArgSVal(I);
       unsigned IndirectionLevel = 1;
       QualType T = PVD->getType();
       while (const MemRegion *R = S.getAsRegion()) {
         if (RegionOfInterest->isSubRegionOf(R)
             && !isPointerToConst(PVD->getType())) {
 
           if (isRegionOfInterestModifiedInFrame(N))
             return nullptr;
 
-          return notModifiedDiagnostics(
+          return notModifiedParameterDiagnostics(
               Ctx, SM, PP, *CallExitLoc, Call, PVD, R, IndirectionLevel);
         }
         QualType PT = T->getPointeeType();
         if (PT.isNull() || PT->isVoidType()) break;
         S = State->getSVal(R, PT);
         T = PT;
         IndirectionLevel++;
       }
     }
 
     return nullptr;
   }
 
 private:
+
+  /// \return Whether the method declaration \p Parent
+  /// syntactically has a binary operation writing into the ivar \p Ivar.
+  bool potentiallyWritesIntoIvar(const Decl *Parent,
+                                 const ObjCIvarDecl *Ivar) {
+    using namespace ast_matchers;
+    if (!Parent || !Parent->getBody())
+      return false;
+    StatementMatcher WriteIntoIvarM = binaryOperator(
+        hasOperatorName("="), hasLHS(ignoringParenImpCasts(objcIvarRefExpr(
+                                  hasDeclaration(equalsNode(Ivar))))));
+    StatementMatcher ParentM = stmt(hasDescendant(WriteIntoIvarM));
+    auto Matches = match(ParentM, *Parent->getBody(), Parent->getASTContext());
+    return !Matches.empty();
+  }
+
   /// Check and lazily calculate whether the region of interest is
   /// modified in the stack frame to which \p N belongs.
   /// The calculation is cached in FramesModifyingRegion.
   bool isRegionOfInterestModifiedInFrame(const ExplodedNode *N) {
     const LocationContext *Ctx = N->getLocationContext();
     const StackFrameContext *SCtx = Ctx->getStackFrame();
     if (!FramesModifyingCalculated.count(SCtx))
       findModifyingFrames(N);
private:
   }
 
   /// \return whether \p Ty points to a const type, or is a const reference.
   bool isPointerToConst(QualType Ty) {
     return !Ty->getPointeeType().isNull() &&
            Ty->getPointeeType().getCanonicalType().isConstQualified();
   }
 
-  std::shared_ptr<PathDiagnosticPiece> notModifiedInConstructorDiagnostics(
+  /// \return Diagnostics piece for the member field not modified
+  /// in a given function.
+  std::shared_ptr<PathDiagnosticPiece> notModifiedMemberDiagnostics(
       const LocationContext *Ctx,
       const SourceManager &SM,
       const PrintingPolicy &PP,
       CallExitBegin &CallExitLoc,
-      const CXXConstructorCall *Call,
+      CallEventRef<> Call,
       const MemRegion *ArgRegion) {
+    const char *TopRegionName = isa<ObjCMethodCall>(Call) ? "self" : "this";
     SmallString<256> sbuf;
     llvm::raw_svector_ostream os(sbuf);
     os << DiagnosticsMsg;
-    bool out = prettyPrintRegionName(
-        "this", "->", /*IsReference=*/true,
+    bool out = prettyPrintRegionName(TopRegionName, "->", /*IsReference=*/true,
                                      /*IndirectionLevel=*/1, ArgRegion, os, PP);
 
     // Return nothing if we have failed to pretty-print.
     if (!out)
       return nullptr;
 
     os << "'";
     PathDiagnosticLocation L =
-        getPathDiagnosticLocation(nullptr, SM, Ctx, Call);
+        getPathDiagnosticLocation(CallExitLoc.getReturnStmt(), SM, Ctx, Call);
     return std::make_shared<PathDiagnosticEventPiece>(L, os.str());
   }
 
+  /// \return Diagnostics piece for the parameter \p PVD not modified
+  /// in a given function.
   /// \p IndirectionLevel How many times \c ArgRegion has to be dereferenced
   /// before we get to the super region of \c RegionOfInterest
   std::shared_ptr<PathDiagnosticPiece>
-  notModifiedDiagnostics(const LocationContext *Ctx,
+  notModifiedParameterDiagnostics(const LocationContext *Ctx,
                          const SourceManager &SM,
                          const PrintingPolicy &PP,
                          CallExitBegin &CallExitLoc,
                          CallEventRef<> Call,
                          const ParmVarDecl *PVD,
                          const MemRegion *ArgRegion,
                          unsigned IndirectionLevel) {
     PathDiagnosticLocation L = getPathDiagnosticLocation(
PathDiagnosticLocation getPathDiagnosticLocation(const ReturnStmt *RS,
     if (RS)
       return PathDiagnosticLocation::createBegin(RS, SM, Ctx);
     return PathDiagnosticLocation(
         Call->getRuntimeDefinition().getDecl()->getSourceRange().getEnd(), SM);
   }
 
   /// Pretty-print region \p ArgRegion starting from parent to \p os.
   /// \return whether printing has succeeded
-  bool prettyPrintRegionName(const char *TopRegionName,
-                             const char *Sep,
+  bool prettyPrintRegionName(StringRef TopRegionName,
+                             StringRef Sep,
                              bool IsReference,
                              int IndirectionLevel,
                              const MemRegion *ArgRegion,
                              llvm::raw_svector_ostream &os,
                              const PrintingPolicy &PP) {
     SmallVector<const MemRegion *, 5> Subregions;
     const MemRegion *R = RegionOfInterest;
     while (R != ArgRegion) {
-      if (!(isa<FieldRegion>(R) || isa<CXXBaseObjectRegion>(R)))
+      if (!(isa<FieldRegion>(R) || isa<CXXBaseObjectRegion>(R) ||
+            isa<ObjCIvarRegion>(R)))
         return false; // Pattern-matching failed.
       Subregions.push_back(R);
       R = cast<SubRegion>(R)->getSuperRegion();
     }
     bool IndirectReference = !Subregions.empty();
 
     if (IndirectReference)
       IndirectionLevel--; // Due to "->" symbol.
bool prettyPrintRegionName(StringRef TopRegionName,
     if (ShouldSurround)
       os << ")";
 
     for (auto I = Subregions.rbegin(), E = Subregions.rend(); I != E; ++I) {
       if (const auto *FR = dyn_cast<FieldRegion>(*I)) {
         os << Sep;
         FR->getDecl()->getDeclName().print(os, PP);
         Sep = ".";
+      } else if (const auto *IR = dyn_cast<ObjCIvarRegion>(*I)) {
+        os << "->";
+        IR->getDecl()->getDeclName().print(os, PP);
+        Sep = ".";
       } else if (isa<CXXBaseObjectRegion>(*I)) {
         continue; // Just keep going up to the base region.
       } else {
         llvm_unreachable("Previous check has missed an unexpected region");
       }
     }
     return true;
   }

cfe/trunk/test/Analysis/diagnostics/no-store-func-path-notes.cpp

int use_partially_init_inherited_struct() {
   return b.y;                          // expected-warning{{Undefined or garbage value returned to caller}}
                                        // expected-note@-1{{Undefined or garbage value returned to caller}}
 }
 
 struct C {
   int x;
   int y;
   C(int pX, int pY) : x(pX) {} // expected-note{{Returning without writing to 'this->y'}}
+
+  C(int pX, int pY, bool Flag) {
+    x = pX;
+    if (Flag) // expected-note{{Assuming 'Flag' is not equal to 0}}
+              // expected-note@-1{{Taking true branch}}
+      return; // expected-note{{Returning without writing to 'this->y'}}
+    y = pY;
+  }
 };
 
 int use_constructor() {
   C c(0, 0); // expected-note{{Calling constructor for 'C'}}
              // expected-note@-1{{Returning from constructor for 'C'}}
   return c.y; // expected-note{{Undefined or garbage value returned to caller}}
               // expected-warning@-1{{Undefined or garbage value returned to caller}}
 }
 
+int coin();
+
+int use_other_constructor() {
+  C c(0, 0, coin()); // expected-note{{Calling constructor for 'C'}}
+                     // expected-note@-1{{Returning from constructor for 'C'}}
+  return c.y; // expected-note{{Undefined or garbage value returned to caller}}
+              // expected-warning@-1{{Undefined or garbage value returned to caller}}
+}
+
 struct D {
   void initialize(int *);
 };
 
 void D::initialize(int *p) {
 
 } // expected-note{{Returning without writing to '*p'}}
 
 int use_d_initializer(D* d) {
   int p; // expected-note {{'p' declared without an initial value}}
   d->initialize(&p); // expected-note{{Calling 'D::initialize'}}
                      // expected-note@-1{{Returning from 'D::initialize'}}
   return p;                     // expected-note{{Undefined or garbage value returned to caller}}
                                 // expected-warning@-1{{Undefined or garbage value returned to caller}}
 }
 
-int coin();
-
 struct S2 {
   int x;
 };
 
 int pointerreference(S2* &s) {
   if (coin()) // expected-note{{Assuming the condition is true}}
               // expected-note@-1{{Taking true branch}}
     return 1; // expected-note{{Returning without writing to 's->x'}}

cfe/trunk/test/Analysis/diagnostics/no-store-func-path-notes.m

-// RUN: %clang_analyze_cc1 -x objective-c -analyzer-checker=core -analyzer-output=text -Wno-objc-root-class -fblocks -verify %s
+// RUN: %clang_analyze_cc1 -x objective-c -analyzer-checker=core,nullability -analyzer-output=text -Wno-objc-root-class -fblocks -verify %s
 
-@interface I
+#include "../Inputs/system-header-simulator-for-nullability.h"
+
+extern int coin();
+
+@interface I : NSObject
 - (int)initVar:(int *)var param:(int)param;
 @end
 
 @implementation I
 - (int)initVar:(int *)var param:(int)param {
   if (param) { // expected-note{{Taking false branch}}
     *var = 1;
     return 0;
^{                     // expected-note {{Calling anonymous block}}
     int p;               // expected-note{{'p' declared without an initial value}}
     initializer1(&p, 0); // expected-note{{Calling 'initializer1'}}
                          // expected-note@-1{{Returning from 'initializer1'}}
     z = p;               // expected-warning{{Assigned value is garbage or undefined}}
                          // expected-note@-1{{Assigned value is garbage or undefined}}
   }();
   return z;
 }
+
+extern void expectNonNull(NSString * _Nonnull a);
+
+@interface A : NSObject
+- (void) func;
+- (void) initAMaybe;
+@end
+
+@implementation A {
+  NSString * a;
+}
+
+- (void) initAMaybe {
+  if (coin()) // expected-note{{Assuming the condition is false}}
+              // expected-note@-1{{Taking false branch}}
+    a = @"string";
+} // expected-note{{Returning without writing to 'self->a'}}
+
+- (void) func {
+  a = nil; // expected-note{{nil object reference stored to 'a'}}
+  [self initAMaybe]; // expected-note{{Calling 'initAMaybe'}}
+                     // expected-note@-1{{Returning from 'initAMaybe'}}
+  expectNonNull(a); // expected-warning{{nil passed to a callee that requires a non-null 1st parameter}}
+                    // expected-note@-1{{nil passed to a callee that requires a non-null 1st parameter}}
+}
+
+@end