[analyzer][NFCI] Allow clients of NoStateChangeFuncVisitor to check entire function calls, rather than each ExplodedNode in it

D105553 added NoStateChangeFuncVisitor, an abstract class to aid in creating
notes such as "Returning without writing to 'x'", or "Returning without changing
the ownership status of allocated memory". Its clients need to define, among
other things, what a change of state is.

For code like this:

f() {
  g();
}

foo() {
  f();
  h();
}

We'd have a path in the ExplodedGraph that looks like this:

             -- <g> -->
            /          \
         ---     <f>    -------->        --- <h> --->
        /                        \      /            \
--------        <foo>             ------    <foo>     -->

When we're interested in whether f neglected to change some property,
NoStateChangeFuncVisitor asks these questions:

                       ÷×~
                -- <g> -->
           ß   /          \$    @&#*
            ---     <f>    -------->        --- <h> --->
           /                        \      /            \
   --------        <foo>             ------    <foo>     -->

Has anything changed in between # and *?
Has anything changed in between & and *?
Has anything changed in between @ and *?
...
Has anything changed in between $ and *?
Has anything changed in between × and ~?
Has anything changed in between ÷ and ~?
...
Has anything changed in between ß and *?
...
This is a rather thorough line of questioning, which is why in D105819, I was
only interested in whether state *right before* and *right after* a function
call changed, and early returned to the CallEnter location:

if (!CurrN->getLocationAs<CallEnter>())
  return;
Except that I made a typo, and forgot to negate the condition. So, in this
patch, I'm fixing that, and under the same hood allow all clients to decide to
do this whole-function check instead of the thorough one.

Differential Revision: https://reviews.llvm.org/D108695

clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitors.h

@@ -633,6 +633,9 @@ class CXXConstructorCall;
 /// Descendants can define what a "state change is", like a change of value
 /// to a memory region, liveness, etc. For function calls where the state did
 /// not change as defined, a custom note may be constructed.
+///
+/// For a minimal example, check out
+/// clang/unittests/StaticAnalyzer/NoStateChangeFuncVisitorTest.cpp.
 class NoStateChangeFuncVisitor : public BugReporterVisitor {
 private:
   /// Frames modifying the state as defined in \c wasModifiedBeforeCallExit.
@@ -643,6 +646,8 @@ private:
   /// many times (going up the path for each node and checking whether the
   /// region was written into) we instead lazily compute the stack frames
   /// along the path.
+  // TODO: Can't we just use a map instead? This is likely not as cheap as it
+  // makes the code difficult to read.
   llvm::SmallPtrSet<const StackFrameContext *, 32> FramesModifying;
   llvm::SmallPtrSet<const StackFrameContext *, 32> FramesModifyingCalculated;
 
@@ -651,6 +656,8 @@ private:
   /// The calculation is cached in FramesModifying.
   bool isModifiedInFrame(const ExplodedNode *CallExitBeginN);
 
+  void markFrameAsModifying(const StackFrameContext *SCtx);
+
   /// Write to \c FramesModifying all stack frames along the path in the current
   /// stack frame which modifies the state.
   void findModifyingFrames(const ExplodedNode *const CallExitBeginN);
@@ -658,11 +665,37 @@ private:
 protected:
   bugreporter::TrackingKind TKind;
 
-  /// \return Whether the state was modified from the current node, \CurrN, to
-  /// the end of the stack fram, at \p CallExitBeginN.
-  virtual bool
-  wasModifiedBeforeCallExit(const ExplodedNode *CurrN,
-                            const ExplodedNode *CallExitBeginN) = 0;
+  /// \return Whether the state was modified from the current node, \p CurrN, to
+  /// the end of the stack frame, at \p CallExitBeginN. \p CurrN and
+  /// \p CallExitBeginN are always in the same stack frame.
+  /// Clients should override this callback when a state change is important
+  /// not only on the entire function call, but inside of it as well.
+  /// Example: we may want to leave a note about the lack of locking/unlocking
+  /// on a particular mutex, but not if inside the function its state was
+  /// changed, but also restored. wasModifiedInFunction() wouldn't know of this
+  /// change.
+  virtual bool wasModifiedBeforeCallExit(const ExplodedNode *CurrN,
+                                         const ExplodedNode *CallExitBeginN) {
+    return false;
+  }
+
+  /// \return Whether the state was modified in the inlined function call in
+  /// between \p CallEnterN and \p CallExitEndN. Mind that the stack frame
+  /// retrieved from a CallEnterN and CallExitEndN is the *caller's* stack
+  /// frame! The inlined function's stack should be retrieved from either the
+  /// immediate successor to \p CallEnterN or immediate predecessor to
+  /// \p CallExitEndN.
+  /// Clients should override this function if a state changes local to the
+  /// inlined function are not interesting, only the change occuring as a
+  /// result of it.
+  /// Example: we want to leave a not about a leaked resource object not being
+  /// deallocated / its ownership changed inside a function, and we don't care
+  /// if it was assigned to a local variable (its change in ownership is
+  /// inconsequential).
+  virtual bool wasModifiedInFunction(const ExplodedNode *CallEnterN,
+                                     const ExplodedNode *CallExitEndN) {
+    return false;
+  }
 
   /// Consume the information on the non-modifying stack frame in order to
   /// either emit a note or not. May suppress the report entirely.
@@ -702,7 +735,6 @@ public:
 };
 
 } // namespace ento
-
 } // namespace clang
 
 #endif // LLVM_CLANG_STATICANALYZER_CORE_BUGREPORTER_BUGREPORTERVISITORS_H

clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp

@@ -52,6 +52,7 @@
 #include "clang/AST/Expr.h"
 #include "clang/AST/ExprCXX.h"
 #include "clang/AST/ParentMap.h"
+#include "clang/Analysis/ProgramPoint.h"
 #include "clang/Basic/LLVM.h"
 #include "clang/Basic/SourceManager.h"
 #include "clang/Basic/TargetInfo.h"
@@ -76,8 +77,10 @@
 #include "llvm/ADT/SetOperations.h"
 #include "llvm/ADT/SmallString.h"
 #include "llvm/ADT/StringExtras.h"
+#include "llvm/Support/Casting.h"
 #include "llvm/Support/Compiler.h"
 #include "llvm/Support/ErrorHandling.h"
+#include "llvm/Support/raw_ostream.h"
 #include <climits>
 #include <functional>
 #include <utility>
@@ -755,6 +758,17 @@ class NoOwnershipChangeVisitor final : public NoStateChangeFuncVisitor {
         Owners.insert(Region);
       return true;
     }
+
+    LLVM_DUMP_METHOD void dump() const { dumpToStream(llvm::errs()); }
+    LLVM_DUMP_METHOD void dumpToStream(llvm::raw_ostream &out) const {
+      out << "Owners: {\n";
+      for (const MemRegion *Owner : Owners) {
+        out << "  ";
+        Owner->dumpToStream(out);
+        out << ",\n";
+      }
+      out << "}\n";
+    }
   };
 
 protected:
@@ -768,31 +782,24 @@ protected:
     return Ret;
   }
 
-  static const ExplodedNode *getCallExitEnd(const ExplodedNode *N) {
-    while (N && !N->getLocationAs<CallExitEnd>())
-      N = N->getFirstSucc();
-    return N;
+  LLVM_DUMP_METHOD static std::string
+  getFunctionName(const ExplodedNode *CallEnterN) {
+    if (const CallExpr *CE = llvm::dyn_cast_or_null<CallExpr>(
+            CallEnterN->getLocationAs<CallEnter>()->getCallExpr()))
+      if (const FunctionDecl *FD = CE->getDirectCallee())
+        return FD->getQualifiedNameAsString();
+    return "";
   }
 
   virtual bool
-  wasModifiedBeforeCallExit(const ExplodedNode *CurrN,
-                            const ExplodedNode *CallExitN) override {
-    if (CurrN->getLocationAs<CallEnter>())
+  wasModifiedInFunction(const ExplodedNode *CallEnterN,
+                        const ExplodedNode *CallExitEndN) override {
+    if (CallEnterN->getState()->get<RegionState>(Sym) !=
+        CallExitEndN->getState()->get<RegionState>(Sym))
       return true;
 
-    // Its the state right *after* the call that is interesting. Any pointers
-    // inside the call that pointed to the allocated memory are of little
-    // consequence if their lifetime ends within the function.
-    CallExitN = getCallExitEnd(CallExitN);
-    if (!CallExitN)
-      return true;
-
-    if (CurrN->getState()->get<RegionState>(Sym) !=
-        CallExitN->getState()->get<RegionState>(Sym))
-      return true;
-
-    OwnerSet CurrOwners = getOwnersAtNode(CurrN);
-    OwnerSet ExitOwners = getOwnersAtNode(CallExitN);
+    OwnerSet CurrOwners = getOwnersAtNode(CallEnterN);
+    OwnerSet ExitOwners = getOwnersAtNode(CallExitEndN);
 
     // Owners in the current set may be purged from the analyzer later on.
     // If a variable is dead (is not referenced directly or indirectly after

clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp

@@ -355,43 +355,82 @@ bool NoStateChangeFuncVisitor::isModifiedInFrame(const ExplodedNode *N) {
   return FramesModifying.count(SCtx);
 }
 
+void NoStateChangeFuncVisitor::markFrameAsModifying(
+    const StackFrameContext *SCtx) {
+  while (!SCtx->inTopFrame()) {
+    auto p = FramesModifying.insert(SCtx);
+    if (!p.second)
+      break; // Frame and all its parents already inserted.
+
+    SCtx = SCtx->getParent()->getStackFrame();
+  }
+}
+
+static const ExplodedNode *getMatchingCallExitEnd(const ExplodedNode *N) {
+  assert(N->getLocationAs<CallEnter>());
+  // The stackframe of the callee is only found in the nodes succeeding
+  // the CallEnter node. CallEnter's stack frame refers to the caller.
+  const StackFrameContext *OrigSCtx = N->getFirstSucc()->getStackFrame();
+
+  // Similarly, the nodes preceding CallExitEnd refer to the callee's stack
+  // frame.
+  auto IsMatchingCallExitEnd = [OrigSCtx](const ExplodedNode *N) {
+    return N->getLocationAs<CallExitEnd>() &&
+           OrigSCtx == N->getFirstPred()->getStackFrame();
+  };
+  while (N && !IsMatchingCallExitEnd(N)) {
+    assert(N->succ_size() <= 1 &&
+           "This function is to be used on the trimmed ExplodedGraph!");
+    N = N->getFirstSucc();
+  }
+  return N;
+}
+
 void NoStateChangeFuncVisitor::findModifyingFrames(
     const ExplodedNode *const CallExitBeginN) {
 
   assert(CallExitBeginN->getLocationAs<CallExitBegin>());
-  const ExplodedNode *LastReturnN = CallExitBeginN;
+
   const StackFrameContext *const OriginalSCtx =
       CallExitBeginN->getLocationContext()->getStackFrame();
 
-  const ExplodedNode *CurrN = CallExitBeginN;
+  const ExplodedNode *CurrCallExitBeginN = CallExitBeginN;
+  const StackFrameContext *CurrentSCtx = OriginalSCtx;
 
-  do {
-    ProgramStateRef State = CurrN->getState();
-    auto CallExitLoc = CurrN->getLocationAs<CallExitBegin>();
-    if (CallExitLoc) {
-      LastReturnN = CurrN;
+  for (const ExplodedNode *CurrN = CallExitBeginN; CurrN;
+       CurrN = CurrN->getFirstPred()) {
+    // Found a new inlined call.
+    if (CurrN->getLocationAs<CallExitBegin>()) {
+      CurrCallExitBeginN = CurrN;
+      CurrentSCtx = CurrN->getStackFrame();
+      FramesModifyingCalculated.insert(CurrentSCtx);
+      // We won't see a change in between two identical exploded nodes: skip.
+      continue;
     }
 
-    FramesModifyingCalculated.insert(
-        CurrN->getLocationContext()->getStackFrame());
+    if (auto CE = CurrN->getLocationAs<CallEnter>()) {
+      if (const ExplodedNode *CallExitEndN = getMatchingCallExitEnd(CurrN))
+        if (wasModifiedInFunction(CurrN, CallExitEndN))
+          markFrameAsModifying(CurrentSCtx);
 
-    if (wasModifiedBeforeCallExit(CurrN, LastReturnN)) {
-      const StackFrameContext *SCtx = CurrN->getStackFrame();
-      while (!SCtx->inTopFrame()) {
-        auto p = FramesModifying.insert(SCtx);
-        if (!p.second)
-          break; // Frame and all its parents already inserted.
-        SCtx = SCtx->getParent()->getStackFrame();
+      // We exited this inlined call, lets actualize the stack frame.
+      CurrentSCtx = CurrN->getStackFrame();
+
+      // Stop calculating at the current function, but always regard it as
+      // modifying, so we can avoid notes like this:
+      //   void f(Foo &F) {
+      //     F.field = 0; // note: 0 assigned to 'F.field'
+      //                  // note: returning without writing to 'F.field'
+      //   }
+      if (CE->getCalleeContext() == OriginalSCtx) {
+        markFrameAsModifying(CurrentSCtx);
+        break;
       }
     }
 
-    // Stop calculation at the call to the current function.
-    if (auto CE = CurrN->getLocationAs<CallEnter>())
-      if (CE->getCalleeContext() == OriginalSCtx)
-        break;
-
-    CurrN = CurrN->getFirstPred();
-  } while (CurrN);
+    if (wasModifiedBeforeCallExit(CurrN, CurrCallExitBeginN))
+      markFrameAsModifying(CurrentSCtx);
+  }
 }
 
 PathDiagnosticPieceRef NoStateChangeFuncVisitor::VisitNode(

clang/unittests/StaticAnalyzer/CMakeLists.txt

@@ -9,6 +9,7 @@ add_clang_unittest(StaticAnalysisTests
   CallDescriptionTest.cpp
   CallEventTest.cpp
   FalsePositiveRefutationBRVisitorTest.cpp
+  NoStateChangeFuncVisitorTest.cpp
   ParamRegionTest.cpp
   RangeSetTest.cpp
   RegisterCustomCheckersTest.cpp

clang/unittests/StaticAnalyzer/CallEventTest.cpp

@@ -81,7 +81,7 @@ TEST(CXXDeallocatorCall, SimpleDestructor) {
     }
   )",
                                                          Diags));
-  EXPECT_EQ(Diags, "test.CXXDeallocator:NumArgs: 1\n");
+  EXPECT_EQ(Diags, "test.CXXDeallocator: NumArgs: 1\n");
 }
 
 } // namespace

clang/unittests/StaticAnalyzer/CheckerRegistration.h

@@ -6,6 +6,7 @@
 //
 //===----------------------------------------------------------------------===//
 
+#include "clang/Analysis/PathDiagnostic.h"
 #include "clang/Frontend/CompilerInstance.h"
 #include "clang/StaticAnalyzer/Core/BugReporter/BugReporter.h"
 #include "clang/StaticAnalyzer/Core/BugReporter/BugType.h"
@@ -26,8 +27,23 @@ public:
   DiagConsumer(llvm::raw_ostream &Output) : Output(Output) {}
   void FlushDiagnosticsImpl(std::vector<const PathDiagnostic *> &Diags,
                             FilesMade *filesMade) override {
-    for (const auto *PD : Diags)
-      Output << PD->getCheckerName() << ":" << PD->getShortDescription() << '\n';
+    for (const auto *PD : Diags) {
+      Output << PD->getCheckerName() << ": ";
+
+      for (PathDiagnosticPieceRef Piece :
+           PD->path.flatten(/*ShouldFlattenMacros*/ true)) {
+        if (Piece->getKind() != PathDiagnosticPiece::Event)
+          continue;
+        if (Piece->getString().empty())
+          continue;
+        // The last event is usually the same as the warning message, skip.
+        if (Piece->getString() == PD->getShortDescription())
+          continue;
+
+        Output << Piece->getString() << " | ";
+      }
+      Output << PD->getShortDescription() << '\n';
+    }
   }
 
   StringRef getName() const override { return "Test"; }

clang/unittests/StaticAnalyzer/FalsePositiveRefutationBRVisitorTest.cpp

@@ -128,7 +128,7 @@ TEST_F(FalsePositiveRefutationBRVisitorTestBase, UnSatInTheMiddleNoReport) {
   EXPECT_TRUE(runCheckerOnCodeWithArgs<addFalsePositiveGenerator>(
       Code, LazyAssumeAndCrossCheckArgs, Diags));
   EXPECT_EQ(Diags,
-            "test.FalsePositiveGenerator:REACHED_WITH_NO_CONTRADICTION\n");
+            "test.FalsePositiveGenerator: REACHED_WITH_NO_CONTRADICTION\n");
   // Single warning. The second report was invalidated by the visitor.
 
   // Without enabling the crosscheck-with-z3 both reports are displayed.
@@ -136,8 +136,8 @@ TEST_F(FalsePositiveRefutationBRVisitorTestBase, UnSatInTheMiddleNoReport) {
   EXPECT_TRUE(runCheckerOnCodeWithArgs<addFalsePositiveGenerator>(
       Code, LazyAssumeArgs, Diags2));
   EXPECT_EQ(Diags2,
-            "test.FalsePositiveGenerator:REACHED_WITH_NO_CONTRADICTION\n"
-            "test.FalsePositiveGenerator:REACHED_WITH_CONTRADICTION\n");
+            "test.FalsePositiveGenerator: REACHED_WITH_NO_CONTRADICTION\n"
+            "test.FalsePositiveGenerator: REACHED_WITH_CONTRADICTION\n");
 }
 
 TEST_F(FalsePositiveRefutationBRVisitorTestBase,
@@ -159,7 +159,7 @@ TEST_F(FalsePositiveRefutationBRVisitorTestBase,
   EXPECT_TRUE(runCheckerOnCodeWithArgs<addFalsePositiveGenerator>(
       Code, LazyAssumeAndCrossCheckArgs, Diags));
   EXPECT_EQ(Diags,
-            "test.FalsePositiveGenerator:REACHED_WITH_NO_CONTRADICTION\n");
+            "test.FalsePositiveGenerator: REACHED_WITH_NO_CONTRADICTION\n");
   // Single warning. The second report was invalidated by the visitor.
 
   // Without enabling the crosscheck-with-z3 both reports are displayed.
@@ -167,8 +167,8 @@ TEST_F(FalsePositiveRefutationBRVisitorTestBase,
   EXPECT_TRUE(runCheckerOnCodeWithArgs<addFalsePositiveGenerator>(
       Code, LazyAssumeArgs, Diags2));
   EXPECT_EQ(Diags2,
-            "test.FalsePositiveGenerator:REACHED_WITH_NO_CONTRADICTION\n"
-            "test.FalsePositiveGenerator:CAN_BE_TRUE\n");
+            "test.FalsePositiveGenerator: REACHED_WITH_NO_CONTRADICTION\n"
+            "test.FalsePositiveGenerator: CAN_BE_TRUE\n");
 }
 
 TEST_F(FalsePositiveRefutationBRVisitorTestBase,
@@ -206,7 +206,7 @@ TEST_F(FalsePositiveRefutationBRVisitorTestBase,
   EXPECT_TRUE(runCheckerOnCodeWithArgs<addFalsePositiveGenerator>(
       Code, LazyAssumeAndCrossCheckArgs, Diags));
   EXPECT_EQ(Diags,
-            "test.FalsePositiveGenerator:REACHED_WITH_NO_CONTRADICTION\n");
+            "test.FalsePositiveGenerator: REACHED_WITH_NO_CONTRADICTION\n");
   // Single warning. The second report was invalidated by the visitor.
 
   // Without enabling the crosscheck-with-z3 both reports are displayed.
@@ -214,8 +214,8 @@ TEST_F(FalsePositiveRefutationBRVisitorTestBase,
   EXPECT_TRUE(runCheckerOnCodeWithArgs<addFalsePositiveGenerator>(
       Code, LazyAssumeArgs, Diags2));
   EXPECT_EQ(Diags2,
-            "test.FalsePositiveGenerator:REACHED_WITH_NO_CONTRADICTION\n"
-            "test.FalsePositiveGenerator:CAN_BE_TRUE\n");
+            "test.FalsePositiveGenerator: REACHED_WITH_NO_CONTRADICTION\n"
+            "test.FalsePositiveGenerator: CAN_BE_TRUE\n");
 }
 
 } // namespace

clang/unittests/StaticAnalyzer/NoStateChangeFuncVisitorTest.cpp

@@ -0,0 +1,302 @@
+//===- unittests/StaticAnalyzer/NoStateChangeFuncVisitorTest.cpp ----------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#include "CheckerRegistration.h"
+#include "clang/Frontend/CompilerInstance.h"
+#include "clang/StaticAnalyzer/Core/BugReporter/BugReporter.h"
+#include "clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitors.h"
+#include "clang/StaticAnalyzer/Core/BugReporter/BugType.h"
+#include "clang/StaticAnalyzer/Core/BugReporter/CommonBugCategories.h"
+#include "clang/StaticAnalyzer/Core/Checker.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/AnalysisManager.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/ExplodedGraph.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/ProgramStateTrait.h"
+#include "clang/StaticAnalyzer/Frontend/AnalysisConsumer.h"
+#include "clang/StaticAnalyzer/Frontend/CheckerRegistry.h"
+#include "llvm/Support/ErrorHandling.h"
+#include "llvm/Support/raw_ostream.h"
+#include "gtest/gtest.h"
+#include <memory>
+
+//===----------------------------------------------------------------------===//
+// Base classes for testing NoStateChangeFuncVisitor.
+//
+// Testing is done by observing a very simple trait change from one node to
+// another -- the checker sets the ErrorPrevented trait to true if
+// 'preventError()' is called in the source code, and sets it to false if
+// 'allowError()' is called. If this trait is false when 'error()' is called,
+// a warning is emitted.
+//
+// The checker then registers a simple NoStateChangeFuncVisitor to add notes to
+// inlined functions that could have, but neglected to prevent the error.
+//===----------------------------------------------------------------------===//
+
+REGISTER_TRAIT_WITH_PROGRAMSTATE(ErrorPrevented, bool)
+
+namespace clang {
+namespace ento {
+namespace {
+
+class ErrorNotPreventedFuncVisitor : public NoStateChangeFuncVisitor {
+public:
+  ErrorNotPreventedFuncVisitor()
+      : NoStateChangeFuncVisitor(bugreporter::TrackingKind::Thorough) {}
+
+  virtual PathDiagnosticPieceRef
+  maybeEmitNoteForObjCSelf(PathSensitiveBugReport &R,
+                           const ObjCMethodCall &Call,
+                           const ExplodedNode *N) override {
+    return nullptr;
+  }
+
+  virtual PathDiagnosticPieceRef
+  maybeEmitNoteForCXXThis(PathSensitiveBugReport &R,
+                          const CXXConstructorCall &Call,
+                          const ExplodedNode *N) override {
+    return nullptr;
+  }
+
+  virtual PathDiagnosticPieceRef
+  maybeEmitNoteForParameters(PathSensitiveBugReport &R, const CallEvent &Call,
+                             const ExplodedNode *N) override {
+    PathDiagnosticLocation L = PathDiagnosticLocation::create(
+        N->getLocation(),
+        N->getState()->getStateManager().getContext().getSourceManager());
+    return std::make_shared<PathDiagnosticEventPiece>(
+        L, "Returning without prevening the error");
+  }
+
+  void Profile(llvm::FoldingSetNodeID &ID) const override {
+    static int Tag = 0;
+    ID.AddPointer(&Tag);
+  }
+};
+
+template <class Visitor>
+class StatefulChecker : public Checker<check::PreCall> {
+  mutable std::unique_ptr<BugType> BT;
+
+public:
+  void checkPreCall(const CallEvent &Call, CheckerContext &C) const {
+    if (Call.isCalled(CallDescription{"preventError", 0})) {
+      C.addTransition(C.getState()->set<ErrorPrevented>(true));
+      return;
+    }
+
+    if (Call.isCalled(CallDescription{"allowError", 0})) {
+      C.addTransition(C.getState()->set<ErrorPrevented>(false));
+      return;
+    }
+
+    if (Call.isCalled(CallDescription{"error", 0})) {
+      if (C.getState()->get<ErrorPrevented>())
+        return;
+      const ExplodedNode *N = C.generateErrorNode();
+      if (!N)
+        return;
+      if (!BT)
+        BT.reset(new BugType(this->getCheckerName(), "error()",
+                             categories::SecurityError));
+      auto R =
+          std::make_unique<PathSensitiveBugReport>(*BT, "error() called", N);
+      R->addVisitor<Visitor>();
+      C.emitReport(std::move(R));
+    }
+  }
+};
+
+} // namespace
+} // namespace ento
+} // namespace clang
+
+//===----------------------------------------------------------------------===//
+// Non-thorough analysis: only the state right before and right after the
+// function call is checked for the difference in trait value.
+//===----------------------------------------------------------------------===//
+
+namespace clang {
+namespace ento {
+namespace {
+
+class NonThoroughErrorNotPreventedFuncVisitor
+    : public ErrorNotPreventedFuncVisitor {
+public:
+  virtual bool
+  wasModifiedInFunction(const ExplodedNode *CallEnterN,
+                        const ExplodedNode *CallExitEndN) override {
+    return CallEnterN->getState()->get<ErrorPrevented>() !=
+           CallExitEndN->getState()->get<ErrorPrevented>();
+  }
+};
+
+void addNonThoroughStatefulChecker(AnalysisASTConsumer &AnalysisConsumer,
+                                   AnalyzerOptions &AnOpts) {
+  AnOpts.CheckersAndPackages = {{"test.StatefulChecker", true}};
+  AnalysisConsumer.AddCheckerRegistrationFn([](CheckerRegistry &Registry) {
+    Registry
+        .addChecker<StatefulChecker<NonThoroughErrorNotPreventedFuncVisitor>>(
+            "test.StatefulChecker", "Description", "");
+  });
+}
+
+TEST(NoStateChangeFuncVisitor, NonThoroughFunctionAnalysis) {
+  std::string Diags;
+  EXPECT_TRUE(runCheckerOnCode<addNonThoroughStatefulChecker>(R"(
+    void error();
+    void preventError();
+    void allowError();
+
+    void g() {
+      //preventError();
+    }
+
+    void f() {
+      g();
+      error();
+    }
+  )", Diags));
+  EXPECT_EQ(Diags,
+            "test.StatefulChecker: Calling 'g' | Returning without prevening "
+            "the error | Returning from 'g' | error() called\n");
+
+  Diags.clear();
+
+  EXPECT_TRUE(runCheckerOnCode<addNonThoroughStatefulChecker>(R"(
+    void error();
+    void preventError();
+    void allowError();
+
+    void g() {
+      preventError();
+      allowError();
+    }
+
+    void f() {
+      g();
+      error();
+    }
+  )", Diags));
+  EXPECT_EQ(Diags,
+            "test.StatefulChecker: Calling 'g' | Returning without prevening "
+            "the error | Returning from 'g' | error() called\n");
+
+  Diags.clear();
+
+  EXPECT_TRUE(runCheckerOnCode<addNonThoroughStatefulChecker>(R"(
+    void error();
+    void preventError();
+    void allowError();
+
+    void g() {
+      preventError();
+    }
+
+    void f() {
+      g();
+      error();
+    }
+  )", Diags));
+  EXPECT_EQ(Diags, "");
+}
+
+} // namespace
+} // namespace ento
+} // namespace clang
+
+//===----------------------------------------------------------------------===//
+// Thorough analysis: only the state right before and right after the
+// function call is checked for the difference in trait value.
+//===----------------------------------------------------------------------===//
+
+namespace clang {
+namespace ento {
+namespace {
+
+class ThoroughErrorNotPreventedFuncVisitor
+    : public ErrorNotPreventedFuncVisitor {
+public:
+  virtual bool
+  wasModifiedBeforeCallExit(const ExplodedNode *CurrN,
+                            const ExplodedNode *CallExitBeginN) override {
+    return CurrN->getState()->get<ErrorPrevented>() !=
+           CallExitBeginN->getState()->get<ErrorPrevented>();
+  }
+};
+
+void addThoroughStatefulChecker(AnalysisASTConsumer &AnalysisConsumer,
+                                AnalyzerOptions &AnOpts) {
+  AnOpts.CheckersAndPackages = {{"test.StatefulChecker", true}};
+  AnalysisConsumer.AddCheckerRegistrationFn([](CheckerRegistry &Registry) {
+    Registry.addChecker<StatefulChecker<ThoroughErrorNotPreventedFuncVisitor>>(
+        "test.StatefulChecker", "Description", "");
+  });
+}
+
+TEST(NoStateChangeFuncVisitor, ThoroughFunctionAnalysis) {
+  std::string Diags;
+  EXPECT_TRUE(runCheckerOnCode<addThoroughStatefulChecker>(R"(
+    void error();
+    void preventError();
+    void allowError();
+
+    void g() {
+      //preventError();
+    }
+
+    void f() {
+      g();
+      error();
+    }
+  )", Diags));
+  EXPECT_EQ(Diags,
+            "test.StatefulChecker: Calling 'g' | Returning without prevening "
+            "the error | Returning from 'g' | error() called\n");
+
+  Diags.clear();
+
+  EXPECT_TRUE(runCheckerOnCode<addThoroughStatefulChecker>(R"(
+    void error();
+    void preventError();
+    void allowError();
+
+    void g() {
+      preventError();
+      allowError();
+    }
+
+    void f() {
+      g();
+      error();
+    }
+  )", Diags));
+  EXPECT_EQ(Diags, "test.StatefulChecker: error() called\n");
+
+  Diags.clear();
+
+  EXPECT_TRUE(runCheckerOnCode<addThoroughStatefulChecker>(R"(
+    void error();
+    void preventError();
+    void allowError();
+
+    void g() {
+      preventError();
+    }
+
+    void f() {
+      g();
+      error();
+    }
+  )", Diags));
+  EXPECT_EQ(Diags, "");
+}
+
+} // namespace
+} // namespace ento
+} // namespace clang

clang/unittests/StaticAnalyzer/RegisterCustomCheckersTest.cpp

@@ -51,7 +51,7 @@ void addCustomChecker(AnalysisASTConsumer &AnalysisConsumer,
 TEST(RegisterCustomCheckers, RegisterChecker) {
   std::string Diags;
   EXPECT_TRUE(runCheckerOnCode<addCustomChecker>("void f() {;}", Diags));
-  EXPECT_EQ(Diags, "test.CustomChecker:Custom diagnostic description\n");
+  EXPECT_EQ(Diags, "test.CustomChecker: Custom diagnostic description\n");
 }
 
 //===----------------------------------------------------------------------===//
@@ -169,7 +169,7 @@ void addDep(AnalysisASTConsumer &AnalysisConsumer,
 TEST(RegisterDeps, UnsatisfiedDependency) {
   std::string Diags;
   EXPECT_TRUE(runCheckerOnCode<addDep>("void f() {int i;}", Diags));
-  EXPECT_EQ(Diags, "test.RegistrationOrder:test.RegistrationOrder\n");
+  EXPECT_EQ(Diags, "test.RegistrationOrder: test.RegistrationOrder\n");
 }
 
 //===----------------------------------------------------------------------===//
@@ -272,7 +272,7 @@ TEST(RegisterDeps, SimpleWeakDependency) {
   std::string Diags;
   EXPECT_TRUE(runCheckerOnCode<addWeakDepCheckerBothEnabled>(
       "void f() {int i;}", Diags));
-  EXPECT_EQ(Diags, "test.RegistrationOrder:test.WeakDep\ntest."
+  EXPECT_EQ(Diags, "test.RegistrationOrder: test.WeakDep\ntest."
                    "Dep\ntest.RegistrationOrder\n");
   Diags.clear();
 
@@ -280,31 +280,33 @@ TEST(RegisterDeps, SimpleWeakDependency) {
   // but the dependencies are switched.
   EXPECT_TRUE(runCheckerOnCode<addWeakDepCheckerBothEnabledSwitched>(
       "void f() {int i;}", Diags));
-  EXPECT_EQ(Diags, "test.RegistrationOrder:test.Dep\ntest."
+  EXPECT_EQ(Diags, "test.RegistrationOrder: test.Dep\ntest."
                    "RegistrationOrder\ntest.WeakDep\n");
   Diags.clear();
 
   // Weak dependencies dont prevent dependent checkers from being enabled.
   EXPECT_TRUE(runCheckerOnCode<addWeakDepCheckerDepDisabled>(
       "void f() {int i;}", Diags));
-  EXPECT_EQ(Diags, "test.RegistrationOrder:test.Dep\ntest.RegistrationOrder\n");
+  EXPECT_EQ(Diags,
+            "test.RegistrationOrder: test.Dep\ntest.RegistrationOrder\n");
   Diags.clear();
 
   // Nor will they be enabled just because a dependent checker is.
   EXPECT_TRUE(runCheckerOnCode<addWeakDepCheckerDepUnspecified>(
       "void f() {int i;}", Diags));
-  EXPECT_EQ(Diags, "test.RegistrationOrder:test.Dep\ntest.RegistrationOrder\n");
+  EXPECT_EQ(Diags,
+            "test.RegistrationOrder: test.Dep\ntest.RegistrationOrder\n");
   Diags.clear();
 
   EXPECT_TRUE(
       runCheckerOnCode<addWeakDepTransitivity>("void f() {int i;}", Diags));
-  EXPECT_EQ(Diags, "test.RegistrationOrder:test.WeakDep2\ntest."
+  EXPECT_EQ(Diags, "test.RegistrationOrder: test.WeakDep2\ntest."
                    "Dep\ntest.RegistrationOrder\n");
   Diags.clear();
 
   EXPECT_TRUE(
       runCheckerOnCode<addWeakDepHasWeakDep>("void f() {int i;}", Diags));
-  EXPECT_EQ(Diags, "test.RegistrationOrder:test.WeakDep2\ntest."
+  EXPECT_EQ(Diags, "test.RegistrationOrder: test.WeakDep2\ntest."
                    "WeakDep\ntest.Dep\ntest.RegistrationOrder\n");
   Diags.clear();
 }
@@ -414,7 +416,7 @@ TEST(RegisterDeps, DependencyInteraction) {
   std::string Diags;
   EXPECT_TRUE(
       runCheckerOnCode<addWeakDepHasStrongDep>("void f() {int i;}", Diags));
-  EXPECT_EQ(Diags, "test.RegistrationOrder:test.StrongDep\ntest."
+  EXPECT_EQ(Diags, "test.RegistrationOrder: test.StrongDep\ntest."
                    "WeakDep\ntest.Dep\ntest.RegistrationOrder\n");
   Diags.clear();
 
@@ -424,14 +426,14 @@ TEST(RegisterDeps, DependencyInteraction) {
   // established in between the modeling portion and the weak dependency.
   EXPECT_TRUE(
       runCheckerOnCode<addWeakDepAndStrongDep>("void f() {int i;}", Diags));
-  EXPECT_EQ(Diags, "test.RegistrationOrder:test.WeakDep\ntest."
+  EXPECT_EQ(Diags, "test.RegistrationOrder: test.WeakDep\ntest."
                    "StrongDep\ntest.Dep\ntest.RegistrationOrder\n");
   Diags.clear();
 
   // If a weak dependency is disabled, the checker itself can still be enabled.
   EXPECT_TRUE(runCheckerOnCode<addDisabledWeakDepHasStrongDep>(
       "void f() {int i;}", Diags));
-  EXPECT_EQ(Diags, "test.RegistrationOrder:test.Dep\ntest."
+  EXPECT_EQ(Diags, "test.RegistrationOrder: test.Dep\ntest."
                    "RegistrationOrder\ntest.StrongDep\n");
   Diags.clear();
 
@@ -439,20 +441,22 @@ TEST(RegisterDeps, DependencyInteraction) {
   // but it shouldn't enable a strong unspecified dependency.
   EXPECT_TRUE(runCheckerOnCode<addDisabledWeakDepHasUnspecifiedStrongDep>(
       "void f() {int i;}", Diags));
-  EXPECT_EQ(Diags, "test.RegistrationOrder:test.Dep\ntest.RegistrationOrder\n");
+  EXPECT_EQ(Diags,
+            "test.RegistrationOrder: test.Dep\ntest.RegistrationOrder\n");
   Diags.clear();
 
   // A strong dependency of a weak dependency is disabled, so neither of them
   // should be enabled.
   EXPECT_TRUE(runCheckerOnCode<addWeakDepHasDisabledStrongDep>(
       "void f() {int i;}", Diags));
-  EXPECT_EQ(Diags, "test.RegistrationOrder:test.Dep\ntest.RegistrationOrder\n");
+  EXPECT_EQ(Diags,
+            "test.RegistrationOrder: test.Dep\ntest.RegistrationOrder\n");
   Diags.clear();
 
   EXPECT_TRUE(
       runCheckerOnCode<addWeakDepHasUnspecifiedButLaterEnabledStrongDep>(
           "void f() {int i;}", Diags));
-  EXPECT_EQ(Diags, "test.RegistrationOrder:test.StrongDep\ntest.WeakDep\ntest."
+  EXPECT_EQ(Diags, "test.RegistrationOrder: test.StrongDep\ntest.WeakDep\ntest."
                    "Dep\ntest.Dep2\ntest.RegistrationOrder\n");
   Diags.clear();
 }