[analyzer] Trust _Nonnull annotations for system framework

[george.karpenkov]

Summary

Changes the analyzer to believe that methods annotated with _Nonnull from system frameworks indeed return non null objects.
Local methods with such annotation are still distrusted.
rdar://24291919

[dcoughlin]

include/clang/StaticAnalyzer/Checkers/Checkers.td

219 ↗

(On Diff #137876)

I think this should probably go in either 'core' or 'apiModeling' (I have a preference for the second). The "nullability" package is for nullability diagnostics. We want this modeling to still take place even when the nullability diagnostics are turned off.

lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp

1216 ↗

(On Diff #137876)

I don't think the "NS" namespace is the right indicator here. There are a lot of classes in other namespaces ("CI" for CoreImage, "UI" for UIKit, etc.).

1219 ↗

(On Diff #137876)

We should do this for C function calls as well. There is a large C function API surface area. We shouldn't do it for function pointers or blocks, since those may be client input.

1230 ↗

(On Diff #137876)

We should not do this for methods on protocols even if they are in the system header. Returns values from protocol represent client inputs, so we don't want to lose coverage in frameworks on defensive code paths that handle when a client returns nil for a method that is declared nonnull.

[george.karpenkov]

lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp

1216 ↗

(On Diff #137876)

That's actually an outdated comment, it currently checks isInSystemHeader()

1230 ↗

(On Diff #137876)

Seems like it is not possible to get a decl here which would correspond to a protocol.
A function would be called with id<MyProtocol>, not with just MyProtocol.

[dcoughlin]

Looks good to me.

cfe/trunk/include/clang/StaticAnalyzer/Checkers/Checkers.td

def NullablePassedToNonnullChecker : Checker<"NullablePassedToNonnull">,
   DescFile<"NullabilityChecker.cpp">;
 
 def NullableReturnedFromNonnullChecker : Checker<"NullableReturnedFromNonnull">,
   HelpText<"Warns when a nullable pointer is returned from a function that has _Nonnull return type.">,
   DescFile<"NullabilityChecker.cpp">;
 
 } // end "nullability"
 
+let ParentPackage = APIModeling in {
+
+def TrustNonnullChecker : Checker<"TrustNonnull">,
+  HelpText<"Trust that returns from framework methods annotated with _Nonnull are not null">,
+  DescFile<"TrustNonnullChecker.cpp">;
+
+}
+
 //===----------------------------------------------------------------------===//
 // Evaluate "builtin" functions.
 //===----------------------------------------------------------------------===//
 
 let ParentPackage = CoreBuiltin in {
 
 def NoReturnFunctionChecker : Checker<"NoReturnFunctions">,
   HelpText<"Evaluate \"panic\" functions that are known to not return to the caller">,

cfe/trunk/include/clang/StaticAnalyzer/Core/PathSensitive/CheckerHelpers.h

 
 #include "clang/AST/Stmt.h"
 #include <tuple>
 
 namespace clang {
 
 class Expr;
 class VarDecl;
+class QualType;
+class AttributedType;
 
 namespace ento {
 
 bool containsMacro(const Stmt *S);
 bool containsEnum(const Stmt *S);
 bool containsStaticLocal(const Stmt *S);
 bool containsBuiltinOffsetOf(const Stmt *S);
 template <class T> bool containsStmt(const Stmt *S) {
   if (isa<T>(S))
       return true;
 
   for (const Stmt *Child : S->children())
     if (Child && containsStmt<T>(Child))
       return true;
 
   return false;
 }
 
 std::pair<const clang::VarDecl *, const clang::Expr *>
 parseAssignment(const Stmt *S);
 
+// Do not reorder! The getMostNullable method relies on the order.
+// Optimization: Most pointers expected to be unspecified. When a symbol has an
+// unspecified or nonnull type non of the rules would indicate any problem for
+// that symbol. For this reason only nullable and contradicted nullability are
+// stored for a symbol. When a symbol is already contradicted, it can not be
+// casted back to nullable.
+enum class Nullability : char {
+  Contradicted, // Tracked nullability is contradicted by an explicit cast. Do
+                // not report any nullability related issue for this symbol.
+                // This nullability is propagated aggressively to avoid false
+                // positive results. See the comment on getMostNullable method.
+  Nullable,
+  Unspecified,
+  Nonnull
+};
+
+/// Get nullability annotation for a given type.
+Nullability getNullabilityAnnotation(QualType Type);
+
 } // end GR namespace
 
 } // end clang namespace
 
 #endif

cfe/trunk/lib/StaticAnalyzer/Checkers/CMakeLists.txt

add_clang_library(clangStaticAnalyzerCheckers
   ReturnUndefChecker.cpp
   SimpleStreamChecker.cpp
   StackAddrEscapeChecker.cpp
   StdLibraryFunctionsChecker.cpp
   StreamChecker.cpp
   TaintTesterChecker.cpp
   TestAfterDivZeroChecker.cpp
   TraversalChecker.cpp
+  TrustNonnullChecker.cpp
   UndefBranchChecker.cpp
   UndefCapturedBlockVarChecker.cpp
   UndefResultChecker.cpp
   UndefinedArraySubscriptChecker.cpp
   UndefinedAssignmentChecker.cpp
   UnixAPIChecker.cpp
   UnreachableCodeChecker.cpp
   VforkChecker.cpp

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

 //
 //===----------------------------------------------------------------------===//
 
 #include "ClangSACheckers.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/CheckerHelpers.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;
 
 namespace {
-// Do not reorder! The getMostNullable method relies on the order.
-// Optimization: Most pointers expected to be unspecified. When a symbol has an
-// unspecified or nonnull type non of the rules would indicate any problem for
-// that symbol. For this reason only nullable and contradicted nullability are
-// stored for a symbol. When a symbol is already contradicted, it can not be
-// casted back to nullable.
-enum class Nullability : char {
-  Contradicted, // Tracked nullability is contradicted by an explicit cast. Do
-                // not report any nullability related issue for this symbol.
-                // This nullability is propagated aggressively to avoid false
-                // positive results. See the comment on getMostNullable method.
-  Nullable,
-  Unspecified,
-  Nonnull
-};
 
 /// Returns the most nullable nullability. This is used for message expressions
 /// like [receiver method], where the nullability of this expression is either
 /// the nullability of the receiver or the nullability of the return type of the
 /// method, depending on which is more nullable. Contradicted is considered to
 /// be the most nullable, to avoid false positive results.
 Nullability getMostNullable(Nullability Lhs, Nullability Rhs) {
   return static_cast<Nullability>(
NullabilityChecker::NullabilityBugVisitor::VisitNode(const ExplodedNode *N,
 
   // Generate the extra diagnostic.
   PathDiagnosticLocation Pos(S, BRC.getSourceManager(),
                              N->getLocationContext());
   return std::make_shared<PathDiagnosticEventPiece>(Pos, InfoText, true,
                                                     nullptr);
 }
 
-static Nullability getNullabilityAnnotation(QualType Type) {
-  const auto *AttrType = Type->getAs<AttributedType>();
-  if (!AttrType)
-    return Nullability::Unspecified;
-  if (AttrType->getAttrKind() == AttributedType::attr_nullable)
-    return Nullability::Nullable;
-  else if (AttrType->getAttrKind() == AttributedType::attr_nonnull)
-    return Nullability::Nonnull;
-  return Nullability::Unspecified;
-}
-
 /// Returns true when the value stored at the given location is null
 /// and the passed in type is nonnnull.
 static bool checkValueAtLValForInvariantViolation(ProgramStateRef State,
                                                   SVal LV, QualType T) {
   if (getNullabilityAnnotation(T) != Nullability::Nonnull)
     return false;
 
   auto RegionVal = LV.getAs<loc::MemRegionVal>();
if (Name.startswith("NS")) {
     // Developers rely on dynamic invariants such as an item should be available
     // in a collection, or a collection is not empty often. Those invariants can
     // not be inferred by any static analysis tool. To not to bother the users
     // with too many false positives, every item retrieval function should be
     // ignored for collections. The instance methods of dictionaries in Cocoa
     // are either item retrieval related or not interesting nullability wise.
     // Using this fact, to keep the code easier to read just ignore the return
     // value of every instance method of dictionaries.
-    if (M.isInstanceMessage() && Name.find("Dictionary") != StringRef::npos) {
+    if (M.isInstanceMessage() && Name.contains("Dictionary")) {
       State =
           State->set<NullabilityMap>(ReturnRegion, Nullability::Contradicted);
       C.addTransition(State);
       return;
     }
     // For similar reasons ignore some methods of Cocoa arrays.
     StringRef FirstSelectorSlot = M.getSelector().getNameForSlot(0);
-    if (Name.find("Array") != StringRef::npos &&
+    if (Name.contains("Array") &&
         (FirstSelectorSlot == "firstObject" ||
          FirstSelectorSlot == "lastObject")) {
       State =
           State->set<NullabilityMap>(ReturnRegion, Nullability::Contradicted);
       C.addTransition(State);
       return;
     }
 
     // Encoding related methods of string should not fail when lossless
     // encodings are used. Using lossless encodings is so frequent that ignoring
     // this class of methods reduced the emitted diagnostics by about 30% on
     // some projects (and all of that was false positives).
-    if (Name.find("String") != StringRef::npos) {
+    if (Name.contains("String")) {
       for (auto Param : M.parameters()) {
         if (Param->getName() == "encoding") {
           State = State->set<NullabilityMap>(ReturnRegion,
                                              Nullability::Contradicted);
           C.addTransition(State);
           return;
         }
       }

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

+//== TrustNonnullChecker.cpp - Checker for trusting annotations -*- C++ -*--==//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+//
+// This checker adds an assumption that methods annotated with _Nonnull
+// which come from system headers actually return a non-null pointer.
+//
+//===----------------------------------------------------------------------===//
+
+#include "ClangSACheckers.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/CheckerHelpers.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h"
+
+using namespace clang;
+using namespace ento;
+
+namespace {
+
+class TrustNonnullChecker : public Checker<check::PostCall> {
+public:
+  void checkPostCall(const CallEvent &Call, CheckerContext &C) const {
+    // Only trust annotations for system headers for non-protocols.
+    if (!Call.isInSystemHeader())
+      return;
+
+    QualType RetType = Call.getResultType();
+    if (!RetType->isAnyPointerType())
+      return;
+
+    ProgramStateRef State = C.getState();
+    if (getNullabilityAnnotation(RetType) == Nullability::Nonnull)
+      if (auto L = Call.getReturnValue().getAs<Loc>())
+        State = State->assume(*L, /*Assumption=*/true);
+
+    C.addTransition(State);
+  }
+};
+
+} // end empty namespace
+
+
+void ento::registerTrustNonnullChecker(CheckerManager &Mgr) {
+  Mgr.registerChecker<TrustNonnullChecker>();
+}

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

 //  This file defines several static functions for use in checkers.
 //
 //===----------------------------------------------------------------------===//
 
 #include "clang/StaticAnalyzer/Core/PathSensitive/CheckerHelpers.h"
 #include "clang/AST/Decl.h"
 #include "clang/AST/Expr.h"
 
+namespace clang {
+
+namespace ento {
+
 // Recursively find any substatements containing macros
-bool clang::ento::containsMacro(const Stmt *S) {
+bool containsMacro(const Stmt *S) {
   if (S->getLocStart().isMacroID())
     return true;
 
   if (S->getLocEnd().isMacroID())
     return true;
 
   for (const Stmt *Child : S->children())
     if (Child && containsMacro(Child))
       return true;
 
   return false;
 }
 
 // Recursively find any substatements containing enum constants
-bool clang::ento::containsEnum(const Stmt *S) {
+bool containsEnum(const Stmt *S) {
   const DeclRefExpr *DR = dyn_cast<DeclRefExpr>(S);
 
   if (DR && isa<EnumConstantDecl>(DR->getDecl()))
     return true;
 
   for (const Stmt *Child : S->children())
     if (Child && containsEnum(Child))
       return true;
 
   return false;
 }
 
 // Recursively find any substatements containing static vars
-bool clang::ento::containsStaticLocal(const Stmt *S) {
+bool containsStaticLocal(const Stmt *S) {
   const DeclRefExpr *DR = dyn_cast<DeclRefExpr>(S);
 
   if (DR)
     if (const VarDecl *VD = dyn_cast<VarDecl>(DR->getDecl()))
       if (VD->isStaticLocal())
         return true;
 
   for (const Stmt *Child : S->children())
     if (Child && containsStaticLocal(Child))
       return true;
 
   return false;
 }
 
 // Recursively find any substatements containing __builtin_offsetof
-bool clang::ento::containsBuiltinOffsetOf(const Stmt *S) {
+bool containsBuiltinOffsetOf(const Stmt *S) {
   if (isa<OffsetOfExpr>(S))
     return true;
 
   for (const Stmt *Child : S->children())
     if (Child && containsBuiltinOffsetOf(Child))
       return true;
 
   return false;
 }
 
 // Extract lhs and rhs from assignment statement
 std::pair<const clang::VarDecl *, const clang::Expr *>
-clang::ento::parseAssignment(const Stmt *S) {
+parseAssignment(const Stmt *S) {
   const VarDecl *VD = nullptr;
   const Expr *RHS = nullptr;
 
   if (auto Assign = dyn_cast_or_null<BinaryOperator>(S)) {
     if (Assign->isAssignmentOp()) {
       // Ordinary assignment
       RHS = Assign->getRHS();
       if (auto DE = dyn_cast_or_null<DeclRefExpr>(Assign->getLHS()))
         VD = dyn_cast_or_null<VarDecl>(DE->getDecl());
     }
   } else if (auto PD = dyn_cast_or_null<DeclStmt>(S)) {
     // Initialization
     assert(PD->isSingleDecl() && "We process decls one by one");
     VD = dyn_cast_or_null<VarDecl>(PD->getSingleDecl());
     RHS = VD->getAnyInitializer();
   }
 
   return std::make_pair(VD, RHS);
 }
+
+Nullability getNullabilityAnnotation(QualType Type) {
+  const auto *AttrType = Type->getAs<AttributedType>();
+  if (!AttrType)
+    return Nullability::Unspecified;
+  if (AttrType->getAttrKind() == AttributedType::attr_nullable)
+    return Nullability::Nullable;
+  else if (AttrType->getAttrKind() == AttributedType::attr_nonnull)
+    return Nullability::Nonnull;
+  return Nullability::Unspecified;
+}
+
+
+} // end namespace ento
+} // end namespace clang

cfe/trunk/test/Analysis/Inputs/system-header-simulator-for-nullability.h

 __attribute__((objc_root_class))
 @interface
 NSObject<NSObject>
 @end
 
 @interface NSString : NSObject<NSCopying>
 - (BOOL)isEqualToString : (NSString *)aString;
 - (NSString *)stringByAppendingString:(NSString *)aString;
++ (_Nonnull NSString *) generateString;
++ (_Nullable NSString *) generatePossiblyNullString;
 @end
 
 void NSSystemFunctionTakingNonnull(NSString *s);
 
 @interface NSSystemClass : NSObject
 - (void) takesNonnull:(NSString *)s;
 @end
 
+NSString* _Nullable getPossiblyNullString();
+NSString* _Nonnull  getString();
+
+@protocol MyProtocol
+- (_Nonnull NSString *) getString;
+@end
+
 NS_ASSUME_NONNULL_END

cfe/trunk/test/Analysis/trustnonnullchecker_test.m

+// RUN: %clang_analyze_cc1 -fblocks -analyze -analyzer-checker=core,nullability,apiModeling  -verify %s
+
+#include "Inputs/system-header-simulator-for-nullability.h"
+
+NSString* getUnknownString();
+
+NSString* _Nonnull trust_nonnull_framework_annotation() {
+  NSString* out = [NSString generateString];
+  if (out) {}
+  return out; // no-warning
+}
+
+NSString* _Nonnull distrust_without_annotation() {
+  NSString* out = [NSString generatePossiblyNullString];
+  if (out) {}
+  return out; // expected-warning{{}}
+}
+
+NSString* _Nonnull nonnull_please_trust_me();
+
+NSString* _Nonnull distrust_local_nonnull_annotation() {
+  NSString* out = nonnull_please_trust_me();
+  if (out) {}
+  return out; // expected-warning{{}}
+}
+
+NSString* _Nonnull trust_c_function() {
+  NSString* out = getString();
+  if (out) {};
+  return out; // no-warning
+}
+
+NSString* _Nonnull distrust_unannoted_function() {
+  NSString* out = getPossiblyNullString();
+  if (out) {};
+  return out; // expected-warning{{}}
+}
+
+NSString * _Nonnull distrustProtocol(id<MyProtocol> o) {
+  NSString* out = [o getString];
+  if (out) {};
+  return out; // expected-warning{{}}
+}