[analyzer] Improve Nullability checker diagnostics

- Include the position of the argument on which the nullability is violated
- Differentiate between a 'method' and a 'function' in the message wording
- Test for the error message text in the tests
- Fix a bug with setting 'IsDirectDereference' which resulted in regular dereferences assumed to have call context.

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

include/clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h

@@ -263,6 +263,10 @@ public:
     Eng.getBugReporter().emitReport(std::move(R));
   }
 
+  /// \brief Returns the word that should be used to refer to the declaration
+  /// in the report.
+  StringRef getDeclDescription(const Decl *D);
+
   /// \brief Get the declaration of the called function (path-sensitive).
   const FunctionDecl *getCalleeDecl(const CallExpr *CE) const;
 

lib/StaticAnalyzer/Checkers/DereferenceChecker.cpp

@@ -230,7 +230,7 @@ void DereferenceChecker::checkLocation(SVal l, bool isLoad, const Stmt* S,
     // dereference.
     if (ExplodedNode *N = C.generateSink(nullState, C.getPredecessor())) {
       ImplicitNullDerefEvent event = {l, isLoad, N, &C.getBugReporter(),
-                                      /*IsDirectDereference=*/false};
+                                      /*IsDirectDereference=*/true};
       dispatchEvent(event);
     }
   }
@@ -272,7 +272,7 @@ void DereferenceChecker::checkBind(SVal L, SVal V, const Stmt *S,
     if (ExplodedNode *N = C.generateSink(StNull, C.getPredecessor())) {
       ImplicitNullDerefEvent event = {V, /*isLoad=*/true, N,
                                       &C.getBugReporter(),
-                                      /*IsDirectDereference=*/false};
+                                      /*IsDirectDereference=*/true};
       dispatchEvent(event);
     }
   }

lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp

@@ -26,13 +26,16 @@
 //===----------------------------------------------------------------------===//
 
 #include "ClangSACheckers.h"
-#include "llvm/Support/Path.h"
+
 #include "clang/StaticAnalyzer/Core/BugReporter/BugType.h"
 #include "clang/StaticAnalyzer/Core/Checker.h"
 #include "clang/StaticAnalyzer/Core/CheckerManager.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h"
 
+#include "llvm/ADT/StringExtras.h"
+#include "llvm/Support/Path.h"
+
 using namespace clang;
 using namespace ento;
 
@@ -89,18 +92,6 @@ enum class ErrorKind : int {
   NullablePassedToNonnull
 };
 
-const char *const ErrorMessages[] = {
-    "Null is assigned to a pointer which is expected to have non-null value",
-    "Null passed to a callee that requires a non-null argument",
-    "Null is returned from a function that is expected to return a non-null "
-    "value",
-    "Nullable pointer is assigned to a pointer which is expected to have "
-    "non-null value",
-    "Nullable pointer is returned from a function that is expected to return a "
-    "non-null value",
-    "Nullable pointer is dereferenced",
-    "Nullable pointer is passed to a callee that requires a non-null argument"};
-
 class NullabilityChecker
     : public Checker<check::Bind, check::PreCall, check::PreStmt<ReturnStmt>,
                      check::PostCall, check::PostStmt<ExplicitCastExpr>,
@@ -169,17 +160,19 @@ private:
   ///
   /// When \p SuppressPath is set to true, no more bugs will be reported on this
   /// path by this checker.
-  void reportBugIfPreconditionHolds(ErrorKind Error, ExplodedNode *N,
-                                    const MemRegion *Region, CheckerContext &C,
+  void reportBugIfPreconditionHolds(StringRef Msg, ErrorKind Error,
+                                    ExplodedNode *N, const MemRegion *Region,
+                                    CheckerContext &C,
                                     const Stmt *ValueExpr = nullptr,
                                     bool SuppressPath = false) const;
 
-  void reportBug(ErrorKind Error, ExplodedNode *N, const MemRegion *Region,
-                 BugReporter &BR, const Stmt *ValueExpr = nullptr) const {
+  void reportBug(StringRef Msg, ErrorKind Error, ExplodedNode *N,
+                 const MemRegion *Region, BugReporter &BR,
+                 const Stmt *ValueExpr = nullptr) const {
     if (!BT)
       BT.reset(new BugType(this, "Nullability", "Memory error"));
-    const char *Msg = ErrorMessages[static_cast<int>(Error)];
-    std::unique_ptr<BugReport> R(new BugReport(*BT, Msg, N));
+
+    auto R = llvm::make_unique<BugReport>(*BT, Msg, N);
     if (Region) {
       R->markInteresting(Region);
       R->addVisitor(llvm::make_unique<NullabilityBugVisitor>(Region));
@@ -384,7 +377,7 @@ static bool checkPreconditionViolation(ProgramStateRef State, ExplodedNode *N,
   return false;
 }
 
-void NullabilityChecker::reportBugIfPreconditionHolds(
+void NullabilityChecker::reportBugIfPreconditionHolds(StringRef Msg,
     ErrorKind Error, ExplodedNode *N, const MemRegion *Region,
     CheckerContext &C, const Stmt *ValueExpr, bool SuppressPath) const {
   ProgramStateRef OriginalState = N->getState();
@@ -396,7 +389,7 @@ void NullabilityChecker::reportBugIfPreconditionHolds(
     N = C.addTransition(OriginalState, N);
   }
 
-  reportBug(Error, N, Region, C.getBugReporter(), ValueExpr);
+  reportBug(Msg, Error, N, Region, C.getBugReporter(), ValueExpr);
 }
 
 /// Cleaning up the program state.
@@ -450,9 +443,13 @@ void NullabilityChecker::checkEvent(ImplicitNullDerefEvent Event) const {
     // Do not suppress errors on defensive code paths, because dereferencing
     // a nullable pointer is always an error.
     if (Event.IsDirectDereference)
-      reportBug(ErrorKind::NullableDereferenced, Event.SinkNode, Region, BR);
-    else
-      reportBug(ErrorKind::NullablePassedToNonnull, Event.SinkNode, Region, BR);
+      reportBug("Nullable pointer is dereferenced",
+                ErrorKind::NullableDereferenced, Event.SinkNode, Region, BR);
+    else {
+      reportBug("Nullable pointer is passed to a callee that requires a "
+                "non-null", ErrorKind::NullablePassedToNonnull,
+                Event.SinkNode, Region, BR);
+    }
   }
 }
 
@@ -537,7 +534,14 @@ void NullabilityChecker::checkPreStmt(const ReturnStmt *S,
     ExplodedNode *N = C.generateErrorNode(State, &Tag);
     if (!N)
       return;
-    reportBugIfPreconditionHolds(ErrorKind::NilReturnedToNonnull, N, nullptr, C,
+
+    SmallString<256> SBuf;
+    llvm::raw_svector_ostream OS(SBuf);
+    OS << "Null is returned from a " << C.getDeclDescription(D) <<
+          " that is expected to return a non-null value";
+
+    reportBugIfPreconditionHolds(OS.str(),
+                                 ErrorKind::NilReturnedToNonnull, N, nullptr, C,
                                  RetExpr);
     return;
   }
@@ -556,7 +560,14 @@ void NullabilityChecker::checkPreStmt(const ReturnStmt *S,
         RequiredNullability == Nullability::Nonnull) {
       static CheckerProgramPointTag Tag(this, "NullableReturnedFromNonnull");
       ExplodedNode *N = C.addTransition(State, C.getPredecessor(), &Tag);
-      reportBugIfPreconditionHolds(ErrorKind::NullableReturnedToNonnull, N,
+
+      SmallString<256> SBuf;
+      llvm::raw_svector_ostream OS(SBuf);
+      OS << "Nullable pointer is returned from a " << C.getDeclDescription(D) <<
+            " that is expected to return a non-null value";
+
+      reportBugIfPreconditionHolds(OS.str(),
+                                   ErrorKind::NullableReturnedToNonnull, N,
                                    Region, C);
     }
     return;
@@ -605,14 +616,21 @@ void NullabilityChecker::checkPreCall(const CallEvent &Call,
     Nullability ArgExprTypeLevelNullability =
         getNullabilityAnnotation(ArgExpr->getType());
 
+    unsigned ParamIdx = Param->getFunctionScopeIndex() + 1;
+
     if (Filter.CheckNullPassedToNonnull && Nullness == NullConstraint::IsNull &&
         ArgExprTypeLevelNullability != Nullability::Nonnull &&
         RequiredNullability == Nullability::Nonnull) {
       ExplodedNode *N = C.generateErrorNode(State);
       if (!N)
         return;
-      reportBugIfPreconditionHolds(ErrorKind::NilPassedToNonnull, N, nullptr, C,
-                                   ArgExpr);
+      SmallString<256> SBuf;
+      llvm::raw_svector_ostream OS(SBuf);
+      OS << "Null passed to a callee that requires a non-null " << ParamIdx
+         << llvm::getOrdinalSuffix(ParamIdx) << " parameter";
+      reportBugIfPreconditionHolds(OS.str(), ErrorKind::NilPassedToNonnull, N,
+                                   nullptr, C,
+                                   ArgExpr, /*SuppressPath=*/false);
       return;
     }
 
@@ -631,14 +649,20 @@ void NullabilityChecker::checkPreCall(const CallEvent &Call,
       if (Filter.CheckNullablePassedToNonnull &&
           RequiredNullability == Nullability::Nonnull) {
         ExplodedNode *N = C.addTransition(State);
-        reportBugIfPreconditionHolds(ErrorKind::NullablePassedToNonnull, N,
+        SmallString<256> SBuf;
+        llvm::raw_svector_ostream OS(SBuf);
+        OS << "Nullable pointer is passed to a callee that requires a non-null "
+           << ParamIdx << llvm::getOrdinalSuffix(ParamIdx) << " parameter";
+        reportBugIfPreconditionHolds(OS.str(),
+                                     ErrorKind::NullablePassedToNonnull, N,
                                      Region, C, ArgExpr, /*SuppressPath=*/true);
         return;
       }
       if (Filter.CheckNullableDereferenced &&
           Param->getType()->isReferenceType()) {
         ExplodedNode *N = C.addTransition(State);
-        reportBugIfPreconditionHolds(ErrorKind::NullableDereferenced, N, Region,
+        reportBugIfPreconditionHolds("Nullable pointer is dereferenced",
+                                     ErrorKind::NullableDereferenced, N, Region,
                                      C, ArgExpr, /*SuppressPath=*/true);
         return;
       }
@@ -1007,7 +1031,9 @@ void NullabilityChecker::checkBind(SVal L, SVal V, const Stmt *S,
     if (!ValueExpr)
       ValueExpr = S;
 
-    reportBugIfPreconditionHolds(ErrorKind::NilAssignedToNonnull, N, nullptr, C,
+    reportBugIfPreconditionHolds("Null is assigned to a pointer which is "
+                                 "expected to have non-null value",
+                                 ErrorKind::NilAssignedToNonnull, N, nullptr, C,
                                  ValueExpr);
     return;
   }
@@ -1029,7 +1055,9 @@ void NullabilityChecker::checkBind(SVal L, SVal V, const Stmt *S,
         LocNullability == Nullability::Nonnull) {
       static CheckerProgramPointTag Tag(this, "NullablePassedToNonnull");
       ExplodedNode *N = C.addTransition(State, C.getPredecessor(), &Tag);
-      reportBugIfPreconditionHolds(ErrorKind::NullableAssignedToNonnull, N,
+      reportBugIfPreconditionHolds("Nullable pointer is assigned to a pointer "
+                                   "which is expected to have non-null value",
+                                   ErrorKind::NullableAssignedToNonnull, N,
                                    ValueRegion, C);
     }
     return;

lib/StaticAnalyzer/Core/CheckerContext.cpp

@@ -35,6 +35,13 @@ StringRef CheckerContext::getCalleeName(const FunctionDecl *FunDecl) const {
   return funI->getName();
 }
 
+StringRef CheckerContext::getDeclDescription(const Decl *D) {
+  if (isa<ObjCMethodDecl>(D) || isa<CXXMethodDecl>(D))
+    return "method";
+  if (isa<BlockDecl>(D))
+    return "anonymous block";
+  return "function";
+}
 
 bool CheckerContext::isCLibraryFunction(const FunctionDecl *FD,
                                         StringRef Name) {

test/Analysis/nullability.mm

@@ -1,5 +1,4 @@
-// RUN: %clang_cc1 -fobjc-arc -analyze -analyzer-checker=core,nullability -verify %s
-// RUN: %clang_cc1 -analyze -analyzer-checker=core,nullability -verify %s
+// RUN: %clang_cc1 -fblocks -analyze -analyzer-checker=core,nullability -verify %s
 
 #define nil 0
 #define BOOL int
@@ -72,16 +71,16 @@ void testBasicRules() {
   // Make every dereference a different path to avoid sinks after errors.
   switch (getRandom()) {
   case 0: {
-    Dummy &r = *p; // expected-warning {{}}
+    Dummy &r = *p; // expected-warning {{Nullable pointer is dereferenced}}
   } break;
   case 1: {
-    int b = p->val; // expected-warning {{}}
+    int b = p->val; // expected-warning {{Nullable pointer is dereferenced}}
   } break;
   case 2: {
-    int stuff = *ptr; // expected-warning {{}}
+    int stuff = *ptr; // expected-warning {{Nullable pointer is dereferenced}}
   } break;
   case 3:
-    takesNonnull(p); // expected-warning {{}}
+    takesNonnull(p); // expected-warning {{Nullable pointer is passed to a callee that requires a non-null 1st parameter}}
     break;
   case 4: {
     Dummy d;
@@ -103,11 +102,11 @@ void testBasicRules() {
   Dummy *q = 0;
   if (getRandom()) {
     takesNullable(q);
-    takesNonnull(q); // expected-warning {{}}
+    takesNonnull(q); // expected-warning {{Null passed to a callee that requires a non-null 1st parameter}}
   }
   Dummy a;
   Dummy *_Nonnull nonnull = &a;
-  nonnull = q; // expected-warning {{}}
+  nonnull = q; // expected-warning {{Null is assigned to a pointer which is expected to have non-null value}}
   q = &a;
   takesNullable(q);
   takesNonnull(q);
@@ -120,14 +119,14 @@ void testArgumentTracking(Dummy *_Nonnull nonnull, Dummy *_Nullable nullable) {
   Dummy *p = nullable;
   Dummy *q = nonnull;
   switch(getRandom()) {
-  case 1: nonnull = p; break; // expected-warning {{}}
+  case 1: nonnull = p; break; // expected-warning {{Nullable pointer is assigned to a pointer which is expected to have non-null value}}
   case 2: p = 0; break;
   case 3: q = p; break;
   case 4: testMultiParamChecking(nonnull, nullable, nonnull); break;
   case 5: testMultiParamChecking(nonnull, nonnull, nonnull); break;
-  case 6: testMultiParamChecking(nonnull, nullable, nullable); break; // expected-warning {{}}
-  case 7: testMultiParamChecking(nullable, nullable, nonnull); // expected-warning {{}}
-  case 8: testMultiParamChecking(nullable, nullable, nullable); // expected-warning {{}}
+  case 6: testMultiParamChecking(nonnull, nullable, nullable); break; // expected-warning {{Nullable pointer is passed to a callee that requires a non-null 3rd parameter}}
+  case 7: testMultiParamChecking(nullable, nullable, nonnull); // expected-warning {{Nullable pointer is passed to a callee that requires a non-null 1st parameter}}
+  case 8: testMultiParamChecking(nullable, nullable, nullable); // expected-warning {{Nullable pointer is passed to a callee that requires a non-null 1st parameter}}
   case 9: testMultiParamChecking((Dummy *_Nonnull)0, nullable, nonnull); break;
   }
 }
@@ -161,20 +160,20 @@ void testObjCMessageResultNullability() {
     break;
   case 3:
     shouldBeNullable = [eraseNullab(getNullableTestObject()) returnsNullable];
-    [o takesNonnull:shouldBeNullable]; // expected-warning {{}}
+    [o takesNonnull:shouldBeNullable]; // expected-warning {{Nullable pointer is passed to a callee that requires a non-null 1st parameter}}
     break;
   case 4:
     shouldBeNullable = [eraseNullab(getNonnullTestObject()) returnsNullable];
-    [o takesNonnull:shouldBeNullable]; // expected-warning {{}}
+    [o takesNonnull:shouldBeNullable]; // expected-warning {{Nullable pointer is passed to a callee that requires a non-null 1st parameter}}
     break;
   case 5:
     shouldBeNullable =
         [eraseNullab(getUnspecifiedTestObject()) returnsNullable];
-    [o takesNonnull:shouldBeNullable]; // expected-warning {{}}
+    [o takesNonnull:shouldBeNullable]; // expected-warning {{Nullable pointer is passed to a callee that requires a non-null 1st parameter}}
     break;
   case 6:
     shouldBeNullable = [eraseNullab(getNullableTestObject()) returnsNullable];
-    [o takesNonnull:shouldBeNullable]; // expected-warning {{}}
+    [o takesNonnull:shouldBeNullable]; // expected-warning {{Nullable pointer is passed to a callee that requires a non-null 1st parameter}}
     break;
   case 7: {
     int *shouldBeNonnull = [eraseNullab(getNonnullTestObject()) returnsNonnull];
@@ -203,7 +202,18 @@ Dummy * _Nonnull testDirectCastNilToNonnull() {
 void testIndirectCastNilToNonnullAndPass() {
   Dummy *p = (Dummy * _Nonnull)0;
   // FIXME: Ideally the cast above would suppress this warning.
-  takesNonnull(p);  // expected-warning {{Null passed to a callee that requires a non-null argument}}
+  takesNonnull(p);  // expected-warning {{Null passed to a callee that requires a non-null 1st parameter}}
+}
+
+void testIndirectNilPassToNonnull() {
+  Dummy *p = 0;
+  takesNonnull(p);  // expected-warning {{Null passed to a callee that requires a non-null 1st parameter}}
+}
+
+void testConditionalNilPassToNonnull(Dummy *p) {
+  if (!p) {
+    takesNonnull(p);  // expected-warning {{Null passed to a callee that requires a non-null 1st parameter}}
+  }
 }
 
 Dummy * _Nonnull testIndirectCastNilToNonnullAndReturn() {
@@ -220,7 +230,7 @@ void testInvalidPropagation() {
 
 void onlyReportFirstPreconditionViolationOnPath() {
   Dummy *p = returnsNullable();
-  takesNonnull(p); // expected-warning {{}}
+  takesNonnull(p); // expected-warning {{Nullable pointer is passed to a callee that requires a non-null 1st parameter}}
   takesNonnull(p); // No warning.
   // The first warning was not a sink. The analysis expected to continue.
   int i = 0;
@@ -269,6 +279,14 @@ void inlinedUnspecified(Dummy *p) {
   if (p) return;
 }
 
+void testNilReturnWithBlock(Dummy *p) {
+  p = 0;
+  Dummy *_Nonnull (^myblock)(void) = ^Dummy *_Nonnull(void) {
+    return p; // TODO: We should warn in blocks.
+  };
+  myblock();
+}
+
 Dummy *_Nonnull testDefensiveInlineChecks(Dummy * p) {
   switch (getRandom()) {
   case 1: inlinedNullable(p); break;
@@ -310,7 +328,7 @@ Dummy *_Nonnull testDefensiveInlineChecks(Dummy * p) {
 
 - (TestObject * _Nonnull)testReturnsNullableInNonnullIndirectly {
   TestObject *local = getNullableTestObject();
-  return local; // expected-warning {{Nullable pointer is returned from a function that is expected to return a non-null value}}
+  return local; // expected-warning {{Nullable pointer is returned from a method that is expected to return a non-null value}}
 }
 
 - (TestObject * _Nonnull)testReturnsCastSuppressedNullableInNonnullIndirectly {

test/Analysis/nullability_nullonly.mm

@@ -31,18 +31,18 @@ void testBasicRules() {
   Dummy *q = 0;
   if (getRandom()) {
     takesNullable(q);
-    takesNonnull(q); // expected-warning {{}}
+    takesNonnull(q); // expected-warning {{Null passed to a callee that requires a non-null 1st parameter}}
   }
 }
 
 Dummy *_Nonnull testNullReturn() {
   Dummy *p = 0;
-  return p; // expected-warning {{}}
+  return p; // expected-warning {{Null is returned from a function that is expected to return a non-null value}}
 }
 
 void onlyReportFirstPreconditionViolationOnPath() {
   Dummy *p = 0;
-  takesNonnull(p); // expected-warning {{}}
+  takesNonnull(p); // expected-warning {{Null passed to a callee that requires a non-null 1st parameter}}
   takesNonnull(p); // No warning.
   // Passing null to nonnull is a sink. Stop the analysis.
   int i = 0;
@@ -143,7 +143,7 @@ TestObject * _Nonnull returnsNilObjCInstanceDirectlyWithSuppressingCast() {
 @implementation SomeClass (MethodReturn)
 - (SomeClass * _Nonnull)testReturnsNilInNonnull {
   SomeClass *local = nil;
-  return local; // expected-warning {{Null is returned from a function that is expected to return a non-null value}}
+  return local; // expected-warning {{Null is returned from a method that is expected to return a non-null value}}
 }
 
 - (SomeClass * _Nonnull)testReturnsCastSuppressedNilInNonnull {