fix(summaries): keep nothing of the blocks constant propagation proves unreachable

docs/usage.md

@@ -132,7 +132,11 @@ variable is not treated that way yet.
 | `plaintext-credential-storage` | A parameter named like a password stored in a database without hashing. Medium confidence, since a name is a hint. |
 | `exploitable-vulnerability` | An API affected by an advisory of a vulnerable requirement, see Dependencies. |
 
-Each flow is judged before it is reported. A dominating guard that proves the value
+Each flow is judged before it is reported. Code constant propagation proves can never
+run (under `if False:`, `while False:`) reports nothing, in the function and through the
+project functions that contain it: their summaries keep nothing of such code, neither a
+sink nor a returned, merged or stored value. Code that may run is kept, whatever decides
+it at run time, a parameter or a module-level name. A dominating guard that proves the value
 safe (`isdigit()`, membership in a constant allowlist, equality with a constant, a
 numeric value proven by `int()`, `len()` or bounded arithmetic, a validator such as
 `re.fullmatch`) refutes it and nothing is reported. A sink that selects code by name,
@@ -913,7 +917,10 @@ such as `metaclass=`, `with` targets of any assignable form and `match` with eve
 pattern kind. `finally` is modelled on the normal path only. A
 `nonlocal` write made by a nested function flows back to the enclosing variable when the
 enclosing function calls that nested function directly by name; a nested function passed
-around and called elsewhere keeps its writes to itself. Methods of module-level classes are
+around and called elsewhere keeps its writes to itself. Where the nested function may
+return without writing the name (no write on some path, a write constant propagation
+proves cannot run, or an exception before it), the variable keeps the value it had
+before the call, joined with what may be written; only a write on every path replaces it. Methods of module-level classes are
 analysed like functions.
 
 ## Looking at the intermediate representation

src/coretrace_python/cache.py

@@ -42,7 +42,7 @@
 from coretrace_python.semantic.symbols import SymbolId
 from coretrace_python.source import FileLocation, Location, SourceId, SourceSpan
 
-CACHE_FORMAT = 15
+CACHE_FORMAT = 16
 
 
 @dataclass(frozen=True)
@@ -300,7 +300,12 @@ def _encode_summary(summary: FunctionSummary) -> dict[str, Any]:
         ],
         "side_effects": sorted(summary.side_effects),
         "nonlocal_writes": [
-            {"name": w.name, "dependencies": sorted(w.dependencies), "externals": sorted(str(s) for s in w.externals)}
+            {
+                "name": w.name,
+                "dependencies": sorted(w.dependencies),
+                "externals": sorted(str(s) for s in w.externals),
+                "kept": w.kept,
+            }
             for w in summary.nonlocal_writes
         ],
         "static": summary.static,
@@ -328,7 +333,10 @@ def _decode_summary(data: Mapping[str, Any]) -> FunctionSummary:
         frozenset(_string(name) for name in data["side_effects"]),
         tuple(
             NonlocalWrite(
-                _string(w["name"]), _indices(w["dependencies"]), frozenset(SymbolId(_string(s)) for s in w["externals"])
+                _string(w["name"]),
+                _indices(w["dependencies"]),
+                frozenset(SymbolId(_string(s)) for s in w["externals"]),
+                bool(w["kept"]),
             )
             for w in data["nonlocal_writes"]
         ),

src/coretrace_python/interprocedural/summaries.py

@@ -332,11 +332,15 @@ class Mutation:
 @dataclass(frozen=True)
 class NonlocalWrite:
     """What the function stores into its ``nonlocal`` ``name``: data of its parameters
-    and results of external symbols, written back to the caller's variable."""
+    and results of external symbols, written back to the caller's variable. ``kept``
+    says it may also return without writing it, on a path with no reachable write, so
+    the caller's variable may keep the value it had. A function with no record for a
+    ``nonlocal`` never writes it (#258)."""
 
     name: str
     dependencies: Dependencies
     externals: frozenset[SymbolId]
+    kept: bool = False
 
 
 @dataclass(frozen=True)
@@ -509,6 +513,11 @@ def evaluate(self, block: BasicBlock, incoming: Mapping[BlockId, State]) -> Stat
         return MappingProxyType(state)
 
     def flow(self, cfg: CFG, block_id: BlockId, incoming: Mapping[BlockId, State]) -> Mapping[BlockId, State]:
+        # A block constant propagation proves unreachable has no effect: no store, call,
+        # return or write, and nothing it would pass on reaches its successors or their
+        # phis (#258). A block that may run is kept, whatever decides it.
+        if not self.reachable(block_id):
+            return {}
         block = self.blocks[block_id]
         state = self.evaluate(block, incoming)
         exits = {target: state for target in block.exception_targets}
@@ -576,9 +585,9 @@ def call(self, call: Call, state: dict[Key, Dep]) -> Dep:
             given = self.graph.arguments_at(self.name, call.location)
             self.record(target.symbol, arguments, named, call.location, None, given)
             # Where its result is called: facts the summary keeps whatever the models;
-            # the consumer decides which kinds of a sink apply there (#240). An invocation
-            # is kept only when it and the call can run, by constant propagation.
-            if call.result is not None and self.runs(call):
+            # the consumer decides which kinds of a sink apply there (#240). The call runs,
+            # as its block is evaluated; an invocation is kept only when it can run too.
+            if call.result is not None:
                 for invocation in invocations(self.function, self.uses, call.result):
                     if self.runs(invocation):
                         self.record(target.symbol, arguments, named, call.location, None, given, invocation.location)
@@ -597,14 +606,17 @@ def call(self, call: Call, state: dict[Key, Dep]) -> Dep:
         return everything | state.get(call.callee, EMPTY)
 
     def nonlocal_result(self, instruction: NonlocalResult, state: dict[Key, Dep]) -> Dep:
-        """What the nested callee left in its nonlocal ``name``, in the caller's terms."""
+        """What the nested callee left in its nonlocal ``name``, in the caller's terms:
+        what it may write, and the value before the call wherever it may leave the name
+        unwritten, or when what it does is not known."""
 
+        previous = self.deep(instruction.previous, state)
         call = self.defs.get(instruction.call)
         if not isinstance(call, Call):
-            return EMPTY
+            return previous
         target = self.graph.target_at(self.name, call.location)
         if not isinstance(target, KnownFunction) or target.name not in self.table:
-            return EMPTY
+            return previous
         callee = self.table[target.name]
         arguments = tuple(self.deep(a, state) for a in call.arguments)
         made = self.defs.get(call.callee)
@@ -615,10 +627,9 @@ def nonlocal_result(self, instruction: NonlocalResult, state: dict[Key, Dep]) ->
         keywords = EMPTY
         for value in (*call.starred, *(v for _, v in call.keywords)):
             keywords |= self.deep(value, state)
-        deps = EMPTY
-        for write in callee.nonlocal_writes:
-            if write.name != instruction.name:
-                continue
+        written = [write for write in callee.nonlocal_writes if write.name == instruction.name]
+        deps = previous if not written or any(write.kept for write in written) else EMPTY
+        for write in written:
             for index in write.dependencies:
                 deps |= arguments[index] if index < len(arguments) else keywords
             deps |= Dep(externals=write.externals)
@@ -681,6 +692,38 @@ def mapped(indices: Dependencies, cleared: Cleared = ()) -> Dep:
     def uses(self) -> DefUse:
         return def_use(self.function)
 
+    def may_return_unwritten(self, name: str) -> bool:
+        """Whether a path from the entry to a return writes no ``nonlocal`` ``name``,
+        through the blocks that can run. An exception edge leaves a block before its
+        write, so its handler is reached unwritten."""
+
+        writes = {
+            block.id
+            for block in self.function.blocks
+            if any(isinstance(i, SetNonlocal) and i.name == name for i in block.instructions)
+        }
+        pending = [self.function.entry]
+        seen: set[BlockId] = set()
+        while pending:
+            block_id = pending.pop()
+            if block_id in seen or not self.reachable(block_id):
+                continue
+            seen.add(block_id)
+            block = self.blocks[block_id]
+            pending.extend(block.exception_targets)
+            if block_id in writes:
+                continue
+            terminator = block.terminator
+            if isinstance(terminator, Return):
+                return True
+            if isinstance(terminator, Branch):
+                pending.extend((terminator.then_block, terminator.else_block))
+            elif isinstance(terminator, Jump):
+                pending.append(terminator.target)
+            elif isinstance(terminator, ForNext):
+                pending.extend((terminator.body, terminator.exit))
+        return False
+
     def runs(self, call: Call) -> bool:
         """Whether ``call`` can run: its block is reachable by constant propagation."""
 
@@ -769,7 +812,7 @@ def summarize(
     problem.mutated_globals = set()
     problem.nonlocal_stores = {}
     for block in function.blocks:
-        if solution.reached(block.id):
+        if solution.reached(block.id) and problem.reachable(block.id):
             problem.evaluate(block, solution.incoming(block.id))
     mutations: list[Mutation] = []
     for index, parameter in enumerate(function.parameters):
@@ -788,9 +831,8 @@ def summarize(
         mutations=tuple(mutations),
         side_effects=frozenset(problem.mutated_globals),
         nonlocal_writes=tuple(
-            NonlocalWrite(name, deps.parameters, deps.externals)
+            NonlocalWrite(name, deps.parameters, deps.externals, problem.may_return_unwritten(name))
             for name, deps in sorted(problem.nonlocal_stores.items())
-            if deps.parameters or deps.externals
         ),
         static=static,
         return_cleared=problem.returns.cleared,

src/coretrace_python/ir/lowering.py

@@ -251,7 +251,8 @@ def write_back_nonlocals(self, node: nodes.Call, call: Value) -> None:
             return
         bindings = self.scopes.scope_for(nested).bindings
         for name in sorted(n for n, b in bindings.items() if b.kind is BindingKind.NONLOCAL):
-            written = self.emit(NonlocalResult(self.new_value(), node.span, call, name))
+            previous = self.expression(nodes.Name(name, node.span))
+            written = self.emit(NonlocalResult(self.new_value(), node.span, call, name, previous))
             self.store(nodes.Name(name, node.span), written)
 
     # ------------------------------------------------------------------ statements

src/coretrace_python/ir/model.py

@@ -9,7 +9,7 @@
 from coretrace_python.source import SourceSpan
 
 # Bumped whenever the instruction set or its meaning changes; part of the cache key.
-PYIR_SCHEMA_VERSION = 1
+PYIR_SCHEMA_VERSION = 2
 
 
 @dataclass(frozen=True)
@@ -245,12 +245,15 @@ class Yield(Operands):
 @dataclass(frozen=True)
 class NonlocalResult(Operands):
     """The value a nested function left in its ``nonlocal`` ``name`` when ``call``
-    returned; the caller stores it back into its own variable."""
+    returned; the caller stores it back into its own variable. ``previous`` is the value
+    that variable held before the call, which it keeps wherever the function may leave
+    the name unwritten (``NonlocalWrite.kept``)."""
 
     result: Value
     location: SourceSpan
     call: Value
     name: str
+    previous: Value
 
 
 @dataclass(frozen=True)

src/coretrace_python/ir/printer.py

@@ -129,7 +129,10 @@ def _instruction(instruction: Instruction) -> str:
         suffix = "" if instruction.value is None else f" {_value(instruction.value)}"
         return f"{_value(instruction.result)} = yield{suffix}"
     if isinstance(instruction, NonlocalResult):
-        return f"{_value(instruction.result)} = nonlocal_result {_value(instruction.call)}, {instruction.name!r}"
+        return (
+            f"{_value(instruction.result)} = nonlocal_result {_value(instruction.call)}, {instruction.name!r}"
+            f" else {_value(instruction.previous)}"
+        )
     if isinstance(instruction, SetAttr):
         return f"set_attr {_value(instruction.object)}, {instruction.attribute!r}, {_value(instruction.value)}"
     if isinstance(instruction, SetItem):

src/coretrace_python/taint/engine.py

@@ -464,14 +464,17 @@ def constructed_keys(self, call: Call, state: Mapping[Key, Taint]) -> Taint:
         return taint
 
     def nonlocal_result(self, instruction: NonlocalResult, state: dict[Key, Taint]) -> Taint:
-        """The taint a nested callee left in its nonlocal ``name``, in the caller's terms."""
+        """The taint a nested callee left in its nonlocal ``name``, in the caller's terms:
+        what it may write, and the value before the call wherever it may leave the name
+        unwritten (``NonlocalWrite.kept``), or when what it does is not known."""
 
+        previous = self.deep(instruction.previous, state)
         call = self.defs.get(instruction.call)
         if not isinstance(call, Call):
-            return Taint.none()
+            return previous
         target = self.graph.target_at(self.name, call.location)
         if not isinstance(target, KnownFunction):
-            return Taint.none()
+            return previous
         summary = self.summaries.summary(target.name)
         arguments = (
             *(self.deep(r, state) for r in (() if summary.static else self.receiver(call, target.name))),
@@ -481,10 +484,9 @@ def nonlocal_result(self, instruction: NonlocalResult, state: dict[Key, Taint])
         keywords = Taint.none()
         for value in (*call.starred, *(v for _, v in call.keywords)):
             keywords = keywords.join(self.deep(value, state))
-        result = Taint.none()
-        for write in summary.nonlocal_writes:
-            if write.name != instruction.name:
-                continue
+        written = [write for write in summary.nonlocal_writes if write.name == instruction.name]
+        result = previous if not written or any(write.kept for write in written) else Taint.none()
+        for write in written:
             for index in sorted(write.dependencies):
                 result = result.join(arguments[index] if index < len(arguments) else keywords)
             for symbol in sorted(write.externals, key=str):

tests/test_cache_engine_version.py

@@ -0,0 +1,42 @@
+"""What makes a fix reach a cached project (#258, review of PR #259).
+
+A change to the analysis that keeps the cache format and the plugins, as the summaries
+dropping unreachable blocks does, keeps every key between commits of one version: a
+cache built before it would still serve the old results. What delivers it is the
+version of the engine, part of every key: once it changes, nothing cached is reused and
+every module is analysed again with the engine that reads it.
+"""
+
+from __future__ import annotations
+
+from pathlib import Path
+
+import pytest
+
+from coretrace_python import engine
+from coretrace_python.cache import ProjectCache
+
+APP = "import os\n\n\ndef helper(cmd):\n    if False:\n        os.system(cmd)\n\n\ndef caller():\n    helper(input())\n"
+
+
+def project(tmp_path: Path) -> Path:
+    root = tmp_path / "project"
+    (root / "app").mkdir(parents=True)
+    (root / "app" / "__init__.py").write_text("", encoding="utf-8")
+    (root / "app" / "main.py").write_text(APP, encoding="utf-8")
+    return root
+
+
+def test_a_new_engine_version_reuses_nothing_cached(tmp_path: Path, monkeypatch: pytest.MonkeyPatch) -> None:
+    root = project(tmp_path)
+    cache = ProjectCache(tmp_path / "cache")
+    built = engine.analyze_project(root, [engine.BUNDLED_PLUGINS], cache=cache)
+    same_version = engine.analyze_project(root, [engine.BUNDLED_PLUGINS], cache=cache)
+
+    monkeypatch.setattr(engine, "__version__", "0.0.0+next")
+    next_version = engine.analyze_project(root, [engine.BUNDLED_PLUGINS], cache=cache)
+
+    assert set(same_version.reused) == set(built.keys) == {"app", "app.main"}
+    assert next_version.reused == ()
+    assert set(next_version.keys.values()).isdisjoint(built.keys.values())
+    assert [f for f in next_version.findings if f.rule_id == "command-injection"] == []