[analyzer] Extend NoStoreFuncVisitor to insert a note on IVars

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

Differential Revision: https://reviews.llvm.org/D49689

git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@337864 91177308-0d34-0410-b5e6-96231b3b80d8

diff --git a/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp b/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
index f17f41a7ac93265e47f104cc313748c14ff9b41d..be16ce10e068c82c7adb0d74c54cacefa9b020bc 100644 (file)
--- a/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
+++ b/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
@@ -22,6 +22,7 @@
 #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"
@@ -306,15 +307,26 @@ public:
 
     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);
     }
 
@@ -331,7 +343,7 @@ public:
           if (isRegionOfInterestModifiedInFrame(N))
             return nullptr;
 
-          return notModifiedDiagnostics(
+          return notModifiedParameterDiagnostics(
               Ctx, SM, PP, *CallExitLoc, Call, PVD, R, IndirectionLevel);
         }
         QualType PT = T->getPointeeType();
@@ -346,6 +358,22 @@ public:
   }
 
 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)
+      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.
@@ -414,19 +442,21 @@ private:
            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,
-        /*IndirectionLevel=*/1, ArgRegion, os, PP);
+    bool out = prettyPrintRegionName(TopRegionName, "->", /*IsReference=*/true,
+                                     /*IndirectionLevel=*/1, ArgRegion, os, PP);
 
     // Return nothing if we have failed to pretty-print.
     if (!out)
@@ -434,14 +464,16 @@ private:
 
     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,
@@ -481,8 +513,8 @@ private:
 
   /// 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,
@@ -491,7 +523,8 @@ private:
     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();
@@ -519,6 +552,10 @@ private:
         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 {

diff --git a/test/Analysis/diagnostics/no-store-func-path-notes.cpp b/test/Analysis/diagnostics/no-store-func-path-notes.cpp
index b96dc4cf2a8568c90fe7438aa04eba8ce524aa73..17ec96ae5b75d5705d2ea854b4890492aa64f865 100644 (file)
--- a/test/Analysis/diagnostics/no-store-func-path-notes.cpp
+++ b/test/Analysis/diagnostics/no-store-func-path-notes.cpp
@@ -97,6 +97,14 @@ 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() {
@@ -106,6 +114,15 @@ int use_constructor() {
               // 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 *);
 };
@@ -122,8 +139,6 @@ int use_d_initializer(D* d) {
                                 // expected-warning@-1{{Undefined or garbage value returned to caller}}
 }
 
-int coin();
-
 struct S2 {
   int x;
 };

diff --git a/test/Analysis/diagnostics/no-store-func-path-notes.m b/test/Analysis/diagnostics/no-store-func-path-notes.m
index 3ae97ef56499a68774d5347f11581a124d419347..51d1515e860a27fb5adfb29598c8fa6c8fae4109 100644 (file)
--- a/test/Analysis/diagnostics/no-store-func-path-notes.m
+++ b/test/Analysis/diagnostics/no-store-func-path-notes.m
@@ -1,6 +1,10 @@
-// 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
 
@@ -44,3 +48,30 @@ int initFromBlock() {
   }();
   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