parser: nesting decides the separator, never the terminator (#2127) - #2151
parser: nesting decides the separator, never the terminator (#2127)#2151gHashTag wants to merge 2 commits into
Conversation
parse_struct_body collected raw lexemes until a comma, so a comma inside a nested type ended the field. Map<Key, Value> became field `generic : Map<Key` plus a phantom field named `Value`; (A, B) behaved the same way. Six declared fields parsed as eight. Two earlier repairs were reverted. Both failed the same way: they let a depth counter decide a block terminator. Attempt (a) let depth suppress RBrace and the scanner ate the struct's closing brace. Attempt (b) let depth suppress Eof and the build hung, because the lexer yields Eof indefinitely while the depth counter never moves, so the state is stationary and the token stream infinite. The rule this commit follows: RBrace, Semicolon and Eof terminate unconditionally at any depth; only Comma consults depth. Termination therefore does not depend on the input being well-formed, which is what a recovery scanner exists for. Measured over all 634 specs outside specs/scratch, candidate against the binary it replaces: 616 unchanged, 4 improved, 14 changed field sets, 0 regressions of any kind. scripts/tri_loop/diffbin.py produced that table and exits non-zero on any regression. Also adds tri cost and tri triage. tri cost reports per-KB spread next to the scaling exponent, because an exponent fitted over a heterogeneous sample is a composition metric rather than a property of the parser -- the confound #2133 named for the ratchet, which #1781 hit for parse scaling. Closes #2127
|
📓 NotebookLM Notebook linked to this PR
This notebook contains session context, decisions, and artifacts for this work. |
Merge-gate diagnosis: both red checks reproduce on the base, with the same file and lineThe order was: establish the cause of the red The two red checks, named exactlyWorkflow
Three-way comparison collapses to two-way, and the base is redThe PR base is
Identical job, identical step, identical file and line number. Over the whole history of Two defects behind the synthesis failure, both pre-existing
Both are independent of this PR and belong in their own issue; I have not touched them here. Every changed field set has a reason-codeThe aggregate "0 regressions" is not admissible alone, so each of the 18 files whose field set moved (14
Zero Why An unbalanced Self-correction on the earlier aggregate: the four files previously labelled A corpus defect this surfaced, which is larger than the PR63 spec files contain 115 field lines of the form Merge-gate, stated as a checklist
Two items remain, so this is not a merge request. |
…an honest cost (Closes #2127) Five tests over five fixtures in bootstrap/tests/struct_body_terminator.rs. Three are liveness tests under a 10 s in-process wall-clock ceiling. Truncated input leaves bracket depth positive, so a depth-gated terminator is never accepted and the field loop runs past the end of the token stream. Asserting on error text alone would pass on a hang, because a hung process produces no text to compare and the CI job timeout would report it as flake. The child is killed before the assertion fires. Those three do not discriminate fixed from unfixed: checked against the pre-fix binary, identical verdict, message and timing. They are regression guards, not evidence. Two new fixtures discriminate. semicolon_phantom is the improvement: Map<K, V; made the pre-fix collector emit a phantom field V with an empty type. Now the struct has exactly its two declared fields. field_swallow is the cost: on a type argument list left open by a comma, three declared fields become one typed Map<K,b:u8,c:u16, where the pre-fix collector truncated and kept three. That corrects how the corpus differential reads. "0 regressions" did not mean no field was lost; it meant loss on malformed input was accepted as a tradeoff. The test pins the loss at one level and inside one struct rather than claiming a reading is right. Diagnostic wording is not pinned: the recorded baseline said "Parse error:" and the binary no longer does, so an assertion on that word failed while the parser was correct. Token names are asserted instead.
|
📓 NotebookLM Notebook linked to this PR
This notebook contains session context, decisions, and artifacts for this work. |
PR DashboardGenerated at: 2026-08-14 19:07:31 UTC
Summary
Seal Status
|
Correction: the "0 regressions" claim was weaker than it sounded, and I found it by trying to make the fixtures failThe EOF fixture is now a test — five tests over five fixtures in The hard timeout, and why it is the pointThree cases run under a 10 s in-process wall-clock ceiling. Truncated input leaves bracket depth positive, so a terminator gated on depth is never accepted and the field loop runs past the end of the token stream. Asserting only on the error message would pass on a hang: a hung process never produces a message to compare, the test would sit until the CI job's own timeout killed it, and the result would read as infrastructure flake instead of as #2127. The ceiling is owned by the test thread, both pipes are drained from helper threads so a full pipe buffer cannot masquerade as a hang, and the child is killed before the assertion fires so a wedged parser cannot outlive the test binary. Those three tests are worth less than they lookRun against the pre-fix binary, all three pass unchanged — identical verdict, identical message, identical timing (106 ms vs 106 ms). Two fixtures that do discriminateField sets from
The first is the improvement and the reason the change is worth making: the pre-fix collector promoted The second row is the correctionThree declared fields become one. That is a loss of fields on malformed input. The corpus differential declared regression as "the fixed binary extracts fewer fields than the base" — and this is exactly that, yet the run reported 0 regressions. Both statements are true because the per-file classifier labelled it So the honest reading of the differential is: "0 regressions" means no field loss was found that I judged unacceptable — not that no field was lost. The 17 files under What this does to the merge-gateThe differential criterion is not satisfied as originally written. Restating it as what was actually established:
Two open, one of them newly reopened by this comment. Still not a merge request, and now less ready than the previous comment implied. One more thing the fixtures surfaced
|
Status: not-ready. Bounded evidence only. Not for merge.The differential behind this PR covered 868 of 1089 files, 79.7 %. The Zero regressions on 79.7 % of the corpus is not the absence of regressions. The Two further reasons not to merge yet:
What would move this to ready, in order:
Leaving open. Not closing, not merging. |
1 participant
What was wrong
parse_struct_bodycollected raw lexemes until a comma, so a comma inside a nested type ended the field. On a fixture with six declared fields:masterplainu8genericMap<Key← truncatedValuetuple(A← truncatedBnestedVec<Vec<u8>>arr[4]u16lasti32Six fields parsed as eight. After this change:
generic : Map<Key,Value>,tuple : (A,B), phantoms gone, exactly six.Why the two earlier attempts were reverted, and what is different
Both reverted repairs failed the same way: they let a depth counter decide a block terminator.
RBrace→ the scanner ate the struct's closing brace (161→171 recovery events, 0→6 specs capturing nothing)Eof→ the build hungThe hang is not bad luck, it is forced. The lexer yields
Eofindefinitely and never changes the depth counter, so if there is any depth at whichEofis not accepted as a terminator, the scanner's state is stationary while the token stream is infinite. A recovery scanner's termination must not depend on its input being well-formed — that input is precisely why it exists.This change follows the rule that both attempts lacked:
Depth is tracked over
(/),[/], and</>(the latter only when<directly follows an identifier, so<as a comparison operator cannot open a level;>>closes two).Evidence
Both binaries built from this tree, run over the same corpus — all 634 specs outside
specs/scratch:No file where the candidate fails, hangs, or leaves more truncated types than the baseline.
The
Eofhazard has a fixture rather than an argument: a struct whose last field is an unclosed generic at end of file exits1withParse error: Expected RBrace, got Eof ('') at line 5:1, in under a second.The limit, stated plainly
On malformed input the new collector absorbs the following field into the type text (
a : Map<K,b:u8) wheremastertruncated the type instead. That is one kind of damage traded for another on input that is already broken. On well-formed input it is strictly better, and that is the only claim made here.Tooling added, and why it belongs in this PR
scripts/tri_loop/diffbin.py(tri diffbin) is the tool that produced the table above; it exits non-zero on any regression, so the check a reviewer would want is now one command. Given that two prior repairs to this exact function were merged and reverted, the check being cheap is part of the fix.tri costandtri triagecome from the same session.tri costreports the per-kilobyte spread next to the scaling exponent, because an exponent fitted over a heterogeneous sample is partly a composition metric rather than a property of the parser — the confound #2133 named for the ratchet. Measured inside one homogeneous family (wNNN_bench_module_NNx2p6_aos_var_call_write, n=39, 342KB…818KB):alpha = 0.940,R^2 = 0.986, per-KB spread10.65%— i.e. linear. See the comment left on #1781.Closes #2127