diff --git a/CHANGELOG.md b/CHANGELOG.md index e73f0194..6ed4c46c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1232,6 +1232,56 @@ true until the next version shipped. ### Fixed +- `compare_to_bash.py` read the wrong argument for the four helpers whose name is not + last (#432, #1036). + + The previous fix replaced "the first quoted argument" with "the last argument". That + is true of 14 of `Expect`'s 18 helpers, but it is a property of most of them rather + than of the class, and the last argument is a real string in each of the other four -- + so a wrong name looked exactly like a right one: + + | call | last argument | the name it records | + | --- | --- | --- | + | `refusal(result, name, *patterns)` | a message PATTERN | `name`, argument 1 | + | `cannot_run(reason, detail="")` | the DETAIL of one run | `reason`, argument 0 | + | `plan_marker(plan, key, name=None)` | a plan KEY | the `name=` keyword only | + | `plan_node(plan, ..., name=None)` | a field of the NODE | the `name=` keyword only | + + `refusal` is the worst: the real name goes MISSING and a fragment of an error message + arrives as an EXTRA, which is two false entries from one call. + + Measured over every pair in the tree at `73e8e3d`, with the table as the only variable: + **68 extras, now 67.** Two were false -- a `plan_marker` key and a `cannot_run` detail, + both on `hilbert_locality` -- and `UNMET_PRECONDITION` appeared in their place, the + reason code `cannot_run` really records. No pair's verdict moved, because `rc` is driven + by MISSING and extras never moved it. That is why nothing caught this: the tool reported + a plausible list, and only the list was evidence either way. + + `UNMET_PRECONDITION` is reported as an extra only because the tool cannot see the bash + side of it. `hilbert_locality.sh:574` and three lines after it check that property with + `check_unrunnable`, which the bash extractor's `check(_num|_ratio|_text|_timing)?` does + not match. Widening it by that one alternative and nothing else takes that pair from + `rc=0 missing=0` to `rc=1 missing=2`, every other pair unchanged. Filed as #1040: it is a + port's worth of work, not a tool fix, and this change is only what made it visible. + + The positions live in a `_NAME_ARG` table, because the tool is deliberately standalone + (`ast`, `re`, `sys`) and importing `Expect` to ask would pull in pytest. A + hand-written derived value goes stale, so it is pinned: a drift guard reads the real + signatures out of `pgc_vacuity.py`, recomputes every entry, and fails with the helper + named. That guard first passed over a missing `refusal` entry -- `name` IS its last + DECLARED parameter, since `*patterns` is not -- and now accounts for the vararg. + + A second coincidence sat inside the clause that fixed the first, found by + @OffgridwithJD in review. `-1` is a claim about the CALL SITE while the guard reads the + SIGNATURE, and they agree only while no optional parameter sits after the name: + `expect.rows(got, want, "THE NAME", "the reason")` read `the reason`, and + `expect.plan_marker(plan, "key", "THE NAME")` read nothing at all, dropping a name + silently. Latent rather than live -- no call site passes a trailing optional + positionally -- but #1037 makes `allow_empty` a reason string, which is exactly that + argument. Closed in the signatures rather than in the reader: `rows`, `row_set`, + `plan_marker` and `plan_node` take everything after the name as keyword-only, so the + wrong call is now a `TypeError`. No call site changed; all four already used keywords. + - A BOGUS-verdict ledger record is refused by naming the verdict, not by field count (#1013). - The star-schema join how-to names clustering on the join key (#752). diff --git a/test/pytest/TESTS.md b/test/pytest/TESTS.md index 2501c763..bd565109 100644 --- a/test/pytest/TESTS.md +++ b/test/pytest/TESTS.md @@ -3508,7 +3508,8 @@ the tool was reporting the wrong string. | shape | read as | | --- | --- | -| the last string argument | the name | +| the last argument | the name, for the 14 helpers that put it there | +| the last argument of `refusal`, `cannot_run`, `plan_marker`, `plan_node` | NOT the name -- see below | | an f-string | a `{}` template, matched against bash interpolations reduced the same way | | `"a" if cond else "b"` | both arms | | `@pytest.mark.parametrize("func,name", ROWS)` | the `name` column, resolved through module constants | @@ -3518,6 +3519,144 @@ the tool was reporting the wrong string. template reducer missed every one of them, because its pattern required `[A-Za-z_]` after the dollar. +### The name is not always the last argument (#1036) + +The fix above replaced "the first quoted argument" with "the last argument", and that is +true of 14 of `Expect`'s 18 helpers. It is not a property of the helpers, only of most of +them, and the four exceptions were then read wrong in silence -- the last argument is a +real string in each case, so a wrong name looks exactly like a right one. + +| call | what the last argument is | the name it records | +| --- | --- | --- | +| `refusal(result, name, *patterns)` | a message PATTERN | `name`, argument 1 | +| `cannot_run(reason, detail="")` | the DETAIL of one run | `reason`, argument 0 | +| `plan_marker(plan, key, name=None)` | a plan KEY | the `name=` keyword only | +| `plan_node(plan, ..., name=None)` | a field of the NODE | the `name=` keyword only | + +`refusal` is the worst of the four: the name goes MISSING and a fragment of an error +message arrives as an EXTRA, so one call produces two false entries -- the same defect the +section above closes, one helper along. + +`plan_marker` and `plan_node` contribute NOTHING when called without `name=`. The key is +not the name even then, only a fragment of one (`plan_marker` records +`name or f"plan carries {key!r}"`), and reporting no name states MISSING rather than +inventing one. + +**Measured over the tree** at `73e8e3d`, with the table as the only variable (the count +is labelled with the tree because it moves as pairs are added): + +| pair | extras before | after | +| --- | --- | --- | +| hilbert_locality | 3 | 2 | +| every other pair | unchanged | unchanged | +| **total** | **68** | **67** | + +Two false extras went (`Columnar Projected Columns`, a `plan_marker` key; and `the two +partitions are not different ({})`, a `cannot_run` detail) and one appeared in their place: +`UNMET_PRECONDITION`, the reason code `cannot_run` actually records. No pair's verdict +moved, because `rc` is driven by MISSING and extras never moved it -- which is why nothing +caught this. + +**`UNMET_PRECONDITION` is an extra only because the tool cannot see the bash side of it**, +and saying otherwise would be the same mistake one level down. `hilbert_locality.sh:574` +and three lines after it DO check that property: + + check_unrunnable "box $box: groups read, Z-order" UNMET_PRECONDITION ... + +The bash extractor reads `check(_num|_ratio|_text|_timing)?`, and `check_unrunnable` +matches no branch of it. Widening that regex by that one alternative and changing nothing +else takes `hilbert_locality` from `rc=0 missing=0` to **`rc=1 missing=2`** -- `box $box: +groups read, Hilbert` and `box $box: groups read, Z-order` -- with every other pair +unchanged. The port emits ONE record named `UNMET_PRECONDITION` where bash emits four per +box, and two of them have no counterpart in the port at all. + +That gap is NOT caused by the change above; the change is what made it visible, and it is +filed as #1040 rather than widened here, because widening the regex reddens a pair and is +a port's worth of work rather than a tool fix. + +Derived from `test/lib.sh` rather than swept for, because three different sweeps gave +three different totals: **`lib.sh` defines 8 check helpers, the tool reads 5, and 3 are +invisible** -- `check_unrunnable`, `check_skip`, `check_ratio_needs_quiet_machine`. +Individual suites define four more of their own (`check_structure`, +`check_reconstruct`, `check_split_happened` in `parallel_copy.sh`, `check_float` in +`parquet_export_stats.sh`), invisible to the same regex. + +**50 invisible invocations over `test/*.sh`**, reconciled between two agents and two +independent methods, which agree helper for helper: `check_unrunnable` 25, `check_skip` 23, +`check_ratio_needs_quiet_machine` 2. + +**The population is half the number.** `test/*.sh` is the 265 top-level suites, which are +the only files the tool grades. Globbing `test/**/*.sh` instead adds the harness selftests +and gives **56**, the extra 6 all in `test/selftest/`, which `compare_to_bash.py` never +reads. Neither number is wrong; a number without its population is. + +`test/selftest/` is out of scope for a second reason as well: it is the SHELL harness's +own self-test, and the two harnesses stay independent, so counting it into a claim about +what the pytest parity tool grades would cross that line even if the tool could read it. + +**And `git grep` will not give you that population.** Git pathspecs are wildmatch without +`FNM_PATHNAME`, so `*` crosses `/` and the natural spelling is silently recursive: + + git grep -e check_unrunnable REV -- 'test/*.sh' 9 files, 4 under selftest/ + git grep -e check_unrunnable REV -- ':(glob)test/*.sh' 5 files, 0 under selftest/ + +Measuring the number at an older revision means reaching for `git grep`, where the +top-level spelling LOOKS right and is not. Use `:(glob)`. + +Getting there took four sweeps that read 89, 64, 54 and 50, and the three wrong ones were +not method-sensitivity -- they were two defects, both worth knowing because any later +re-derivation meets them: + +- **A `\bNAME\s` sweep counts each helper's own definition line.** `lib.sh:1231` is + `check_unrunnable() {# check_unrunnable NAME REASON_CODE DETAIL` -- the trailing + USAGE COMMENT repeats the name followed by a space, so the definition matches as though + it were a call. Same shape at `lib.sh:1407`. Two more matches were ordinary prose. That + is 89 (definitions included) and 54 (comments included). +- **A command-position match misses an invocation after `&&`.** `hilbert_curve.sh:321` is + `[ -n "$_a" ] && check_unrunnable "$_a" "$2" "$3"`. Anchoring on `^` alone gives 24 for + that helper rather than 25. + +Strip trailing comments as well as whole-line ones, exclude definitions, and accept a call +after `;`, `&&` or `||`, and the number is reproducible. + +`refusal` moved no pair either: it is used only by `test_raises_sqlstate.py` and +`test_guards_pinned.py`, neither of which has a bash twin. Its arm drives the real +extractor rather than a pair. + +### A second coincidence, inside the clause that fixed the first + +`-1` is a claim about the CALL SITE. The drift guard reads the SIGNATURE. They agree only +while no OPTIONAL parameter sits after the name, because an optional one can still be +passed positionally: + +| written | read as | +| --- | --- | +| `expect.rows(got, want, "THE NAME", "the reason")` | `the reason` | +| `expect.plan_marker(plan, "key", "THE NAME")` | nothing at all | + +Both were legal, both read wrong, and every guard here stayed green. The second is worse: +a DROPPED name reports the bash property MISSING, and MISSING is what drives `rc`. + +Latent rather than live -- no call site in the tree passes a trailing optional +positionally -- but #1037 makes `allow_empty` a reason STRING, which is exactly the +argument somebody writes positionally next to a name. + +**Closed in the signatures rather than patched in the reader.** `rows`, `row_set`, +`plan_marker` and `plan_node` now take everything after the name as keyword-only, so the +wrong call is a `TypeError` instead of a silently misread name: + + Expect.rows() takes 4 positional arguments but 5 were given + +`test_no_later_argument_can_overtake_the_name` holds it, and it is a signature fact, which +is what this guard is already good at reading. `cannot_run` needs no change: its name is +argument 0 and nothing after it can overtake it. + +**The table is a hand-written derived value, so it is pinned.** The tool is deliberately +standalone (`ast`, `re`, `sys`) and cannot import `Expect` to ask where each name sits. +`test_the_tools_table_agrees_with_the_signatures_it_describes` reads the real signatures +out of `pgc_vacuity.py`, recomputes every entry, and fails with the helper named when the +two disagree. + ### Removal proof | mutation | red | @@ -3526,6 +3665,13 @@ the dollar. | drop the conditional-name case | its own arm, and the whole-tree arm | | drop parametrize resolution | its own arm, and the whole-tree arm | | read the name column by position instead of by its declared name | its own arm, and the whole-tree arm | +| delete the `_NAME_ARG` table entirely | all four #1036 arms | +| drop the `refusal` entry | its own arm, and the drift guard | +| `plan_marker` `None` -> `-1`, taking the key | its own arm, and the drift guard | +| `cannot_run` `0` -> `-1`, taking the detail | its own arm | +| a wrong entry for a helper no BEHAVIOURAL arm covers (`at_least`) | the drift guard, and the whole-tree arm -- the four behavioural arms stay green, which is the point of it | +| add a helper to `Expect` whose name is not last | the drift guard, naming it | +| revert any one of the four `*` keyword-only markers | `test_no_later_argument_can_overtake_the_name`, naming the helper | `test_the_ported_suites_in_this_tree_are_graded_one_for_one` catches all four. It is the arm that matters: a guard over invented sources proves the extractor reads python, not that @@ -3542,4 +3688,10 @@ the tool grades THIS tree. | `test_a_parametrized_name_is_resolved_from_the_decorator` | the idiom a repeated bash property should be ported to, with a no-`name` decorator as the control | | `test_the_parametrize_reader_takes_the_column_called_name` | the declared column, not position | | `test_the_two_harnesses_interpolations_land_on_one_template` | bash and python spell interpolation differently and must meet | +| `test_refusal_names_its_second_argument_not_its_last_pattern` | the name is in the middle; the last argument is a pattern | +| `test_refusal_with_no_pattern_is_not_the_arm_that_proves_it` | the control: that shape reads the same under either rule, so it proves nothing alone | +| `test_cannot_run_names_its_reason_not_its_detail` | the only helper whose name is argument zero | +| `test_a_helper_whose_name_is_optional_takes_it_only_from_the_keyword` | `plan_marker` and `plan_node` carry no name positionally; absent beats a key | +| `test_the_tools_table_agrees_with_the_signatures_it_describes` | the drift guard: every entry re-derived from the real signatures | +| `test_no_later_argument_can_overtake_the_name` | nothing after the name may be passed positionally, so `-1` is true of every CALL and not just every signature | | `test_the_ported_suites_in_this_tree_are_graded_one_for_one` | the standing arm: every pair in the tree, graded | diff --git a/test/pytest/compare_to_bash.py b/test/pytest/compare_to_bash.py index f7f03178..69683c56 100755 --- a/test/pytest/compare_to_bash.py +++ b/test/pytest/compare_to_bash.py @@ -116,12 +116,40 @@ def test_a_role_with_only_schema_usage_is_refused(..., func, name): return out +# WHERE THE NAME SITS, for the helpers where it is not the last argument (#1036). +# +# The rule for most of `Expect` is "the name is the last argument", and for 14 of its 18 +# helpers that is true. It is not a property of the helpers, though, only of most of them, +# and the four below were read wrong in silence: the last argument is a real string in each +# case, so a wrong name looked exactly like a right one. +# +# refusal(result, name, *patterns) the last argument is a PATTERN +# cannot_run(reason, detail="") records `name=reason`, the FIRST argument +# plan_marker(plan, key, name=None) the last argument is a plan KEY +# plan_node(plan, ..., name=None) the last argument describes the NODE +# +# A value here is the index of the call argument carrying the name; `None` means no +# positional argument carries it and only a `name=` keyword can. `-1`, the default for +# every helper not listed, means the last one. +# +# This is a hand-written derived value, so it is pinned: the drift guard in +# `test_compare_to_bash.py` re-derives every entry from the real signatures in +# `pgc_vacuity.py` and fails with the helper named when the two disagree. Add a helper +# whose name is not last and that arm goes red before this table is wrong in the field. +_NAME_ARG = { + "refusal": 1, + "cannot_run": 0, + "plan_marker": None, + "plan_node": None, +} + + def _py_names(src): """Every assertion name in the port, by parsing rather than matching. The name is the LAST argument of an `expect.(...)` call, or the value of a `name=` keyword, read through `_as_names` so a conditional carries both of - its arms. + its arms. Four helpers put it somewhere else and are read through `_NAME_ARG`. """ tree = ast.parse(src) out = _parametrized_names(tree) @@ -137,7 +165,12 @@ def _py_names(src): out.extend(_as_names(kw.value)) if not is_expect or not node.args: continue - out.extend(_as_names(node.args[-1])) + idx = _NAME_ARG.get(func.attr, -1) + if idx is None: + continue + if idx != -1 and len(node.args) <= idx: + continue + out.extend(_as_names(node.args[idx])) return out diff --git a/test/pytest/expected_tests.txt b/test/pytest/expected_tests.txt index cfeb422e..9d4b63a7 100644 --- a/test/pytest/expected_tests.txt +++ b/test/pytest/expected_tests.txt @@ -64,7 +64,18 @@ # correctly against a tree holding only their own arms and both be wrong for the merge -- # measured by @jdatcmd on #1022, where two independent derivations of 280 had 283 as the # merged truth. Derived here by collection: `307 tests collected`. -guard_tests 307 +# 298 -> 303 when the same file gained five arms for the helpers whose name is not the +# last argument (#1036), four of them behavioural and one a drift guard over the table. +# Re-derived by collection, not by adding five: `303 tests collected`. +# 303 -> 304 when review found a second coincidence: `-1` is a claim about the CALL SITE +# and the drift guard reads the SIGNATURE. Re-derived by collection: `304 tests collected`. +# MERGED with #1035 (#982's six ledger arms), whose arms are disjoint from these. +# Re-derived by collection on the merged tree, NOT by adding the two deltas. +# MERGED with #1037 (#1031's three allow_empty arms). Its keyword-only change +# and this branch's REASON check are complementary, and pgc_vacuity.py kept both. +# Re-derived by collection on the merged tree: the two 307s and the 310 that +# preceded this are each right for a DIFFERENT tree and none of them is this one. +guard_tests 313 # The complement: tests that need the driver and a throwaway cluster. Until #1016 these ran # in no CI job at all -- a quarter of the corpus, green when somebody ran them by hand and diff --git a/test/pytest/pgc_vacuity.py b/test/pytest/pgc_vacuity.py index 8fbf5136..79b7983a 100644 --- a/test/pytest/pgc_vacuity.py +++ b/test/pytest/pgc_vacuity.py @@ -438,7 +438,7 @@ def num(self, got, want, name): if got != want: raise AssertionError(f"{name}: got {got!r} want {want!r}") - def row_set(self, got, want, name, allow_empty=None): + def row_set(self, got, want, name, *, allow_empty=None): """Compare two result sets as SETS, order deliberately ignored. The counterpart to ordered_rows, and the port of pgc_set_hash. It exists so @@ -645,7 +645,7 @@ def wrote(self, cur, want, name): # -- row sets ---------------------------------------------------------- @_resolving - def rows(self, got, want, name, allow_empty=None): + def rows(self, got, want, name, *, allow_empty=None): """Compare two result sets. Refuses two empty sides unless declared. Both sides empty is issue #418: it passes while asserting nothing, because @@ -781,7 +781,7 @@ def sqlstate(self, exc, want, name): # -- plans ------------------------------------------------------------- @_resolving - def plan_node(self, plan, node_type=None, provider=None, name=None): + def plan_node(self, plan, *, node_type=None, provider=None, name=None): """Assert a node exists, by EXACT equality on a typed EXPLAIN JSON field. `EXPLAIN (FORMAT JSON)` arrives from psycopg as parsed Python, so there is @@ -925,7 +925,7 @@ def run_failed(self, result, name): ) @_resolving - def plan_marker(self, plan, key, name=None, absent=False): + def plan_marker(self, plan, key, *, name=None, absent=False): """Assert a plan node carries (or does not carry) a Columnar property KEY. This is the faithful port of `pgc_is_columnar_scan` (`lib.sh`), which greps diff --git a/test/pytest/test_compare_to_bash.py b/test/pytest/test_compare_to_bash.py index 25ee5abd..dd8fd653 100644 --- a/test/pytest/test_compare_to_bash.py +++ b/test/pytest/test_compare_to_bash.py @@ -160,6 +160,210 @@ def test_the_two_harnesses_interpolations_land_on_one_template(expect): "a bare variable reference too") +def test_refusal_names_its_second_argument_not_its_last_pattern(expect): + """`refusal(result, name, *patterns)` puts the name in the MIDDLE. + + The last argument is a pattern -- a fragment of the message the refusal must carry -- + so the last-argument rule read a substring of an error message as the property's name. + The real name went MISSING and the pattern arrived as an EXTRA: two false entries from + one call, which is the same defect this file exists to close, one helper along. + """ + src = ('def t(expect):\n' + ' expect.refusal(result, "a role with no privilege is refused",\n' + ' "permission denied", "for table")\n') + got = _names(src) + expect.text(", ".join(sorted(got)), "a role with no privilege is refused", + "the name is read and neither pattern is") + expect.num(len(got), 1, "one call contributes exactly one name") + + +def test_refusal_with_no_pattern_is_not_the_arm_that_proves_it(expect): + """THE CONTROL that keeps the arm above honest. + + `expect.refusal(result, NAME)` has the name last, so it is read correctly by the rule + this change replaces AND by the rule that replaces it. An arm built only on that shape + would pass against the defect, which is how the shape got missed in the first place. + """ + src = 'def t(expect):\n expect.refusal(result, "the write is refused")\n' + expect.text(", ".join(_names(src)), "the write is refused", + "the no-pattern shape reads the same either way, so it proves nothing alone") + + +def test_cannot_run_names_its_reason_not_its_detail(expect): + """`cannot_run(reason, detail="")` records `name=reason`: the FIRST argument. + + It is the only helper whose name is argument zero, and the detail beside it is prose + about one run -- "the two partitions are not different ({})" -- which can never match + a bash check name. Reading it produced an extra that no bash suite could ever satisfy. + """ + src = ('def t(expect):\n' + ' expect.cannot_run("MISSING_DEPENDENCY",\n' + ' "the two partitions are not different")\n') + got = _names(src) + expect.text(", ".join(got), "MISSING_DEPENDENCY", + "the reason CODE is the name, and the detail is not a name at all") + expect.num(len(got), 1, "the detail contributes nothing") + + +def test_a_helper_whose_name_is_optional_takes_it_only_from_the_keyword(expect): + """`plan_marker` and `plan_node` carry no name positionally. Better absent than wrong. + + `plan_marker(plan, key, name=None)` records `name or f"plan carries {key!r}"`, so the + KEY is not the name even when no name is given -- it is a fragment of one. The + last-argument rule emitted the bare key as a name the bash suite does not have, and + `Columnar Projected Columns` duly appeared as an extra on a pair that is complete. + + With no name= the call contributes NOTHING, which reports MISSING rather than inventing + a name: the rule this file already applies to a name it cannot read. + """ + named = _names('def t(expect):\n' + ' expect.plan_marker(plan, "Columnar Projected Columns",\n' + ' name="the plan projects two columns")\n') + expect.text(", ".join(named), "the plan projects two columns", + "the name= keyword is the name, and the key is not also collected") + expect.num(len(named), 1, "one call, one name -- the key is not a second entry") + + bare = _names('def t(expect):\n' + ' expect.plan_marker(plan, "Columnar Projected Columns")\n') + expect.num(len(bare), 0, "with no name= the key is still not a name") + + node = _names('def t(expect):\n' + ' expect.plan_node(plan, provider="columnar",\n' + ' name="the scan is columnar")\n') + expect.text(", ".join(node), "the scan is columnar", + "plan_node reads its name= and not the arguments describing the node") + + +def test_the_tools_table_agrees_with_the_signatures_it_describes(expect): + """THE DRIFT GUARD, and the reason the table is allowed to be a hand-written map. + + `compare_to_bash.py` is deliberately standalone -- `ast`, `re`, `sys` -- so it cannot + import `Expect` to ask where each name sits, and a hand-written table is a derived + value that goes stale the day somebody adds a helper. This arm is what stops that: it + reads the REAL signatures out of `pgc_vacuity.py` and recomputes, for every public + helper, which call argument carries the name the helper records. + + It is not a copy of the table. The table says where to look; this derives where to look + from the source of truth and compares. A helper added with its name anywhere but last, + or a signature reordered, fails here with the helper named. + """ + from compare_to_bash import _NAME_ARG + + # The ONE thing a signature cannot state: which parameter becomes the record's name. + # `cannot_run` records `name=reason`; every other helper calls its parameter `name`. + # Pinned below against the body, so this line cannot quietly become wrong either. + records_name_as = {"cannot_run": "reason"} + + src = (HERE / "pgc_vacuity.py").read_text() + tree = ast.parse(src) + klass = [n for n in ast.walk(tree) + if isinstance(n, ast.ClassDef) and n.name == "Expect"] + expect.num(len(klass), 1, "premise: exactly one Expect class to read") + + helpers = [f for f in klass[0].body + if isinstance(f, ast.FunctionDef) and not f.name.startswith("_")] + # `records` and `count` take no arguments and record no name. + helpers = [f for f in helpers if [a.arg for a in f.args.args if a.arg != "self"]] + expect.at_least(len(helpers), 15, + "premise: the Expect helpers were found, not an empty list") + + disagree, checked = [], 0 + for f in helpers: + params = [a.arg for a in f.args.args if a.arg != "self"] + ndef = len(f.args.defaults) + required = params[:len(params) - ndef] if ndef else params + param = records_name_as.get(f.name, "name") + # Optional => no positional carries it; only a `name=` keyword can. + want = required.index(param) if param in required else None + got = _NAME_ARG.get(f.name, -1) + # -1 is "the last positional". That equals the name's own index only when the + # name really is last AT THE CALL SITE, and a `*args` AFTER it means it is not: + # `refusal(result, name, *patterns)` declares `name` last and is still called + # with patterns beyond it. Without that clause this arm accepted a missing + # `refusal` entry, which is the very shape it is here to catch. + if (got == -1 and want is not None and want == len(required) - 1 + and f.args.vararg is None): + got = want + checked += 1 + if got != want: + disagree.append(f"{f.name}: table says {got!r}, signature says {want!r}") + + expect.num(checked, len(helpers), "inputs == sum(buckets): every helper was compared") + expect.text("; ".join(disagree) or "none", "none", + "every entry in the table matches the signature it describes") + + # The one hand-written semantic claim above, pinned against the body it describes: + # read cannot_run's own `_record(...)` call and check which parameter it names. + fn = [f for f in helpers if f.name == "cannot_run"] + expect.num(len(fn), 1, "premise: cannot_run is among the helpers read") + recorded = [kw.value.id for call in ast.walk(fn[0]) + if isinstance(call, ast.Call) + and isinstance(call.func, ast.Attribute) and call.func.attr == "_record" + for kw in call.keywords + if kw.arg == "name" and isinstance(kw.value, ast.Name)] + expect.text(", ".join(recorded), "reason", + "cannot_run really does record its reason as the name") + +def test_no_later_argument_can_overtake_the_name(expect): + """`-1` is a claim about the CALL SITE, and the arm above only reads the SIGNATURE. + + Found by @OffgridwithJD reviewing the change this file documents, inside the very + clause that fixed the vararg coincidence. The guard asks "which parameter carries the + name", which is a fact about the declaration. `-1` says "the last argument", which is + a fact about the call. They agree only while no OPTIONAL parameter sits after the + name, because an optional one may still be passed POSITIONALLY: + + expect.rows(got, want, "THE NAME", "the reason") -> read 'the reason' + expect.plan_marker(plan, "key", "THE NAME") -> read nothing at all + + Both were legal, both read wrong, and every guard in this file stayed green. The + second is the worse one: a DROPPED name reports the bash property MISSING, and + MISSING is what drives `rc`. + + Latent rather than live -- no call site in the tree passes a trailing optional + positionally -- but #1037 makes `allow_empty` a reason STRING, which is exactly the + argument somebody writes positionally next to a name. + + So the property is closed in the SIGNATURES rather than patched in the reader: every + parameter after the name is keyword-only, and this arm holds that. A wrong call is + then a `TypeError`, not a silently misread name. + """ + src = (HERE / "pgc_vacuity.py").read_text() + klass = [n for n in ast.walk(ast.parse(src)) + if isinstance(n, ast.ClassDef) and n.name == "Expect"] + expect.num(len(klass), 1, "premise: exactly one Expect class to read") + helpers = [f for f in klass[0].body + if isinstance(f, ast.FunctionDef) and not f.name.startswith("_") + and [a.arg for a in f.args.args if a.arg != "self"]] + expect.at_least(len(helpers), 15, "premise: the helpers were found, not an empty list") + + from compare_to_bash import _NAME_ARG + + overtakable, checked = [], 0 + for f in helpers: + params = [a.arg for a in f.args.args if a.arg != "self"] + checked += 1 + if "name" not in params: + # Carried only as a keyword, or named something else (`cannot_run`, whose + # name is argument 0 and cannot be overtaken by anything after it). + continue + if _NAME_ARG.get(f.name, -1) is None: + # The table says NO positional argument carries the name, so the reader + # skips the call entirely. If `name` can still be written positionally the + # name is DROPPED, which reports the bash property MISSING and moves `rc`. + overtakable.append(f"{f.name}: the table reads no positional name, yet name " + f"can be passed positionally") + continue + after = params[params.index("name") + 1:] + if after: + overtakable.append(f"{f.name}: {', '.join(after)} can be passed positionally " + f"after name") + + expect.num(checked, len(helpers), "inputs == sum(buckets): every helper was examined") + expect.text("; ".join(overtakable) or "none", "none", + "no positional argument can be written after the name and be read as it") + + def test_the_ported_suites_in_this_tree_are_graded_one_for_one(expect): """THE STANDING ARM, and the reason this file is not only about fixtures.