Add use-after-move rule on a generic worklist dataflow engine (M6)

Phase 2+ backlog item #2: a small, Clang-independent forward "may"
dataflow fixed-point engine (sentinel::dataflow::runForwardWorklist),
plus a fourth rule built on it that flags reading a local/parameter
after std::move(...) with no reassignment in between. This is the
first rule needing a real fixed point across CFG blocks -- a
moved-from fact can flow into a loop body from the previous iteration
via a back edge, which lock-order-inversion's single AST walk can't
see. Extracted FunctionCollector out of LockOrderInversionRule so both
CFG-based rules share it.

Self-scanning the codebase (same dogfooding discipline as M4/M5)
caught a real false-positive class before it shipped: a VarDecl's own
declaration wasn't treated as killing a prior moved-from fact tied to
it, so a variable declared fresh inside a loop body looked moved-from
on the next iteration once the fixed point carried the fact around the
back edge. Fixed, with a regression fixture.

Author: czhao-dev

README.md

@@ -7,16 +7,19 @@ defects with precise source locations.
 
 This is the MVP scoped in [`docs/spec.md`](docs/spec.md) -- three rules,
 text/JSON/SARIF output, YAML config with suppression, and one flagship
-cross-file rule with real graph-algorithm teeth -- plus SARIF output from
-the Phase 2+ backlog. See [`docs/architecture.md`](docs/architecture.md)
-for how it's built internally.
+cross-file rule with real graph-algorithm teeth -- plus two items from the
+Phase 2+ backlog: SARIF output and a fourth rule, `use-after-move`, built
+on a generic worklist dataflow engine. See
+[`docs/architecture.md`](docs/architecture.md) for how it's built
+internally.
 
 ## Rules
 
 | Rule | Kind | What it catches |
 |---|---|---|
 | `missing-override` | AST | A virtual method that overrides a base method without the `override` keyword. |
 | `redundant-move-return` | AST | `return std::move(local);`, which suppresses NRVO for no benefit. |
+| `use-after-move` | CFG + worklist dataflow | A local variable or parameter read after `std::move(...)`, with no reassignment in between. |
 | `lock-order-inversion` | CFG + cross-file graph | A cycle in lock acquisition order across the whole project (a potential deadlock), found via Tarjan's SCC over a project-wide lock graph. |
 
 `lock-order-inversion` is the flagship: it tracks which locks are held
@@ -28,6 +31,16 @@ be individually lock-order-safe and still form a cycle once merged --
 that's the case the flagship is built to catch (see
 `tests/projects/lock-cycle/`).
 
+`use-after-move` is the other CFG-based rule, and the only one that needs
+a real fixed point: it runs a forward "may" dataflow analysis
+(`sentinel::dataflow::runForwardWorklist`, a small generic worklist engine
+reused by any future rule that needs one) over each function's
+`clang::CFG`, tracking which variables are moved-from and not yet
+reassigned. A moved-from fact can flow into a loop body from the
+*previous* iteration via a back edge -- unlike `lock-order-inversion`'s
+single AST walk, this genuinely needs the fixed point to catch that case
+(see `tests/fixtures/use-after-move/loop.cpp`).
+
 ## Building
 
 Requires CMake 3.20+, a C++20 compiler, and LLVM/Clang dev packages
@@ -159,8 +172,18 @@ scaling studies).
   treated as an immediate acquisition.
 - `redundant-move-return` doesn't verify all of a function's return paths
   agree on the same NRVO candidate.
-- `--header-filter` and the main-file-only default only apply to the two
-  per-TU AST rules (enforced in `TranslationUnitContext::report()`);
+- `use-after-move` only recognizes a direct `x = ...` assignment as
+  reinitializing `x`; a method call that logically resets it
+  (`x.clear()`, `x.reset()`) or passing `&x` to an out-parameter is not,
+  and can produce a false positive on a use right after. It also doesn't
+  specially model short-circuit (`&&`/`||`) or `?:` operators, which
+  `clang::CFG` splits across blocks while the underlying AST subtree is
+  still shared -- plain statements (the common shape of this bug) are
+  unaffected. Diagnostics don't point back to the specific move site,
+  since a "may" analysis with multiple incoming paths doesn't always have
+  one unambiguous site to blame.
+- `--header-filter` and the main-file-only default only apply to the
+  per-TU AST/CFG rules (enforced in `TranslationUnitContext::report()`);
   `lock-order-inversion`'s `finalize()`-time diagnostics don't currently
   go through that check.
 

demo/README.md

@@ -1,13 +1,15 @@
 # cpp-sentinel demo script
 
-A ~5-minute walkthrough hitting all three rules, one of them cross-file.
-This directory is a tiny, real (buildable) two-file project with three
+A ~5-minute walkthrough hitting all four rules, one of them cross-file.
+This directory is a tiny, real (buildable) two-file project with four
 intentional bugs:
 
 - `shape.cpp`: `Circle::describe()` overrides `Shape::describe()` without
   `override` (**missing-override**).
 - `logger.cpp`: `buildGreeting()` does `return std::move(name);` on a
   local `std::string` (**redundant-move-return**).
+- `logger.cpp`: `logTwice()` moves `message` into `archived`, then passes
+  `message` to `logEvent()` anyway (**use-after-move**).
 - `shape.cpp`'s `Circle::describe()` acquires `shapeMutex` then
   `logMutex`; `logger.cpp`'s `logEvent()` acquires them in the opposite
   order. Neither file is unsafe on its own -- merged, they form a lock
@@ -33,9 +35,9 @@ cmake -S demo -B demo/build
 ```
 
 Expected text output: a `missing-override` warning in `shape.cpp`, a
-`redundant-move-return` warning in `logger.cpp`, and a `lock-order-inversion`
-error naming both files' acquisition sites -- the cross-file cycle, the
-flagship result.
+`redundant-move-return` warning and a `use-after-move` warning in
+`logger.cpp`, and a `lock-order-inversion` error naming both files'
+acquisition sites -- the cross-file cycle, the flagship result.
 
 ```bash
 # 3. Same run, structured output.
@@ -48,6 +50,6 @@ flagship result.
 ./build/tools/cpp-sentinel/cpp-sentinel explain lock-order-inversion
 ```
 
-That's the full demo: one command surfacing three different classes of
+That's the full demo: one command surfacing four different classes of
 defect, including one that only exists once two separate translation
 units are considered together.

demo/logger.cpp

@@ -10,3 +10,9 @@ std::string buildGreeting() {
   std::string name = "world";
   return std::move(name);
 }
+
+void logTwice(std::string message) {
+  std::string archived = std::move(message);
+  // Bug: message was just moved-from above -- triggers use-after-move.
+  logEvent(message);
+}

docs/architecture.md

@@ -19,12 +19,17 @@ RuleFrontendAction -> ASTConsumer     |
    |     (missing-override,          |
    |      redundant-move-return)     |
    |   - runOnTranslationUnit()      |
-   |     walks the AST directly for  |
-   |     CFG/graph-based rules       |
-   |     (lock-order-inversion),     |
-   |     accumulating LockOrderFacts |
-   |     into that rule's own        |
-   |     LockFactStore               |
+   |     for CFG-based rules:        |
+   |      - lock-order-inversion     |
+   |        walks the AST directly,  |
+   |        accumulating             |
+   |        LockOrderFacts into its  |
+   |        own LockFactStore        |
+   |      - use-after-move builds a  |
+   |        clang::CFG per function  |
+   |        and runs a forward       |
+   |        worklist dataflow fixed  |
+   |        point over it            |
    +----+-----------------------------+
         |
  every TU processed
@@ -117,6 +122,54 @@ point, `std::unique_lock`'s `defer_lock` constructor form is treated as an
 immediate acquisition, and only `lock_guard`/`unique_lock` RAII wrappers
 are recognized (not `scoped_lock`).
 
+## The generic worklist dataflow engine and `use-after-move`
+
+`include/sentinel/dataflow/WorklistDataflowEngine.h` is a small, generic
+forward "may" dataflow fixed-point solver: `runForwardWorklist(nodes,
+entryIndex, analysis)` takes a plain index/adjacency graph (`Node` --
+deliberately not `clang::CFGBlock` itself, so the algorithm is
+unit-testable with a hand-built graph and no C++ parse, the same reasoning
+that keeps `graph/SCC.h` decoupled from any AST type -- see
+`WorklistDataflowEngineTest.cpp`) and an `Analysis` type supplying a
+`Domain`, `join`/`bottom`/`entry`, and a pure per-node `transfer`. It's
+the first rule in this codebase that needs a real fixed point rather than
+a single walk: `lock-order-inversion` visits each loop body once (a
+documented trim above), but a moved-from fact can flow into a loop body
+from the *previous* iteration via a back edge, so `use-after-move` needs
+the fixed point to catch that case correctly (see
+`tests/fixtures/use-after-move/loop.cpp`, and
+`WorklistDataflowEngineTest.LoopReachesFixedPointThroughBackEdge` for the
+engine-level version of the same scenario).
+
+`UseAfterMoveRule` (`lib/rules/UseAfterMoveRule.cpp`) adapts a
+`clang::CFG` (one built per function via `CFG::buildCFG`) into that engine
+in two passes:
+
+1. **Fixed-point pass**: `UseAfterMoveAnalysis::transfer()` walks each
+   block's statements with `Domain = std::set<const VarDecl*>` (the
+   currently moved-from-and-not-reassigned variables), joined by set union
+   at merge points. This must be *pure* -- called repeatedly during
+   iteration, so it never reports diagnostics itself.
+2. **Reporting pass**: once `IN[]` is known correct for every block, the
+   rule re-walks each block's statements exactly once more from its final
+   `IN[]`, this time actually reporting a diagnostic for every read of an
+   already-moved-from variable.
+
+Both passes share one `transferStatement()` helper (classify a
+statement's `DeclRefExpr`s into kills/gens/uses via a small
+`RecursiveASTVisitor`) so the fixed-point and reporting logic can't drift
+apart. Known limitations are documented in
+`include/sentinel/rules/UseAfterMoveRule.h` and the README; the one worth
+calling out here since it was caught by dogfooding rather than reasoned
+out ahead of time: a `VarDecl`'s own declaration has to be treated as a
+kill of any prior fact tied to that same declaration, or a variable
+declared fresh inside a loop body (e.g. `for (...) { Diagnostic diag; ...;
+report(std::move(diag)); }`) looks moved-from on the *next* iteration's
+brand-new instance, once the fixed point carries that fact around the back
+edge -- a real false positive this rule produced against its own
+`LockOrderInversionRule.cpp` and `SCC.cpp` before the fix (see
+`tests/fixtures/use-after-move/redeclared-each-iteration.cpp`).
+
 ## Header filtering
 
 `TranslationUnitContext::report()` also checks `HeaderFilter`

include/sentinel/core/FunctionCollector.h

@@ -0,0 +1,25 @@
+#pragma once
+
+#include <clang/AST/Decl.h>
+#include <clang/AST/RecursiveASTVisitor.h>
+
+#include <vector>
+
+namespace sentinel {
+
+// Collects every function definition (has a body, is the defining
+// declaration -- not a forward declaration) in a translation unit. Shared
+// by the per-TU rules that walk function bodies themselves rather than
+// through the MatchFinder (lock-order-inversion, use-after-move).
+class FunctionCollector : public clang::RecursiveASTVisitor<FunctionCollector> {
+public:
+  std::vector<const clang::FunctionDecl *> functions;
+
+  bool VisitFunctionDecl(clang::FunctionDecl *decl) {
+    if (decl->hasBody() && decl->isThisDeclarationADefinition())
+      functions.push_back(decl);
+    return true;
+  }
+};
+
+} // namespace sentinel

include/sentinel/dataflow/WorklistDataflowEngine.h

@@ -0,0 +1,86 @@
+#pragma once
+
+#include <cstddef>
+#include <queue>
+#include <vector>
+
+namespace sentinel::dataflow {
+
+// One node in the graph the engine iterates over. Deliberately not
+// clang::CFGBlock itself: keeping the engine's graph representation a
+// plain index/adjacency structure means the fixed-point algorithm can be
+// unit-tested with a hand-built graph, no real C++ parse required (see
+// WorklistDataflowEngineTest.cpp) -- the same reasoning that keeps
+// graph/SCC.h decoupled from any AST type. A caller adapting a real
+// clang::CFG builds one Node per CFGBlock (indexed by getBlockID()) from
+// that block's preds()/succs().
+struct Node {
+  std::vector<size_t> predecessors;
+  std::vector<size_t> successors;
+};
+
+// Forward, "may"-style iterative worklist dataflow fixed point. Direction
+// is always forward and the join is always caller-supplied (typically set
+// union for a "may" analysis) -- nothing in this codebase needs a backward
+// or "must" analysis yet, so those aren't generalized for here.
+//
+// `Analysis` must provide:
+//   - using Domain = ...;  (copyable, comparable via equal())
+//   - Domain entry() const;  IN[[entryIndex]]
+//   - Domain bottom() const;  identity element for join: join(bottom(), x) == x
+//   - Domain join(const Domain &a, const Domain &b) const;
+//   - bool equal(const Domain &a, const Domain &b) const;
+//   - Domain transfer(size_t nodeIndex, Domain in) const;
+//
+// transfer() must be pure: called repeatedly during fixed-point iteration,
+// so it must not have observable side effects (e.g. must not report
+// diagnostics itself -- a caller that needs to act at the exact
+// statement/instruction where a fact becomes relevant should re-walk each
+// node's contents starting from the returned IN[node] once the fixed
+// point below has been reached, since this engine only computes
+// node-boundary state).
+//
+// Returns IN[node] for every node, indexed the same as `nodes`.
+template <typename Analysis>
+std::vector<typename Analysis::Domain> runForwardWorklist(const std::vector<Node> &nodes,
+                                                          size_t entryIndex,
+                                                          const Analysis &analysis) {
+  using Domain = typename Analysis::Domain;
+
+  std::vector<Domain> in(nodes.size(), analysis.bottom());
+  std::vector<Domain> out(nodes.size(), analysis.bottom());
+  std::vector<bool> queued(nodes.size(), false);
+  std::queue<size_t> worklist;
+
+  auto enqueue = [&](size_t index) {
+    if (!queued[index]) {
+      queued[index] = true;
+      worklist.push(index);
+    }
+  };
+
+  for (size_t i = 0; i < nodes.size(); ++i)
+    enqueue(i);
+
+  while (!worklist.empty()) {
+    size_t index = worklist.front();
+    worklist.pop();
+    queued[index] = false;
+
+    Domain newIn = index == entryIndex ? analysis.entry() : analysis.bottom();
+    for (size_t pred : nodes[index].predecessors)
+      newIn = analysis.join(newIn, out[pred]);
+
+    in[index] = newIn;
+    Domain newOut = analysis.transfer(index, newIn);
+    if (!analysis.equal(newOut, out[index])) {
+      out[index] = std::move(newOut);
+      for (size_t succ : nodes[index].successors)
+        enqueue(succ);
+    }
+  }
+
+  return in;
+}
+
+} // namespace sentinel::dataflow

include/sentinel/rules/UseAfterMoveRule.h

@@ -0,0 +1,54 @@
+#pragma once
+
+#include "sentinel/core/Rule.h"
+
+namespace sentinel::rules {
+
+// Flags a read of a local variable or parameter after it was passed to
+// std::move(...), with no reassignment in between -- the object is left
+// in a valid-but-unspecified moved-from state, and reading it there is
+// almost always a bug rather than intentional.
+//
+// Built on the generic forward worklist dataflow engine
+// (sentinel::dataflow::runForwardWorklist), the first rule in this
+// codebase that needs a real fixed point across clang::CFG blocks: unlike
+// lock-order-inversion's single AST walk (loop bodies visited once),
+// a moved-from fact can flow into a loop body from the *previous*
+// iteration via a back edge, so getting this right needs the fixed point.
+//
+// Domain: the set of VarDecls currently moved-from and not yet
+// reassigned, joined by union at merge points ("may" analysis: flagged if
+// a use is reachable from the move along *any* path, even if other paths
+// reassign first -- consistent with warning-tool practice of erring
+// toward reporting a real, reachable bug rather than staying silent
+// because *some* path is safe).
+//
+// Known limitations (deliberate MVP trims, documented rather than
+// silently mishandled):
+//  - Only a direct `x = ...` assignment is recognized as reinitializing
+//    x; a method call that logically resets it (`x.clear()`, `x.reset()`)
+//    or passing `&x` to an out-parameter is not, and can produce a false
+//    positive on a use immediately after.
+//  - Doesn't specially model short-circuit (`&&`/`||`) or `?:` operators,
+//    which clang::CFG splits across multiple blocks/elements while the
+//    underlying AST subtree is still shared; a move or use inside such an
+//    operator's untaken/split branch may be mis-ordered relative to real
+//    control flow. Plain statements (declarations, assignments, calls,
+//    returns) -- by far the common shape of this bug -- are unaffected.
+//  - Only variables of class type are tracked (via getAsCXXRecordDecl());
+//    moving a scalar is a no-op, so there's nothing meaningful to flag.
+//  - The diagnostic doesn't point back to the specific move site: under a
+//    "may" analysis with multiple incoming paths, there isn't always one
+//    unambiguous site to point to.
+class UseAfterMoveRule : public Rule {
+public:
+  std::string id() const override { return "use-after-move"; }
+  std::string shortDescription() const override {
+    return "Local variable or parameter used after being passed to "
+           "std::move(...), with no reassignment in between";
+  }
+
+  void runOnTranslationUnit(TranslationUnitContext &context) override;
+};
+
+} // namespace sentinel::rules

lib/CMakeLists.txt

@@ -39,6 +39,7 @@ add_library(sentinel-lib OBJECT
   rules/MissingOverrideRule.cpp
   rules/RedundantMoveReturnRule.cpp
   rules/LockOrderInversionRule.cpp
+  rules/UseAfterMoveRule.cpp
 )
 
 target_include_directories(sentinel-lib PUBLIC

lib/rules/LockOrderInversionRule.cpp

@@ -1,4 +1,5 @@
 #include "sentinel/rules/LockOrderInversionRule.h"
+#include "sentinel/core/FunctionCollector.h"
 #include "sentinel/core/RuleRegistry.h"
 #include "sentinel/core/SuppressionState.h"
 #include "sentinel/core/TranslationUnitContext.h"
@@ -7,7 +8,6 @@
 
 #include <clang/AST/ASTContext.h>
 #include <clang/AST/ExprCXX.h>
-#include <clang/AST/RecursiveASTVisitor.h>
 #include <clang/Basic/SourceManager.h>
 
 #include <llvm/Support/MemoryBuffer.h>
@@ -219,17 +219,6 @@ class FunctionLockWalker {
   }
 };
 
-class FunctionCollector : public RecursiveASTVisitor<FunctionCollector> {
-public:
-  std::vector<const FunctionDecl *> functions;
-
-  bool VisitFunctionDecl(FunctionDecl *decl) {
-    if (decl->hasBody() && decl->isThisDeclarationADefinition())
-      functions.push_back(decl);
-    return true;
-  }
-};
-
 std::string edgeKey(const std::string &from, const std::string &to) {
   return from + "\x1f" + to;
 }