r266157 - [analyzer] Nullability: Treat nil _Nonnull ivar as invariant violation.

[Devin Coughlin via cfe-commits]

Author: dcoughlin
Date: Tue Apr 12 19:41:54 2016
New Revision: 266157

URL: http://llvm.org/viewvc/llvm-project?rev=266157&view=rev
Log:
[analyzer] Nullability: Treat nil _Nonnull ivar as invariant violation.

Treat a _Nonnull ivar that is nil as an invariant violation in a similar
fashion to how a nil _Nonnull parameter is treated as a precondition violation.

This avoids warning on defensive returns of nil on defensive internal
checks, such as the following common idiom:

@class InternalImplementation
@interface PublicClass {
  InternalImplementation * _Nonnull _internal;
}
-(id _Nonnull)foo;
@end

@implementation PublicClass
-(id _Nonnull)foo {
  if (!_internal)
    return nil; // no-warning

  return [_internal foo];
}
@end

rdar://problem/24485171

Modified:
    cfe/trunk/lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp
    cfe/trunk/test/Analysis/nullability.mm

Modified: cfe/trunk/lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp?rev=266157&r1=266156&r2=266157&view=diff
==============================================================================
--- cfe/trunk/lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp (original)
+++ cfe/trunk/lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp Tue Apr 12 19:41:54 2016
@@ -356,29 +356,70 @@ static Nullability getNullabilityAnnotat
   return Nullability::Unspecified;
 }
 
-template <typename ParamVarDeclRange>
+/// 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 (!RegionVal)
+    return false;
+
+  auto StoredVal =
+  State->getSVal(RegionVal->getRegion()).getAs<DefinedOrUnknownSVal>();
+  if (!StoredVal)
+    return false;
+
+  if (getNullConstraint(*StoredVal, State) == NullConstraint::IsNull)
+    return true;
+
+  return false;
+}
+
 static bool
-checkParamsForPreconditionViolation(const ParamVarDeclRange &Params,
+checkParamsForPreconditionViolation(ArrayRef<ParmVarDecl *> Params,
                                     ProgramStateRef State,
                                     const LocationContext *LocCtxt) {
   for (const auto *ParamDecl : Params) {
     if (ParamDecl->isParameterPack())
       break;
 
-    if (getNullabilityAnnotation(ParamDecl->getType()) != Nullability::Nonnull)
-      continue;
+    SVal LV = State->getLValue(ParamDecl, LocCtxt);
+    if (checkValueAtLValForInvariantViolation(State, LV,
+                                              ParamDecl->getType())) {
+      return true;
+    }
+  }
+  return false;
+}
+
+static bool
+checkSelfIvarsForInvariantViolation(ProgramStateRef State,
+                                    const LocationContext *LocCtxt) {
+  auto *MD = dyn_cast<ObjCMethodDecl>(LocCtxt->getDecl());
+  if (!MD || !MD->isInstanceMethod())
+    return false;
 
-    auto RegVal = State->getLValue(ParamDecl, LocCtxt)
-                      .template getAs<loc::MemRegionVal>();
-    if (!RegVal)
-      continue;
-
-    auto ParamValue = State->getSVal(RegVal->getRegion())
-                          .template getAs<DefinedOrUnknownSVal>();
-    if (!ParamValue)
-      continue;
+  const ImplicitParamDecl *SelfDecl = LocCtxt->getSelfDecl();
+  if (!SelfDecl)
+    return false;
+
+  SVal SelfVal = State->getSVal(State->getRegion(SelfDecl, LocCtxt));
+
+  const ObjCObjectPointerType *SelfType =
+      dyn_cast<ObjCObjectPointerType>(SelfDecl->getType());
+  if (!SelfType)
+    return false;
+
+  const ObjCInterfaceDecl *ID = SelfType->getInterfaceDecl();
+  if (!ID)
+    return false;
 
-    if (getNullConstraint(*ParamValue, State) == NullConstraint::IsNull) {
+  for (const auto *IvarDecl : ID->ivars()) {
+    SVal LV = State->getLValue(IvarDecl, SelfVal);
+    if (checkValueAtLValForInvariantViolation(State, LV, IvarDecl->getType())) {
       return true;
     }
   }
@@ -405,7 +446,8 @@ static bool checkInvariantViolation(Prog
   else
     return false;
 
-  if (checkParamsForPreconditionViolation(Params, State, LocCtxt)) {
+  if (checkParamsForPreconditionViolation(Params, State, LocCtxt) ||
+      checkSelfIvarsForInvariantViolation(State, LocCtxt)) {
     if (!N->isSink())
       C.addTransition(State->set<InvariantViolated>(true), N);
     return true;

Modified: cfe/trunk/test/Analysis/nullability.mm
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Analysis/nullability.mm?rev=266157&r1=266156&r2=266157&view=diff
==============================================================================
--- cfe/trunk/test/Analysis/nullability.mm (original)
+++ cfe/trunk/test/Analysis/nullability.mm Tue Apr 12 19:41:54 2016
@@ -444,3 +444,45 @@ void callMethodInSystemHeader() {
   // expected-warning at -2{{Nullable pointer is passed to a callee that requires a non-null 1st parameter}}
   #endif
 }
+
+// Test to make sure the analyzer doesn't warn when an a nullability invariant
+// has already been found to be violated on an instance variable.
+
+ at class MyInternalClass;
+ at interface MyClass : NSObject {
+  MyInternalClass * _Nonnull _internal;
+}
+ at end
+
+ at interface MyInternalClass : NSObject {
+  @public
+  id _someIvar;
+}
+-(id _Nonnull)methodWithInternalImplementation;
+ at end
+
+ at interface MyClass () {
+  MyInternalClass * _Nonnull _nilledOutInternal;
+}
+ at end
+
+ at implementation MyClass
+-(id _Nonnull)methodWithInternalImplementation {
+  if (!_internal)
+    return nil; // no-warning
+
+  return [_internal methodWithInternalImplementation];
+}
+
+- (id _Nonnull)methodReturningIvarInImplementation; {
+  return _internal == 0 ? nil : _internal->_someIvar; // no-warning
+}
+
+-(id _Nonnull)methodWithNilledOutInternal {
+  // The cast below should (but does not yet) suppress the warning on the
+  // assignment.
+  _nilledOutInternal = (id _Nonnull)nil; // expected-warning {{Null is assigned to a pointer which is expected to have non-null value}}
+
+  return nil; // no-warning
+}
+ at end