fix(output): make the dump walks linear and the ops listing deterministic - #1093
Merged
Conversation
`Ops::operators` and `Ops::operands` were collected from `HashMap` keys. `RandomState` reseeds per map instance, so the listings were reordered on every run — and even between two parses in one process. That made `bca ops` output impossible to diff between runs, check into a repository, or use as a cache key, in the tree renderer and in every serialized format alike. Sort both vectors at the end of `compute_operators_and_operands`. Byte-lexicographic rather than first-appearance order: `finalize` merges a child space's maps into its parent, so an insertion-ordered map would give the parent "what it saw directly, then whatever each child contributed" — a walk artifact that shifts when nesting changes. Sorting is also stable across platforms and hasher versions. No metric value moves: Halstead's n1/n2 are set cardinalities. The cost is O(n log n) per space over that space's vocabulary and is paid only on the `ops` seam; the metric walk never calls this function. `check_ops` no longer re-sorts the value under test, which turns every per-language caller into an ordering regression test. Fixes #1091
`finalize` rebuilt the innermost still-open space's operator and operand lists on every call — every time the walk left a function — and every result but the last was immediately overwritten. A file with F function spaces therefore rebuilt the root's whole vocabulary F times, and since the vocabularies are now sorted, re-sorted it F times too. Every state except the bottom one already gets its vocabulary computed when `finalize` pops it, so only the root needs the trailing rebuild. Do it once in `ops_inner`, after the walk drains. On hlo_instruction.cc (4057 lines, ~180 spaces) `bca ops -O json` goes from ~39 ms to ~27 ms per run, measured interleaved over 20 runs each — faster than before the sort was introduced. Output is byte-identical.
Two ways the #1091 ordering tests could have gone quietly vacuous. `is_sorted` is trivially true for an empty or single-entry vector, so `assert_sorted_spaces` walked the tree without requiring any space to hold an observable ordering; only the root's non-emptiness was checked. Require at least two entries per space per vocabulary. The test's stated reason for its language mix — that the operator vocabulary is the union of a token-id-keyed map and a text-keyed one that primitive types land in, and that sorting interleaves them rather than concatenating — was never asserted. Each case now names one entry from each map and pins their relative order, so a fixture that stopped exercising the text-keyed map fails instead of narrowing in silence.
The hand-written `_native.pyi` was the one place `Ast.ops()` still promised only "deduplicated" after #1091 made the order contractual. `ast.rs`, `_types.py`'s `OpsDict`, and `STABILITY.md` all say "sorted, deduplicated"; the stub is what a Python user sees on IDE hover, so the two disagreed about a guarantee that now holds until 3.0.
`Frame`'s doc justified storing a prefix length instead of an owned `String` with "every other queued frame sits at this level or deeper". That is refutable: an uncle frame queued below this one on the stack has a strictly smaller `prefix_len`. The property that actually holds — and that `String::truncate`'s panic-freedom rests on — is that `prefix_len` is non-decreasing from the bottom of the stack to the top, so only frames popped before this one can rewrite the buffer. Separately, the #1054 rail test explained its hand-built `Ops` fixture by saying parsed iteration order is not stable run to run. Sorting the vocabularies made it stable, so the note now gives the reason that still applies: the fixture pins the exact shape the expected rails spell out.
dekobon
force-pushed
the
fix/1054-dump-prefix-quadratic
branch
from
July 27, 2026 22:07
7763366 to
394352f
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1093 +/- ##
==========================================
+ Coverage 98.04% 98.09% +0.05%
==========================================
Files 271 272 +1
Lines 67235 67426 +191
Branches 66805 66996 +191
==========================================
+ Hits 65922 66143 +221
- Misses 871 873 +2
+ Partials 442 410 -32
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
`dump_ops` documents that it propagates any `std::io::Error` the writer produces, and `bca ops | head` closes the pipe mid-stream, so it is a real path — but nothing exercised it. Every `?` on a `write!` or a `set_color` was an untaken branch, which is most of what kept `dump_ops.rs` at 89% region coverage. Add a `WriteColor` that succeeds for N operations then fails with `BrokenPipe`, and sweep N across every operation a full dump performs. Each budget must yield an `Err` carrying the writer's own error kind, and the walk must stop there: the harness keeps counting past the failure, so a swallowed error that let the walk continue shows up as extra attempts. Verified by turning one `write!(..)?` into `let _ = write!(..)` — the sweep fails on the stop-there assertion. Region coverage for the file goes 89.37% -> 94.32%, with no new uncovered lines.
`dump_node` carries the same untested contract `dump_ops` did: it documents that it propagates any `std::io::Error` the writer produces, `bca dump | head` closes the pipe mid-stream, and every existing test writes into an infallible `Vec`. Its whole coverage gap was that shape — 26 uncovered regions and zero uncovered lines, which is what a file made only of untaken error branches looks like. Lift `FailAfter` and the sweep out of `dump_ops`'s test module into a shared `output::test_support`, so the two walks share one harness rather than a copy each, and the next walk needs a fixture instead of a harness. The sweep's own contract — that every fallible `WriteColor` entry point counts against the budget, including the `flush` and `reset` no walk uses today — moves there too, next to the code it constrains. The AST fixture is the existing `int a = 42;` parse: the sweep re-runs the whole dump once per write position, so cost is quadratic in node count and the smallest tree that still nests is the right one. Verified by turning `paint`'s `color(..)?` into `let _ = color(..)` — the sweep fails on the stop-there assertion. Region coverage: dump.rs 97.40% -> 98.64%, dump_ops.rs 89.37% -> 93.83% against its pre-sweep baseline.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Two output-path defects found while working on the
dumpwalk, plus theperformance and test follow-through each one exposed.
#1054 — quadratic prefix growth in the
dumpwalksEvery queued node carried an owned copy of its ancestors' box-drawing
prefix, a string that grows ~3 bytes per nesting level, so a wide-and-deep
tree held O(depth²) resident bytes and copied O(depth) per node.
Separately, the flush-left check called
tree_sitter::Node::parent— whichresolves by descending from the root — once per node.
All three walks (
dump,dump_metrics,dump_ops) now share one prefixbuffer that is extended on descent and truncated on the next visit, and
carry each node's connector glyph on the work stack so only the node the
walk starts from needs a parent lookup. The child staging buffer went away
with it.
#1091 — nondeterministic operator/operand order
Ops::operatorsandOps::operandswere collected fromHashMapkeys.RandomStatereseeds per map instance, so the listings were reordered onevery run — and even between two parses in one process.
bca opsoutputcould not be diffed between runs, checked into a repository, or used as a
cache key, in the tree renderer and in every serialized format alike.
Both vectors are now sorted byte-lexicographically:
Why sorting rather than an ordered map
indexmap(already in the dependency graph transitively) andordermapwere both evaluated and rejected — for reasons about this code, not the
crates:
HalsteadMaps, which everymetricsrun populates for every node. The ordering only matters on theopsseam, a separate command. That taxes the hot path for a propertynothing on it observes.
finalizemerges a childspace's maps into its parent, so parent order would be "what it saw
directly, then whatever each child contributed" — a walk artifact that
shifts when nesting changes, not source order. Byte-lexicographic order
is also stable across platforms, rustc versions, and hashbrown bumps.
A fixed-seed hasher was rejected outright: deterministic per build, but the
order still churns on any capacity change or hasher-version bump.
Full analysis in
#1091 (comment).
Performance
Removing the redundant rebuild the sort exposed left
opsnet fasterthan before the issue.
finalizerebuilt the innermost still-openspace's vocabulary on every call — every time the walk left a function —
and every result but the last was overwritten, so a file with F function
spaces rebuilt the root's whole vocabulary F times. Only the root needs
that; it now happens once, in
ops_inner.hlo_instruction.cc(4 057 lines, ~180 spaces),bca ops -O json,20 runs each, interleaved:
Output byte-identical between the two.
Behaviour change
Ops::operators/Ops::operandswere documented as "arbitrary order", sotightening to "sorted" is additive for callers, but it does change observed
output. Recorded in
CHANGELOG.mdunder Unreleased and inSTABILITY.md,which now states the order is contractual from the
2.1line and will notchange before
3.0. No metric value moves — Halstead'sn1/n2are setcardinalities.
Tests
ops_vocabularies_are_sorted_1091walks every space of Rust, C++, Java,Python, and TypeScript samples, requiring each vocabulary to be sorted
and to hold at least two entries so
is_sortedcannot pass vacuously.Each case also pins one entry from the token-id-keyed operator map
against one from the text-keyed
primitive_operatorsmap, proving theunion is sorted as a whole rather than concatenated.
ops_are_stable_across_repeated_parses_1091— two parses of the same40-space source in one process must serialize identically. This failed
before the fix.
check_opsno longer re-sorts the value under test, which turns all ~20existing per-language callers into ordering regression tests.
sort_unstablecalls: 20 of 27opstestsfail, including both new ones.
Review
review,audit-tests,rust-optimize, and/code-review --fixwere allrun. Findings resolved in-branch: the redundant per-space rebuild above,
two vacuous-assertion gaps in the new tests, a stale
_native.pyidocstring that still promised only "deduplicated", a refutable invariant in
Frame's doc (the prefix-length scheme rests onprefix_lenbeingnon-decreasing bottom-to-top, not on "every other frame sits deeper"), and
a test comment that this branch's own sort commit made false.
One pre-existing inconsistency was found and deliberately left:
dump_tree_helper'searly-out still trusts
child_count() == 0whilepush_childrennowderives last-child from the child the cursor actually walks last. These
agree on well-formed trees; reconciling them belongs in its own issue.
make pre-commitgreen.Fixes #1054
Fixes #1091