diff --git a/scripts/check_preyield_permit_release.py b/scripts/check_preyield_permit_release.py index cdccd78da13db..1dc6f55973b96 100644 --- a/scripts/check_preyield_permit_release.py +++ b/scripts/check_preyield_permit_release.py @@ -51,10 +51,32 @@ multi-permit site (``self.total`` + ``self.internal``, as in ``gateway/turn_admission.py``) an ``except`` arm that releases only one of them used to satisfy the check. - * **Every handler arm must release.** A sibling ``except ValueError:`` that - re-raises without releasing leaks, even when a later - ``except BaseException:`` arm does release — the narrow arm is the one that - runs. + * **Every handler arm that CAN exit must release.** A sibling + ``except ValueError:`` that re-raises without releasing leaks, even when a + later ``except BaseException:`` arm does release — the narrow arm is the + one that runs. + +Two further sharpenings (Argus runtime oracle on PR #863, filed as a +pre-existing gap present on BOTH builds): + + * **Exit is a REACHABILITY question, not a last-statement question.** The + arm classifier used to accept only a BARE trailing ``raise``/``return``, + so an arm exiting through a compound statement (``if cond: raise``, a + ``raise`` inside an inner ``try``/``with``/``for``, ``if cond: return``) + was classified as swallowing and excused from releasing — five measured + 1-permit leaks. It now asks whether ANY path through the arm can leave. + * **An unreachable release does not count.** Matching by ``ast.walk`` + credited a release buried in a nested ``def``, under ``if False:``, in a + loop over an empty literal, or after an unconditional ``raise`` — four + more measured leaks. Only statically-reachable statements are credited; + anything the compiler cannot settle still counts, so a release under a + runtime condition is not rejected. + * **A chain whose only arm SWALLOWS is safe.** ``except BaseException: + pass`` falls through to the yield still legitimately holding the permit + (measured: no leak), but the ``BaseException`` coverage test used to be + asked of the *exiting* arms only, so an all-swallowing chain produced an + empty set and was reported as unprotected. Coverage is now asked of the + whole chain and the release demand only of the arms that can exit. DOES NOT COVER (stated boundary, not a hidden gap) -------------------------------------------------- @@ -74,6 +96,13 @@ assignment itself is a raise-capable node in the window), so this errs toward a false positive, not a miss; a new spelling is WONTFIX unless it appears in production code. + * **A release whose REACHABILITY depends on runtime values.** Two measured + 1-permit leaks sit here and are not caught by any build of this guard: a + release that runs only in a nested ``except ValueError`` that never fires, + and a release under a runtime-false condition. The check credits any path + the compiler cannot settle (see ``_released_in_stmt``); narrowing that + would reject the legitimate ``if acquired: release()`` shape the live + multi-permit site uses. Static reachability is the declared line. NO LONGER A GAP: releasing the WRONG object used to satisfy the check, because matching was by method name. Release is now matched by RECEIVER @@ -171,45 +200,271 @@ def _receiver_key(call: ast.Call) -> str | None: return None -def _objects_called(node: ast.AST, names: set[str]) -> set[str]: - """Receivers of every ``.()`` call anywhere under ``node``.""" +def _static_truth(node: ast.AST) -> bool | None: + """``True``/``False`` for a test the compiler can settle; ``None`` otherwise.""" + if isinstance(node, ast.Constant): + try: + return bool(node.value) + except Exception: # pragma: no cover - bool() is total on constants + return None + return None + + +def _statically_empty_iterable(node: ast.AST) -> bool: + """True for a literal that provably yields nothing (``[]``, ``()``, ``{}``).""" + if isinstance(node, (ast.List, ast.Tuple, ast.Set)): + return not node.elts and not any(isinstance(e, ast.Starred) for e in node.elts) + if isinstance(node, ast.Dict): + return not node.keys + if isinstance(node, ast.Constant) and isinstance(node.value, (str, bytes)): + return len(node.value) == 0 + return False + + +def _direct_releases(node: ast.AST) -> set[str]: + """Release receivers in ``node``, NOT descending into a nested callable.""" found: set[str] = set() - for sub in ast.walk(node): + stack: list[ast.AST] = [node] + while stack: + current = stack.pop() + if current is not node and isinstance( + current, (ast.FunctionDef, ast.AsyncFunctionDef, ast.Lambda) + ): + # Its body runs on its own lifecycle, if it is ever called at all. + continue if ( - isinstance(sub, ast.Call) - and isinstance(sub.func, ast.Attribute) - and sub.func.attr in names + isinstance(current, ast.Call) + and isinstance(current.func, ast.Attribute) + and current.func.attr in _RELEASE_METHODS ): - key = _receiver_key(sub) + key = _receiver_key(current) if key is not None: found.add(key) + stack.extend(ast.iter_child_nodes(current)) return found +def _released_in_stmt(stmt: ast.stmt) -> set[str]: + """Release receivers reachable when ``stmt`` runs. + + A textually-present release that can never execute must not satisfy the + check: ``ast.walk`` credited a release buried in a nested ``def``, under + ``if False:``, or in the body of a loop over an empty literal, none of + which ever hand the permit back. + """ + if isinstance(stmt, (ast.FunctionDef, ast.AsyncFunctionDef)): + return set() + if isinstance(stmt, ast.If): + truth = _static_truth(stmt.test) + found = _direct_releases(stmt.test) + if truth is not False: + found |= _released_in(stmt.body) + if truth is not True: + found |= _released_in(stmt.orelse) + return found + if isinstance(stmt, (ast.For, ast.AsyncFor)): + found = _direct_releases(stmt.iter) + if not _statically_empty_iterable(stmt.iter): + found |= _released_in(stmt.body) + return found | _released_in(stmt.orelse) + if isinstance(stmt, ast.While): + found = _direct_releases(stmt.test) + if _static_truth(stmt.test) is not False: + found |= _released_in(stmt.body) + return found | _released_in(stmt.orelse) + if isinstance(stmt, ast.Try): + found = ( + _released_in(stmt.body) + | _released_in(stmt.orelse) + | _released_in(stmt.finalbody) + ) + for handler in stmt.handlers: + found |= _released_in(handler.body) + return found + if isinstance(stmt, (ast.With, ast.AsyncWith)): + found: set[str] = set() + for item in stmt.items: + found |= _direct_releases(item.context_expr) + return found | _released_in(stmt.body) + return _direct_releases(stmt) + + def _released_in(body: list[ast.stmt]) -> set[str]: + """Receivers released on some STATICALLY REACHABLE path through ``body``.""" released: set[str] = set() for stmt in body: - released |= _objects_called(stmt, _RELEASE_METHODS) + released |= _released_in_stmt(stmt) + if not _falls_through(stmt): + # Everything after an unconditional raise/return is dead code. + break return released -def _handler_exits(handler: ast.ExceptHandler) -> bool: - """True if this arm leaves the generator instead of falling through. +def _body_falls_through(body: list[ast.stmt]) -> bool: + """True if control can reach the end of ``body``.""" + for stmt in body: + if not _falls_through(stmt): + return False + return True - An arm that SWALLOWS (no ``raise``/``return``) continues on to the yield - with the permit still legitimately held — demanding a release there would - be a false positive, and acting on it would be a double release. Only an - arm that exits owes the permit back. + +def _falls_through(stmt: ast.stmt) -> bool: + """True if control can continue past ``stmt`` to the next statement.""" + if isinstance(stmt, (ast.Raise, ast.Return, ast.Break, ast.Continue)): + return False + if isinstance(stmt, ast.If): + truth = _static_truth(stmt.test) + if truth is True: + return _body_falls_through(stmt.body) + if truth is False: + return _body_falls_through(stmt.orelse) + return _body_falls_through(stmt.body) or _body_falls_through(stmt.orelse) + if isinstance(stmt, (ast.With, ast.AsyncWith)): + return _body_falls_through(stmt.body) + if isinstance(stmt, ast.Try): + if stmt.finalbody and not _body_falls_through(stmt.finalbody): + return False + through_body = _body_falls_through(stmt.body) + if through_body and stmt.orelse: + through_body = _body_falls_through(stmt.orelse) + return through_body or any(_body_falls_through(h.body) for h in stmt.handlers) + # Loops may run zero times or `break`; a match/other compound statement is + # assumed to fall through. Erring this way keeps an arm out of the + # "provably exits" class only when we cannot prove it, which is safe: + # `_handler_may_exit` below is what demands the release. + return True + + +def _hashable_literal(node: ast.AST) -> bool: + """True only for a literal that provably hashes without raising. + + A set display and a dict display HASH their elements/keys at construction + time, so inert elements are not enough: ``{[1]}`` and ``{[1]: 2}`` each + raise ``TypeError`` while every sub-expression in them is inert. """ - for stmt in reversed(handler.body): - if isinstance(stmt, (ast.Raise, ast.Return)): - return True - if isinstance(stmt, ast.Pass): - continue - break + if isinstance(node, ast.Constant): + return True + if isinstance(node, ast.Tuple): + return all(_hashable_literal(e) for e in node.elts) return False +def _inert_expr(node: ast.AST) -> bool: + """True only for an expression that provably cannot raise. + + Deliberately a WHITELIST. Anything not listed here — a call, an attribute + load, a subscript, an ``await``, arithmetic on a name, a bare NAME load — + can leave the arm by raising, and must therefore keep the arm in the + "may exit" class. + """ + if isinstance(node, ast.Constant): + return True + if isinstance(node, (ast.Tuple, ast.List)): + return all(_inert_expr(e) for e in node.elts) + if isinstance(node, ast.Set): + return all(_hashable_literal(e) for e in node.elts) + if isinstance(node, ast.Dict): + return all(k is not None and _hashable_literal(k) for k in node.keys) and all( + _inert_expr(v) for v in node.values + ) + return False + + +def _inert_target(node: ast.AST) -> bool: + """True only for an assignment target that provably cannot raise. + + A target is not a value. A ``Tuple``/``List`` target is an UNPACK, which + raises ``TypeError`` on a non-iterable and ``ValueError`` on an arity + mismatch even when every name in it is inert; an ``Attribute``/``Subscript`` + target runs ``__setattr__``/``__setitem__``. Only a bare local name is + safe, and only in ``Store`` context. + """ + return isinstance(node, ast.Name) and isinstance(node.ctx, ast.Store) + + +def _inert_signature(fn: ast.FunctionDef | ast.AsyncFunctionDef) -> bool: + """True only when executing this ``def`` statement evaluates nothing. + + Only the BODY of a nested function is deferred. Decorators, defaults, + keyword-only defaults, parameter annotations and the return annotation are + all evaluated when the ``def`` executes, so any of them can leave the arm. + """ + if fn.decorator_list: + return False + if getattr(fn, "type_params", ()): + return False + args = fn.args + if not all(_inert_expr(d) for d in args.defaults): + return False + if not all(d is None or _inert_expr(d) for d in args.kw_defaults): + return False + if fn.returns is not None and not _inert_expr(fn.returns): + return False + slots = [*args.posonlyargs, *args.args, *args.kwonlyargs, args.vararg, args.kwarg] + return all( + a is None or a.annotation is None or _inert_expr(a.annotation) for a in slots + ) + + +def _inert_stmt(stmt: ast.stmt) -> bool: + """True only for a statement that provably cannot leave the arm.""" + if isinstance(stmt, (ast.Pass, ast.Break, ast.Continue, ast.Global, ast.Nonlocal)): + return True + if isinstance(stmt, ast.Expr): + return _inert_expr(stmt.value) + if isinstance(stmt, ast.Assign): + return all(_inert_target(t) for t in stmt.targets) and _inert_expr(stmt.value) + if isinstance(stmt, ast.AnnAssign): + # The annotation is evaluated at runtime unless the module carries + # `from __future__ import annotations`; we do not read the module's + # future-imports here, so an annotation that is not itself inert keeps + # the arm in the may-exit class. + return ( + _inert_target(stmt.target) + and _inert_expr(stmt.annotation) + and (stmt.value is None or _inert_expr(stmt.value)) + ) + if isinstance(stmt, (ast.FunctionDef, ast.AsyncFunctionDef)): + # Defining a nested callable does not run its BODY; its body is its own + # lifecycle. The signature, however, runs now. + return _inert_signature(stmt) + return False + + +def _handler_may_exit(handler: ast.ExceptHandler) -> bool: + """True unless every statement in this arm PROVABLY cannot leave it. + + An arm that provably SWALLOWS continues on to the yield with the permit + still legitimately held — demanding a release there would be a false + positive, and acting on it would be a double release. + + Two rules have been wrong here, in opposite directions: + + * "is the LAST statement a bare ``raise``/``return``?" (scanning + ``reversed(handler.body)``, breaking on any other statement type) excused + every arm that exits through a compound statement — ``if cond: raise``, + ``raise`` inside an inner ``try``/``with``/``for``, ``if cond: return`` — + each a measured 1-permit leak. + * "does it contain an ``ast.Raise``/``ast.Return`` node?" excused every arm + that leaves for a reason that is not a ``raise`` STATEMENT: a call that + raises, an ``assert``, arithmetic that divides by zero, or a plain + logging call given a bad format argument. All four are measured + 1-permit leaks that the pre-#873 guard caught. + + "This arm swallows" is not decidable from the AST — the SAME arm text is + safe with a benign logger and leaks with a bad format argument. So the + question is inverted and answered on a whitelist: an arm is excused only + when every statement in it is provably inert (``pass``, a bare constant, an + assignment between names/constants, a nested ``def``). Anything else keeps + the arm in the "may exit" class and owes the permit back. That is a false + positive at worst; the other direction is a missed leak. + + This makes the arm check ask the same question as the window check, which + has always treated a call/attribute/subscript/await as raise-capable. + """ + return not all(_inert_stmt(stmt) for stmt in handler.body) + + def _build_parent_map(fn: ast.AST) -> dict[int, tuple[ast.AST, str]]: """Map id(child) -> (parent, field_name) for everything in ``fn``'s own body.""" parents: dict[int, tuple[ast.AST, str]] = {} @@ -256,20 +511,27 @@ def _try_releases(node: ast.Try, required: set[str]) -> bool: Two sound shapes: * a ``finally`` that releases them all (runs on every exceptional exit), or - * a handler chain that covers ``BaseException`` and, in EVERY arm that - exits, releases them all. A sibling ``except ValueError:`` arm that + * a handler chain that covers ``BaseException`` and, in every arm that CAN + exit, releases them all. A sibling ``except ValueError:`` arm that re-raises without releasing is the leak an any-arm check missed. + + An arm that provably swallows owes nothing: control falls through to the + yield still holding the permit, which is correct. That is why the + ``BaseException`` coverage question is asked of the WHOLE chain and not + only of the exiting arms — ``except BaseException: pass`` as the sole arm + is safe, and treating its empty exiting-set as "unprotected" flagged it. """ if _released_in(node.finalbody) >= required: return True - exiting = [h for h in node.handlers if _handler_exits(h)] - if not exiting: + if not node.handlers: return False - if not any(_is_baseexception_handler(h) for h in exiting): + if not any(_is_baseexception_handler(h) for h in node.handlers): # Nothing here catches a cancel; the window is still unprotected. return False - return all(_released_in(h.body) >= required for h in exiting) + return all( + _released_in(h.body) >= required for h in node.handlers if _handler_may_exit(h) + ) def _releases_on_the_way_out( diff --git a/tests/scripts/test_preyield_permit_release.py b/tests/scripts/test_preyield_permit_release.py index 9c3568ffb1e1d..4ec65511aef0c 100644 --- a/tests/scripts/test_preyield_permit_release.py +++ b/tests/scripts/test_preyield_permit_release.py @@ -637,7 +637,13 @@ def test_guard_does_not_demand_a_release_from_a_swallowing_arm(): """An arm that SWALLOWS falls through to the yield still holding the permit. Demanding a release there would be a false positive, and acting on it would - double-release — so only arms that exit (raise/return) owe the permit back. + double-release — so only arms that can exit owe the permit back. + + The arm body must be PROVABLY inert. This fixture used to hold + ``_LOG.warning("tolerated; still admitted")``, which is not: driven with a + benign logger that arm leaks 0, and with a bad format argument — identical + AST — it leaks 1. A fixture whose safety depends on runtime values cannot + state the property, so it is narrowed to statements that cannot leave. """ guard = _load_guard() source = textwrap.dedent( @@ -650,10 +656,355 @@ async def gate(sem, key): try: _LOG.info("admitted %s", key) except ValueError: - _LOG.warning("tolerated; still admitted") + tolerated = True + pass + except BaseException: + sem.release() + raise + try: + yield + finally: + sem.release() + """ + ) + _sites, violations = guard.scan_source(source, "new.py") + assert violations == [] + + +@pytest.mark.parametrize( + "shape,arm", + [ + ("a call that raises", "except BaseException:\n _reraise()\n"), + ("an assert", "except BaseException:\n assert surface\n"), + ("arithmetic", "except BaseException:\n x = 1 / surface\n"), + ("a logging call", 'except BaseException:\n _LOG.info("swallowed")\n'), + ("an attribute load", "except BaseException:\n x = surface.depth\n"), + ("a subscript", "except BaseException:\n x = surface[0]\n"), + ("an await", "except BaseException:\n await _drain()\n"), + # A bare NAME load raises UnboundLocalError when the binding has not + # run yet — an ordinary possibility inside an except arm, and a + # measured 1-permit leak. + ("a name load", "except BaseException:\n x = surface\n"), + # An assignment TARGET is not a value: a tuple/list target UNPACKS, + # which raises TypeError on a non-iterable and ValueError on an arity + # mismatch, even when every name in it is inert. + ("a tuple-target unpack", "except BaseException:\n a, b = 0\n"), + ("a starred tuple-target unpack", "except BaseException:\n a, *b = 0\n"), + ("a list-target unpack", "except BaseException:\n [a, b] = 0\n"), + ("an attribute target", "except BaseException:\n surface.x = 0\n"), + ("a subscript target", "except BaseException:\n surface[0] = 0\n"), + # Only a nested def's BODY is deferred; its signature runs now. + ( + "a nested def with a raising default", + "except BaseException:\n def _later(x=_boom()):\n pass\n", + ), + ( + "a nested def with a raising kw-only default", + "except BaseException:\n def _later(*, x=_boom()):\n pass\n", + ), + ( + "a nested def with a raising arg annotation", + "except BaseException:\n def _later(x: _boom()):\n pass\n", + ), + ( + "a nested def with a raising return annotation", + "except BaseException:\n def _later() -> _boom():\n pass\n", + ), + # Set/dict displays HASH at construction, so inert elements are not + # enough to make the display inert. + ("an unhashable set literal", "except BaseException:\n s = {[1]}\n"), + ("an unhashable dict key", "except BaseException:\n d = {[1]: 2}\n"), + # AnnAssign evaluates its annotation at runtime without PEP 563. + ("a raising annotation", "except BaseException:\n x: _boom() = 1\n"), + ], +) +def test_guard_flags_an_arm_that_can_leave_without_a_raise_statement(shape, arm): + """An arm leaves for many reasons that are not an ``ast.Raise`` node. + + Asking "does this arm contain a ``raise``/``return`` STATEMENT?" excused a + call that raises, an ``assert``, arithmetic that divides by zero, and a + plain logging call given a bad format argument. The first whitelist then + excused a second family: an unpacking assignment target, a nested ``def`` + whose SIGNATURE (defaults / annotations) is evaluated at definition time, a + set/dict display that hashes an unhashable element, and a bare name load + that can be an unbound local. Every shape here is a measured 1-permit + leak. The window check has always treated a call / attribute / subscript / + await as raise-capable; the arm check now agrees, and additionally refuses + to excuse anything whose targets or signature can raise. + """ + guard = _load_guard() + _sites, violations = guard.scan_source(_sibling_arm_source(arm), "new.py") + assert [v["function"] for v in violations] == ["gate"], ( + f"an arm that can leave via {shape} must be asked to release" + ) + + +@pytest.mark.parametrize( + "shape,arm", + [ + ("pass", "except BaseException:\n pass\n"), + ("a bare constant", 'except BaseException:\n "swallowed"\n'), + ("a constant assignment", "except BaseException:\n x = 0\n"), + ("a constant tuple value", "except BaseException:\n x = (1, 2)\n"), + ("a hashable set literal", "except BaseException:\n s = {1, 2}\n"), + ( + "a constant annotated assignment", + 'except BaseException:\n x: "int" = 0\n', + ), + ( + "a nested def", + "except BaseException:\n def _later():\n sem.release()\n", + ), + ( + "a nested def with a constant default", + "except BaseException:\n def _later(x=0):\n sem.release()\n", + ), + ], +) +def test_guard_still_excuses_a_provably_inert_arm(shape, arm): + """The other side: an arm that provably cannot leave still owes nothing. + + Without this the swallow exemption would collapse to "never excuse + anything" and the finding-3 false-positive fix would be undone. + """ + guard = _load_guard() + _sites, violations = guard.scan_source(_sibling_arm_source(arm), "new.py") + assert violations == [], f"an arm holding only {shape} cannot leave" + + +# -------------------------------------------------------------------------- +# 3b. Reachability: "can control reach the end of this arm?", not "is the last +# statement a bare raise/return?". Every shape below is a MEASURED +# 1-permit leak (real asyncio.Semaphore, synchronous fault at the pre-yield +# log) that was CLEAN on both the #860 guard and the #863 candidate. +# -------------------------------------------------------------------------- + + +def _sibling_arm_source(narrow_arm: str) -> str: + """A two-arm gate: a narrow arm under test + a correct BaseException arm. + + The sibling matters: with only the narrow arm the try has no + ``BaseException`` coverage and is flagged for that reason instead, so the + fixture would pass for the wrong reason. The correct sibling is what makes + the narrow arm's own classification the thing under test. + """ + return textwrap.dedent( + """ + from contextlib import asynccontextmanager, suppress + + @asynccontextmanager + async def gate(sem, surface): + await sem.acquire() + try: + _LOG.info("admitted %s", surface) + {arm} + except BaseException: + sem.release() + raise + try: + yield + finally: + sem.release() + """ + ).replace("{arm}", textwrap.indent(textwrap.dedent(narrow_arm), " ").rstrip()) + + +@pytest.mark.parametrize( + "shape,arm", + [ + ("if cond: raise", "except Boom:\n if surface:\n raise\n"), + ( + "raise inside an inner try/finally", + "except Boom:\n try:\n raise\n finally:\n pass\n", + ), + ( + "raise inside a with", + "except Boom:\n with suppress(ValueError):\n raise\n", + ), + ("raise inside a for", "except Boom:\n for _ in (1,):\n raise\n"), + ("if cond: return", "except Boom:\n if surface:\n return\n"), + ], +) +def test_guard_flags_a_conditionally_exiting_arm_that_does_not_release(shape, arm): + """FINDING 1: an arm that exits through a COMPOUND statement still owes. + + The old rule read ``reversed(handler.body)`` and returned True only on a + BARE trailing ``Raise``/``Return``, breaking on any other statement type. + So each shape here was classified as swallowing and excused from + releasing, while the correct ``BaseException`` sibling made the chain look + satisfied. All five are measured 1-permit leaks. + """ + guard = _load_guard() + _sites, violations = guard.scan_source(_sibling_arm_source(arm), "new.py") + assert [v["function"] for v in violations] == ["gate"], ( + f"an arm that exits via {shape} must be asked to release" + ) + + +def test_conditionally_exiting_arm_is_accepted_once_it_releases(): + """The discriminator's other side: releasing first clears the same shape. + + Argus's CONTROL I6 — measured NOT to leak — so the new rule must not flag + it, or the fixture above would pass by flagging everything. + """ + guard = _load_guard() + releasing = "except Boom:\n sem.release()\n if surface:\n raise\n" + _sites, violations = guard.scan_source(_sibling_arm_source(releasing), "new.py") + assert violations == [] + + +@pytest.mark.parametrize( + "shape,arm", + [ + ( + "nested def", + "except BaseException:\n def _later():\n sem.release()\n raise\n", + ), + ( + "nested def under a condition", + "except BaseException:\n if surface:\n def _later():\n sem.release()\n raise\n", + ), + ( + "lambda under a condition", + "except BaseException:\n if surface:\n _later = lambda: sem.release()\n raise\n", + ), + ( + "if False", + "except BaseException:\n if False:\n sem.release()\n raise\n", + ), + ( + "loop over an empty literal", + "except BaseException:\n for _ in []:\n sem.release()\n raise\n", + ), + ( + "after an unconditional raise", + "except BaseException:\n raise\n sem.release()\n", + ), + ], +) +def test_guard_does_not_credit_an_unreachable_release(shape, arm): + """FINDING 2: ``ast.walk`` counted a release that can never run. + + A release buried in a nested ``def`` (its own lifecycle), under + ``if False:``, or in the body of a loop over an empty literal is textually + present and never executes. All three are measured 1-permit leaks. + """ + guard = _load_guard() + source = textwrap.dedent( + """ + from contextlib import asynccontextmanager + + @asynccontextmanager + async def gate(sem, surface): + await sem.acquire() + try: + _LOG.info("admitted %s", surface) + {arm} + try: + yield + finally: + sem.release() + """ + ).replace("{arm}", textwrap.indent(textwrap.dedent(arm), " ").rstrip()) + _sites, violations = guard.scan_source(source, "new.py") + assert [v["function"] for v in violations] == ["gate"], ( + f"a release reachable only via {shape} must not satisfy the check" + ) + + +@pytest.mark.parametrize( + "shape,arm", + [ + ("under a runtime condition", "if surface:\n sem.release()\n"), + ("in a loop over a non-empty literal", "for _ in (1,):\n sem.release()\n"), + ("in a with body", "with suppress(ValueError):\n sem.release()\n"), + ], +) +def test_guard_still_credits_a_conditionally_reachable_release(shape, arm): + """The reachability narrowing must not reject releases that CAN run. + + Only statically-dead paths lose credit; anything the compiler cannot + settle stays creditable, or the finding-2 fixture would pass by rejecting + every non-trivial release. + """ + guard = _load_guard() + source = textwrap.dedent( + """ + from contextlib import asynccontextmanager, suppress + + @asynccontextmanager + async def gate(sem, surface): + await sem.acquire() + try: + _LOG.info("admitted %s", surface) except BaseException: + {arm} + raise + try: + yield + finally: + sem.release() + """ + ).replace("{arm}", textwrap.indent(textwrap.dedent(arm), " ").rstrip()) + # Two releases: the one under test in the handler, and the finally around + # the yield. If the fixture ever loses one, it is no longer the shape. + assert source.count("sem.release()") == 2 + _sites, violations = guard.scan_source(source, "new.py") + assert violations == [], f"a release {shape} is reachable and must count" + + +def test_guard_does_not_flag_a_sole_swallowing_baseexception_arm(): + """FINDING 3: ``except BaseException: pass`` as the ONLY arm is SAFE. + + The arm swallows and control proceeds to the yield still legitimately + holding the permit — measured NOT to leak. The old code asked for a + ``BaseException`` among the EXITING arms only, so a chain whose sole arm + swallows produced an empty exiting-set and was reported as unprotected, + contradicting the guard's own swallow rationale. + """ + guard = _load_guard() + source = textwrap.dedent( + """ + from contextlib import asynccontextmanager + + @asynccontextmanager + async def gate(sem, surface): + await sem.acquire() + try: + _LOG.info("admitted %s", surface) + except BaseException: + pass + try: + yield + finally: + sem.release() + """ + ) + _sites, violations = guard.scan_source(source, "new.py") + assert violations == [] + + +def test_guard_does_not_flag_a_narrow_releasing_arm_beside_a_swallowing_base(): + """FINDING 3 (H2): narrow arm releases+raises, BaseException swallows. + + Neither path leaks — the narrow arm hands the permit back before exiting, + and the BaseException arm falls through to the yield still holding it. + """ + guard = _load_guard() + source = textwrap.dedent( + """ + from contextlib import asynccontextmanager + + @asynccontextmanager + async def gate(sem, surface): + await sem.acquire() + try: + _LOG.info("admitted %s", surface) + except ValueError: sem.release() raise + except BaseException: + pass try: yield finally: @@ -664,6 +1015,34 @@ async def gate(sem, key): assert violations == [] +def test_swallowing_chain_still_needs_baseexception_coverage(): + """A swallow-only chain that does NOT cover BaseException is still a leak. + + ``except ValueError: pass`` catches nothing a cancel raises, so the window + remains unprotected — finding 3's relaxation must not extend to it. + """ + guard = _load_guard() + source = textwrap.dedent( + """ + from contextlib import asynccontextmanager + + @asynccontextmanager + async def gate(sem, surface): + await sem.acquire() + try: + _LOG.info("admitted %s", surface) + except ValueError: + pass + try: + yield + finally: + sem.release() + """ + ) + _sites, violations = guard.scan_source(source, "new.py") + assert [v["function"] for v in violations] == ["gate"] + + def test_guard_enumerates_both_permits_at_the_live_turn_admission_site(): """Positive coverage on the REAL multi-permit site, named not counted.""" guard = _load_guard()