[Static Analyzer] Remove sinks from nullability checks.

[xazax.hun]

Summary

With this patch a nullability violation no longer generates a sink node, so it does not cause any coverage loss for other checkers.
From now on, it also do not warns, when a nullability precondition is violated. These changes are related, because in order to be able to remove the sinks (without generating duplicate reports for the same symbol), there should be a mechanism to turn off this checker for a path.

[zaks.anna]

lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp

166 ↗

(On Diff #33458)

The difference is not clear by looking at the signature; maybe you should rename the method to make it more clear what the difference is.

Please, use doxygen style comments everywhere!

228 ↗

(On Diff #33458)

preconditions -> precondition
I am not sure what you mean by postcondition and where that term is defined.
turned of -> turned off
so this check does not -> so that this checker would not lead to reduced coverage

371 ↗

(On Diff #33458)

Looks like you are assuming that you cannot get to this point with OriginalState having the PreconditionViolation bit set. This seems fragile. At minimum, you should assert this, but maybe changing the checkPreconditionViolation API would be better?

395 ↗

(On Diff #33458)

Maybe you should only do this if there was at least one symbol with nullability info removed in the loop above? Would that work?

430 ↗

(On Diff #33458)

Please, spell check your comments.

We are not dereferencing null.. we are dereferencing a nullable pointer..
Am I wrong?

565 ↗

(On Diff #33458)

I do not understand how this works..

Does this ever report an issue? I thought this version of the bugReport will only report an issue if PreconditionViolation is not set. (Same for the code below and above..)

Is this tested?

[xazax.hun]

Addressed the comments.

lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp

395 ↗

(On Diff #33458)

That would not work.
Only "nullable" nullability and contradictions are tracked for the symbols right now. If nothing is marked nullable, this branch would never be taken.

430 ↗

(On Diff #33458)

You are right.

565 ↗

(On Diff #33458)

Yes, it works.

The reason is that, reportBug does report bugs, when PreconditionViolated is set to true. It does not report bugs, when one of the nonnull argument is constrained to be null. PreconditionViolated is used at the beginning of some callbacks to return early.

However I think this behavior might be confusing, so I refactored the code. From now on reportBugConditionally does not report any bug, when PreconditionViolated is set to true, and it does set it to true when a reported bug should suppress other bugs along the path.

[xazax.hun]

Made sure that inlined defensive checks do not generate false positives.

[zaks.anna]

lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp

166 ↗

(On Diff #33704)

It is still not clear what the condition is..
More context in the name would be better; for example, how about reportBugIfNotOnDefensiveCodePath or reportBugIfPreconditionHolds

374 ↗

(On Diff #33704)

Shouldn't this be part of checkPreconditionViolation?

806 ↗

(On Diff #33704)

Maybe we should only check these at the time the bug is about to be reported..

That way the code would be less error prone..

[xazax.hun]

Style fixes according to the review.

lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp

806 ↗

(On Diff #33704)

This is checked before error reporting as well.
The purpose of this check at the beginning of the callbacks is optimization only.

[zaks.anna]

LGTM. Thanks!
Anna.

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

PathDiagnosticPiece *VisitNode(const ExplodedNode *N,
                                    BugReporterContext &BRC,
                                    BugReport &BR) override;
 
   private:
     // The tracked region.
     const MemRegion *Region;
   };
 
+  /// When any of the nonnull arguments of the analyzed function is null, do not
+  /// report anything and turn off the check.
+  ///
+  /// 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,
+                                    const Stmt *ValueExpr = nullptr,
+                                    bool SuppressPath = false) const;
+
   void reportBug(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)];
     assert(Msg);
     std::unique_ptr<BugReport> R(new BugReport(*BT, Msg, N));
     if (Region) {
return Lhs.getValue() == Rhs.getValue() &&
          Lhs.getNullabilitySource() == Rhs.getNullabilitySource();
 }
 
 } // end anonymous namespace
 
 REGISTER_MAP_WITH_PROGRAMSTATE(NullabilityMap, const MemRegion *,
                                NullabilityState)
 
+// If the nullability precondition of a function is violated, we should not
+// report nullability related issues on that path. For this reason once a
+// precondition is not met on a path, this checker will be esentially turned off
+// for the rest of the analysis. We do not want to generate a sink node however,
+// so this checker would not lead to reduced coverage.
+REGISTER_TRAIT_WITH_PROGRAMSTATE(PreconditionViolated, bool)
+
 enum class NullConstraint { IsNull, IsNotNull, Unknown };
 
 static NullConstraint getNullConstraint(DefinedOrUnknownSVal Val,
                                         ProgramStateRef State) {
   ConditionTruthVal Nullness = State->isNull(Val);
   if (Nullness.isConstrainedFalse())
     return NullConstraint::IsNotNull;
   if (Nullness.isConstrainedTrue())
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;
 }
 
+template <typename ParamVarDeclRange>
+static bool
+checkParamsForPreconditionViolation(const ParamVarDeclRange &Params,
+                                    ProgramStateRef State,
+                                    const LocationContext *LocCtxt) {
+  for (const auto *ParamDecl : Params) {
+    if (ParamDecl->isParameterPack())
+      break;
+
+    if (getNullabilityAnnotation(ParamDecl->getType()) != Nullability::Nonnull)
+      continue;
+
+    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;
+
+    if (getNullConstraint(*ParamValue, State) == NullConstraint::IsNull) {
+      return true;
+    }
+  }
+  return false;
+}
+
+static bool checkPreconditionViolation(ProgramStateRef State, ExplodedNode *N,
+                                       CheckerContext &C) {
+  if (State->get<PreconditionViolated>())
+    return true;
+
+  const LocationContext *LocCtxt = C.getLocationContext();
+  const Decl *D = LocCtxt->getDecl();
+  if (!D)
+    return false;
+
+  if (const auto *BlockD = dyn_cast<BlockDecl>(D)) {
+    if (checkParamsForPreconditionViolation(BlockD->parameters(), State,
+                                            LocCtxt)) {
+      if (!N->isSink())
+        C.addTransition(State->set<PreconditionViolated>(true), N);
+      return true;
+    }
+    return false;
+  }
+
+  if (const auto *FuncDecl = dyn_cast<FunctionDecl>(D)) {
+    if (checkParamsForPreconditionViolation(FuncDecl->parameters(), State,
+                                            LocCtxt)) {
+      if (!N->isSink())
+        C.addTransition(State->set<PreconditionViolated>(true), N);
+      return true;
+    }
+    return false;
+  }
+  return false;
+}
+
+void NullabilityChecker::reportBugIfPreconditionHolds(
+    ErrorKind Error, ExplodedNode *N, const MemRegion *Region,
+    CheckerContext &C, const Stmt *ValueExpr, bool SuppressPath) const {
+  ProgramStateRef OriginalState = N->getState();
+
+  if (checkPreconditionViolation(OriginalState, N, C))
+    return;
+  if (SuppressPath) {
+    OriginalState = OriginalState->set<PreconditionViolated>(true);
+    N = C.addTransition(OriginalState, N);
+  }
+
+  reportBug(Error, N, Region, C.getBugReporter(), ValueExpr);
+}
+
 /// Cleaning up the program state.
 void NullabilityChecker::checkDeadSymbols(SymbolReaper &SR,
                                           CheckerContext &C) const {
   ProgramStateRef State = C.getState();
   NullabilityMapTy Nullabilities = State->get<NullabilityMap>();
   for (NullabilityMapTy::iterator I = Nullabilities.begin(),
                                   E = Nullabilities.end();
        I != E; ++I) {
     if (!SR.isLiveRegion(I->first)) {
       State = State->remove<NullabilityMap>(I->first);
     }
   }
+  // When one of the nonnull arguments are constrained to be null, nullability
+  // preconditions are violated. It is not enough to check this only when we
+  // actually report an error, because at that time interesting symbols might be
+  // reaped.
+  if (checkPreconditionViolation(State, C.getPredecessor(), C))
+    return;
+  C.addTransition(State);
 }
 
 /// This callback triggers when a pointer is dereferenced and the analyzer does
 /// not know anything about the value of that pointer. When that pointer is
 /// nullable, this code emits a warning.
 void NullabilityChecker::checkEvent(ImplicitNullDerefEvent Event) const {
+  if (Event.SinkNode->getState()->get<PreconditionViolated>())
+    return;
+
   const MemRegion *Region =
       getTrackRegion(Event.Location, /*CheckSuperregion=*/true);
   if (!Region)
     return;
 
   ProgramStateRef State = Event.SinkNode->getState();
   const NullabilityState *TrackedNullability =
       State->get<NullabilityMap>(Region);
 
   if (!TrackedNullability)
     return;
 
   if (Filter.CheckNullableDereferenced &&
       TrackedNullability->getValue() == Nullability::Nullable) {
     BugReporter &BR = *Event.BR;
+    // 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);
   }
 }
 
 /// This method check when nullable pointer or null value is returned from a
 /// function that has nonnull return type.
 ///
 /// TODO: when nullability preconditons are violated, it is ok to violate the
 /// nullability postconditons (i.e.: when one of the nonnull parameters are null
 /// this check should not report any nullability related issue).
 void NullabilityChecker::checkPreStmt(const ReturnStmt *S,
                                       CheckerContext &C) const {
   auto RetExpr = S->getRetValue();
   if (!RetExpr)
     return;
 
   if (!RetExpr->getType()->isAnyPointerType())
     return;
 
   ProgramStateRef State = C.getState();
+  if (State->get<PreconditionViolated>())
+    return;
+
   auto RetSVal =
       State->getSVal(S, C.getLocationContext()).getAs<DefinedOrUnknownSVal>();
   if (!RetSVal)
     return;
 
   AnalysisDeclContext *DeclCtxt =
       C.getLocationContext()->getAnalysisDeclContext();
   const FunctionType *FuncType = DeclCtxt->getDecl()->getFunctionType();
   if (!FuncType)
     return;
 
   NullConstraint Nullness = getNullConstraint(*RetSVal, State);
 
   Nullability StaticNullability =
       getNullabilityAnnotation(FuncType->getReturnType());
 
   if (Filter.CheckNullReturnedFromNonnull &&
       Nullness == NullConstraint::IsNull &&
       StaticNullability == Nullability::Nonnull) {
     static CheckerProgramPointTag Tag(this, "NullReturnedFromNonnull");
-    ExplodedNode *N = C.addTransition(State, C.getPredecessor(), &Tag);
-    reportBug(ErrorKind::NilReturnedToNonnull, N, nullptr, C.getBugReporter(),
-              S);
+    ExplodedNode *N = C.generateSink(State, C.getPredecessor(), &Tag);
+    reportBugIfPreconditionHolds(ErrorKind::NilReturnedToNonnull, N, nullptr, C,
+                                 RetExpr);
     return;
   }
 
   const MemRegion *Region = getTrackRegion(*RetSVal);
   if (!Region)
     return;
 
   const NullabilityState *TrackedNullability =
       State->get<NullabilityMap>(Region);
   if (TrackedNullability) {
     Nullability TrackedNullabValue = TrackedNullability->getValue();
     if (Filter.CheckNullableReturnedFromNonnull &&
         Nullness != NullConstraint::IsNotNull &&
         TrackedNullabValue == Nullability::Nullable &&
         StaticNullability == Nullability::Nonnull) {
       static CheckerProgramPointTag Tag(this, "NullableReturnedFromNonnull");
       ExplodedNode *N = C.addTransition(State, C.getPredecessor(), &Tag);
-      reportBug(ErrorKind::NullableReturnedToNonnull, N, Region,
-                C.getBugReporter());
+      reportBugIfPreconditionHolds(ErrorKind::NullableReturnedToNonnull, N,
+                                   Region, C);
     }
     return;
   }
   if (StaticNullability == Nullability::Nullable) {
     State = State->set<NullabilityMap>(Region,
                                        NullabilityState(StaticNullability, S));
     C.addTransition(State);
   }
 }
 
 /// This callback warns when a nullable pointer or a null value is passed to a
 /// function that expects its argument to be nonnull.
 void NullabilityChecker::checkPreCall(const CallEvent &Call,
                                       CheckerContext &C) const {
   if (!Call.getDecl())
     return;
 
   ProgramStateRef State = C.getState();
+  if (State->get<PreconditionViolated>())
+    return;
+
   ProgramStateRef OrigState = State;
 
   unsigned Idx = 0;
   for (const ParmVarDecl *Param : Call.parameters()) {
     if (Param->isParameterPack())
       break;
 
     const Expr *ArgExpr = nullptr;
for (const ParmVarDecl *Param : Call.parameters()) {
 
     Nullability ParamNullability = getNullabilityAnnotation(Param->getType());
     Nullability ArgStaticNullability =
         getNullabilityAnnotation(ArgExpr->getType());
 
     if (Filter.CheckNullPassedToNonnull && Nullness == NullConstraint::IsNull &&
         ArgStaticNullability != Nullability::Nonnull &&
         ParamNullability == Nullability::Nonnull) {
-      static CheckerProgramPointTag Tag(this, "NullPassedToNonnull");
-      ExplodedNode *N = C.generateSink(State, C.getPredecessor(), &Tag);
-      reportBug(ErrorKind::NilPassedToNonnull, N, nullptr, C.getBugReporter(),
+      ExplodedNode *N = C.generateSink(State);
+      reportBugIfPreconditionHolds(ErrorKind::NilPassedToNonnull, N, nullptr, C,
                                    ArgExpr);
       return;
     }
 
     const MemRegion *Region = getTrackRegion(*ArgSVal);
     if (!Region)
       continue;
 
     const NullabilityState *TrackedNullability =
         State->get<NullabilityMap>(Region);
 
     if (TrackedNullability) {
       if (Nullness == NullConstraint::IsNotNull ||
           TrackedNullability->getValue() != Nullability::Nullable)
         continue;
 
       if (Filter.CheckNullablePassedToNonnull &&
           ParamNullability == Nullability::Nonnull) {
-        static CheckerProgramPointTag Tag(this, "NullablePassedToNonnull");
-        ExplodedNode *N = C.generateSink(State, C.getPredecessor(), &Tag);
-        reportBug(ErrorKind::NullablePassedToNonnull, N, Region,
-                  C.getBugReporter(), ArgExpr);
+        ExplodedNode *N = C.addTransition(State);
+        reportBugIfPreconditionHolds(ErrorKind::NullablePassedToNonnull, N,
+                                     Region, C, ArgExpr, /*SuppressPath=*/true);
         return;
       }
       if (Filter.CheckNullableDereferenced &&
           Param->getType()->isReferenceType()) {
-        static CheckerProgramPointTag Tag(this, "NullableDereferenced");
-        ExplodedNode *N = C.generateSink(State, C.getPredecessor(), &Tag);
-        reportBug(ErrorKind::NullableDereferenced, N, Region,
-                  C.getBugReporter(), ArgExpr);
+        ExplodedNode *N = C.addTransition(State);
+        reportBugIfPreconditionHolds(ErrorKind::NullableDereferenced, N, Region,
+                                     C, ArgExpr, /*SuppressPath=*/true);
         return;
       }
       continue;
     }
     // No tracked nullability yet.
     if (ArgStaticNullability != Nullability::Nullable)
       continue;
     State = State->set<NullabilityMap>(
void NullabilityChecker::checkPostCall(const CallEvent &Call,
   if (Call.getKind() == CE_ObjCMessage)
     return;
   const FunctionType *FuncType = Decl->getFunctionType();
   if (!FuncType)
     return;
   QualType ReturnType = FuncType->getReturnType();
   if (!ReturnType->isAnyPointerType())
     return;
+  ProgramStateRef State = C.getState();
+  if (State->get<PreconditionViolated>())
+    return;
+
   const MemRegion *Region = getTrackRegion(Call.getReturnValue());
   if (!Region)
     return;
-  ProgramStateRef State = C.getState();
 
   // CG headers are misannotated. Do not warn for symbols that are the results
   // of CG calls.
   const SourceManager &SM = C.getSourceManager();
   StringRef FilePath = SM.getFilename(SM.getSpellingLoc(Decl->getLocStart()));
   if (llvm::sys::path::filename(FilePath).startswith("CG")) {
     State = State->set<NullabilityMap>(Region, Nullability::Contradicted);
     C.addTransition(State);
void NullabilityChecker::checkPostObjCMessage(const ObjCMethodCall &M,
                                               CheckerContext &C) const {
   auto Decl = M.getDecl();
   if (!Decl)
     return;
   QualType RetType = Decl->getReturnType();
   if (!RetType->isAnyPointerType())
     return;
 
+  ProgramStateRef State = C.getState();
+  if (State->get<PreconditionViolated>())
+    return;
+
   const MemRegion *ReturnRegion = getTrackRegion(M.getReturnValue());
   if (!ReturnRegion)
     return;
 
-  ProgramStateRef State = C.getState();
   auto Interface = Decl->getClassInterface();
   auto Name = Interface ? Interface->getName() : "";
   // In order to reduce the noise in the diagnostics generated by this checker,
   // some framework and programming style based heuristics are used. These
   // heuristics are for Cocoa APIs which have NS prefix.
   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
void NullabilityChecker::checkPostStmt(const ExplicitCastExpr *CE,
                                        CheckerContext &C) const {
   QualType OriginType = CE->getSubExpr()->getType();
   QualType DestType = CE->getType();
   if (!OriginType->isAnyPointerType())
     return;
   if (!DestType->isAnyPointerType())
     return;
 
+  ProgramStateRef State = C.getState();
+  if (State->get<PreconditionViolated>())
+    return;
+
   Nullability DestNullability = getNullabilityAnnotation(DestType);
 
   // No explicit nullability in the destination type, so this cast does not
   // change the nullability.
   if (DestNullability == Nullability::Unspecified)
     return;
 
-  ProgramStateRef State = C.getState();
   auto RegionSVal =
       State->getSVal(CE, C.getLocationContext()).getAs<DefinedOrUnknownSVal>();
   const MemRegion *Region = getTrackRegion(*RegionSVal);
   if (!Region)
     return;
 
   // When 0 is converted to nonnull mark it as contradicted.
   if (DestNullability == Nullability::Nonnull) {
const TypedValueRegion *TVR =
       dyn_cast_or_null<TypedValueRegion>(L.getAsRegion());
   if (!TVR)
     return;
 
   QualType LocType = TVR->getValueType();
   if (!LocType->isAnyPointerType())
     return;
 
+  ProgramStateRef State = C.getState();
+  if (State->get<PreconditionViolated>())
+    return;
+
   auto ValDefOrUnknown = V.getAs<DefinedOrUnknownSVal>();
   if (!ValDefOrUnknown)
     return;
 
-  ProgramStateRef State = C.getState();
   NullConstraint RhsNullness = getNullConstraint(*ValDefOrUnknown, State);
 
   Nullability ValNullability = Nullability::Unspecified;
   if (SymbolRef Sym = ValDefOrUnknown->getAsSymbol())
     ValNullability = getNullabilityAnnotation(Sym->getType());
 
   Nullability LocNullability = getNullabilityAnnotation(LocType);
   if (Filter.CheckNullPassedToNonnull &&
       RhsNullness == NullConstraint::IsNull &&
       ValNullability != Nullability::Nonnull &&
       LocNullability == Nullability::Nonnull) {
     static CheckerProgramPointTag Tag(this, "NullPassedToNonnull");
-    ExplodedNode *N = C.addTransition(State, C.getPredecessor(), &Tag);
-    reportBug(ErrorKind::NilAssignedToNonnull, N, nullptr, C.getBugReporter(),
+    ExplodedNode *N = C.generateSink(State, C.getPredecessor(), &Tag);
+    reportBugIfPreconditionHolds(ErrorKind::NilAssignedToNonnull, N, nullptr, C,
                                  S);
     return;
   }
   // Intentionally missing case: '0' is bound to a reference. It is handled by
   // the DereferenceChecker.
 
   const MemRegion *ValueRegion = getTrackRegion(*ValDefOrUnknown);
   if (!ValueRegion)
     return;
 
   const NullabilityState *TrackedNullability =
       State->get<NullabilityMap>(ValueRegion);
 
   if (TrackedNullability) {
     if (RhsNullness == NullConstraint::IsNotNull ||
         TrackedNullability->getValue() != Nullability::Nullable)
       return;
     if (Filter.CheckNullablePassedToNonnull &&
         LocNullability == Nullability::Nonnull) {
       static CheckerProgramPointTag Tag(this, "NullablePassedToNonnull");
       ExplodedNode *N = C.addTransition(State, C.getPredecessor(), &Tag);
-      reportBug(ErrorKind::NullableAssignedToNonnull, N, ValueRegion,
-                C.getBugReporter());
+      reportBugIfPreconditionHolds(ErrorKind::NullableAssignedToNonnull, N,
+                                   ValueRegion, C);
     }
     return;
   }
 
   const auto *BinOp = dyn_cast<BinaryOperator>(S);
 
   if (ValNullability == Nullability::Nullable) {
     // Trust the static information of the value more than the static

cfe/trunk/test/Analysis/nullability.mm

void testCast() {
   takesNonnull(p);
 }
 
 void testInvalidPropagation() {
   Dummy *p = returnsUnspecified();
   takesNullable(p);
   takesNonnull(p);
 }
+
+void onlyReportFirstPreconditionViolationOnPath() {
+  Dummy *p = returnsNullable();
+  takesNonnull(p); // expected-warning {{}}
+  takesNonnull(p); // No warning.
+  // The first warning was not a sink. The analysis expected to continue.
+  int i = 0;
+  i = 5 / i; // expected-warning {{Division by zero}}
+  (void)i;
+}
+
+Dummy *_Nonnull doNotWarnWhenPreconditionIsViolatedInTopFunc(
+    Dummy *_Nonnull p) {
+  if (!p) {
+    Dummy *ret =
+        0; // avoid compiler warning (which is not generated by the analyzer)
+    if (getRandom())
+      return ret; // no warning
+    else
+      return p; // no warning
+  } else {
+    return p;
+  }
+}
+
+Dummy *_Nonnull doNotWarnWhenPreconditionIsViolated(Dummy *_Nonnull p) {
+  if (!p) {
+    Dummy *ret =
+        0; // avoid compiler warning (which is not generated by the analyzer)
+    if (getRandom())
+      return ret; // no warning
+    else
+      return p; // no warning
+  } else {
+    return p;
+  }
+}
+
+void testPreconditionViolationInInlinedFunction(Dummy *p) {
+  doNotWarnWhenPreconditionIsViolated(p);
+}
+
+void inlinedNullable(Dummy *_Nullable p) {
+  if (p) return;
+}
+void inlinedNonnull(Dummy *_Nonnull p) {
+  if (p) return;
+}
+void inlinedUnspecified(Dummy *p) {
+  if (p) return;
+}
+
+Dummy *_Nonnull testDefensiveInlineChecks(Dummy * p) {
+  switch (getRandom()) {
+  case 1: inlinedNullable(p); break;
+  case 2: inlinedNonnull(p); break;
+  case 3: inlinedUnspecified(p); break;
+  }
+  if (getRandom())
+    takesNonnull(p);
+  return p;
+}