[analyzer] Fix false negative when using a nullable parameter directly without binding to a variable

[tripleCC]

Summary

If a parameter has a nullability annotation, the nullability information of the parameter should be set as a NullabilityState trait at the beginning of the function.
I don't have commit access.

[steakhal]

I believe, zaks.anna and vsavchenko are no longer involved in the project.
I think it makes sense to have the code owners NoQ and xazax.hun as reviewers, and I also tend to review quite a lot nowadays.
And we usually use the [analyzer] tag instead of [StaticAnalyzer] for the patches.
It's useful to use the right tags to trigger the right herald scripts to get the right circle notified.

clang/lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp

569–573

Shouldn't we only do this for the analysis entrypoints only? (aka. top-level functions)
I assume this checker already did some modeling of the attributes, hence we have the warnings in the tests.

569–594

Uh, the diffing here looks terrible.
What you probably want: Fold the States, and if you are done, transition - but only if we have any parameters.
We need to have a single addTransition() call if we want a single execution path modeled in the graph. We probably don't want one path on which the first parameter's annotation is known; and a separate one where only the second, etc.

[tripleCC]

add inTopFrame condition. call the addTransition function only once for all params.

[steakhal]

clang/lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp

569–594

Please note that each iteration of the loop will overwrite the state you made in the previous iteration this way.
Its most likely not what you want.
Because C.getState() will always return the same state no matter the context here.

[tripleCC]

clang/lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp

569–573

Thanking for your reviewing. You are correct, I added an inTopFrame() condition here. It only makes sense for top-level functions.

569–594

I made a rookie mistake here. I should call the addTransition function outside the for loop.

[tripleCC]

Do not overwrite state

[tripleCC]

clang/lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp

569–594

Updated

[steakhal]

Now it looks much better. Let me check it in depth tomorrow.

[steakhal]

LGTM; I'll commit this on Monday.

[tripleCC]

In D153017#4428432, @steakhal wrote:

LGTM; I'll commit this on Monday.

Hi steakhal, sorry to bother you again. Do you mind committing the revision for me ?

[tripleCC]

In D153017#4428432, @steakhal wrote:

LGTM; I'll commit this on Monday.

Oops, It seems that I missed the invitation, I just noticed the invitation email in my gmail now ...
I will request Chris to send the invitation again.
Thank you for your recommendation.

clang/lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp

 // to suppress warnings would be to add asserts or guarding if statements to the
 // code. In addition to the nullability propagation this checker also uses some
 // heuristics to suppress potential false positives.
 //
 //===----------------------------------------------------------------------===//
 
 #include "clang/StaticAnalyzer/Checkers/BuiltinCheckerRegistration.h"
 
+#include "clang/Analysis/AnyCall.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 "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/CheckerHelpers.h"
 
 #include "llvm/ADT/StringExtras.h"
 #include "llvm/Support/Path.h"
 
 using namespace clang;
 using namespace ento;
 
 namespace {
enum class ErrorKind : int {
   NullableDereferenced,
   NullablePassedToNonnull
 };
 
 class NullabilityChecker
     : public Checker<check::Bind, check::PreCall, check::PreStmt<ReturnStmt>,
                      check::PostCall, check::PostStmt<ExplicitCastExpr>,
                      check::PostObjCMessage, check::DeadSymbols, eval::Assume,
-                     check::Location, check::Event<ImplicitNullDerefEvent>> {
+                     check::Location, check::Event<ImplicitNullDerefEvent>,
+                     check::BeginFunction> {
 
 public:
   // If true, the checker will not diagnose nullabilility issues for calls
   // to system headers. This option is motivated by the observation that large
   // projects may have many nullability warnings. These projects may
   // find warnings about nullability annotations that they have explicitly
   // added themselves higher priority to fix than warnings on calls to system
   // libraries.
   bool NoDiagnoseCallsToSystemHeaders = false;
 
   void checkBind(SVal L, SVal V, const Stmt *S, CheckerContext &C) const;
   void checkPostStmt(const ExplicitCastExpr *CE, CheckerContext &C) const;
   void checkPreStmt(const ReturnStmt *S, CheckerContext &C) const;
   void checkPostObjCMessage(const ObjCMethodCall &M, CheckerContext &C) const;
   void checkPostCall(const CallEvent &Call, CheckerContext &C) const;
   void checkPreCall(const CallEvent &Call, CheckerContext &C) const;
   void checkDeadSymbols(SymbolReaper &SR, CheckerContext &C) const;
   void checkEvent(ImplicitNullDerefEvent Event) const;
   void checkLocation(SVal Location, bool IsLoad, const Stmt *S,
                      CheckerContext &C) const;
+  void checkBeginFunction(CheckerContext &Ctx) const;
   ProgramStateRef evalAssume(ProgramStateRef State, SVal Cond,
                              bool Assumption) const;
 
   void printState(raw_ostream &Out, ProgramStateRef State, const char *NL,
                   const char *Sep) const override;
 
   enum CheckKind {
     CK_NullPassedToNonnull,
else {
       reportBug("Nullable pointer is passed to a callee that requires a "
                 "non-null",
                 ErrorKind::NullablePassedToNonnull, CK_NullableDereferenced,
                 Event.SinkNode, Region, BR);
     }
   }
 }
 
+void NullabilityChecker::checkBeginFunction(CheckerContext &C) const {
+  const LocationContext *LCtx = C.getLocationContext();
+  auto AbstractCall = AnyCall::forDecl(LCtx->getDecl());
+  if (!AbstractCall)
+    return;
+
+  ProgramStateRef State = C.getState();
+  for (auto Parameter : AbstractCall->parameters()) {
+    if (!isValidPointerType(Parameter->getType()))
+      continue;
+
+    Nullability RequiredNullability =
+        getNullabilityAnnotation(Parameter->getType());
+    if (RequiredNullability != Nullability::Nullable)
+      continue;
+
+    const VarRegion *ParamRegion = State->getRegion(Parameter, LCtx);
+    auto StoredVal = State->getSVal(ParamRegion).getAs<loc::MemRegionVal>();
+    if (!StoredVal)
+      continue;
+
+    State = C.getState()->set<NullabilityMap>(
+        StoredVal->getRegion(), NullabilityState(RequiredNullability));
+    C.addTransition(State);
+  }
+}
+
 // Whenever we see a load from a typed memory region that's been annotated as
 // 'nonnull', we want to trust the user on that and assume that it is is indeed
 // non-null.
 //
 // We do so even if the value is known to have been assigned to null.
 // The user should be warned on assigning the null value to a non-null pointer
 // as opposed to warning on the later dereference of this pointer.
 //

clang/test/Analysis/nullability.mm

void testArgumentTracking(Dummy *_Nonnull nonnull, Dummy *_Nullable nullable) {
   case 5: testMultiParamChecking(nonnull, nonnull, nonnull); break;
   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;
   }
 }
 
+void testArgumentTrackingDirectly(Dummy *_Nonnull nonnull, Dummy *_Nullable nullable) {
+  switch(getRandom()) {
+  case 1: testMultiParamChecking(nonnull, nullable, nonnull); break;
+  case 2: testMultiParamChecking(nonnull, nonnull, nonnull); break;
+  case 3: testMultiParamChecking(nonnull, nullable, nullable); break; // expected-warning {{Nullable pointer is passed to a callee that requires a non-null 3rd parameter}}
+  case 4: testMultiParamChecking(nullable, nullable, nonnull); // expected-warning {{Nullable pointer is passed to a callee that requires a non-null 1st parameter}}
+  case 5: testMultiParamChecking(nullable, nullable, nullable); // expected-warning {{Nullable pointer is passed to a callee that requires a non-null 1st parameter}}
+  case 6: testMultiParamChecking((Dummy *_Nonnull)0, nullable, nonnull); break;
+  }
+}
+
 Dummy *_Nonnull testNullableReturn(Dummy *_Nullable a) {
   Dummy *p = a;
   return p; // expected-warning {{Nullable pointer is returned from a function that is expected to return a non-null value}}
 }
 
 Dummy *_Nonnull testNullReturn() {
   Dummy *p = 0;
   return p; // expected-warning {{Null returned from a function that is expected to return a non-null value}}