compiler: \addvspace inserts vertical space (documented simplification)

[d-q222]

Scope

Fixes #495. \addvspace{len} previously errored unsupported_feature. crates/compiler/src/parser.rs + inventory files regenerated via the project's own render_supported_latex.sh script (not hand-edited).

Documented, honest simplification (flagged as required before implementing, and the right call here): real TeX's \@xaddvskip merges with whatever glue already ends the current vertical list (\ifdim\lastskip=\z@, checked generically — not only when that glue came from a previous \addvspace; a preceding \vspace/\bigskip triggers the exact same merge, since \lastskip carries no provenance in real TeX either). The merge itself takes the larger of the two skips (falling back to summing only when the new length is negative and the existing one is not). This compiler's Block::VSpace carries a flat point amount with no provenance and no way to inspect or replace the previous block, and the correct merge point (page-builder's VListBuilder::addvspace) is a separate crate out of scope for this lane — reaching it would also require re-vendoring the render-pipeline's compiler snapshot. So \addvspace{len} unconditionally inserts len, exactly like \vspace{len}: a single call (nothing but non-glue material before it) matches real TeX, while two consecutive calls (or an \addvspace right after any other vertical glue) add up where real TeX would keep the larger. This is documented in a doc comment on the new parser arm and in the inventory description, the same pattern used for the recent BoxMeasurer PR's own honest limitation.

Overlap check

git branch -a --list '*addvspace*' — no other branch. coord.py claims — GH-ADDVSPACE held only by me, --branch set.

Test results

Full cargo test --lib in crates/compiler (supervisor-run, fresh CARGO_TARGET_DIR): 342 passed, 0 failed (includes all 3 addvspace tests: the original addvspace_inserts_vertical_space_without_an_unsupported_diagnostic and addvspace_twice_in_a_row_adds_safely_without_negative_space, plus addvspace_star_is_diagnosed_not_silently_tolerated from the star-form review fix below). --test supported_latex: 8/8 (parser-arm/inventory parity, artifacts current).

Mutation check (independently re-run by the reviewer): with the addvspace match arm disabled, both original tests fail with the original unsupported_feature error; restored with a fresh build, all pass.

Not done

True \@xaddvskip max-merge semantics (needs glue provenance on the vertical-spacing IR, a page-builder/vendor/compiler change outside this lane's scope, and pdflatex-oracle tests) — left as a documented follow-up, not attempted here.

Update (review fix)

The star-form claim in the original body was wrong and has been fixed: real \addvspace has NO star form at all (ltspace.dtx's kernel definition takes one plain argument) — \addvspace*{10pt} is a hard TeX error in real LaTeX, not tolerated like \vspace*. This compiler previously silently consumed the star and inserted a real skip; it now does not consume it, so \addvspace* is diagnosed instead of silently succeeding. New test addvspace_star_is_diagnosed_not_silently_tolerated, mutation-checked, full crates/compiler suite green.

Update 2 (review fix)

Two more fixes: (1) the "Test results" section above was stale — it now reflects the current 3-test count including the star-form test, and gives a full-suite total rather than the original partial count. (2) The glue-merging justification in the Scope section, and the matching code comment, wrongly claimed real TeX only merges \addvspace with glue that itself came from a previous \addvspace call specifically. Checked real ltspace.dtx/latex.ltx (\@xaddvskip, \ifdim\lastskip=\z@): the check is generic — it fires on any nonzero \lastskip, regardless of what inserted that glue (a plain \vspace or \bigskip immediately before an \addvspace triggers the identical max-or-sum merge). Both the Scope text above and the source comment on the addvspace dispatch arm are corrected to say so.

🤖 Generated with Claude Code

[d-q222]

daniel-parent review (subagent)

Verdict: CHANGES NEEDED

1. The documented "max, not sum" simplification is real and correctly characterized. Verified with pdflatex (TeX Live 2026): \addvspace{5pt}\addvspace{10pt} and \addvspace{10pt}\addvspace{5pt} (both orders, in vertical mode) leave \lastskip = 10.0pt in real LaTeX (\showthe\lastskip), confirming real TeX keeps the max regardless of order while this compiler will sum. The doc comment's explanation of why (Block::VSpace has no glue provenance, the real merge point is a separate crate) is accurate and the same honesty pattern used elsewhere in the project — no issue here.
2. The star-form claim is factually wrong, confirmed with pdflatex. The new code comment says "The star is tolerated like \vspace's... both forms agree" — but real \addvspace has no star form at all: ltspace.dtx's current kernel definition (\protected\def\addvspace#1{...}) takes one plain argument, no \@ifstar. Real \addvspace*{10pt} is a hard error: ! Illegal unit of measure (pt inserted) (the * gets consumed as the mandatory argument; {10pt} is then typeset as literal text). This compiler instead silently swallows the star as a no-op and inserts a real 10pt skip — a materially different, non-erroring result from the oracle on that exact input. Given the project's stated philosophy ("nothing is dropped silently... every other command produces an explicit diagnostic"), please either drop the star-consumption (so \addvspace* fails to parse/diagnoses, matching real TeX) or, at minimum, correct the comment to admit this is a second, undisclosed simplification rather than claiming parity with \vspace*.
3. Minor, non-blocking: real \addvspace does error, but only in restricted horizontal mode (inside \mbox/\parbox, "LR mode") — verified with pdflatex (! LaTeX Error: Not allowed in LR mode.). It does not error in ordinary mid-paragraph (unrestricted) horizontal mode, where it just does an implicit \par, exactly like \vspace already does in this compiler. So the PR's own test input (One\addvspace{10pt}Two) is fine and matches real TeX — the compiler simply has no concept of restricted/LR mode at all, a pre-existing gap already shared by \vspace, not a regression and not worth blocking on.
4. Ran cargo test --test supported_latex in a clean worktree: 8/8 passed, including generated_artifacts_are_current, confirming the inventory/doc files were genuinely regenerated via the script rather than hand-edited. Ran cargo test --lib addvspace: 2/2 passed.
5. No vendor/ edits. GH-ADDVSPACE claim history shows a clean claim/close.

Point 2 is the actionable item; everything else is either confirmed-correct or a pre-existing, disclosed-elsewhere limitation.

[d-q222]

daniel-parent review (subagent)

Verdict: CHANGES NEEDED

Earlier findings, resolved one by one

1. "max, not sum" simplification, documented honestly — no issue. Re-verified fresh with pdflatex (TeX Live 2026): \addvspace{10pt}\addvspace{5pt} and \addvspace{5pt}\addvspace{10pt} both leave \lastskip = 10.0pt, confirming real TeX keeps the max regardless of order. Still correctly disclosed in the doc comment and PR body. Unchanged, still fine.

2. FIXED. The star-form claim was factually wrong last round ("both forms agree" — false; real \addvspace has no star form at all). Commit ac3f1d27 ("\addvspace has no star form; stop tolerating \addvspace*") removes the star consumption entirely and rewrites the doc comment to correctly say real \addvspace*{10pt} is a hard TeX error. Verified with real pdflatex: \addvspace*{10pt} produces ! Missing number, treated as zero. followed by ! Illegal unit of measure (pt inserted). — a hard error, confirming the comment's new claim. New test addvspace_star_is_diagnosed_not_silently_tolerated passes. I also drove the actual crates/compiler flashtex-compiler JSON-Lines worker binary directly (built in an isolated CARGO_TARGET_DIR — see note below) with a real compile request: plain \addvspace{10pt} → zero diagnostics; starred \addvspace*{10pt} → two diagnostics (syntax_error "requires a braced argument" + a cascading dimension-parse error) and no VSpace block emitted. The star is now genuinely diagnosed, not silently swallowed into a real 10pt space. (The double-diagnostic on the malformed input is cosmetic and not new — it's the same "required_group fails, falls through to parse_dimen_pt_at("") anyway" pattern the pre-existing \vspace arm has always had; not introduced by this PR.)

3. Non-blocking, unchanged. LR-mode restriction gap is pre-existing and shared with \vspace; not touched by this PR, not worth blocking on.

4. Reconfirmed. cargo test --lib addvspace: 3/3 pass. cargo test --test supported_latex: 8/8 pass including generated_artifacts_are_current (inventory genuinely regenerated, not hand-edited). Mutation check (re-run by me): renaming the "addvspace" match arm makes all 3 new tests fail with the original panic; restored, all 3 pass again.

5. Reconfirmed, no vendor edits. muse-precheck.sh 504: forbidden paths none, no hard-rule violations. GH-ADDVSPACE claim is state=closed, gh_ref correctly set to PR compiler: \addvspace inserts vertical space (documented simplification) #504 — a clean claim/close, same as noted last round; the precheck's "no active claim" WARN is the expected false positive for a properly closed claim.

New findings

1. (Minor, blocking only in the sense of "please fix before ready") PR body's Test results section is stale. It says "lib: 341 passed, incl. 2 new" and the mutation-check line says "test result: ok. 2 passed; 0 failed" — but the star-fix commit added a third test (addvspace_star_is_diagnosed_not_silently_tolerated). Current actual state: 342 lib tests total, 3 new, and re-running the mutation check today fails all 3, not 2. Please update both count mentions in the body before marking ready — purely a documentation freshness issue, not a code problem.

2. (Informational only, not a PR defect) flashtex build/flashtex check (via crates/flashtex-cli) cannot show this fix at all: flashtex-cli's Cargo.toml deliberately path-deps flashtex-compiler to the frozen crates/render-pipeline/vendor/compiler snapshot (pinned c95977d6, well behind main; that copy's vocabulary.rs still lists addvspace as unimplemented), not to crates/compiler. This is documented, expected project architecture (VENDORING.md, and the same "compiler: X" + later "render-pipeline: X" re-pin pattern visible in recent main history) — not something this PR should or could fix. Noted so nobody mistakes a flashtex check run against the CLI as a real test of this change; the JSON-worker repro above is the correct substitute.

3. (Process note for reviewer tooling, not a PR defect) The shared CARGO_TARGET_DIR is unsafe for invoking a crate's plain (non-hash-suffixed) [[bin]] output while other reviewers may be building the same crate concurrently — I first got a stale/clobbered flashtex-compiler binary reporting unsupported_feature, contradicting the passing cargo test output, until I rebuilt in an isolated target dir. cargo test results from the shared dir are trustworthy (hash-suffixed binaries); a find-located plain binary from it is not.

Everything substantive from the original review is now fixed and independently re-verified (real pdflatex comparison, real compiler-binary repro, mutation check). The only open item is the trivial test-count update in the body.

[d-q222]

daniel-parent review (subagent)

Verdict: CHANGES NEEDED

Prior findings

1. "max, not sum" for consecutive \addvspace calls — confirmed genuinely correct. Re-verified with a technique that avoids a real pitfall in naive \showthe\lastskip tests at the outer (page) vertical-list level: TeX's page builder drains the main vertical list's tail essentially immediately, so a bare \showthe\lastskip in the document body after a \vskip/\addvspace often reports 0 regardless of what really happened. Wrapping the probe in a \vbox{...} (an internal vertical list, not page-built) and capturing \lastskip into a global skip register just before the box closes gives a trustworthy reading. With that: \addvspace{5pt}\addvspace{10pt} and \addvspace{10pt}\addvspace{5pt} both leave \lastskip=10.0pt — real TeX genuinely keeps the max regardless of order, confirmed with pdflatex (TeX Live 2026). This central claim is correct and well disclosed.

2. Star-form fix — still fixed, reconfirmed. addvspace_star_is_diagnosed_not_silently_tolerated passes; \addvspace*{10pt} is diagnosed, not silently accepted. No regression.

3. (From the previous round) PR body's Test results section — still stale, NOT fixed. The body still reads "lib: 341 passed, incl. 2 new" and the mutation-check line still says "test result: ok. 2 passed; 0 failed". I rebuilt in a fresh, isolated CARGO_TARGET_DIR (the shared target-review-muse2 gave an inconsistent/stale result under concurrent use, consistent with the process note from last round) and reconfirmed the real current state: cargo test --lib addvspace → 3 passed (addvspace_inserts_vertical_space_without_an_unsupported_diagnostic, addvspace_twice_in_a_row_adds_safely_without_negative_space, addvspace_star_is_diagnosed_not_silently_tolerated), 339 filtered out → 342 lib tests total, not 341/"2 new". Mutation check (renamed the "addvspace" match arm) in the same isolated target dir: all 3 new tests fail (assertion failed: !BUILT_INS.contains(&name) at parser.rs:7251), restored and all 3 pass again. This was flagged by name as the one remaining item before marking ready last round and has not been touched.

New finding

4. The doc comment's/PR body's explanation of the \vspace/\bigskip interaction is factually wrong for the current oracle (TeX Live 2026), and says the opposite of what actually happens. Both the PR body and the new parser.rs doc comment claim: "\vspace/\bigskip glue next to an \addvspace is kept in full" (i.e., implying real TeX would not add anything on top there, unlike this compiler, which sums Block::VSpace entries). I verified this directly against /Library/TeX/texbin/pdflatex (TeX Live 2026, the same release already cited elsewhere in this PR):

   • In the current LaTeX2e kernel (<2025-11-01>), \bigskip/\medskip/\smallskip are no longer plain \vskip; \show\bigskip gives \vspace\bigskipamount. \vspace's expansion (\@vspace/\@vspacer) deliberately appends a zero-glue sentinel (\vskip\z@skip) after its own glue, specifically so a subsequent \addvspace — whose logic branches on \ifdim\lastskip=\z@ — always sees a "no preceding skip" state and unconditionally inserts its own argument, rather than comparing/merging.
   • Direct measurement (not just \lastskip, to sidestep any glue-comparison subtlety): a \vbox containing \hrule\bigskip\addvspace{10pt}\hrule measures 22.8pt tall, versus 12.8pt for \bigskip alone and 10.8pt for \addvspace{10pt} alone. Real TeX adds the two amounts together here — it does not keep the bigskip glue "in full" while discarding/no-oping the addvspace, and it does not top-up-to-the-max either. This is, incidentally, exactly what this compiler already does (summing consecutive Block::VSpace entries in the render-pipeline) — so the code is not wrong here, but the stated justification for why the simplification is safe is backwards on this specific, easy-to-hit point.
   • For contrast, I did confirm a genuine "avoids double-spacing" case exists in real TeX that this compiler does not replicate: \addvspace{10pt} immediately after an itemize environment's own trailing list-end skip is a complete no-op in real TeX (measured/\lastskip-confirmed identical before and after, natural component of the existing skip already ≥10pt) — but the PR's phrasing ("two consecutive calls add up") doesn't really cover this case either, since it's not two \addvspace calls, it's one \addvspace after unrelated list-end glue.

   Net: the headline "max, not sum" claim for genuine back-to-back \addvspace calls holds up (finding 1). But the more general/causal explanation offered for why — and the specific claim about \vspace/\bigskip adjacency — is inaccurate and should be corrected in both the PR body and the parser.rs comment (either drop the \vspace/\bigskip-specific claim, or replace it with the correct statement that modern \bigskip-family commands actually stack with a following \addvspace too, same as this compiler already does, and that the real gap is narrower: any single preceding vertical skip — from any source — whose value already meets or exceeds the \addvspace argument). This is a documentation-honesty issue, not a rendering-correctness bug — the compiler's actual behavior isn't shown to be wrong by any of this — but given this project's bar (and given the star-form claim was treated as a blocking issue last round for the same kind of "confidently wrong claim about real TeX" reason), this should be corrected before ready.

Reconfirmed, no issues

• No vendor/ changes; muse-precheck.sh 504 reports no forbidden paths, no hard-rule violations.
• cargo test --test supported_latex: 8/8 pass, including generated_artifacts_are_current (inventory genuinely regenerated).
• Worktree build (git fetch origin pull/504/head + detached worktree, isolated CARGO_TARGET_DIR): builds and tests cleanly; worktree removed after review.
• GH-ADDVSPACE claim: clean claim/close, no overlap.

Summary

Two items to fix before ready: (a) update the stale test-count numbers in the PR body (341→342, "2 new"→"3 new", mutation-check "2 passed"→"3 passed" — flagged last round, still unfixed), and (b) correct the \vspace/\bigskip-adjacency claim in the PR body and the parser.rs doc comment, since it's the opposite of what TeX Live 2026's actual kernel does. Both are documentation-only; I found no evidence the compiler's actual rendering behavior is wrong.

[d-q222]

daniel-muse-lead: merged origin/main, now MERGEABLE.

Merge (not rebase) origin/main into this branch. Real conflicts: vocabulary.rs (word-list union — combined both sides' additions), parser.rs (the penalty arm's error-diagnostic text had been independently corrected on main; kept main's fix, re-inserted the addvspace arm as a sibling match arm rather than inline with penalty). Generated artifacts (docs/user/compiler.md, both supported-latex.json copies, coverage.md) regenerated via sh crates/compiler/scripts/render_supported_latex.sh, not hand-merged.

One real post-merge bug found and fixed (not just conflict markup): main's outer command-dispatch list that routes bigskip/medskip/vspace/etc. into vertical_command had been reorganized independently and no longer included addvspace, so it silently fell through to the generic unsupported() catch-all and tripped its own debug_assert(!BUILT_INS.contains(name)) sanity check — all 3 tests failed identically on that assert. Fixed by adding addvspace back to that dispatch list alongside vspace. Also updated Block::VSpace construction/pattern-match sites for the stretch_pt/shrink_pt fields GH-VSKIP-GLUE-STRETCH (#606) added to main during the wind-down.

A second merge was needed for a review-fix commit (397a217d, correcting the \addvspace/\lastskip comment's real-TeX semantics) that landed on this branch's remote after my worktree was created — merged that in too, preserving the corrected comment text.

Literal test output after both merges, cargo test --lib addvspace (fresh CARGO_TARGET_DIR):

running 3 tests
test parser::tests::addvspace_star_is_diagnosed_not_silently_tolerated ... ok
test parser::tests::addvspace_inserts_vertical_space_without_an_unsupported_diagnostic ... ok
test parser::tests::addvspace_twice_in_a_row_adds_safely_without_negative_space ... ok

test result: ok. 3 passed; 0 failed; 0 ignored; 0 measured; 412 filtered out; finished in 0.01s

Full crates/compiler suite (fresh target dir, before the second merge but unaffected by it — only a comment changed): all suites green, 0 failures.

Now MERGEABLE against main.