test: a skipped arm records under the name it would have used (#994)

.github/scripts/skip-loop-arms.py

@@ -0,0 +1,220 @@
+#!/usr/bin/env python3
+"""Compare every skip loop's names against the arms its sibling branch emits.
+
+#994. A `for VAR in <names>; do check_skip "$VAR" ...; done` exists so that a
+skipped arm records under the name it would have used. The loop DUPLICATES those
+names, so a rename in the sibling branch desynchronises them silently and the
+skip starts recording under a name nothing emits.
+
+WHY A SCRIPT AND NOT awk IN THE PART. The first version used two awk extractors
+that each `exit` on first match, so a file with three loops over one variable
+compared only the first -- and the one it compared was the one whose both sides
+had just been written together, so it agreed by construction.
+
+WHAT IS AND IS NOT COMPARED, reported rather than assumed:
+
+    loops         every `for VAR in ... check_skip "$VAR"` found
+    armless       the sibling branch names no arms; the skip IS the record
+    interpolated  either side contains a shell expansion, so a literal set
+                  comparison would be wrong in both directions
+    compared      the rest, where a mismatch is a real finding
+
+`interpolated` is printed because a comparison nobody makes and a comparison that
+passes are indistinguishable in a total of mismatches.
+"""
+
+import pathlib
+import re
+import sys
+
+# Derived, not listed: any function defined in the corpus whose body reaches
+# check/pgc_record emits a record under its first argument.
+_HELPER_DEF = re.compile(r"^([a-z_][a-z0-9_]*)\(\)\s*\{", re.M)
+_LOOP = re.compile(r"^\s*for\s+([a-z_][a-z0-9_]*)\s+in\b")
+
+
+_HEREDOC = re.compile(r"<<-?\s*'?\"?([A-Za-z_][A-Za-z0-9_]*)'?\"?\s*$")
+
+
+def blank_heredocs(lines):
+    """Return lines with heredoc BODIES blanked out.
+
+    Shell-shaped tokens inside a heredoc are not shell. `native_parquet_flba.sh`
+    embeds python whose `if phys != {...}:` a naive depth count reads as a shell
+    `if`, which unbalanced the walk and made a ten-arm branch report six. The
+    tool that finds silently-lost arms was silently losing arms.
+    """
+    out, term = [], None
+    for l in lines:
+        if term is None:
+            m = _HEREDOC.search(l)
+            out.append(l)
+            if m:
+                term = m.group(1)
+        else:
+            out.append("")
+            if l.strip() == term:
+                term = None
+    return out
+
+
+def emitters(text):
+    out = {"check", "check_num", "check_skip", "check_text", "check_timing"}
+    for m in _HELPER_DEF.finditer(text):
+        name, start = m.group(1), m.end()
+        end = text.find("\n}", start)
+        body = text[start:end if end > 0 else len(text)]
+        if re.search(r"\bcheck\w*\b|\bpgc_record\b", body):
+            out.add(name)
+    return out
+
+
+def names_in(lines, emit):
+    pat = re.compile(r"^\s*(?:%s)\s+\"([^\"]*)\"" % "|".join(sorted(emit, key=len, reverse=True)))
+    out = set()
+    for l in lines:
+        m = pat.match(l)
+        if not m:
+            continue
+        n = m.group(1)
+        # A BARE VARIABLE REFERENCE IS THE SKIP MECHANISM, NOT AN ARM. A sibling
+        # branch often contains its own `check_skip "$VAR"` loop; counting that as
+        # an arm name made four comparable sites read as INTERPOLATED and therefore
+        # uncompared -- a silent loss of coverage in the tool that reports coverage.
+        if re.fullmatch(r"\$\{?[a-z_][a-z0-9_]*\}?", n):
+            continue
+        out.add(n)
+    return out
+
+
+def loop_blocks(lines, var):
+    """Every `for <var> in` header block and the line index it ends on."""
+    out = []
+    for i, l in enumerate(lines):
+        m = _LOOP.match(l)
+        if not m or m.group(1) != var:
+            continue
+        j = i
+        while j < len(lines) and not lines[j].rstrip().endswith("; do"):
+            j += 1
+        out.append((i, j))
+    return out
+
+
+def _sibling_before(lines, start):
+    """If the loop sits in an `else` branch, return the `then` branch above it."""
+    depth = 0
+    j = start - 1
+    while j >= 0:
+        t = lines[j].strip()
+        if t == "fi" or t.startswith("fi "):
+            depth += 1
+        elif t.startswith("if ") or t.startswith("if["):
+            if depth == 0:
+                return []          # reached our own `if` without meeting an `else`
+            depth -= 1
+        elif (t == "else" or t.startswith("elif ")) and depth == 0:
+            # collect from here back to the matching `if`
+            k, d2, idx = j - 1, 0, []
+            while k >= 0:
+                u = lines[k].strip()
+                if u == "fi" or u.startswith("fi "):
+                    d2 += 1
+                elif u.startswith("if ") or u.startswith("if["):
+                    if d2 == 0:
+                        return idx
+                    d2 -= 1
+                idx.append(k)
+                k -= 1
+            return []          # ran off the top without a balanced `if`: unparsed
+        j -= 1
+    return []
+
+
+def sibling_lines(real, struct, start):
+    """Walk the heredoc-free view, return the corresponding real lines."""
+    idx = sibling(struct, start, want_index=True)
+    return [real[i] for i in idx]
+
+
+def sibling(lines, start, want_index=False):
+    """The branch the loop is the alternative to, looking BOTH ways.
+
+    A loop in the `then` branch has its arms after the `else`. A loop in the
+    `else` branch has them BEFORE it, after the `if`. The first version looked
+    only forward, so the two outer pyarrow gates -- whose thirteen arms sit in
+    the `then` branch -- read as ARMLESS: a false negative of exactly the kind
+    this tool exists to find, in the tool. @OffgridwithJD made the same mistake
+    classifying the sites in #994, which is why there were six rather than four.
+    """
+    back = _sibling_before(lines, start)
+    if back:
+        return back
+    depth, j = 0, start
+    while j < len(lines):
+        s = lines[j].strip()
+        if s.startswith("if ") or s.startswith("if["):
+            depth += 1
+        elif s == "fi" or s.startswith("fi "):
+            if depth == 0:
+                return []
+            depth -= 1
+        elif (s == "else" or s.startswith("elif ")) and depth == 0:
+            k, d2, idx = j + 1, 0, []
+            while k < len(lines):
+                t = lines[k].strip()
+                if t.startswith("if "):
+                    d2 += 1
+                elif t == "fi":
+                    if d2 == 0:
+                        return idx
+                    d2 -= 1
+                idx.append(k)
+                k += 1
+            return idx
+        j += 1
+    return []
+
+
+def main(testdir):
+    root = pathlib.Path(testdir)
+    files = sorted(list(root.glob("*.sh")) + list((root / "selftest").glob("*.sh")))
+    lib = (root / "lib.sh").read_text(errors="replace") if (root / "lib.sh").exists() else ""
+
+    loops = compared = interpolated = armless = 0
+    bad = []
+    for f in files:
+        text = f.read_text(errors="replace")
+        if "check_skip \"$" not in text:
+            continue
+        lines = text.splitlines()
+        # STRUCTURE IS READ FROM A HEREDOC-FREE VIEW; names still come from the
+        # real lines, because a heredoc never contains a check.
+        struct = blank_heredocs(lines)
+        emit = emitters(text) | emitters(lib)
+        for var in sorted(set(re.findall(r'check_skip\s+"\$([a-z_][a-z0-9_]*)"', text))):
+            for start, hdr_end in loop_blocks(struct, var):
+                loops += 1
+                loop_names = {s for s in re.findall(r'"([^"]*)"', "\n".join(lines[start:hdr_end + 1]))
+                              if s and not s.startswith("$")}
+                arms = names_in(sibling_lines(lines, struct, hdr_end), emit)
+                if not arms:
+                    armless += 1
+                    continue
+                if any("$" in n for n in loop_names | arms):
+                    interpolated += 1
+                    continue
+                compared += 1
+                if loop_names != arms:
+                    bad.append(f"{f.name}:{start + 1}")
+    print(f"loops {loops}")
+    print(f"compared {compared}")
+    print(f"interpolated {interpolated}")
+    print(f"armless {armless}")
+    for b in bad:
+        print(f"MISMATCH {b}")
+    return 0
+
+
+if __name__ == "__main__":
+    sys.exit(main(sys.argv[1] if len(sys.argv) > 1 else "test"))

CHANGELOG.md

@@ -483,6 +483,51 @@ true until the next version shipped.
 
 ### Fixed
 
+- A skipped arm records under the name it would have used, so a skipped arm and
+  a deleted one are no longer indistinguishable (#994).
+
+  Four `check_skip` calls stood in for 19 named arms under a name none of those
+  arms has. When the condition failed, those 19 produced no record at all: a
+  reader could not tell which arms did not run, and the ledger could not tell a
+  skipped arm from a deleted one, because a skipped arm's row has no matching
+  record exactly as a removed check's would.
+
+  The convention was already in the tree, 30 lines below one of the offenders:
+  skip under each arm's own name, in a loop.
+
+  The issue counted 17. It is 19. Two arms in `sorted_pathkeys.sh` go through
+  `ansp`, which records under its first argument, and a sweep that looked for
+  `check` did not see them.
+
+  One arm's name interpolated the very variable whose emptiness causes its own
+  skip, so the skip would have recorded a key no real run emits. That name is now
+  stable and the collation it names moves into the display, which is not the key.
+
+  Six sites, not four, and 19 arms. Two arms go through `ansp`, which records
+  under its first argument. Two sites hold their arms in the `then` branch with
+  the skip in the `else`, which a classifier looking only forward reads as having
+  no arms at all.
+
+  `340` also skipped five arms behind a branch whose comment said they had
+  already been skipped above. Above had skipped the three premises, not these
+  five, so on a box with no non-root user five arms produced no record.
+
+  A new selftest part asserts every skip loop names exactly the arms its sibling
+  branch would emit. The loop duplicates those names, so a rename desynchronises
+  them silently and the skip records under a name nothing emits, which is the
+  failure this change exists to remove. That is not hypothetical: writing this,
+  a name from another open PR's rename went into the loop, and the comparison is
+  what caught it.
+
+  The part reports what it did not compare. One loop's sibling arm is generated
+  by a loop of its own, so a literal comparison would be wrong in both
+  directions. That loop is the one this change repairs, and it is not covered.
+  A total of zero mismatches would otherwise read as a corpus in agreement.
+
+  This is the precondition for arming the orphan guard in #983. Until a skipped
+  arm records under its own name, absence cannot mean removal.
+
+
 - A conftest can no longer switch a vacuity rule off by rebinding a name the
   layer reads (#924).
 

test/check_ledger.tsv

@@ -859,6 +859,10 @@ harness_selftest	460-the-controller-must-record-the-binary	premise: the mutation
 harness_selftest	460-the-controller-must-record-the-binary	premise: this part was given an executable pg_config to read the prefix from	never	-
 harness_selftest	460-the-controller-must-record-the-binary	so a child suite under PGC_SKIP_BUILD reaches verified, not source-only (#961)	never	-
 harness_selftest	460-the-controller-must-record-the-binary	the controller's stamp carries BOTH fields (#961)	never	-
+harness_selftest	470-a-skipped-arm-records-under-its-own-name	every comparable skip loop names exactly the arms its sibling would emit (#994)	never	-
+harness_selftest	470-a-skipped-arm-records-under-its-own-name	premise: and it actually compared some of them	never	-
+harness_selftest	470-a-skipped-arm-records-under-its-own-name	premise: the sweep found skip loops at all	never	-
+harness_selftest	470-a-skipped-arm-records-under-its-own-name	premise: the sweep tool is present	never	-
 native_join_runtime_filter	native_join_runtime_filter	3-table answer equals filter-off	never	-
 native_join_runtime_filter	native_join_runtime_filter	3-table join order matches filter-off	never	-
 native_join_runtime_filter	native_join_runtime_filter	3-table plan has coordinator	never	-

test/check_ledger_budget.txt

@@ -34,4 +34,4 @@ suites_not_covered 250
 #   Without that it is a hand-maintained count that drifts, which is the failure
 #   this repository has spent a day proving. It is not a ceiling; it is a
 #   measurement that must be true.
-checks_never_observed_red 905
+checks_never_observed_red 909

test/native_parquet_flba.sh

@@ -208,7 +208,19 @@ if phys != {"i32": "INT32", "i64": "INT64"}:
     sys.exit("unexpected physical types: %s" % phys)
 PYINT
 	if [ $? -ne 0 ]; then
-		check_skip "the integer-backed decimal case" "SKIP  this pyarrow does not store decimals as integers as expected" "this pyarrow does not store decimals as integers"
+		# ONE SKIP PER ARM, UNDER THE ARM'S OWN NAME (#994): a single skip under a
+		# name none of the six arms has leaves all six with no record, so a skipped
+		# arm and a deleted one are indistinguishable to a reader and to the ledger.
+		for _fl_n in "INT32-backed DECIMAL reads" \
+				"INT64-backed DECIMAL reads" \
+				"INT32-backed DECIMAL keeps its null" \
+				"parquet_schema advises numeric for an INT32 DECIMAL" \
+				"parquet_schema advises numeric for an INT64 DECIMAL" \
+				"an INT64 DECIMAL still binds to bigint as the unscaled integer"; do
+			check_skip "$_fl_n" \
+				"SKIP  $_fl_n (this pyarrow does not store decimals as integers as expected)" \
+				"this pyarrow does not store decimals as integers"
+		done
 	else
 		check "INT32-backed DECIMAL reads" \
 			"$(q "SELECT string_agg(d::text, ',' ORDER BY d) FROM pgcolumnar.read_parquet('$W/dec_i32.parquet') AS t(d numeric);")" \
@@ -231,7 +243,24 @@ PYINT
 			"-3500000,0,1250000"
 	fi
 else
-	check_skip "the foreign-producer FLBA cases" "SKIP  pyarrow not available; foreign-producer FLBA cases skipped" "pyarrow not available"
+	# ONE SKIP PER ARM, UNDER THE ARM'S OWN NAME (#994). This is the OUTER gate:
+	# without pyarrow none of the ten arms in the `then` branch runs, and a single
+	# skip named for none of them left all ten with no record. Only one branch ever
+	# fires, so naming the six that the inner gate also names duplicates nothing.
+	for _fl_o in "pyarrow uuid reads as uuid" \
+			"pyarrow decimal128 values are exact" \
+			"crafted out-of-range scale is rejected, not decoded" \
+			"backend survived the crafted scale" \
+			"INT32-backed DECIMAL reads" \
+			"INT64-backed DECIMAL reads" \
+			"INT32-backed DECIMAL keeps its null" \
+			"parquet_schema advises numeric for an INT32 DECIMAL" \
+			"parquet_schema advises numeric for an INT64 DECIMAL" \
+			"an INT64 DECIMAL still binds to bigint as the unscaled integer"; do
+		check_skip "$_fl_o" \
+			"SKIP  $_fl_o (pyarrow not available)" \
+			"pyarrow not available"
+	done
 fi
 
 pgc_summary

test/native_parquet_pushdown.sh

@@ -211,7 +211,14 @@ if f.metadata.num_row_groups != 4:
     sys.exit("expected 4 row groups")
 PYDEC
 	if [ $? -ne 0 ]; then
-		check_skip "the integer-DECIMAL pushdown case" "SKIP  could not build the integer-DECIMAL pushdown file" "could not build the fixture file"
+		# ONE SKIP PER ARM, UNDER THE ARM'S OWN NAME (#994).
+		for _pd_n in "INT64-backed DECIMAL: predicate skips 3 of 4 groups" \
+				"INT64-backed DECIMAL: skipping did not drop rows" \
+				"INT64-backed DECIMAL: unfiltered scan skips nothing"; do
+			check_skip "$_pd_n" \
+				"SKIP  $_pd_n (could not build the integer-DECIMAL pushdown file)" \
+				"could not build the fixture file"
+		done
 	else
 		psql_run "CREATE FOREIGN TABLE ftdec (d numeric) SERVER pq
 		          OPTIONS (path '$PGC_WORKDIR/dec_push.parquet');"
@@ -225,7 +232,16 @@ PYDEC
 			"$(skipped_for_t ftdec 'd >= 0')" "0"
 	fi
 else
-	check_skip "the integer-DECIMAL pushdown case" "SKIP  pyarrow not available; integer-DECIMAL pushdown case skipped" "pyarrow not available"
+	# ONE SKIP PER ARM, UNDER THE ARM'S OWN NAME (#994). The OUTER gate: without
+	# pyarrow none of the three arms runs, and a single skip named for none of them
+	# left all three with no record.
+	for _pd_o in "INT64-backed DECIMAL: predicate skips 3 of 4 groups" \
+			"INT64-backed DECIMAL: skipping did not drop rows" \
+			"INT64-backed DECIMAL: unfiltered scan skips nothing"; do
+		check_skip "$_pd_o" \
+			"SKIP  $_pd_o (pyarrow not available)" \
+			"pyarrow not available"
+	done
 fi
 
 pgc_summary