Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 27 additions & 0 deletions packages/core/syntax/src/sexpr/formatter/core.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
10 changes: 8 additions & 2 deletions packages/core/syntax/src/sexpr/formatter/lists/bindings.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
Expand All @@ -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;
}
Expand Down
10 changes: 8 additions & 2 deletions packages/core/syntax/src/sexpr/formatter/lists/clauses.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
Expand Down Expand Up @@ -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;
}
Expand Down
14 changes: 12 additions & 2 deletions packages/core/syntax/src/sexpr/formatter/lists/general.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
Expand Down Expand Up @@ -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<String> {
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
Expand Down
15 changes: 14 additions & 1 deletion packages/core/syntax/src/sexpr/parser.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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());
}
Expand Down
169 changes: 169 additions & 0 deletions packages/core/syntax/src/sexpr/tests/formatter.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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<Vec<crate::sexpr::ReaderPrefix>> {
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()
);
}
}
Loading