diff --git a/CHANGELOG.md b/CHANGELOG.md index da78edaa..ccf5f590 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1268,6 +1268,19 @@ true until the next version shipped. now re-records. It names the two cases that remain: a declaration that no longer resolves, and the implicit base projection, which is not readable by name at all. +- A `conftest.py` can no longer switch off the order-collapse guard by rebinding + the module-level name it used to read (#924). + + The scan looked up `_ORDER_KILLERS` on each call. A conftest is imported before + collection, so `pgc_vacuity._ORDER_KILLERS = ()` turned the refusal off for every + test in that directory, with no reason recorded. Two lines, less to type than + the honest form. Measured: the same collapse test was uncollectable with no extra + file, and reported `1 passed` with only that rebind. + + The killer names are bound at definition time, in a default argument, the same + way `query_error` already binds its prefix. There is no module-level name left + to rebind. `_RECORDERS` is a registry the layer writes, not rule data, and is + unchanged. - The pytest harness no longer reports results against a library another process installed (#956). diff --git a/test/pytest/TESTS.md b/test/pytest/TESTS.md index b14e6d85..a77278c6 100644 --- a/test/pytest/TESTS.md +++ b/test/pytest/TESTS.md @@ -1114,6 +1114,7 @@ tests port that pair and its premise check. | `test_ordered_rows_fails_on_the_wrong_order` | the oracle detects order | proves it can fail, not merely that it permits | | `test_layer_refuses_sorting_the_input_to_an_ordered_claim` | `sorted()` feeding `ordered_rows` is uncollectable, found by AST | `ordered_rows(sorted(got), sorted(want))` cannot fail on order | | `test_layer_refuses_a_name_bound_to_a_sorted_call` | `g = sorted(got)` one line above the claim is the same collapse | the inline spelling was the only one caught, so the guard was blind to the version least likely to be noticed | +| `test_a_conftest_cannot_switch_off_the_order_collapse_guard` | a conftest rebinding `_ORDER_KILLERS` cannot silence the scan | two lines of conftest is less to type than the honest form, and that is the hatch the layer forbids | | `test_layer_refuses_a_list_sorted_in_place` | `got.sort()` kills the order and leaves the name spelled the same | nothing at the call site says anything happened | | `test_layer_allows_a_name_sorted_after_the_claim` | **control** | a name sorted AFTER the claim did not affect it; refusing that would be a false red | | `test_the_order_killer_scan_is_one_function_deep` | **pinned limit** | a sort behind a helper is not caught, and this arm reddens if that documented limit ever moves | diff --git a/test/pytest/pgc_vacuity.py b/test/pytest/pgc_vacuity.py index 1b63090e..99598ad6 100644 --- a/test/pytest/pgc_vacuity.py +++ b/test/pytest/pgc_vacuity.py @@ -1171,85 +1171,106 @@ def pytest_collection_finish(session): # sorted() to reach for. # # Parsed, not grepped, for the same reason as the except scan below. -_ORDER_KILLERS = ("sorted", "set", "frozenset") - - -def _order_killed_names(fn): - """Names bound to an order-killing value earlier in one function body. - - -> {name: (lineno, how)} +# THE KILLER LIST IS BOUND AT DEFINITION TIME, in a default argument, and that +# is the whole mechanism rather than a style choice (#924). +# +# `_ORDER_KILLERS` used to live as a module-level name. A conftest imported +# before collection rebound it to () and the order-collapse refusal stopped +# firing for every test in that directory, with no reason recorded. Two lines, +# less to type than the honest form, which is the hatch the layer's own +# false-positive budget forbids. Measured on main: the same collapse test was +# uncollectable with no extra file, and reported `1 passed` with only +# +# import pgc_vacuity +# pgc_vacuity._ORDER_KILLERS = () +# +# The layer already closed this shape for QUERY_ERROR. The same default-argument +# bind is used here: the tuple is evaluated once, when the factory is defined, +# and is not read from the module namespace afterwards. There is no module-level +# name left to rebind. +def _order_guard(_killers=("sorted", "set", "frozenset")): + def _order_killed_names(fn): + """Names bound to an order-killing value earlier in one function body. + + -> {name: (lineno, how)} + + The inline spelling is only the shortest way to write the collapse. These two + are the same defect and read as more careful code, which is worse: + + g = sorted(got) # bound to an order-killing call + expect.ordered_rows(g, want) + + got.sort() # killed in place + expect.ordered_rows(got, want) + + WHAT THIS DOES NOT SEE, stated because a guard's blind spots are part of its + meaning: it is one function deep, so a helper that sorts and returns is invisible; + it does not follow aliases (`h = g`), attributes (`self.rows.sort()`), branches, + or a name re-bound to something honest after being killed. It is a floor, not a + proof of order-sensitivity. The suite's own removal proofs are what establish + that an ordered claim can actually fail on order. + """ + killed = {} + for node in ast.walk(fn): + # X = sorted(...) / set(...) / frozenset(...) + if isinstance(node, ast.Assign) and isinstance(node.value, ast.Call): + f = node.value.func + if isinstance(f, ast.Name) and f.id in _killers: + for t in node.targets: + if isinstance(t, ast.Name): + killed.setdefault(t.id, (node.lineno, f"{f.id}()")) + # X.sort() -- in place, and the name keeps its spelling at the call site + elif isinstance(node, ast.Expr) and isinstance(node.value, ast.Call): + f = node.value.func + if (isinstance(f, ast.Attribute) and f.attr == "sort" + and isinstance(f.value, ast.Name)): + killed.setdefault(f.value.id, (node.lineno, ".sort()")) + return killed + + + def _sorted_ordered_sites(path): + try: + tree = ast.parse(pathlib.Path(path).read_text()) + except (OSError, SyntaxError): + return [] + out = [] + name = pathlib.Path(path).name + # Per function, because a killed name means nothing outside the body that + # killed it, and a module-level walk would carry one test's `g` into the next. + fns = [n for n in ast.walk(tree) + if isinstance(n, (ast.FunctionDef, ast.AsyncFunctionDef))] + for fn in fns: + killed = _order_killed_names(fn) + for node in ast.walk(fn): + if not isinstance(node, ast.Call): + continue + f = node.func + if not (isinstance(f, ast.Attribute) and f.attr in ("ordered_rows", + "ordering_observable")): + continue + for arg in node.args: + if (isinstance(arg, ast.Call) and isinstance(arg.func, ast.Name) + and arg.func.id in _killers): + out.append( + f"{name}:{node.lineno} {arg.func.id}() feeds an ordered claim" + ) + elif isinstance(arg, ast.Name) and arg.id in killed: + where, how = killed[arg.id] + # Only a kill that already happened. A name sorted AFTER the + # claim was made did not affect it, and flagging that would be + # a false red -- the thing this whole layer exists to refuse. + if where < node.lineno: + out.append( + f"{name}:{node.lineno} {arg.id} was order-killed by " + f"{how} at line {where} and feeds an ordered claim" + ) + return out - The inline spelling is only the shortest way to write the collapse. These two - are the same defect and read as more careful code, which is worse: - g = sorted(got) # bound to an order-killing call - expect.ordered_rows(g, want) + return _order_killed_names, _sorted_ordered_sites - got.sort() # killed in place - expect.ordered_rows(got, want) - WHAT THIS DOES NOT SEE, stated because a guard's blind spots are part of its - meaning: it is one function deep, so a helper that sorts and returns is invisible; - it does not follow aliases (`h = g`), attributes (`self.rows.sort()`), branches, - or a name re-bound to something honest after being killed. It is a floor, not a - proof of order-sensitivity. The suite's own removal proofs are what establish - that an ordered claim can actually fail on order. - """ - killed = {} - for node in ast.walk(fn): - # X = sorted(...) / set(...) / frozenset(...) - if isinstance(node, ast.Assign) and isinstance(node.value, ast.Call): - f = node.value.func - if isinstance(f, ast.Name) and f.id in _ORDER_KILLERS: - for t in node.targets: - if isinstance(t, ast.Name): - killed.setdefault(t.id, (node.lineno, f"{f.id}()")) - # X.sort() -- in place, and the name keeps its spelling at the call site - elif isinstance(node, ast.Expr) and isinstance(node.value, ast.Call): - f = node.value.func - if (isinstance(f, ast.Attribute) and f.attr == "sort" - and isinstance(f.value, ast.Name)): - killed.setdefault(f.value.id, (node.lineno, ".sort()")) - return killed - - -def _sorted_ordered_sites(path): - try: - tree = ast.parse(pathlib.Path(path).read_text()) - except (OSError, SyntaxError): - return [] - out = [] - name = pathlib.Path(path).name - # Per function, because a killed name means nothing outside the body that - # killed it, and a module-level walk would carry one test's `g` into the next. - fns = [n for n in ast.walk(tree) - if isinstance(n, (ast.FunctionDef, ast.AsyncFunctionDef))] - for fn in fns: - killed = _order_killed_names(fn) - for node in ast.walk(fn): - if not isinstance(node, ast.Call): - continue - f = node.func - if not (isinstance(f, ast.Attribute) and f.attr in ("ordered_rows", - "ordering_observable")): - continue - for arg in node.args: - if (isinstance(arg, ast.Call) and isinstance(arg.func, ast.Name) - and arg.func.id in _ORDER_KILLERS): - out.append( - f"{name}:{node.lineno} {arg.func.id}() feeds an ordered claim" - ) - elif isinstance(arg, ast.Name) and arg.id in killed: - where, how = killed[arg.id] - # Only a kill that already happened. A name sorted AFTER the - # claim was made did not affect it, and flagging that would be - # a false red -- the thing this whole layer exists to refuse. - if where < node.lineno: - out.append( - f"{name}:{node.lineno} {arg.id} was order-killed by " - f"{how} at line {where} and feeds an ordered claim" - ) - return out +_order_killed_names, _sorted_ordered_sites = _order_guard() def _broad_except_sites(path): diff --git a/test/pytest/test_ordered.py b/test/pytest/test_ordered.py index 196ae9ec..bcf07749 100644 --- a/test/pytest/test_ordered.py +++ b/test/pytest/test_ordered.py @@ -137,6 +137,43 @@ def test_sorted_afterwards(expect): passed=1, failed=0) +def test_a_conftest_cannot_switch_off_the_order_collapse_guard(pytester, expect): + """#924. The scan reads `_ORDER_KILLERS` from the module, and a conftest + is imported before collection, so two lines switch the guard off. + + Measured on main: the same collapse test is uncollectable with no extra + file, and reports `1 passed` when the only extra file is + + import pgc_vacuity + pgc_vacuity._ORDER_KILLERS = () + + That is less to type than the honest form, and the run records no reason. + The layer already closed this shape for QUERY_ERROR by binding the prefix + at definition time; the killer list was still a module-level name. + """ + pytester.makepyfile( + """ + def test_order_collapsed(expect): + got = ["b", "a"] + g = sorted(got) + expect.ordered_rows(g, ["a", "b"], "rows in order") + """ + ) + plain = pytester.runpytest("-p", "pgc_vacuity") + expect.run_failed(plain, "premise: the collapse is refused when nobody rebinds") + plain.stderr.fnmatch_lines(["*order-killed*"]) + + pytester.makeconftest( + """ + import pgc_vacuity + pgc_vacuity._ORDER_KILLERS = () + """ + ) + hatched = pytester.runpytest("-p", "pgc_vacuity") + expect.run_failed(hatched, "and it is still refused after a conftest rebinds the name") + hatched.stderr.fnmatch_lines(["*order-killed*"]) + + def test_the_order_killer_scan_is_one_function_deep(pytester, expect): """A named limit, pinned so it cannot quietly become a claim of completeness.