[analyzer] Trust _Nonnull annotations, and trust analyzer knowledge about receiver nullability

[george.karpenkov]

Summary

Previously, the checker was using the nullability of the expression, which is nonnull IFF both receiver and method are annotated as _Nonnull. However, the receiver could be known to the analyzer to be nonnull without being explicitly marked as _Nonnull.

rdar://40635584

[NoQ]

LG, i have no sensible remarks, so i made a few non-sensible remarks.

clang/lib/StaticAnalyzer/Checkers/TrustNonnullChecker.cpp

28 ↗

(On Diff #149019)

We usually skip private: at the top of class because it's implied, but in this case i think it's actually justified.

77–79 ↗

(On Diff #149019)

Do we want to generate a sink when L is already known to be null? Theoretically it may happen due to inlining or because another checker models the call to return null. This would not happen if the annotation was indeed correct and our checkers behave correctly. If this at all happens, currently we'll continue the analysis, because addTransition(nullptr), suddenly, doesn't generate a sink.

clang/test/Analysis/trustnonnullchecker_test.m

11–15 ↗

(On Diff #149019)

Is this the actual pattern that we're explicitly trying to suppress? I.e., we declare that it's fine to check a definitely-nonnull pointer for null-ness and get no warning for that(?)

Or is the original problem more about, like, correlated if()s in which we're trying to refute a practically path?

[george.karpenkov]

clang/lib/StaticAnalyzer/Checkers/TrustNonnullChecker.cpp

77–79 ↗

(On Diff #149019)

I'm not sure I follow here; this checker only performs API modeling and does not report bugs, what sink should there be?

clang/test/Analysis/trustnonnullchecker_test.m

11–15 ↗

(On Diff #149019)

if is here to force the analyzer to split paths; the original problem would be when if would not be direct, but the path would still be (sometimes undesirably) split

[NoQ]

clang/lib/StaticAnalyzer/Checkers/TrustNonnullChecker.cpp

77–79 ↗

(On Diff #149019)

You can still have CheckerContext.generateSink() that makes a sink node that isn't supposed to be a node that has a bug report attached to it.

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

 #include "clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h"
 
 using namespace clang;
 using namespace ento;
 
 namespace {
 
 class TrustNonnullChecker : public Checker<check::PostCall> {
+private:
+  /// \returns Whether we trust the result of the method call to be
+  /// a non-null pointer.
+  bool isNonNullPtr(const CallEvent &Call, CheckerContext &C) const {
+    QualType ExprRetType = Call.getResultType();
+    if (!ExprRetType->isAnyPointerType())
+      return false;
+
+    if (getNullabilityAnnotation(ExprRetType) == Nullability::Nonnull)
+      return true;
+
+    // The logic for ObjC instance method calls is more complicated,
+    // as the return value is nil when the receiver is nil.
+    if (!isa<ObjCMethodCall>(&Call))
+      return false;
+
+    const auto *MCall = cast<ObjCMethodCall>(&Call);
+    const ObjCMethodDecl *MD = MCall->getDecl();
+
+    // Distrust protocols.
+    if (isa<ObjCProtocolDecl>(MD->getDeclContext()))
+      return false;
+
+    QualType DeclRetType = MD->getReturnType();
+    if (getNullabilityAnnotation(DeclRetType) != Nullability::Nonnull)
+      return false;
+
+    // For class messages it is sufficient for the declaration to be
+    // annotated _Nonnull.
+    if (!MCall->isInstanceMessage())
+      return true;
+
+    // Alternatively, the analyzer could know that the receiver is not null.
+    SVal Receiver = MCall->getReceiverSVal();
+    ConditionTruthVal TV = C.getState()->isNonNull(Receiver);
+    if (TV.isConstrainedTrue())
+      return true;
+
+    return false;
+  }
+
 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 (isNonNullPtr(Call, C))
       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/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;
++ (NSString * _Nonnull) generateString;
++ (NSString *) generateImplicitlyNonnullString;
++ (NSString * _Nullable) 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;
+- (NSString * _Nonnull) 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 trust_instancemsg_annotation(NSString* _Nonnull param) {
+  NSString* out = [param stringByAppendingString:@"string"];
+  if (out) {}
+  return out; // no-warning
+}
+
+NSString* _Nonnull distrust_instancemsg_noannotation(NSString* param) {
+  if (param) {}
+  NSString* out = [param stringByAppendingString:@"string"];
+  if (out) {}
+  return out; // expected-warning{{}}
+}
+
+NSString* _Nonnull trust_analyzer_knowledge(NSString* param) {
+  if (!param)
+    return @"";
+  NSString* out = [param stringByAppendingString:@"string"];
+  if (out) {}
+  return out; // no-warning
+}
+
+NSString* _Nonnull trust_assume_nonnull_macro() {
+  NSString* out = [NSString generateImplicitlyNonnullString];
+  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_unannoted_function() {
   return out; // expected-warning{{}}
 }
 
 NSString * _Nonnull distrustProtocol(id<MyProtocol> o) {
   NSString* out = [o getString];
   if (out) {};
   return out; // expected-warning{{}}
 }
+