[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

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

391–396

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

409–410

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

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

391–396

which line and which operation does this comment refer to?

409–410

yeah could be replaced with a StatementVisitor

[NoQ]

clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp

391–396

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

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

484–485

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

ooops I was wrong.

clang/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 ObjCIvarDecl *IVD = ivarForRegionOfInterest(MC, State)) {
+
+        // Find declaration associated with the runtime definition.
+        const Decl *PD = Call->getRuntimeDefinition().getDecl();
+        if (!PD)
+          PD = MC->getDecl();
+
+        if (potentiallyWritesIntoIvar(PD, IVD) &&
+            !isRegionOfInterestModifiedInFrame(N)) {
+
+          PathDiagnosticLocation L = getPathDiagnosticLocation(
+            CallExitLoc->getReturnStmt(), SM, Ctx, Call);
+          SmallString<256> sbuf;
+          llvm::raw_svector_ostream os(sbuf);
+          os << DiagnosticsMsg << "self." << IVD->getName() << "'";
+          return std::make_shared<PathDiagnosticEventPiece>(L, os.str());
+        }
+      }
+    }
+
+
     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,
                                                    CCall, ThisRegion);
     }
for (unsigned I = 0; I < Call->getNumArgs() && I < parameters.size(); ++I) {
         IndirectionLevel++;
       }
     }
 
     return nullptr;
   }
 
 private:
+
+  /// For a method declaration and a program state,
+  /// find an ivar corresponding to the region of interest.
+  ObjCIvarDecl *ivarForRegionOfInterest(const ObjCMethodCall *MC,
+                                        ProgramStateRef State) {
+    auto *MD = MC->getDecl();
+    auto *P = MD->getParent();
+    auto *ID = dyn_cast<ObjCInterfaceDecl>(P);
+
+    if (!ID)
+      return nullptr;
+
+    const ObjCImplementationDecl *ImplDecl = ID->getImplementation();
+    if (!ImplDecl)
+      return nullptr;
+
+    SVal Receiver = MC->getReceiverSVal();
+    for (ObjCIvarDecl *IVD : ImplDecl->ivars()) {
+      SVal LValue = State->getLValue(IVD, Receiver);
+      const MemRegion *LR = LValue.getAsRegion();
+      if (LR == RegionOfInterest)
+        return IVD; // found
+    }
+
+    return nullptr;
+  }
+
+  /// \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;
+    StatementMatcher WriteIntoIvarM = binaryOperator(
+        hasOperatorName("="), hasLHS(ignoringParenImpCasts(objcIvarRefExpr(
+                                  hasDeclaration(equalsNode(Ivar))))));
+    DeclarationMatcher ParentM = decl(hasDescendant(WriteIntoIvarM));
+    auto Matches = match(ParentM, *Parent, 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(
       const LocationContext *Ctx,
       const SourceManager &SM,
       const PrintingPolicy &PP,
       CallExitBegin &CallExitLoc,
-      const CXXConstructorCall *Call,
+      CallEventRef<> Call,
       const MemRegion *ArgRegion) {
     SmallString<256> sbuf;
     llvm::raw_svector_ostream os(sbuf);
     os << DiagnosticsMsg;
     bool out = prettyPrintRegionName(
         "this", "->", /*IsReference=*/true,
         /*IndirectionLevel=*/1, ArgRegion, os, PP);
 

clang/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) 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}}
+}
+
+- (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'}}
+@end