|
| 1 | +# 2026-09-19 — `check-merge-dropped-symbols.py` now prints in a stable order |
| 2 | + |
| 3 | +Adopts `2026-09-19-partial-clone-blob-vs-absence#p0` ("Sort the checker's findings so two runs can |
| 4 | +be compared"). |
| 5 | + |
| 6 | +## What was wrong |
| 7 | + |
| 8 | +The checker printed the same findings in a different order on different runs, so diffing two runs |
| 9 | +showed differences that were not really there. Reproduced on `origin/main`'s own version, range |
| 10 | +`e53b2142^..e53b2142` (8 merges, 6 dropped definitions, rc 1): **ten runs under |
| 11 | +`PYTHONHASHSEED=random` produced exactly two distinct orderings**, 6 of one and 4 of the other. The |
| 12 | +two differ only in which path's block comes first — `tests/test_class_format.rs` or |
| 13 | +`test-data/src/indy/make_indy_fixtures.py`. |
| 14 | + |
| 15 | +## Where it actually came from — the proposal's diagnosis was close but not exact |
| 16 | + |
| 17 | +The proposal said findings are "accumulated in a set and printed in iteration order". They are not: |
| 18 | +`findings` is a **list**, and the names within a single path were **already** `sorted()`. Auditing |
| 19 | +all four sets in the file: |
| 20 | + |
| 21 | +| set | reaches output order? | |
| 22 | +|---|---| |
| 23 | +| `found` in `symbols()` | no — consumed by `sorted()` at the diff site | |
| 24 | +| `names` in `excused()` | no — membership tests only (`name not in accounted`) | |
| 25 | +| `theirs_symbols - symbols(...)` | no — already wrapped in `sorted()` | |
| 26 | +| **`changed` in `check()`** | **yes** — a set of paths, iterated directly | |
| 27 | + |
| 28 | +So **one** of four candidate sources reaches the output, and it is the path iteration, not the |
| 29 | +finding accumulation. The symptom the proposal described is real; the sentence naming its cause |
| 30 | +is not, and a fix aimed literally at "the print site" would have worked by accident. |
| 31 | + |
| 32 | +## The change |
| 33 | + |
| 34 | +One line in `check()`: |
| 35 | + |
| 36 | +```python |
| 37 | +for path in sorted(filter(None, changed)): |
| 38 | +``` |
| 39 | + |
| 40 | +Sorted **at the source rather than at the print site**, because `findings` is also *returned* by |
| 41 | +`check()`. Sorting in `main()` would order what gets printed and leave the return value still |
| 42 | +dependent on hash order, which is a smaller fix wearing the same clothes. |
| 43 | + |
| 44 | +## Verification |
| 45 | + |
| 46 | +Bidirectional, in the product call site (not a copy of it): |
| 47 | + |
| 48 | +| version | distinct orderings / 10 runs | rc | |
| 49 | +|---|---|---| |
| 50 | +| `origin/main` (before) | **2** | 1 | |
| 51 | +| with the fix | **1** | 1 | |
| 52 | +| fix removed again (mutation) | **2** | 1 | |
| 53 | +| restored | **1** | 1 | |
| 54 | + |
| 55 | +Findings themselves are untouched: 15 output lines before and after, identical when both are |
| 56 | +sorted as sets, same rc. The fix changes order and nothing else. |
| 57 | + |
| 58 | +## What this costs |
| 59 | + |
| 60 | +- **The output no longer reflects traversal order.** Nothing reads it, so the practical cost is |
| 61 | + zero, but it is a real property that was removed rather than nothing at all. |
| 62 | +- **Nothing locks it.** This repo has **no test harness for `scripts/`** (0 `test*.py` files). |
| 63 | + Measured with the nondeterminism deliberately put back: all four python checkers exit 0 |
| 64 | + (`check-worklog-json`, `check-dod-ci-parity`, `check-named-exception-classes-are-loadable`, |
| 65 | + `check-merge-dropped-symbols` itself) and `cargo fmt --check` exits 0. The two Rust axes not |
| 66 | + re-run under the mutation — `clippy` and `cargo test` — do not read this script's output at all, |
| 67 | + so the honest summary is that **the guard against this class of defect is zero**. This fix is a |
| 68 | + habit, not a rule, and a later edit can undo it silently. That is the one genuine gap this round |
| 69 | + leaves, and it is filed as the follow-up proposal rather than built, because a harness is a |
| 70 | + larger decision than a one-line ordering fix. |
| 71 | +- **Scope deliberately not widened.** The other three `scripts/` checkers were not audited for the |
| 72 | + same class of defect; that is part of the same follow-up. |
0 commit comments