test: read a suite's own recorders, and refuse the one it cannot (#1053)

CHANGELOG.md

@@ -58,6 +58,42 @@ true until the next version shipped.
 
   `test/pytest/TESTS.md` also described the ledger as FIVE tab-separated columns and
   omitted `majors` from the list, from the day that column landed (#1010) until now.
+- `compare_to_bash.py` could not read a name a suite passed through its OWN wrapper,
+  and published a bare `{}` in its place (#1053).
+
+  #1051 taught the extractor the recorders `lib.sh` shares. A suite may also define
+  its own, and there are two shapes, only one of which is a gap:
+
+      COMPOSE   check "non-owner refused: ${1%%(*}"     the definition states a
+                                                        TEMPLATE naming the property
+      FORWARD   check_text "$label" "$got" "$want"      the definition states nothing
+
+  A composing wrapper is already read correctly -- `non-owner refused: {}` covers all
+  nine of `native_ownership`'s call sites, which is why that pair grades one-for-one.
+  A forwarding wrapper's definition yields the bare template `{}`, and 17 of those
+  were being published: a "property" with no content, sitting in MISSING where no port
+  can ever assert it, and MATCHING a port name that is entirely one interpolation.
+
+  The grader now derives each suite's own recorders by the rule that already works for
+  `lib.sh` -- a function forwarding a bare positional into a known recorder's name
+  slot, transitively, seeded from `pgc_record` -- reads the call sites of the
+  forwarding ones, drops the bare `{}`, and leaves composers alone. 145 names across
+  14 suites become readable. `sorted_pathkeys` alone gains 18, and they are not a
+  random 18: that suite pairs every "plans no Sort" with an "and still answers
+  correctly", so the grader could see every claim about the PLAN and none about the
+  ANSWER.
+
+  AND IT REFUSES what it cannot read. `hilbert_curve.sh` defines two helpers taking a
+  newline-separated LIST of names in one argument, so no rule about argument positions
+  can read them; the grader now exits 2 naming both rather than grading the rest. One
+  suite of 264, a true positive, with no pytest twin.
+
+  MEASURED BEFORE BUILDING, and it changed the design: refusing on "the name position
+  is not a bare positional" also refuses every COMPOSING wrapper -- 32 suites,
+  including `hilbert_cluster`, `hilbert_locality` and `native_ownership`, three pairs
+  that are COMPLETE today -- to fix nothing. Every graded pair is unchanged by what
+  shipped.
+
 - `iceberg_fdw.sh` is ported to pytest: the FDW's partition and metrics pruning
   (#388, #432).
 

test/pytest/TESTS.md

@@ -3854,6 +3854,13 @@ the tool grades THIS tree.
 | `test_the_derivation_finds_a_wrapper_planted_in_a_fixture` | the derivation on a fixture where the answer is known: four forwarding shapes found, and a function owning its own literal name rejected |
 | `test_the_comment_stripper_keeps_a_parameter_expansion` | `${shape#*|}` and `$#` are not comments; `#` opens one only at a word boundary |
 | `test_an_empty_helper_group_fabricates_names_rather_than_reading_none` | an empty alternation matches everywhere, so a position with no helper would invent `$PGC_DB` as a check name rather than read none |
+| `test_a_suites_own_forwarding_wrapper_is_read` | a suite's own wrapper that forwards a bare positional has its names read from the CALL SITES |
+| `test_a_composing_wrapper_is_left_alone` | a wrapper that COMPOSES its name already states a template; refusing it would break three COMPLETE pairs |
+| `test_a_helper_whose_name_cannot_be_resolved_is_refused` | a helper taking a LIST of names in one argument is refused by name, not skipped |
+| `test_the_refusal_names_exactly_the_suites_it_refuses` | the refused SET is pinned by name, not counted; both directions, so an entry cannot outlive its cause |
+| `test_a_bare_interpolation_is_not_published_as_a_name` | a forwarding wrapper's `{}` is dropped: it names nothing and can match a wholly-interpolated port name |
+| `test_the_grader_itself_refuses_the_suite_it_cannot_read` | `main` exits 2 and prints no verdict, with a readable suite as the control |
+| `test_a_helper_reaching_only_the_primitive_is_found` | the closure is seeded from `pgc_record`, not the `check` family; four suites turn on it |
 | `test_a_longer_helper_name_is_not_shadowed_by_a_shorter_one` | `check_ratio` must not eat `check_ratio_needs_quiet_machine` |
 | `test_the_suite_local_helpers_are_known_and_excluded` | the four suite-local helpers, and that none of their suites is graded |
 | `test_every_pair_in_the_tree_is_declared` | the declaration is asserted BOTH ways, so a new pair cannot be silently ungraded |

test/pytest/compare_to_bash.py

@@ -250,6 +250,12 @@ def _as_names(node):
 # Hand-written so the tool stays standalone, and pinned like `_NAME_ARG`: the drift
 # guard in `test_compare_to_bash.py` DERIVES this table from `lib.sh` -- membership
 # and position both -- and fails with the helper named when the two disagree.
+# The primitive every recorder reaches, and the argument IT names its check in. The
+# seed of the closure below; named rather than inlined so an arm can assert that no
+# suite calls it directly.
+_RECORD_PRIMITIVE = "pgc_record"
+_RECORD_NAME_ARG = 2
+
 _BASH_NAME_ARG = {
     "check_ratio_needs_quiet_machine": 1,
     "check_unrunnable": 1,
@@ -271,6 +277,115 @@ def _as_names(node):
 _BASH_HELPERS = tuple(_BASH_NAME_ARG)
 
 
+def _strip_comments(text):
+    r"""-> the text with shell comments removed, and NOTHING else removed.
+
+    `#` starts a comment only at a word boundary. `${shape#*|}` and `$#` are not
+    comments, and cutting at the first `#` truncates the line to something that
+    parses as a different program. That exact slip has produced two wrong counts in
+    this repo, so the fixtures for it are in the arm below rather than in a comment.
+    """
+    out = []
+    for line in text.splitlines():
+        res, i, quote = [], 0, None
+        while i < len(line):
+            ch = line[i]
+            if quote:
+                if ch == quote:
+                    quote = None
+                res.append(ch)
+            elif ch in "\"'":
+                quote = ch
+                res.append(ch)
+            elif ch == "#" and (i == 0 or line[i - 1] in " \t;&|()"):
+                break
+            else:
+                res.append(ch)
+            i += 1
+        out.append("".join(res))
+    return "\n".join(out)
+
+
+def _bodies(text):
+    """-> [(function name, body)] with each body ended by ITS OWN closing brace.
+
+    Per-line brace depth, not `find("\n}")`: 199 definitions in this tree are written
+    on one line (`q() { psql ...; }`), and a scan for a brace in the first column
+    swallows every following definition into the first one's body.
+    """
+    out, lines = [], text.splitlines()
+    for i, line in enumerate(lines):
+        m = re.match(r"[ \t]*([A-Za-z_][A-Za-z0-9_]*)[ \t]*\(\)[ \t]*\{", line)
+        if not m:
+            continue
+        depth = line.count("{") - line.count("}")
+        body, j = [line[m.end():]], i + 1
+        while j < len(lines) and depth > 0:
+            depth += lines[j].count("{") - lines[j].count("}")
+            body.append(lines[j])
+            j += 1
+        out.append((m.group(1), "\n".join(body)))
+    return out
+
+
+def _words(text):
+    """-> the shell words of a call's argument list, quotes kept."""
+    return re.findall(r'"[^"]*"|\S+', text)
+
+
+def _derive_recorders(lib, seed=("pgc_record", 2)):
+    """-> {helper: which argument holds the check name}, derived from what lib.sh DOES.
+
+    THE POPULATION IS THE POINT (#1045). The #1040 guard derived its population by
+    SPELLING -- every `lib.sh` function whose name begins `check`. It was green for
+    weeks while `diff_query` went unread, and correctly so: `diff_query` was never in
+    its population. The guard was not broken; the definition of the thing it guards
+    was. 225 names across 59 suites were outside it.
+
+    So: start from `pgc_record`, the primitive that actually records, and take the
+    closure. A function is a recorder at position N when it passes its own `$N` --
+    directly, or renamed once through a `local` -- into the name slot of a helper
+    already known to be one. `diff_query` calls `check`, which calls `pgc_record`.
+    One level of indirection was the entire gap.
+
+    The seed is not returned. It is `lib.sh`'s own primitive and no suite calls it,
+    which the arm below asserts rather than assumes: the day a suite calls it, the
+    extractor has to learn it and this stops being true quietly.
+    """
+    lib = _strip_comments(lib)
+    defs = _bodies(lib)
+    known = {seed[0]: seed[1]}
+
+    changed = True
+    while changed:
+        changed = False
+        for fn, body in defs:
+            if fn in known:
+                continue
+            aliases = {m.group(1): int(m.group(2)) for m in
+                       re.finditer(r'\b([A-Za-z_][A-Za-z0-9_]*)="\$\{?(\d+)\}?"', body)}
+            for rec, pos in sorted(known.items()):
+                for m in re.finditer(r'\b' + rec + r'([ \t]+.*)$', body, re.M):
+                    args = _words(m.group(1))
+                    if len(args) < pos:
+                        continue
+                    slot = args[pos - 1]
+                    inner = re.fullmatch(r'"\$\{?([A-Za-z_0-9]+)\}?"', slot)
+                    if not inner:
+                        continue          # a literal, or something not a bare $x
+                    tok = inner.group(1)
+                    n = int(tok) if tok.isdigit() else aliases.get(tok)
+                    if n is None:
+                        continue
+                    known[fn] = n
+                    changed = True
+                    break
+                if fn in known:
+                    break
+
+    del known[seed[0]]
+    return known
+
 def _template(name):
     """-> the name with every interpolation reduced to `{}`.
 
@@ -341,9 +456,114 @@ def _bash_names(text):
     return out
 
 
+def _suite_recorders(text):
+    """-> ({helper: which argument holds the name}, [helpers whose name is unreadable]).
+
+    A SUITE'S OWN RECORDERS, derived from its own definitions by the rule that already
+    works for `lib.sh`: seed from the shared table, and any function forwarding a bare
+    positional into a known recorder's name slot is itself a recorder (#1053).
+
+    TWO SHAPES, AND ONLY ONE IS A GAP. The distinction is the whole of this function:
+
+        COMPOSE   check "non-owner refused: ${1%%(*}"    the definition states a
+                                                        TEMPLATE naming the property,
+                                                        and it covers every call site
+        FORWARD   check_text "$label" ...                the definition states nothing;
+                                                        the NAME is at the call sites
+
+    A composing wrapper is already read, correctly, out of the suite file -- which is
+    why `native_ownership` grades one-for-one today. Treating it as unreadable and
+    refusing it would have broken three COMPLETE pairs to fix nothing; measured, at 32
+    suites refused including `hilbert_cluster`, `hilbert_locality` and
+    `native_ownership`. So only FORWARDING wrappers are returned here, and the call
+    sites are where their names are read.
+
+    The unreadable list is the refuse half: a helper that reaches a recorder with a
+    name slot this cannot resolve at all. Skipping it silently is how 147 names in 14
+    suites came to be ungraded.
+    """
+    body_text = _strip_comments(text)
+    known = dict(_BASH_NAME_ARG)
+    known[_RECORD_PRIMITIVE] = _RECORD_NAME_ARG
+    forwarding, unreadable = {}, []
+
+    changed = True
+    while changed:
+        changed = False
+        for fn, body in _bodies(body_text):
+            if fn in known or fn in unreadable:
+                continue
+            aliases = {m.group(1): int(m.group(2)) for m in
+                       re.finditer(r'\b([A-Za-z_][A-Za-z0-9_]*)="\$\{?(\d+)\}?"', body)}
+            for rec, pos in sorted(known.items()):
+                m = re.search(r"\b" + rec + r"([ \t]+.*)$", body, re.M)
+                if not m:
+                    continue
+                args = _words(m.group(1))
+                if len(args) < pos:
+                    continue
+                slot = args[pos - 1]
+                bare = re.fullmatch(r'"\$\{?([A-Za-z_0-9]+)\}?"', slot)
+                if bare:
+                    token = bare.group(1)
+                    n = int(token) if token.isdigit() else aliases.get(token)
+                    if n is None:
+                        # Reaches a recorder, and which argument carries the name
+                        # cannot be decided. REFUSE rather than skip.
+                        unreadable.append(fn)
+                    else:
+                        known[fn] = n
+                        forwarding[fn] = n
+                    changed = True
+                    break
+                # A literal or a composed name: the definition states the property and
+                # `_bash_names` already reads it. Not a forwarder, not a refusal.
+                break
+    return forwarding, sorted(unreadable)
+
+
+def _names_in(text):
+    """-> every check name the suite states, including through its OWN wrappers.
+
+    A BARE `{}` IS DROPPED. A forwarding wrapper's definition reads as `"$label"`,
+    which reduces to the template `{}` -- a property with no content. Published, it
+    sits in MISSING naming nothing a port could assert, and it MATCHES a port name
+    that is entirely one interpolation, which is a spurious pass. 17 of them were
+    being published. A wrong name is worse than an absent one, which is the argument
+    #1051 turned on.
+    """
+    forwarding, _ = _suite_recorders(text)
+    names = list(_bash_names(text))
+    for helper, pos in sorted(forwarding.items()):
+        names += re.findall(_pattern_for(pos, (helper,)), _strip_comments(text))
+    # THE FILTER IS APPLIED ONCE, AT THE END, AND TO BOTH SOURCES. A forwarder calling
+    # another forwarder -- `ans() { ansp "$1" h c "$2"; }` -- is a call site like any
+    # other to the pattern, and it yields `$1`. Filtering only the definitions left
+    # that one through, which the fixture below caught.
+    return [n for n in names if _template(n) != "{}"]
+
+
 def main(bash_file, py_file):
     """-> the exit status: 1 when a bash property has no counterpart."""
-    bash_names = _bash_names(open(bash_file).read())
+    bash_src = open(bash_file).read()
+
+    # THE REFUSE HALF (#1053). A helper that reaches a recorder whose name argument
+    # cannot be resolved makes every name it carries invisible, and grading the rest
+    # would report a verdict about a suite the tool has only partly read. That is the
+    # shape this whole issue is about, so it is a refusal rather than a silent skip.
+    _forwarding, unreadable = _suite_recorders(bash_src)
+    if unreadable:
+        print(f"REFUSED: {bash_file} defines {len(unreadable)} helper(s) that record a "
+              f"check under a name this cannot resolve:")
+        for helper in unreadable:
+            print(f"  unreadable  {helper}")
+        print()
+        print("Every check they carry is invisible, so any verdict here would be about "
+              "the part of the suite that happens to be readable. Give the helper a "
+              "name argument in a position the extractor can see, or record directly.")
+        return 2
+
+    bash_names = _names_in(bash_src)
     py_names = _py_names(open(py_file).read())
 
     bset, pset = set(bash_names), set(py_names)

test/pytest/expected_tests.txt

@@ -107,7 +107,20 @@
 # No behavioural arm can reach it -- the two sources are equal wherever the display
 # prints every bucket -- so the guarantee rested on a comment until review objected
 # that comments rot where arms do not. Re-derived by collection: `324 tests collected`.
-guard_tests 324
+# 322 -> 329 when the grader learned a suite's OWN recorders (#1053): the forwarding
+# shapes, a composing wrapper left alone, the refusal of a helper whose name position
+# cannot be resolved, the corpus budget pinned at one suite, the bare-{} drop, the
+# grader's refusal exercised through `main`, and a helper reaching only `pgc_record`.
+#
+# MERGED with #1048's source-text pin (324). Both sides moved this key and git
+# conflicted on it, which is the LOUD case. The quiet one is `cluster_tests`, which
+# both sides left at 320 and which merged silently -- and silence is not agreement,
+# it is the absence of a signal. Both keys re-derived by collection below.
+# Re-derived by collection on the MERGED tree: `331 tests collected`. Not 329, not
+# 324, and the fact that 322 + 7 + 2 happens to reach it is a coincidence of this
+# merge rather than a method -- the deltas were measured against different trees and
+# adding them is what produced a number no tree collected earlier today.
+guard_tests 331
 
 # 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
@@ -151,8 +164,11 @@ guard_tests 324
 # COLLECTED, NOT FIFTY-SIX FUNCTIONS -- most families are one function parametrised
 # over their probes. Re-derived by collection on this tree: `320 tests collected`.
 #
-# guard_tests is UNCHANGED at 322 here and #1054 moves it to 323 on its own tree.
-# Whichever of the two lands second re-derives BOTH numbers by collection: the two
-# branches touch different keys in this file, so git merges it without a conflict and
-# there is nothing to notice.
+# THE TWO KEYS FAILED DIFFERENTLY IN THE SAME MERGE, which is the lesson worth
+# keeping. `guard_tests` moved on both sides, so git CONFLICTED and demanded an
+# answer -- the loud case. `cluster_tests` was 320 on both sides and merged silently
+# -- and silence is not agreement, it is the absence of a signal. It happened to be
+# right here; two independent 322s merged just as silently earlier today and the
+# merged tree collected 323. Re-derive BOTH after every merge, not the one git
+# complained about.
 cluster_tests 320