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

[tripleCC via Phabricator via cfe-commits]

tripleCC added inline comments.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp:569-594
+void NullabilityChecker::checkBeginFunction(CheckerContext &C) const {
+  const LocationContext *LCtx = C.getLocationContext();
+  auto AbstractCall = AnyCall::forDecl(LCtx->getDecl());
+  if (!AbstractCall)
+    return;
+
+  ProgramStateRef State = C.getState();
----------------
steakhal wrote:
> tripleCC wrote:
> > tripleCC wrote:
> > > steakhal wrote:
> > > > steakhal wrote:
> > > > > Uh, the diffing here looks terrible.
> > > > > What you probably want: Fold the `State`s, 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.
> > > > 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.
> > > Thanking for your reviewing. You are correct, I added an `inTopFrame()`  condition here. It only makes sense for top-level functions.
> > I made a rookie mistake here. I should call the `addTransition` function outside the for loop. 
> > 
> 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.
Updated


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D153017/new/

https://reviews.llvm.org/D153017