From 739d4d37dff94320afdf77c9841ab0159602402d Mon Sep 17 00:00:00 2001 From: takeokunn Date: Mon, 3 Aug 2026 20:27:06 +0900 Subject: [PATCH] fix(syntax): stop the formatter dropping reader prefixes `edit format` silently dropped reader prefixes at exit 0, with output that reparses -- the corrupted-Lisp-still-reparses failure mode. On the Clojure standard library it turned (let ~(subvec bindings 0 2) ...) -> (let (subvec bindings 0 2) ...) and in system-fare-utils `(symbol-macrolet ,(loop ...) ,@body)` lost its unquote, after which the `loop` was re-laid-out as a binding list. **The stated mechanism was wrong in every particular.** `BODY_FORMS` lives in `reindent.rs`, whose only consumers are `SyntaxTree::reindent` and the `inspect indentation` report -- `edit format` never calls it, so the `("do", 2)` entry cannot affect this, and the theory did not explain the Common Lisp `let` repro at all. What actually happens: `Formatter::format_node` writes a node's reader prefixes before dispatching on its head, so a shape renderer may open its **own subject's** delimiter freely. Six renderers also open a *child's* delimiter -- the binding list of `let`/`do`, one binding entry, `flet`/`labels` bindings and each binding, and `cond`/`case` clauses -- and nothing wrote that child's prefix. The corruption is dialect-independent; fixing the dialect table would have left the CL `let` case broken. Fixed with a shared `carries_reader_prefix` predicate at those six sites, handing a prefixed child back to `format_node`. This is also the right *layout* answer: `` `(a b) `` in a `let`'s binding slot is a quasiquoted datum, not a binding list. Opaque reader forms needed no entry -- the parser gives them `NodeKind::Atom`, so the existing guard already covers them, verified before assuming it. Second bug, in the parser: a reader prefix immediately before a closing delimiter was dropped in every dialect -- `(a ')` parsed as `(a)`. `form()` accumulated prefixes, found a close delimiter, and called `close_list()` without consuming them. It now returns `MissingReaderForm`, exactly as a prefix at EOF already did, so `form` and its scanning twin `skip_form` finally agree. PR #96 pinned this as a *stated* expectation precisely so a fix would have to update it deliberately; that test now asserts the refusal. Differential over 5353 deduplicated files from nine dialects: formatted output differs between binaries : 27 tree-inequality (BEFORE binary) : 27 tree-inequality (AFTER binary) : 0 newly refused by the AFTER binary : 0 The 27 changed files are **exactly** the 27 the old binary corrupted; the other 5326 are byte-identical. The oracle is tree equality, added as invariant 6 in `tests/corpus.rs` with spans excluded. Its negative control is the important part: reverting one guard makes it fail with "reader_prefixes: [Unquote] became reader_prefixes: []" while **idempotence and the reparse check stay silent** -- which is why this went unnoticed. That oracle also cannot see the second bug, since input and output parse identically wrong; only the refusal catches it. Janet's column-sensitive long strings are deferred, and the reason is not effort: `stringend` strips `indent_col` bytes after each newline only if all of them are spaces, so moving the literal flips a mode rather than shifting a value. Both candidate fixes are real design decisions -- freezing every form containing one (29 of 210 corpus files, and in docstring-heavy Janet that is most `defn`s), or teaching the strictly top-down column model that an atom can pin its column. Scope measured with Janet itself as oracle: 24 of 210 files changed meaning before, 23 after, of which 22 are pure long-string cases and the 23rd is the separate bare-`@` reader divergence. treefmt formats 6 tracked files; all 6 are byte-identical under both binaries, so no recipe regeneration is needed. --- .../core/syntax/src/sexpr/formatter/core.rs | 27 +++ .../src/sexpr/formatter/lists/bindings.rs | 10 +- .../src/sexpr/formatter/lists/clauses.rs | 10 +- .../src/sexpr/formatter/lists/general.rs | 14 +- packages/core/syntax/src/sexpr/parser.rs | 15 +- .../core/syntax/src/sexpr/tests/formatter.rs | 169 ++++++++++++++++++ .../core/syntax/src/sexpr/tests/parser.rs | 113 ++++++++++-- tests/cli/format/binding_forms.rs | 51 ++++++ tests/corpus.rs | 80 ++++++++- 9 files changed, 464 insertions(+), 25 deletions(-) diff --git a/packages/core/syntax/src/sexpr/formatter/core.rs b/packages/core/syntax/src/sexpr/formatter/core.rs index e545576ae..1ea819949 100644 --- a/packages/core/syntax/src/sexpr/formatter/core.rs +++ b/packages/core/syntax/src/sexpr/formatter/core.rs @@ -1196,6 +1196,33 @@ impl Formatter { } } + /// Whether `node` carries reader-prefix text that only + /// [`Self::format_node`] emits. + /// + /// [`Self::format_node`] writes a node's prefixes (or their canonical + /// expansion) *before* it dispatches on the node's head, so every + /// shape-specific renderer in `formatter/lists` may open its own subject's + /// delimiter freely: the prefix is already out. What those renderers may + /// not do is open a *child's* delimiter, because nothing wrote that + /// child's prefix — and six of them did exactly that, for the child they + /// treat as a binding list, a binding entry, or a clause. + /// + /// The result was silent corruption rather than a visible defect: + /// `` (let `(a b) x) `` re-emitted as `(let (a b) x)` still parses, so it + /// passes every reparse-based write guard while meaning something else. + /// + /// Handing such a child back to [`Self::format_inline_or_node`] is also + /// the right *layout* answer and not merely the safe one — `` `(a b) `` in + /// a `let`'s binding slot is a quasiquoted datum, not a binding list, and + /// laying it out as the shape it is not was never correct. + /// + /// An opaque reader form (`#+sbcl (…)`) needs no entry here: the parser + /// gives it [`NodeKind::Atom`], so every one of those renderers already + /// falls through its `kind != List` guard. + pub(super) fn carries_reader_prefix(node: &Node) -> bool { + !node.reader_prefixes().is_empty() + } + pub(super) fn is_opaque_reader_form(&self, node: &Node) -> bool { node.opaque_reader_form || node diff --git a/packages/core/syntax/src/sexpr/formatter/lists/bindings.rs b/packages/core/syntax/src/sexpr/formatter/lists/bindings.rs index 5926f431a..d7d2a5981 100644 --- a/packages/core/syntax/src/sexpr/formatter/lists/bindings.rs +++ b/packages/core/syntax/src/sexpr/formatter/lists/bindings.rs @@ -79,7 +79,10 @@ impl Formatter { output: &mut String, ) { let node = tree.node(node_id); - if node.kind != NodeKind::List || node.children.is_empty() { + if node.kind != NodeKind::List + || node.children.is_empty() + || Self::carries_reader_prefix(node) + { self.format_inline_or_node(tree, node_id, depth, output); return; } @@ -104,7 +107,10 @@ impl Formatter { output: &mut String, ) { let node = tree.node(node_id); - if node.kind != NodeKind::List || node.children.len() <= 2 { + if node.kind != NodeKind::List + || node.children.len() <= 2 + || Self::carries_reader_prefix(node) + { self.format_inline_or_node(tree, node_id, depth, output); return; } diff --git a/packages/core/syntax/src/sexpr/formatter/lists/clauses.rs b/packages/core/syntax/src/sexpr/formatter/lists/clauses.rs index 59c867221..a6bed0c39 100644 --- a/packages/core/syntax/src/sexpr/formatter/lists/clauses.rs +++ b/packages/core/syntax/src/sexpr/formatter/lists/clauses.rs @@ -46,7 +46,10 @@ impl Formatter { output: &mut String, ) { let node = tree.node(node_id); - if node.kind != NodeKind::List || node.children.is_empty() { + if node.kind != NodeKind::List + || node.children.is_empty() + || Self::carries_reader_prefix(node) + { self.format_node(tree, node_id, depth, output); return; } @@ -131,7 +134,10 @@ impl Formatter { output: &mut String, ) { let node = tree.node(node_id); - if node.kind != NodeKind::List || node.children.len() <= 2 { + if node.kind != NodeKind::List + || node.children.len() <= 2 + || Self::carries_reader_prefix(node) + { self.format_inline_or_node(tree, node_id, depth, output); return; } diff --git a/packages/core/syntax/src/sexpr/formatter/lists/general.rs b/packages/core/syntax/src/sexpr/formatter/lists/general.rs index fd9348bb2..1bfcd9f29 100644 --- a/packages/core/syntax/src/sexpr/formatter/lists/general.rs +++ b/packages/core/syntax/src/sexpr/formatter/lists/general.rs @@ -50,7 +50,10 @@ impl Formatter { output: &mut String, ) { let node = tree.node(node_id); - if node.kind != NodeKind::List || node.children.is_empty() { + if node.kind != NodeKind::List + || node.children.is_empty() + || Formatter::carries_reader_prefix(node) + { self.format_inline_or_node(tree, node_id, depth, output); return; } @@ -159,7 +162,14 @@ impl Formatter { /// two-element list `(name value)` — the only shape alignment applies to. fn alignable_binding_name(&self, tree: &SyntaxTree, node_id: NodeId) -> Option { let node = tree.node(node_id); - if node.kind != NodeKind::List || node.children.len() != 2 { + // A prefixed entry is excluded for the same reason a bare symbol is — + // it is not the `(name value)` shape — and, additionally, because the + // aligned branch writes the entry's opening delimiter itself and would + // drop the prefix; see `Formatter::carries_reader_prefix`. + if node.kind != NodeKind::List + || node.children.len() != 2 + || Formatter::carries_reader_prefix(node) + { return None; } // Start column `0`, for the same reason as diff --git a/packages/core/syntax/src/sexpr/parser.rs b/packages/core/syntax/src/sexpr/parser.rs index 0b95babe7..eee8b7ea6 100644 --- a/packages/core/syntax/src/sexpr/parser.rs +++ b/packages/core/syntax/src/sexpr/parser.rs @@ -653,7 +653,20 @@ impl<'a> Parser<'a> { byte if self.policy.delimiter_from_open(byte).is_some() => { self.open_list_with_prefixes(prefixes); } - byte if self.policy.delimiter_from_close(byte).is_some() => self.close_list()?, + // A reader prefix with a closing delimiter after it is a truncated + // form, exactly as one at end of input is, and is refused the same + // way. This arm used to fall through to `close_list` while + // `prefixes` stayed on the floor, so `(a ')` parsed clean as `(a)`: + // the quote vanished from the tree, `edit format` re-emitted the + // document without it, and the result still parsed -- a silent + // meaning change no write guard could see. `skip_form`, the + // scanning twin of this function, already refused the same shape. + byte if self.policy.delimiter_from_close(byte).is_some() => { + if let Some(prefix) = prefixes.first() { + return Err(ParseError::MissingReaderForm(prefix.span.start().get())); + } + self.close_list()?; + } byte if DialectReaderPolicy::is_raw_delimiter(byte) => { return Err(self.raw_delimiter_error()); } diff --git a/packages/core/syntax/src/sexpr/tests/formatter.rs b/packages/core/syntax/src/sexpr/tests/formatter.rs index 2e87c8358..43540da59 100644 --- a/packages/core/syntax/src/sexpr/tests/formatter.rs +++ b/packages/core/syntax/src/sexpr/tests/formatter.rs @@ -2387,3 +2387,172 @@ fn no_line_is_indented_at_or_left_of_its_enclosing_delimiter() { } } } + +/// A reader prefix on a *child* the layout has a special shape for — a +/// binding list, an `flet` binding, a `cond`/`case` clause, a `do` var-list — +/// survives the reformat. +/// +/// [`Formatter::format_node`] writes a node's prefixes before it dispatches on +/// the node's head, so a renderer may open its own subject's delimiter freely. +/// Six renderers in `formatter/lists` opened a *child's* delimiter too, and +/// nothing had written that child's prefix: `` (let `(a b) x) `` came back as +/// `(let (a b) x)`. +/// +/// That is the worst shape a defect in this tool can take. The output still +/// parses, so every reparse-based write guard passes it, and formatting it +/// again reproduces it exactly, so idempotence passes too. Only comparing the +/// input tree with the output tree sees it — which is what +/// `no_shape_specific_layout_drops_a_child_reader_prefix` below does, and what +/// `tests/corpus.rs`'s invariant 6 does at scale. +#[test] +fn a_reader_prefix_on_a_shape_specific_child_survives_formatting() { + let cases = [ + // The primary-dialect repro. `let`'s second child is laid out by + // `format_sequence_list`, which opened `(` itself. + (Dialect::CommonLisp, "(let `(a b) x)", "(let `(a b)\n x)\n"), + (Dialect::CommonLisp, "(let '(a b) x)", "(let '(a b)\n x)\n"), + ( + Dialect::CommonLisp, + "(let* `(a b) x)", + "(let* `(a b)\n x)\n", + ), + // `flet`'s bindings list, via `format_local_callable_bindings` … + ( + Dialect::CommonLisp, + "(flet `((f (x) x)) y)", + "(flet `((f (x) x))\n y)\n", + ), + // … and one binding inside it, via `format_local_callable_binding`. + ( + Dialect::CommonLisp, + "(flet ('(f (x) x)) y)", + "(flet ('(f (x) x))\n y)\n", + ), + // `#.` is a prefix like any other here: dropping it turns a read-time + // computation into an ordinary call. + ( + Dialect::CommonLisp, + "(flet (#.(f (x) x)) y)", + "(flet (#.(f (x) x))\n y)\n", + ), + // `cond` and `case` clauses, via `format_body_clause`. + ( + Dialect::CommonLisp, + "(cond '(a b c))", + "(cond\n '(a b c))\n", + ), + ( + Dialect::CommonLisp, + "(case x '(a b c))", + "(case x\n '(a b c))\n", + ), + // `do`'s var-list, via `format_clause_sequence_form`. + ( + Dialect::CommonLisp, + "(do `((i 0 (1+ i))) ((> i 3)) x)", + "(do `((i 0 (1+ i)))\n ((> i 3))\n x)\n", + ), + // Not a Common Lisp defect wearing other dialects' clothes: the + // renderers are shared, so every dialect that reaches one has it. + ( + Dialect::Janet, + "(do x ~(def a b))", + "(do x\n ~(def a b))\n", + ), + ( + Dialect::Janet, + "(do x ,(def a b))", + "(do x\n ,(def a b))\n", + ), + (Dialect::Fennel, "(do x `(fn a b))", "(do x\n `(fn a b))\n"), + // Clojure's binding *vector* resolves to the same `ListStyle::Binding`. + (Dialect::Clojure, "(let `[a b] x)", "(let `[a b]\n x)\n"), + ]; + + for (dialect, input, expected) in cases { + let tree = SyntaxTree::parse_with_dialect(input, dialect).expect("valid"); + assert_eq!( + Formatter::with_dialect(2, dialect).format(&tree), + expected, + "{} / {input}", + dialect.label() + ); + } +} + +/// The same defect at a width that cannot compact, so the fallback is the +/// multi-line renderer rather than `compact_node`. +/// +/// Pinned separately because the two paths emit a prefix for different +/// reasons — `compact_node` writes `reader_prefix_spans` itself, while +/// `format_node` writes them before dispatching — so a fix reaching only one +/// of them would leave this case corrupting. +#[test] +fn a_reader_prefix_survives_on_a_binding_list_too_wide_to_compact() { + let input = "(let `((alpha 111111111) (beta 222222222) (gamma 33333333) \ + (delta 4444444) (epsilon 5555555)) x)"; + let tree = SyntaxTree::parse_with_dialect(input, Dialect::CommonLisp).expect("valid"); + let formatted = Formatter::with_dialect(2, Dialect::CommonLisp).format(&tree); + assert!( + formatted.starts_with("(let `("), + "quasiquote dropped from a wide binding list:\n{formatted}" + ); +} + +/// The tree-equality oracle as a test rather than as a corpus sweep: parse the +/// input, parse the formatted output, and compare the reader-prefix stack at +/// every expression. +/// +/// Spans are excluded because moving them is the formatter's whole job. +/// A prefix stack is not, and it is invisible to every other check in this +/// file, because dropping one leaves text that parses. +#[test] +fn no_shape_specific_layout_drops_a_child_reader_prefix() { + fn prefix_stacks(tree: &SyntaxTree) -> Vec> { + let root = tree.root_view(); + let mut stacks = Vec::new(); + let mut stack = vec![&root]; + while let Some(view) = stack.pop() { + stacks.push(view.reader_prefixes.clone()); + stack.extend(view.children.iter().rev()); + } + stacks + } + + let cases = [ + (Dialect::CommonLisp, "(let `(a b) x)"), + (Dialect::CommonLisp, "(let* '((x 1)) x)"), + (Dialect::CommonLisp, "(flet `((f (x) x)) y)"), + (Dialect::CommonLisp, "(labels ('(f (x) (g x))) y)"), + (Dialect::CommonLisp, "(cond '(a b c) `(d e f))"), + (Dialect::CommonLisp, "(case x '(a b c))"), + (Dialect::CommonLisp, "(ecase x `((a) b c))"), + (Dialect::CommonLisp, "(do `((i 0 (1+ i))) '((> i 3) r) x)"), + ( + Dialect::CommonLisp, + "(defmacro m (v) `(symbol-macrolet ,(loop for x in v collect x) body))", + ), + (Dialect::Clojure, "(let `[a b] x)"), + (Dialect::Clojure, "(with-open `[a b] body)"), + (Dialect::Janet, "(do x ~(def a b))"), + (Dialect::Fennel, "(do x `(fn a b))"), + // A prefix on the last element of a list, which is where an + // off-by-one in prefix handling shows up first. + (Dialect::CommonLisp, "(a b 'c)"), + (Dialect::CommonLisp, "(let ((x 1)) 'y)"), + (Dialect::CommonLisp, "(cond (a 'b) (t 'c))"), + ]; + + for (dialect, input) in cases { + let tree = SyntaxTree::parse_with_dialect(input, dialect).expect("valid input"); + let formatted = Formatter::with_dialect(2, dialect).format(&tree); + let reparsed = + SyntaxTree::parse_with_dialect(&formatted, dialect).expect("formatted output reparses"); + assert_eq!( + prefix_stacks(&tree), + prefix_stacks(&reparsed), + "{} / {input} formatted to:\n{formatted}", + dialect.label() + ); + } +} diff --git a/packages/core/syntax/src/sexpr/tests/parser.rs b/packages/core/syntax/src/sexpr/tests/parser.rs index fa001fc92..14b736147 100644 --- a/packages/core/syntax/src/sexpr/tests/parser.rs +++ b/packages/core/syntax/src/sexpr/tests/parser.rs @@ -567,6 +567,87 @@ fn rejects_reader_prefixes_without_a_form() { } } +/// A reader prefix with a *closing delimiter* after it is refused, in every +/// dialect, exactly as one at end of input is. +/// +/// It used to parse clean with the prefix silently dropped: `form` collected +/// the prefixes, found `)` instead of a datum, and called `close_list` +/// without ever consuming them. `(a ')` therefore became the tree for `(a)`, +/// `edit format` re-emitted it without the quote, and the result parsed — a +/// meaning change no reparse-based write guard could see. `skip_form`, the +/// scanning twin of `form`, already refused the same shape. +/// +/// The position reported is the *prefix's*, not the delimiter's, matching +/// [`ParseError::MissingReaderForm`]'s end-of-input spelling above. +#[test] +fn rejects_a_reader_prefix_before_a_closing_delimiter() { + let dialects = [ + Dialect::CommonLisp, + Dialect::EmacsLisp, + Dialect::Scheme, + Dialect::Racket, + Dialect::Clojure, + Dialect::Fennel, + Dialect::Lfe, + Dialect::Unknown, + ]; + + for dialect in dialects { + assert_eq!( + SyntaxTree::parse_with_dialect("(a ')", dialect).unwrap_err(), + ParseError::MissingReaderForm(3), + "dialect: {}", + dialect.label() + ); + assert_eq!( + SyntaxTree::parse_with_dialect("(a `)", dialect).unwrap_err(), + ParseError::MissingReaderForm(3), + "dialect: {}", + dialect.label() + ); + // Nested, and alone, so the fix is not merely about a top-level list. + assert_eq!( + SyntaxTree::parse_with_dialect("(a (b ') c)", dialect).unwrap_err(), + ParseError::MissingReaderForm(6), + "dialect: {}", + dialect.label() + ); + assert_eq!( + SyntaxTree::parse_with_dialect("(')", dialect).unwrap_err(), + ParseError::MissingReaderForm(1), + "dialect: {}", + dialect.label() + ); + // A prefix that *does* have a form after it is untouched, including on + // the last element of a list — the neighbouring case a fix here could + // plausibly over-reach into. + let tree = SyntaxTree::parse_with_dialect("(a 'b)", dialect) + .unwrap_or_else(|error| panic!("{} rejected (a 'b): {error:?}", dialect.label())); + assert_eq!(tree.root_view().children[0].children.len(), 2); + assert_eq!( + tree.root_view().children[0].children[1].reader_prefixes, + vec![ReaderPrefix::Quote], + "dialect: {}", + dialect.label() + ); + } +} + +/// A stray closing delimiter with no prefix in front of it still reports the +/// delimiter, not a missing reader form — the prefix check added for +/// `rejects_a_reader_prefix_before_a_closing_delimiter` must not swallow the +/// pre-existing diagnostic. +#[test] +fn a_bare_stray_closing_delimiter_still_reports_itself() { + assert_eq!( + SyntaxTree::parse(")").unwrap_err(), + ParseError::UnexpectedClose { + delimiter: ')', + position: 0 + } + ); +} + #[test] fn rejects_reader_comments_without_a_form() { for input in ["#;", "#_"] { @@ -1555,23 +1636,23 @@ fn janet_dangling_quote_is_refused() { "{error:?}" ); - // `(a ')` is *not* refused, and that is a known defect rather than an - // intended rule. `form` accumulates prefixes, finds a closing delimiter - // rather than a datum, and calls `close_list` without ever consuming the - // prefixes it collected -- so the quote is dropped and the document parses - // clean. Janet refuses the same input ("mismatched delimiter )"). + // `(a ')` is now refused too, which is the change this stated expectation + // was written to force. `form` used to accumulate prefixes, find a closing + // delimiter rather than a datum, and call `close_list` without ever + // consuming them -- so the quote was dropped and the document parsed clean. + // Janet refuses the same input ("mismatched delimiter )"), and so does + // every other dialect here now; the defect was cross-dialect and + // pre-existing, and `skip_form` -- the scanning twin of `form` -- had + // already been refusing this shape, so the two paths now agree. // - // It is pinned here because it is *cross-dialect and pre-existing*: Common - // Lisp, Scheme, Clojure, Emacs Lisp, Fennel and the legacy reader all drop - // it identically, on this commit and before it. Fixing it means changing - // every dialect's behaviour, which is a separate change from adding a - // Janet reader arm; this test exists so that change has to update a stated - // expectation instead of a silent one. - let tree = SyntaxTree::parse_with_dialect("(a ')", Dialect::Janet) - .expect("known defect: the prefix is dropped rather than refused"); - let form = &tree.root_view().children[0]; - assert_eq!(form.children.len(), 1, "the quote is dropped, not recorded"); - assert!(form.children[0].reader_prefixes.is_empty()); + // See `rejects_a_reader_prefix_before_a_closing_delimiter` for the full + // eight-dialect matrix and the `(a 'b)` control. + let error = SyntaxTree::parse_with_dialect("(a ')", Dialect::Janet) + .expect_err("a prefix with a closing delimiter after it is not a form"); + assert!( + matches!(error, ParseError::MissingReaderForm(3)), + "{error:?}" + ); } /// `'` keeps meaning quote everywhere else, and this pins the other dialects so diff --git a/tests/cli/format/binding_forms.rs b/tests/cli/format/binding_forms.rs index 450d0d7e6..389186216 100644 --- a/tests/cli/format/binding_forms.rs +++ b/tests/cli/format/binding_forms.rs @@ -1,4 +1,55 @@ use super::assert_format_output; +use super::{fresh_temp_dir, fs, paredit, predicate}; +use std::path::Path; + +/// The `--write` path is where a dropped reader prefix actually destroys +/// someone's file, so it is pinned through the real binary and against the +/// bytes on disk, not only through the library. +/// +/// `treefmt` runs exactly this command over this repository's own tracked Lisp +/// files (`flake.nix`, `mkFormatFilesFor`), so a formatter that changes meaning +/// is a build hazard as well as a correctness defect. +#[test] +fn cli_format_write_keeps_a_reader_prefix_on_a_binding_list() { + let dir = fresh_temp_dir("format-write-binding-prefix"); + let file = dir.join(Path::new("macro.lisp")); + fs::write(&file, "(let `(a b) x)\n").expect("write fixture"); + + paredit() + .args(["edit", "format", "--write", "--file"]) + .arg(&file) + .assert() + .success(); + + assert_eq!( + fs::read_to_string(&file).expect("read rewritten fixture"), + "(let `(a b)\n x)\n" + ); +} + +/// `(a ')` is a truncated form, and is refused rather than rewritten to `(a)`. +/// +/// The refusal matters more than the diagnostic: the old behaviour left a +/// document that parsed, so `--write` replaced the file, `--check` called it +/// formatted, and nothing downstream had any way to notice the quote was gone. +#[test] +fn cli_format_refuses_a_reader_prefix_before_a_closing_delimiter() { + let dir = fresh_temp_dir("format-dangling-prefix"); + let file = dir.join(Path::new("truncated.lisp")); + fs::write(&file, "(a ')\n").expect("write fixture"); + + paredit() + .args(["edit", "format", "--write", "--file"]) + .arg(&file) + .assert() + .failure() + .stderr(predicate::str::contains("is missing a form")); + + assert_eq!( + fs::read_to_string(&file).expect("read untouched fixture"), + "(a ')\n" + ); +} #[test] fn cli_formats_symbol_macrolet_indentation() { diff --git a/tests/corpus.rs b/tests/corpus.rs index 2bfea0aa4..3b7b2d118 100644 --- a/tests/corpus.rs +++ b/tests/corpus.rs @@ -6,7 +6,7 @@ //! closes it by asserting *invariants* rather than outputs, over code the //! authors did not write. //! -//! Five invariants, each of which holds regardless of what the file contains: +//! Six invariants, each of which holds regardless of what the file contains: //! //! 1. **Parsing terminates without panicking.** A parse *error* is fine — real //! Lisp carries reader macros this tool does not model — but a panic is a @@ -27,6 +27,16 @@ //! oracle from outside the round-trip: a child rendered at or left of the //! delimiter that contains it no longer reads as being inside it, in any //! Lisp anyone writes. +//! 6. **Formatting preserves the tree.** Parse the input, parse the formatted +//! output, and compare the two trees with spans excluded — spans are the +//! one thing formatting is meant to change. This is the second oracle from +//! outside the round-trip, and it catches what invariant 3 structurally +//! cannot: a formatter that *drops* something renders the same wrong text +//! on every pass, so idempotence holds, and the corrupted text still +//! parses, so the reparse check holds too. Six real dialects' worth of +//! `edit format` silently dropping a reader prefix +//! (`` (let `(a b) x) `` → `(let (a b) x)`) passed invariants 1-5 and +//! failed only this one. //! //! ## Where the corpus comes from //! @@ -146,6 +156,45 @@ const MAX_COLUMN_VIOLATIONS_PER_FILE: usize = 5; /// A byte range in the formatted text, half open. type Region = (usize, usize); +/// One expression's identity for invariant 6, spans deliberately excluded. +/// +/// Spans are the one thing formatting is *supposed* to change, so +/// [`paredit_cli::sexpr::ExpressionView`]'s own `PartialEq` — which compares +/// them — cannot answer this question. What is compared instead is everything +/// the reader would have had to see differently for the document to mean +/// something else: the node kind, which delimiter pair it uses, its +/// reader-prefix stack, and an atom's text. +#[derive(PartialEq, Eq, Debug)] +struct Shape { + kind: ExpressionKind, + delimiter: Option, + reader_prefixes: Vec, + text: Option, + children: usize, +} + +/// Every expression of `tree`, in a fixed pre-order, as [`Shape`]s. +/// +/// Flattened rather than compared tree-against-tree so that a mismatch names a +/// position, and iterative for the same reason [`Layout::of`] is: corpus files +/// nest deeply enough to overflow a test thread's stack. +fn shapes(tree: &SyntaxTree) -> Vec { + let root = tree.root_view(); + let mut shapes = Vec::new(); + let mut stack = vec![&root]; + while let Some(view) = stack.pop() { + shapes.push(Shape { + kind: view.kind, + delimiter: view.delimiter, + reader_prefixes: view.reader_prefixes.clone(), + text: view.text.clone(), + children: view.children.len(), + }); + stack.extend(view.children.iter().rev()); + } + shapes +} + /// The lists and the opaque tokens of one parsed document. /// /// Both are collected from the parse tree rather than by re-scanning the text, @@ -444,7 +493,7 @@ struct Tally { verbatim_lines: usize, } -/// Checks the five invariants on one file, returning failures rather than +/// Checks the six invariants on one file, returning failures rather than /// panicking so one run reports every offender. fn check_file(path: &Path, tally: &mut Tally) -> Vec { let Ok(metadata) = fs::metadata(path) else { @@ -502,6 +551,33 @@ fn check_file(path: &Path, tally: &mut Tally) -> Vec { )); } + // Invariant 6: formatting preserves the tree. Idempotence + // (invariant 3) cannot see a dropped reader prefix — a + // consistently wrong rendering is a fixed point, and the + // corrupted text reparses cleanly, so invariant 3 and the + // reparse check above both pass on it. Comparing the input's + // tree with the output's is an oracle from outside that + // round-trip, and it is what caught `` (let `(a b) x) `` + // rendering as `(let (a b) x)`. + let before = shapes(&tree); + let after = shapes(&reparsed); + if before != after { + let first = before + .iter() + .zip(&after) + .position(|(left, right)| left != right); + failures.push(format!( + "{}: formatting changed the tree ({} expressions in, {} out{})", + path.display(), + before.len(), + after.len(), + first.map_or(String::new(), |index| format!( + "; first difference at expression {index}: {:?} became {:?}", + before[index], after[index] + )) + )); + } + // Invariant 5: every line sits inside the form that encloses it. // Checked on the formatted text, not on the input: the input's // layout is whoever wrote it, and this is an assertion about what