From 3de39dbcb227cb84f58bd79efcabbc4991685cb3 Mon Sep 17 00:00:00 2001 From: David Sherret Date: Sat, 12 Sep 2026 12:12:01 -0400 Subject: [PATCH 1/5] feat(cst): add sort_properties_by and sort_elements_by Reorders an object's properties or an array's elements, moving what was written with each one along with it. The children of a container read as `open [ sep lead element trail ]... tail close`. The lead (the comments and blank lines above an element) and the trail (its comma, and a comment written after it on the same line) travel with the element. The separator -- the line break that ended the previous element's line plus the indentation under it, or on a single line the space between the two -- stays put, since it positions whatever comes next rather than belonging to either element. So does the tail, and so does anything written on the open token's line. Commas are re-decided per position, carrying over whichever of a trailing comma or none the container was written with, and a comma is added where the new order needs one the author left out. The comma is claimed wherever it was written, including on a later line, so leading-comma style can't leave one stranded in front of the element that follows. Two normalizations are needed to keep the result meaning what it did. A blank line that lands directly under the open token is dropped, since a gap there reads as belonging to the container. And a line comment that no longer ends its line gains a line break, without which it would comment out the next element or the closing token. `name_decoded` becomes the public `decoded_name`, since it is the natural sort key and was private and gated behind the serde_json feature. Its name now matches `decoded_value` elsewhere in the crate. tests/sort_fuzz.rs checks 40,000 generated documents -- trivia in every slot, blank lines, CRLF, missing and trailing commas, duplicate and escaped names -- asserting each result re-parses, holds the same members in the comparator's order, keeps every comment, and sorts idempotently. --- src/cst/mod.rs | 598 ++++++++++++++++++++++++++++++++++++++++++++- tests/sort_fuzz.rs | 306 +++++++++++++++++++++++ 2 files changed, 895 insertions(+), 9 deletions(-) create mode 100644 tests/sort_fuzz.rs diff --git a/src/cst/mod.rs b/src/cst/mod.rs index 46c36e9..4f3ab5d 100644 --- a/src/cst/mod.rs +++ b/src/cst/mod.rs @@ -1,5 +1,10 @@ //! CST for manipulating JSONC. //! +//! Unlike the AST, this keeps every comment and every piece of whitespace, so a document can be +//! edited and written back out with everything the author wrote still in place. Properties and +//! elements can also be reordered with [`CstObject::sort_properties_by`] and +//! [`CstArray::sort_elements_by`], which carry each one's comments along with it. +//! //! # Example //! //! ``` @@ -31,6 +36,7 @@ //! use std::cell::RefCell; +use std::cmp::Ordering; use std::collections::VecDeque; use std::fmt::Display; use std::iter::Peekable; @@ -769,6 +775,28 @@ impl CstContainerNode { self.raw_insert_children(None, children); } + /// Replaces every child of this container, reparenting the new children. + fn raw_set_children(&self, children: Vec) { + let weak_parent = WeakParent::from_container(self); + let mut container = match self { + CstContainerNode::Root(node) => node.0.borrow_mut(), + CstContainerNode::Object(node) => node.0.borrow_mut(), + CstContainerNode::ObjectProp(node) => node.0.borrow_mut(), + CstContainerNode::Array(node) => node.0.borrow_mut(), + }; + // a child that isn't in the new list has left the tree, so it loses its parent + for child in &container.value { + child.set_parent(None); + } + container.value = children; + for (i, child) in container.value.iter().enumerate() { + child.set_parent(Some(ParentInfo { + parent: weak_parent.clone(), + child_index: i, + })); + } + } + fn raw_insert_children(&self, index: Option<&mut usize>, children: Vec) { if children.is_empty() { return; @@ -1767,6 +1795,72 @@ impl CstObject { self.insert_or_append(Some(index), prop_name, value) } + /// Sorts the properties of the object with the given comparator. + /// + /// What was written with a property travels with it: the comments and blank lines above it, its + /// indentation, and a comment written after it on the same line. What belongs to no property + /// stays where it is, which includes whatever follows the open brace and whatever precedes the + /// close brace. Each property gains or loses a comma to suit its new position, and whether the + /// object ends with a trailing comma is preserved. + /// + /// Two exceptions are worth knowing about. A blank line that ends up directly under the open + /// brace is dropped rather than moved, since a gap there reads as belonging to the object rather + /// than to the property beneath it. And a line comment that no longer ends its line gains a line + /// break, so that it can't comment out whatever follows it. + /// + /// The sort is stable, so properties that compare equal keep the order they were written in. The + /// comparator must not modify this object, as changes made while it runs are discarded, and it + /// must describe a total order, as [`slice::sort_by`] panics otherwise. + /// + /// # Example + /// + /// ``` + /// use jsonc_parser::ParseOptions; + /// use jsonc_parser::cst::CstRootNode; + /// + /// let json_text = r#"{ + /// "b": 2, // written about b + /// // written about a + /// "a": 1 + /// }"#; + /// + /// let root = CstRootNode::parse(json_text, &ParseOptions::default()).unwrap(); + /// let root_obj = root.object_value().unwrap(); + /// root_obj.sort_properties_by_key(|prop| prop.decoded_name()); + /// + /// assert_eq!(root.to_string(), r#"{ + /// // written about a + /// "a": 1, + /// "b": 2 // written about b + /// }"#); + /// ``` + pub fn sort_properties_by(&self, mut compare: impl FnMut(&CstObjectProp, &CstObjectProp) -> Ordering) { + sort_comma_separated_children(&self.clone().into(), |groups| { + groups.sort_by( + |left, right| match (left.element.as_object_prop(), right.element.as_object_prop()) { + (Some(left), Some(right)) => compare(&left, &right), + // an object holds properties, so this only happens if the tree has been manipulated into + // holding something else, in which case leaving the order alone is the safe answer + _ => Ordering::Equal, + }, + ) + }); + } + + /// Sorts the properties of the object by a key, which is worked out once per property. + /// + /// Behaves like [`CstObject::sort_properties_by`] in every other respect. + pub fn sort_properties_by_key(&self, mut key: impl FnMut(&CstObjectProp) -> K) { + sort_comma_separated_children(&self.clone().into(), |groups| { + let mut keyed = std::mem::take(groups) + .into_iter() + .map(|group| (group.element.as_object_prop().map(|prop| key(&prop)), group)) + .collect::>(); + keyed.sort_by(|left, right| left.0.cmp(&right.0)); + groups.extend(keyed.into_iter().map(|(_, group)| group)); + }); + } + fn insert_or_append(&self, index: Option, prop_name: &str, value: CstInputValue) -> CstObjectProp { self.ensure_multiline(); insert_or_append_to_container( @@ -1808,7 +1902,7 @@ impl CstObject { pub fn to_serde_value(&self) -> Option { let mut map = serde_json::map::Map::new(); for prop in self.properties() { - if let (Some(name), Some(value)) = (prop.name_decoded(), prop.to_serde_value()) { + if let (Some(name), Some(value)) = (prop.decoded_name(), prop.to_serde_value()) { map.insert(name, value); } } @@ -1856,6 +1950,17 @@ impl CstObjectProp { None } + /// Name of the object property with any escapes in it resolved. + /// + /// Returns `None` if the name doesn't exist or can't be decoded, which sorts such a property + /// above every property whose name does decode. + pub fn decoded_name(&self) -> Option { + match self.name()? { + ObjectPropName::String(s) => s.decoded_value().ok(), + ObjectPropName::Word(w) => Some(w.0.borrow().value.clone()), + } + } + pub fn property_index(&self) -> usize { let child_index = self.child_index(); let Some(parent) = self.parent().and_then(|p| p.as_object()) else { @@ -1999,14 +2104,6 @@ impl CstObjectProp { pub fn to_serde_value(&self) -> Option { self.value()?.to_serde_value() } - - #[cfg(feature = "serde_json")] - fn name_decoded(&self) -> Option { - match self.name()? { - ObjectPropName::String(s) => s.decoded_value().ok(), - ObjectPropName::Word(w) => Some(w.0.borrow().value.clone()), - } - } } impl Display for CstObjectProp { @@ -2130,6 +2227,64 @@ impl CstArray { self.insert_or_append(Some(index), value) } + /// Sorts the elements of the array with the given comparator. + /// + /// What was written with an element travels with it: the comments and blank lines above it, its + /// indentation, and a comment written after it on the same line. What belongs to no element + /// stays where it is, which includes whatever follows the open bracket and whatever precedes the + /// close bracket. Each element gains or loses a comma to suit its new position, and whether the + /// array ends with a trailing comma is preserved. + /// + /// A blank line that ends up directly under the open bracket is dropped rather than moved, and a + /// line comment that no longer ends its line gains a line break so that it can't comment out + /// whatever follows it. + /// + /// The sort is stable, so elements that compare equal keep the order they were written in. The + /// comparator must not modify this array, as changes made while it runs are discarded, and it + /// must describe a total order, as [`slice::sort_by`] panics otherwise. + /// + /// # Example + /// + /// ``` + /// use jsonc_parser::ParseOptions; + /// use jsonc_parser::cst::CstRootNode; + /// + /// let json_text = r#"[ + /// "b", // written about b + /// // written about a + /// "a" + /// ]"#; + /// + /// let root = CstRootNode::parse(json_text, &ParseOptions::default()).unwrap(); + /// let array = root.array_value().unwrap(); + /// array.sort_elements_by_key(|element| element.to_string()); + /// + /// assert_eq!(root.to_string(), r#"[ + /// // written about a + /// "a", + /// "b" // written about b + /// ]"#); + /// ``` + pub fn sort_elements_by(&self, mut compare: impl FnMut(&CstNode, &CstNode) -> Ordering) { + sort_comma_separated_children(&self.clone().into(), |groups| { + groups.sort_by(|left, right| compare(&left.element, &right.element)) + }); + } + + /// Sorts the elements of the array by a key, which is worked out once per element. + /// + /// Behaves like [`CstArray::sort_elements_by`] in every other respect. + pub fn sort_elements_by_key(&self, mut key: impl FnMut(&CstNode) -> K) { + sort_comma_separated_children(&self.clone().into(), |groups| { + let mut keyed = std::mem::take(groups) + .into_iter() + .map(|group| (key(&group.element), group)) + .collect::>(); + keyed.sort_by(|left, right| left.0.cmp(&right.0)); + groups.extend(keyed.into_iter().map(|(_, group)| group)); + }); + } + /// Ensures the array spans multiple lines. pub fn ensure_multiline(&self) { ensure_multiline(&self.clone().into()); @@ -2523,6 +2678,227 @@ impl<'a> CstBuilder<'a> { } } +/// What sits between two elements and stays where it is, because it positions whatever comes next +/// rather than belonging to either element. +struct Separator { + /// The line break that ended the previous element line, or on a single line the space between + /// the two elements. + before: Vec, + /// The indentation directly in front of the element. + indent: Vec, +} + +/// An element of a comma separated container along with the trivia that travels with it. +struct SortableGroup { + /// Where the element was written, so that a sort changing nothing can leave the tree alone. + index: usize, + /// What was written before the element and belongs to it: its comments and the blank lines above it. + leading: Vec, + element: CstNode, + /// Whatever separates the element from its comma, the comma, and any comment written after that + /// on the same line. + trailing: Vec, +} + +/// Reorders the elements of an object or array, moving what was written with each element along +/// with it and leaving the separators between them where they are. +/// +/// `sort` is handed the groups in the order they were written and is expected to sort them stably. +fn sort_comma_separated_children(container: &CstContainerNode, sort: impl FnOnce(&mut Vec)) { + let children = container.children(); + // the surrounding tokens are what the elements sit between, so there's nothing to sort without them + if children.len() < 2 || !children[0].is_token() || !children[children.len() - 1].is_token() { + return; + } + let region = &children[1..children.len() - 1]; + + // Split the region into the groups that move and the separators that stay put. Each group is + // preceded by exactly one separator, so the two line up. + let mut separators: Vec = Vec::new(); + let mut groups: Vec = Vec::new(); + let mut index = 0; + let tail = loop { + let run_start = index; + while index < region.len() && !is_sortable_element(®ion[index]) { + index += 1; + } + if index == region.len() { + // what follows the last element belongs to no element and stays where it is + break region[run_start..].to_vec(); + } + let (separator, leading) = split_separator(®ion[run_start..index]); + separators.push(separator); + let element = region[index].clone(); + let trailing_end = trailing_run_end(region, index + 1); + groups.push(SortableGroup { + index: groups.len(), + leading, + element, + trailing: region[index + 1..trailing_end].to_vec(), + }); + index = trailing_end; + }; + + if groups.len() < 2 { + return; + } + + // whether the author ended the container with a comma, which the new last element takes over + let ends_with_comma = groups[groups.len() - 1].trailing.iter().any(|n| n.is_comma()); + sort(&mut groups); + if groups + .iter() + .enumerate() + .all(|(position, group)| position == group.index) + { + return; + } + + let last_index = groups.len() - 1; + for (position, group) in groups.iter_mut().enumerate() { + set_group_comma(group, position < last_index || ends_with_comma); + } + // a blank line here reads as a gap under the open token rather than as something written with + // the element that follows, so it doesn't travel with whatever sorted to the top + let first_leading = &mut groups[0].leading; + let blank_count = first_leading.iter().take_while(|n| n.is_newline()).count(); + first_leading.drain(..blank_count); + + let mut new_children = Vec::with_capacity(children.len()); + new_children.push(children[0].clone()); + for (separator, group) in separators.into_iter().zip(groups) { + new_children.extend(separator.before); + new_children.extend(group.leading); + new_children.extend(separator.indent); + new_children.push(group.element); + new_children.extend(group.trailing); + } + new_children.extend(tail); + new_children.push(children[children.len() - 1].clone()); + let newline_kind = container + .root_node() + .map(|root| root.newline_kind()) + .unwrap_or(CstNewlineKind::LineFeed); + restore_line_comment_line_ends(&mut new_children, newline_kind); + container.raw_set_children(new_children); +} + +/// Whether the node is something an object or array holds rather than the punctuation and trivia +/// written around it. +fn is_sortable_element(node: &CstNode) -> bool { + !node.is_trivia() && !node.is_token() +} + +/// Splits what was written between two elements into the separator, which stays where it is, and +/// the trivia belonging to the element that follows. +/// +/// The separator is the line break that ended the previous element's line together with the +/// indentation under it, or on a single line the whitespace between the two elements. Both +/// position whatever comes next, so they belong to the slot rather than to either element. What +/// sits between them, such as blank lines and the comments written above the element, came with +/// that element and travels with it. +fn split_separator(run: &[CstNode]) -> (Separator, Vec) { + let Some(newline) = run.iter().position(|n| n.is_newline()) else { + // nothing indents anything on a single line, so all that's here is the space between the two + let before = run.iter().take_while(|n| n.is_whitespace()).count(); + return ( + Separator { + before: run[..before].to_vec(), + indent: Vec::new(), + }, + run[before..].to_vec(), + ); + }; + let leading_start = newline + 1; + let indent_len = run[leading_start..] + .iter() + .rev() + .take_while(|n| n.is_whitespace()) + .count(); + let indent_start = run.len() - indent_len; + ( + Separator { + before: run[..leading_start].to_vec(), + indent: run[indent_start..].to_vec(), + }, + run[leading_start..indent_start].to_vec(), + ) +} + +/// The end of the run after an element that was written with it: whatever separates the element +/// from its comma, the comma itself, and any comment written after that on the same line. +/// +/// The comma comes along wherever the author put it, including on a later line, so that it can +/// never be mistaken for something belonging to the element that follows. +fn trailing_run_end(region: &[CstNode], start: usize) -> usize { + let mut end = start; + for (index, node) in region.iter().enumerate().skip(start) { + if is_sortable_element(node) { + break; + } else if node.is_comma() { + end = index + 1; + break; + } + } + // a comment after that was written with the element too, but only when nothing else shares its line + if rest_of_line_is_trivia(region, end) { + for (index, node) in region.iter().enumerate().skip(end) { + if node.is_newline() { + break; + } else if node.is_comment() { + end = index + 1; + } + } + } + end +} + +/// Whether the rest of the line holds nothing but whitespace and comments, which is what decides +/// whether a comment there was written with what precedes it or with what follows. +fn rest_of_line_is_trivia(region: &[CstNode], start: usize) -> bool { + region + .iter() + .skip(start) + .take_while(|n| !n.is_newline()) + .all(|n| n.is_whitespace() || n.is_comment()) +} + +/// Adds or removes the element's comma so that it suits the element's new position. +fn set_group_comma(group: &mut SortableGroup, wants_comma: bool) { + match group.trailing.iter().position(|n| n.is_comma()) { + Some(index) if !wants_comma => { + group.trailing.remove(index); + // the space that offset the comma has nothing left to offset + if index > 0 && group.trailing[index - 1].is_whitespace() { + group.trailing.remove(index - 1); + } + } + None if wants_comma => group.trailing.insert(0, CstToken::new(',').into()), + _ => {} + } +} + +/// Puts back the line break a line comment needs in order to end where it did. +/// +/// A line comment runs to the end of its line, so moving one can leave it in front of what used to +/// come earlier, commenting out the next element or the closing token. +fn restore_line_comment_line_ends(children: &mut Vec, newline_kind: CstNewlineKind) { + let mut index = 0; + while index < children.len() { + if is_line_comment(&children[index]) + && let Some(next) = children[index + 1..].iter().position(|n| !n.is_whitespace()) + && !children[index + 1 + next].is_newline() + { + children.insert(index + 1, CstNewline::new(newline_kind).into()); + } + index += 1; + } +} + +fn is_line_comment(node: &CstNode) -> bool { + matches!(node, CstNode::Leaf(CstLeafNode::Comment(comment)) if comment.is_line_comment()) +} + fn remove_comma_separated(node: CstNode) { fn check_next_node_same_line(trailing_comma: &CstToken) -> bool { for sibling in trailing_comma.next_siblings() { @@ -3655,6 +4031,7 @@ value3: true #[test] fn remove_comment() { + #[track_caller] fn run_test(json: &str, expected: &str) { let cst = build_cst(json); let root_value = cst.value().unwrap(); @@ -4078,6 +4455,209 @@ value3: true CstRootNode::parse("[1, 2]", &options).unwrap(); } + #[test] + fn sort_properties() { + #[track_caller] + fn run_test(json: &str, expected: &str) { + let cst = build_cst(json); + let root_obj = cst.object_value().unwrap(); + root_obj.sort_properties_by_key(|prop| prop.decoded_name()); + assert_eq!(cst.to_string(), expected); + // the result is still the same json, and sorting it again changes nothing + build_cst(&cst.to_string()); + let sorted = cst.to_string(); + root_obj.sort_properties_by_key(|prop| prop.decoded_name()); + assert_eq!(cst.to_string(), sorted); + } + + run_test("{\n \"b\": 2,\n \"a\": 1\n}", "{\n \"a\": 1,\n \"b\": 2\n}"); + // a single line object keeps the spacing that separates its properties + run_test("{ \"b\": 2, \"a\": 1 }", "{ \"a\": 1, \"b\": 2 }"); + run_test("{\"b\":2,\"a\":1}", "{\"a\":1,\"b\":2}"); + // the trailing comma the object was written with belongs to whatever ends up last + run_test("{\n \"b\": 2,\n \"a\": 1,\n}", "{\n \"a\": 1,\n \"b\": 2,\n}"); + // nothing to do + run_test("{}", "{}"); + run_test("{ \"a\": 1 }", "{ \"a\": 1 }"); + run_test("{\n \"a\": 1,\n \"b\": 2\n}", "{\n \"a\": 1,\n \"b\": 2\n}"); + // values are moved as they were written, not reformatted + run_test( + "{\n \"b\": { \"z\": 1 },\n \"a\": [3, 1]\n}", + "{\n \"a\": [3, 1],\n \"b\": { \"z\": 1 }\n}", + ); + // word (unquoted) names sort by the same name the parser reads + run_test("{\n b: 2,\n a: 1\n}", "{\n a: 1,\n b: 2\n}"); + // an escape is decoded to find the name, and left as written when the property moves + run_test( + "{\n \"b\": 2,\n \"\\u0061\": 1\n}", + "{\n \"\\u0061\": 1,\n \"b\": 2\n}", + ); + // properties sharing a name keep the order they were written in + run_test( + "{\n \"b\": 2,\n \"a\": \"first\",\n \"a\": \"second\"\n}", + "{\n \"a\": \"first\",\n \"a\": \"second\",\n \"b\": 2\n}", + ); + // a comma is added where the new order needs one, even if the author left it out + run_test("{\n \"b\": 2\n \"a\": 1\n}", "{\n \"a\": 1,\n \"b\": 2\n}"); + // a comma written at the start of a line belongs to the property above it + run_test("{\n \"b\": 2\n , \"a\": 1\n}", "{\n \"a\": 1, \"b\": 2\n\n}"); + // the space that offset a removed comma goes with it + run_test("{ \"b\": 2 , \"a\": 1 }", "{ \"a\": 1, \"b\": 2 }"); + // properties keep their indentation when they change lines + run_test("{\n \"b\": 2, \"a\": 1\n}", "{\n \"a\": 1, \"b\": 2\n}"); + // carriage returns survive the move + run_test( + "{\r\n \"b\": 2,\r\n \"a\": 1\r\n}", + "{\r\n \"a\": 1,\r\n \"b\": 2\r\n}", + ); + } + + #[test] + fn sort_properties_moves_comments_and_blank_lines() { + #[track_caller] + fn run_test(json: &str, expected: &str) { + let cst = build_cst(json); + let root_obj = cst.object_value().unwrap(); + root_obj.sort_properties_by_key(|prop| prop.decoded_name()); + assert_eq!(cst.to_string(), expected); + build_cst(&cst.to_string()); + let sorted = cst.to_string(); + root_obj.sort_properties_by_key(|prop| prop.decoded_name()); + assert_eq!(cst.to_string(), sorted); + } + + // a comment above a property was written with it and travels with it + run_test( + "{\n // about b\n \"b\": 2,\n \"a\": 1\n}", + "{\n \"a\": 1,\n // about b\n \"b\": 2\n}", + ); + // so does a comment written after it on the same line, which loses the comma it sat behind + run_test( + "{\n \"b\": 2, // about b\n \"a\": 1\n}", + "{\n \"a\": 1,\n \"b\": 2 // about b\n}", + ); + // and gains one when it moves off the end + run_test( + "{\n \"b\": 2,\n \"a\": 1 // about a\n}", + "{\n \"a\": 1, // about a\n \"b\": 2\n}", + ); + // a comment on the open brace line belongs to no property and stays where it is + run_test( + "{ // about the object\n \"b\": 2,\n \"a\": 1\n}", + "{ // about the object\n \"a\": 1,\n \"b\": 2\n}", + ); + // as does one written under the last property + run_test( + "{\n \"b\": 2,\n \"a\": 1\n // dangling\n}", + "{\n \"a\": 1,\n \"b\": 2\n // dangling\n}", + ); + // a comment between two properties on one line was written above the second of them + run_test( + "{ \"b\": 2, /* between */ \"a\": 1 }", + "{ /* between */ \"a\": 1, \"b\": 2 }", + ); + // a block comment above a property travels like a line comment does + run_test( + "{\n /* about b */\n \"b\": 2,\n \"a\": 1\n}", + "{\n \"a\": 1,\n /* about b */\n \"b\": 2\n}", + ); + // a blank line above a property travels with it + run_test( + "{\n \"c\": 3,\n \"a\": 1,\n\n \"b\": 2\n}", + "{\n \"a\": 1,\n\n \"b\": 2,\n \"c\": 3\n}", + ); + // but one that ends up under the open brace reads as a gap rather than as part of a property + run_test("{\n \"b\": 2,\n\n \"a\": 1\n}", "{\n \"a\": 1,\n \"b\": 2\n}"); + } + + #[test] + fn sort_keeps_line_comments_ending_their_line() { + #[track_caller] + fn run_test(json: &str, expected: &str) { + let cst = build_cst(json); + let root_obj = cst.object_value().unwrap(); + root_obj.sort_properties_by_key(|prop| prop.decoded_name()); + assert_eq!(cst.to_string(), expected); + // without the line break the comment would swallow whatever follows it + build_cst(&cst.to_string()); + let sorted = cst.to_string(); + root_obj.sort_properties_by_key(|prop| prop.decoded_name()); + assert_eq!(cst.to_string(), sorted); + } + + // a line comment that would swallow the property after it gains a line break + run_test( + "{\"b\": 2, \"a\": 1 // about a\n}", + "{\"a\": 1, // about a\n \"b\": 2\n}", + ); + // and one that would swallow the close brace gains one too + run_test( + "{ \"b\": 2, // about b\n \"a\": 1 }", + "{ \"a\": 1,\n \"b\": 2 // about b\n }", + ); + // a block comment needs no such help + run_test( + "{\"b\": 2, \"a\": 1 /* about a */}", + "{\"a\": 1, /* about a */ \"b\": 2}", + ); + } + + #[test] + fn sort_properties_keeps_the_tree_usable() { + let cst = build_cst("{\n \"b\": 2,\n \"a\": 1\n}"); + let root_obj = cst.object_value().unwrap(); + let b = root_obj.get("b").unwrap(); + root_obj.sort_properties_by_key(|prop| prop.decoded_name()); + + // the handle taken before the sort still points at the same property in its new place + assert_eq!(b.decoded_name().unwrap(), "b"); + assert_eq!(b.property_index(), 1); + assert_eq!( + root_obj + .properties() + .iter() + .map(|p| p.decoded_name().unwrap()) + .collect::>(), + ["a", "b"] + ); + // and the property that moved can still be edited afterwards + b.set_value(json!(3)); + assert_eq!(cst.to_string(), "{\n \"a\": 1,\n \"b\": 3\n}"); + } + + #[test] + fn sort_elements() { + #[track_caller] + fn run_test(json: &str, expected: &str) { + let cst = build_cst(json); + let array = cst.array_value().unwrap(); + array.sort_elements_by_key(|element| element.to_string()); + assert_eq!(cst.to_string(), expected); + build_cst(&cst.to_string()); + let sorted = cst.to_string(); + array.sort_elements_by_key(|element| element.to_string()); + assert_eq!(cst.to_string(), sorted); + } + + run_test("[3, 1, 2]", "[1, 2, 3]"); + run_test("[\n 3,\n 1\n]", "[\n 1,\n 3\n]"); + // the trailing comma the array was written with belongs to whatever ends up last + run_test("[\n 3,\n 1,\n]", "[\n 1,\n 3,\n]"); + // a comment above an element travels with it + run_test("[\n // about 3\n 3,\n 1\n]", "[\n 1,\n // about 3\n 3\n]"); + // as does one written after it on the same line + run_test("[\n 3, // about 3\n 1\n]", "[\n 1,\n 3 // about 3\n]"); + // a line comment that would swallow the close bracket gains a line break + run_test("[2, // about 2\n1]", "[1,\n2 // about 2\n]"); + // a blank line above an element travels with it + run_test("[\n 3,\n\n 1\n]", "[\n 1,\n 3\n]"); + // nothing to do + run_test("[]", "[]"); + run_test("[1]", "[1]"); + // an array sorts before an object by text, so these are already in order + run_test("[\n [3, 2],\n {\"a\": 1}\n]", "[\n [3, 2],\n {\"a\": 1}\n]"); + } + #[track_caller] fn build_cst(text: &str) -> CstRootNode { CstRootNode::parse(text, &crate::ParseOptions::default()).unwrap() diff --git a/tests/sort_fuzz.rs b/tests/sort_fuzz.rs new file mode 100644 index 0000000..dad31e3 --- /dev/null +++ b/tests/sort_fuzz.rs @@ -0,0 +1,306 @@ +//! Checks the CST sorting against randomly generated JSONC. +//! +//! Reordering has to move each element's comments and commas with it without ever changing what +//! the document holds, and the ways to get that wrong are mostly odd trivia placements rather than +//! odd values. So the generator varies the trivia in every slot it can appear in and the values +//! barely at all, then asserts the invariants that must hold however the pieces land. + +#![cfg(feature = "cst")] + +use jsonc_parser::ParseOptions; +use jsonc_parser::cst::CstRootNode; + +#[test] +fn sorting_generated_documents_preserves_them() { + let mut random = Random::new(0x5eed_1234_9abc_def0); + for _ in 0..20_000 { + check(&mut random, Shape::Object); + check(&mut random, Shape::Array); + } +} + +fn check(random: &mut Random, shape: Shape) { + let text = generate(random, shape); + let Ok(root) = CstRootNode::parse(&text, &ParseOptions::default()) else { + // the generator is allowed to produce something the parser rejects; nothing to sort then + return; + }; + let before = contents(&root); + let comments_before = comments(&text); + + match shape { + Shape::Object => { + let Some(object) = root.object_value() else { + return; + }; + object.sort_properties_by_key(|prop| prop.decoded_name()); + } + Shape::Array => { + let Some(array) = root.array_value() else { + return; + }; + array.sort_elements_by_key(|element| element.to_string()); + } + } + + let sorted = root.to_string(); + let reparsed = CstRootNode::parse(&sorted, &ParseOptions::default()) + .unwrap_or_else(|err| panic!("did not re-parse: {err}\n--- input ---\n{text}\n--- output ---\n{sorted}")); + + let after = contents(&reparsed); + assert_eq!( + sorted_lines(&before), + sorted_lines(&after), + "contents changed\n--- input ---\n{text}\n--- output ---\n{sorted}" + ); + // only the key decides the order; members sharing one keep the order they were written in + assert!( + after.windows(2).all(|pair| pair[0].0 <= pair[1].0), + "not in order\n--- input ---\n{text}\n--- output ---\n{sorted}" + ); + assert_eq!( + keyed_order(&before), + keyed_order(&after), + "members sharing a key changed order\n--- input ---\n{text}\n--- output ---\n{sorted}" + ); + assert_eq!( + comments_before, + comments(&sorted), + "comments changed\n--- input ---\n{text}\n--- output ---\n{sorted}" + ); + + // sorting what is already sorted leaves it alone + match shape { + Shape::Object => reparsed + .object_value() + .unwrap() + .sort_properties_by_key(|prop| prop.decoded_name()), + Shape::Array => reparsed + .array_value() + .unwrap() + .sort_elements_by_key(|element| element.to_string()), + } + assert_eq!( + reparsed.to_string(), + sorted, + "not idempotent\n--- input ---\n{text}\n--- output ---\n{sorted}" + ); +} + +#[derive(Clone, Copy)] +enum Shape { + Object, + Array, +} + +/// The sort key and value of each member, which reordering must leave untouched as a set. +fn contents(root: &CstRootNode) -> Vec<(String, String)> { + if let Some(object) = root.object_value() { + object + .properties() + .iter() + .map(|prop| { + let value = prop.value().map(|v| v.to_string()).unwrap_or_default(); + (prop.decoded_name().unwrap_or_default(), value.trim().to_string()) + }) + .collect() + } else if let Some(array) = root.array_value() { + array + .elements() + .iter() + .map(|e| (e.to_string(), e.to_string())) + .collect() + } else { + Vec::new() + } +} + +fn sorted_lines(values: &[(String, String)]) -> Vec<(String, String)> { + let mut values = values.to_vec(); + values.sort(); + values +} + +/// The members grouped under their key, in the order they appear. +/// +/// A stable sort never reorders members sharing a key, so this has to come out the same before and +/// after however they were interleaved to begin with. +fn keyed_order(values: &[(String, String)]) -> Vec<(String, Vec)> { + let mut grouped: Vec<(String, Vec)> = Vec::new(); + for (key, value) in values { + match grouped.iter_mut().find(|(existing, _)| existing == key) { + Some((_, values)) => values.push(value.clone()), + None => grouped.push((key.clone(), vec![value.clone()])), + } + } + grouped.sort_by(|left, right| left.0.cmp(&right.0)); + grouped +} + +/// Every comment in the text, which reordering may move but never drop, merge, or invent. +fn comments(text: &str) -> Vec { + let mut comments = Vec::new(); + let bytes = text.as_bytes(); + let mut index = 0; + let mut in_string = false; + while index < bytes.len() { + match bytes[index] { + b'\\' if in_string => index += 1, + b'"' => in_string = !in_string, + b'/' if !in_string && index + 1 < bytes.len() && bytes[index + 1] == b'/' => { + let start = index; + while index < bytes.len() && bytes[index] != b'\n' && bytes[index] != b'\r' { + index += 1; + } + comments.push(text[start..index].trim_end().to_string()); + continue; + } + b'/' if !in_string && index + 1 < bytes.len() && bytes[index + 1] == b'*' => { + let start = index; + index += 2; + while index + 1 < bytes.len() && !(bytes[index] == b'*' && bytes[index + 1] == b'/') { + index += 1; + } + index = (index + 2).min(bytes.len()); + comments.push(text[start..index].to_string()); + continue; + } + _ => {} + } + index += 1; + } + comments.sort(); + comments +} + +fn generate(random: &mut Random, shape: Shape) -> String { + let newline = if random.chance(4) { "\r\n" } else { "\n" }; + let multiline = random.chance(2); + let member_count = 2 + random.below(4); + let (open, close) = match shape { + Shape::Object => ('{', '}'), + Shape::Array => ('[', ']'), + }; + + let mut text = String::new(); + text.push(open); + text.push_str(&trivia(random, newline, multiline, true)); + for index in 0..member_count { + text.push_str(&trivia(random, newline, multiline, false)); + if multiline { + text.push_str(" "); + } + match shape { + Shape::Object => { + text.push_str(&format!("\"{}\"", name(random, index))); + text.push(':'); + text.push(' '); + } + Shape::Array => {} + } + text.push_str(&value(random, index)); + let last = index + 1 == member_count; + if !last || random.chance(3) { + // a comma sometimes lands after the trailing trivia, which is legal and worth exercising + if random.chance(6) { + text.push_str(&trailing_trivia(random, newline)); + text.push(','); + } else { + text.push(','); + text.push_str(&trailing_trivia(random, newline)); + } + } else { + text.push_str(&trailing_trivia(random, newline)); + } + } + text.push_str(&trivia(random, newline, multiline, false)); + text.push(close); + text +} + +/// Trivia written above a member: line breaks, blank lines, and comments on their own line. +fn trivia(random: &mut Random, newline: &str, multiline: bool, after_open: bool) -> String { + let mut text = String::new(); + if after_open && random.chance(4) { + text.push_str(" // about the whole thing"); + text.push_str(newline); + return text; + } + if multiline { + text.push_str(newline); + if random.chance(4) { + text.push_str(newline); + } + if random.chance(3) { + text.push_str(" "); + text.push_str(if random.chance(2) { + "// written above" + } else { + "/* written above */" + }); + text.push_str(newline); + } + } else if !after_open { + text.push(' '); + if random.chance(5) { + text.push_str("/* between */ "); + } + } + text +} + +/// Trivia written after a member on its own line. +fn trailing_trivia(random: &mut Random, newline: &str) -> String { + if random.chance(4) { + let mut text = String::from(" // trailing"); + text.push_str(newline); + text + } else if random.chance(6) { + String::from(" /* trailing */") + } else { + String::new() + } +} + +fn name(random: &mut Random, index: usize) -> String { + match random.below(4) { + 0 => format!("key{}", random.below(5)), + 1 => format!("\\u006bey{}", index), + 2 => format!("Key{}", random.below(5)), + _ => format!("key{}", index), + } +} + +fn value(random: &mut Random, index: usize) -> String { + match random.below(5) { + 0 => format!("{}", index), + 1 => format!("\"value{}\"", index), + 2 => String::from("{ \"nested\": true }"), + 3 => String::from("[1, 2]"), + _ => String::from("null"), + } +} + +/// A tiny deterministic generator, so a failure can be reproduced from the seed alone. +struct Random(u64); + +impl Random { + fn new(seed: u64) -> Self { + Self(seed) + } + + fn next(&mut self) -> u64 { + self.0 ^= self.0 << 13; + self.0 ^= self.0 >> 7; + self.0 ^= self.0 << 17; + self.0 + } + + fn below(&mut self, bound: u64) -> usize { + (self.next() % bound) as usize + } + + fn chance(&mut self, one_in: u64) -> bool { + self.next() % one_in == 0 + } +} From e08d240f3c9ceec08d5d821173cae142872d6da3 Mon Sep 17 00:00:00 2001 From: David Sherret Date: Sat, 12 Sep 2026 13:00:59 -0400 Subject: [PATCH 2/5] perf(cst): copy the runs around each element instead of collecting them Splitting a container was building four little Vecs per element: the two halves of the separator, the trivia above the element, and the run after it. Every one of those is a stretch of the container's own children that reordering only copies, so they are ranges now and the children are copied straight out of the original list when it is written back. The one run that was edited rather than copied was the trailing one, since the comma has to suit the element's new position. Recording where the comma sits and settling it while writing the children out does the same job without the copy, and replaces a mutating pass over the groups. Sorting by key now uses sort_by_cached_key, which works the key out once per element the way the hand-rolled decorate and undecorate did, without the intermediate Vec. Measured on a shuffled object, time per sort: properties before after 100 50.6us 31.2us 1,000 420us 257us 10,000 5.50ms 3.15ms With a comment above every third property and after every fifth, 10,000 properties goes 5.80ms to 4.24ms. --- src/cst/mod.rs | 135 +++++++++++++++++++++++++++---------------------- 1 file changed, 74 insertions(+), 61 deletions(-) diff --git a/src/cst/mod.rs b/src/cst/mod.rs index 4f3ab5d..7277948 100644 --- a/src/cst/mod.rs +++ b/src/cst/mod.rs @@ -1,9 +1,7 @@ //! CST for manipulating JSONC. //! //! Unlike the AST, this keeps every comment and every piece of whitespace, so a document can be -//! edited and written back out with everything the author wrote still in place. Properties and -//! elements can also be reordered with [`CstObject::sort_properties_by`] and -//! [`CstArray::sort_elements_by`], which carry each one's comments along with it. +//! edited and written back out with everything the author wrote still in place. //! //! # Example //! @@ -40,6 +38,7 @@ use std::cmp::Ordering; use std::collections::VecDeque; use std::fmt::Display; use std::iter::Peekable; +use std::ops::Range; use std::rc::Rc; use std::rc::Weak; @@ -1852,12 +1851,7 @@ impl CstObject { /// Behaves like [`CstObject::sort_properties_by`] in every other respect. pub fn sort_properties_by_key(&self, mut key: impl FnMut(&CstObjectProp) -> K) { sort_comma_separated_children(&self.clone().into(), |groups| { - let mut keyed = std::mem::take(groups) - .into_iter() - .map(|group| (group.element.as_object_prop().map(|prop| key(&prop)), group)) - .collect::>(); - keyed.sort_by(|left, right| left.0.cmp(&right.0)); - groups.extend(keyed.into_iter().map(|(_, group)| group)); + groups.sort_by_cached_key(|group| group.element.as_object_prop().map(|prop| key(&prop))) }); } @@ -2276,12 +2270,7 @@ impl CstArray { /// Behaves like [`CstArray::sort_elements_by`] in every other respect. pub fn sort_elements_by_key(&self, mut key: impl FnMut(&CstNode) -> K) { sort_comma_separated_children(&self.clone().into(), |groups| { - let mut keyed = std::mem::take(groups) - .into_iter() - .map(|group| (key(&group.element), group)) - .collect::>(); - keyed.sort_by(|left, right| left.0.cmp(&right.0)); - groups.extend(keyed.into_iter().map(|(_, group)| group)); + groups.sort_by_cached_key(|group| key(&group.element)) }); } @@ -2680,24 +2669,32 @@ impl<'a> CstBuilder<'a> { /// What sits between two elements and stays where it is, because it positions whatever comes next /// rather than belonging to either element. +/// +/// Both parts are stretches of the container's own children, which moving elements around only +/// ever copies, so they're held as ranges rather than as lists of their own. struct Separator { /// The line break that ended the previous element line, or on a single line the space between /// the two elements. - before: Vec, + before: Range, /// The indentation directly in front of the element. - indent: Vec, + indent: Range, } /// An element of a comma separated container along with the trivia that travels with it. +/// +/// Every part of it is a stretch of the container's own children, which reordering only ever +/// copies, so they're held as ranges rather than as lists of their own. struct SortableGroup { /// Where the element was written, so that a sort changing nothing can leave the tree alone. index: usize, /// What was written before the element and belongs to it: its comments and the blank lines above it. - leading: Vec, + leading: Range, element: CstNode, /// Whatever separates the element from its comma, the comma, and any comment written after that /// on the same line. - trailing: Vec, + trailing: Range, + /// Where the element's comma sits, if it was written with one. + comma: Option, } /// Reorders the elements of an object or array, moving what was written with each element along @@ -2724,19 +2721,22 @@ fn sort_comma_separated_children(container: &CstContainerNode, sort: impl FnOnce } if index == region.len() { // what follows the last element belongs to no element and stays where it is - break region[run_start..].to_vec(); + break run_start..region.len(); } - let (separator, leading) = split_separator(®ion[run_start..index]); + let (separator, leading) = split_separator(region, run_start..index); separators.push(separator); - let element = region[index].clone(); - let trailing_end = trailing_run_end(region, index + 1); + let trailing = index + 1..trailing_run_end(region, index + 1); groups.push(SortableGroup { index: groups.len(), leading, - element, - trailing: region[index + 1..trailing_end].to_vec(), + element: region[index].clone(), + comma: region[trailing.clone()] + .iter() + .position(|n| n.is_comma()) + .map(|at| trailing.start + at), + trailing: trailing.clone(), }); - index = trailing_end; + index = trailing.end; }; if groups.len() < 2 { @@ -2744,7 +2744,7 @@ fn sort_comma_separated_children(container: &CstContainerNode, sort: impl FnOnce } // whether the author ended the container with a comma, which the new last element takes over - let ends_with_comma = groups[groups.len() - 1].trailing.iter().any(|n| n.is_comma()); + let ends_with_comma = groups[groups.len() - 1].comma.is_some(); sort(&mut groups); if groups .iter() @@ -2754,26 +2754,26 @@ fn sort_comma_separated_children(container: &CstContainerNode, sort: impl FnOnce return; } - let last_index = groups.len() - 1; - for (position, group) in groups.iter_mut().enumerate() { - set_group_comma(group, position < last_index || ends_with_comma); - } // a blank line here reads as a gap under the open token rather than as something written with // the element that follows, so it doesn't travel with whatever sorted to the top let first_leading = &mut groups[0].leading; - let blank_count = first_leading.iter().take_while(|n| n.is_newline()).count(); - first_leading.drain(..blank_count); + first_leading.start += region[first_leading.clone()] + .iter() + .take_while(|n| n.is_newline()) + .count(); + let last_index = groups.len() - 1; let mut new_children = Vec::with_capacity(children.len()); new_children.push(children[0].clone()); - for (separator, group) in separators.into_iter().zip(groups) { - new_children.extend(separator.before); - new_children.extend(group.leading); - new_children.extend(separator.indent); + for (position, (separator, group)) in separators.into_iter().zip(groups).enumerate() { + new_children.extend_from_slice(®ion[separator.before]); + new_children.extend_from_slice(®ion[group.leading]); + new_children.extend_from_slice(®ion[separator.indent]); new_children.push(group.element); - new_children.extend(group.trailing); + let wants_comma = position < last_index || ends_with_comma; + push_trailing(&mut new_children, region, group.trailing, group.comma, wants_comma); } - new_children.extend(tail); + new_children.extend_from_slice(®ion[tail]); new_children.push(children[children.len() - 1].clone()); let newline_kind = container .root_node() @@ -2797,31 +2797,32 @@ fn is_sortable_element(node: &CstNode) -> bool { /// position whatever comes next, so they belong to the slot rather than to either element. What /// sits between them, such as blank lines and the comments written above the element, came with /// that element and travels with it. -fn split_separator(run: &[CstNode]) -> (Separator, Vec) { - let Some(newline) = run.iter().position(|n| n.is_newline()) else { - // nothing indents anything on a single line, so all that's here is the space between the two - let before = run.iter().take_while(|n| n.is_whitespace()).count(); +fn split_separator(region: &[CstNode], run: Range) -> (Separator, Range) { + let nodes = ®ion[run.clone()]; + let Some(newline) = nodes.iter().position(|n| n.is_newline()) else { + // nothing indents anything on a single line, so all that is here is the space between the two + let before = nodes.iter().take_while(|n| n.is_whitespace()).count(); return ( Separator { - before: run[..before].to_vec(), - indent: Vec::new(), + before: run.start..run.start + before, + indent: run.end..run.end, }, - run[before..].to_vec(), + run.start + before..run.end, ); }; let leading_start = newline + 1; - let indent_len = run[leading_start..] + let indent_len = nodes[leading_start..] .iter() .rev() .take_while(|n| n.is_whitespace()) .count(); - let indent_start = run.len() - indent_len; + let indent_start = nodes.len() - indent_len; ( Separator { - before: run[..leading_start].to_vec(), - indent: run[indent_start..].to_vec(), + before: run.start..run.start + leading_start, + indent: run.start + indent_start..run.end, }, - run[leading_start..indent_start].to_vec(), + run.start + leading_start..run.start + indent_start, ) } @@ -2863,18 +2864,30 @@ fn rest_of_line_is_trivia(region: &[CstNode], start: usize) -> bool { .all(|n| n.is_whitespace() || n.is_comment()) } -/// Adds or removes the element's comma so that it suits the element's new position. -fn set_group_comma(group: &mut SortableGroup, wants_comma: bool) { - match group.trailing.iter().position(|n| n.is_comma()) { - Some(index) if !wants_comma => { - group.trailing.remove(index); +/// Writes out what followed the element, with its comma added or dropped to suit its new position. +fn push_trailing( + out: &mut Vec, + region: &[CstNode], + trailing: Range, + comma: Option, + wants_comma: bool, +) { + match comma { + Some(comma) if !wants_comma => { // the space that offset the comma has nothing left to offset - if index > 0 && group.trailing[index - 1].is_whitespace() { - group.trailing.remove(index - 1); - } + let end = if comma > trailing.start && region[comma - 1].is_whitespace() { + comma - 1 + } else { + comma + }; + out.extend_from_slice(®ion[trailing.start..end]); + out.extend_from_slice(®ion[comma + 1..trailing.end]); + } + None if wants_comma => { + out.push(CstToken::new(',').into()); + out.extend_from_slice(®ion[trailing]); } - None if wants_comma => group.trailing.insert(0, CstToken::new(',').into()), - _ => {} + _ => out.extend_from_slice(®ion[trailing]), } } From d0feef87e4823041f1f7b74bc3866a66de8bb0ba Mon Sep 17 00:00:00 2001 From: David Sherret Date: Sat, 12 Sep 2026 15:49:47 -0400 Subject: [PATCH 3/5] feat(cst): build the sort so a comment heading a group can stay put Sorting is started with sort_properties()/sort_elements() now and told how to order things with by() or by_key(), which leaves somewhere to put the options the flat methods had nowhere for. The first of those is maintain_comment_headers(). A comment with a blank line above it reads as a heading for the elements beneath rather than as a description of the first of them, so it stays where it was written and the elements sort past it. Without it the heading is carried off to wherever its first element happens to land: { { "prop": 1, "prop": 1, "prop1": 1, // section => "prop2": 2, // section "prop1": 1 "prop2": 2 } } The blank line is what tells the two kinds of comment apart: one written flush against its property describes that property and still travels with it, which is why this is an option rather than the only behaviour. The fuzzer now picks the setting at random, since both have to hold the same invariants. --- src/cst/mod.rs | 319 ++++++++++++++++++++++++++++++++------------- tests/sort_fuzz.rs | 44 +++++-- 2 files changed, 262 insertions(+), 101 deletions(-) diff --git a/src/cst/mod.rs b/src/cst/mod.rs index 7277948..148ebee 100644 --- a/src/cst/mod.rs +++ b/src/cst/mod.rs @@ -1794,22 +1794,27 @@ impl CstObject { self.insert_or_append(Some(index), prop_name, value) } - /// Sorts the properties of the object with the given comparator. - /// - /// What was written with a property travels with it: the comments and blank lines above it, its - /// indentation, and a comment written after it on the same line. What belongs to no property - /// stays where it is, which includes whatever follows the open brace and whatever precedes the - /// close brace. Each property gains or loses a comma to suit its new position, and whether the - /// object ends with a trailing comma is preserved. + fn insert_or_append(&self, index: Option, prop_name: &str, value: CstInputValue) -> CstObjectProp { + self.ensure_multiline(); + insert_or_append_to_container( + &CstContainerNode::Object(self.clone()), + self.properties().into_iter().map(|c| c.into()).collect(), + index, + InsertValue::Property(prop_name, value), + ) + .as_object_prop() + .unwrap() + } + + /// Sorts the properties of the object. /// - /// Two exceptions are worth knowing about. A blank line that ends up directly under the open - /// brace is dropped rather than moved, since a gap there reads as belonging to the object rather - /// than to the property beneath it. And a line comment that no longer ends its line gains a line - /// break, so that it can't comment out whatever follows it. + /// What was written with a property travels with it: the comments and blank lines above it, and + /// a comment written after it on the same line. What belongs to no property stays where it is, + /// which includes whatever follows the open brace and whatever precedes the close brace. Each + /// property gains or loses a comma to suit its new position, and whether the object ends with a + /// trailing comma is preserved. /// - /// The sort is stable, so properties that compare equal keep the order they were written in. The - /// comparator must not modify this object, as changes made while it runs are discarded, and it - /// must describe a total order, as [`slice::sort_by`] panics otherwise. + /// Nothing moves until [`PropertySort::by`] or [`PropertySort::by_key`] says how to order them. /// /// # Example /// @@ -1825,7 +1830,7 @@ impl CstObject { /// /// let root = CstRootNode::parse(json_text, &ParseOptions::default()).unwrap(); /// let root_obj = root.object_value().unwrap(); - /// root_obj.sort_properties_by_key(|prop| prop.decoded_name()); + /// root_obj.sort_properties().by_key(|prop| prop.decoded_name()); /// /// assert_eq!(root.to_string(), r#"{ /// // written about a @@ -1833,38 +1838,11 @@ impl CstObject { /// "b": 2 // written about b /// }"#); /// ``` - pub fn sort_properties_by(&self, mut compare: impl FnMut(&CstObjectProp, &CstObjectProp) -> Ordering) { - sort_comma_separated_children(&self.clone().into(), |groups| { - groups.sort_by( - |left, right| match (left.element.as_object_prop(), right.element.as_object_prop()) { - (Some(left), Some(right)) => compare(&left, &right), - // an object holds properties, so this only happens if the tree has been manipulated into - // holding something else, in which case leaving the order alone is the safe answer - _ => Ordering::Equal, - }, - ) - }); - } - - /// Sorts the properties of the object by a key, which is worked out once per property. - /// - /// Behaves like [`CstObject::sort_properties_by`] in every other respect. - pub fn sort_properties_by_key(&self, mut key: impl FnMut(&CstObjectProp) -> K) { - sort_comma_separated_children(&self.clone().into(), |groups| { - groups.sort_by_cached_key(|group| group.element.as_object_prop().map(|prop| key(&prop))) - }); - } - - fn insert_or_append(&self, index: Option, prop_name: &str, value: CstInputValue) -> CstObjectProp { - self.ensure_multiline(); - insert_or_append_to_container( - &CstContainerNode::Object(self.clone()), - self.properties().into_iter().map(|c| c.into()).collect(), - index, - InsertValue::Property(prop_name, value), - ) - .as_object_prop() - .unwrap() + pub fn sort_properties(&self) -> PropertySort<'_> { + PropertySort { + object: self, + options: SortOptions::default(), + } } /// Replaces this node with a new value. @@ -2221,21 +2199,11 @@ impl CstArray { self.insert_or_append(Some(index), value) } - /// Sorts the elements of the array with the given comparator. - /// - /// What was written with an element travels with it: the comments and blank lines above it, its - /// indentation, and a comment written after it on the same line. What belongs to no element - /// stays where it is, which includes whatever follows the open bracket and whatever precedes the - /// close bracket. Each element gains or loses a comma to suit its new position, and whether the - /// array ends with a trailing comma is preserved. + /// Sorts the elements of the array. /// - /// A blank line that ends up directly under the open bracket is dropped rather than moved, and a - /// line comment that no longer ends its line gains a line break so that it can't comment out - /// whatever follows it. - /// - /// The sort is stable, so elements that compare equal keep the order they were written in. The - /// comparator must not modify this array, as changes made while it runs are discarded, and it - /// must describe a total order, as [`slice::sort_by`] panics otherwise. + /// Behaves like [`CstObject::sort_properties`], moving what was written with an element along + /// with it. Nothing moves until [`ElementSort::by`] or [`ElementSort::by_key`] says how to + /// order them. /// /// # Example /// @@ -2251,7 +2219,7 @@ impl CstArray { /// /// let root = CstRootNode::parse(json_text, &ParseOptions::default()).unwrap(); /// let array = root.array_value().unwrap(); - /// array.sort_elements_by_key(|element| element.to_string()); + /// array.sort_elements().by_key(|element| element.to_string()); /// /// assert_eq!(root.to_string(), r#"[ /// // written about a @@ -2259,19 +2227,11 @@ impl CstArray { /// "b" // written about b /// ]"#); /// ``` - pub fn sort_elements_by(&self, mut compare: impl FnMut(&CstNode, &CstNode) -> Ordering) { - sort_comma_separated_children(&self.clone().into(), |groups| { - groups.sort_by(|left, right| compare(&left.element, &right.element)) - }); - } - - /// Sorts the elements of the array by a key, which is worked out once per element. - /// - /// Behaves like [`CstArray::sort_elements_by`] in every other respect. - pub fn sort_elements_by_key(&self, mut key: impl FnMut(&CstNode) -> K) { - sort_comma_separated_children(&self.clone().into(), |groups| { - groups.sort_by_cached_key(|group| key(&group.element)) - }); + pub fn sort_elements(&self) -> ElementSort<'_> { + ElementSort { + array: self, + options: SortOptions::default(), + } } /// Ensures the array spans multiple lines. @@ -2667,6 +2627,130 @@ impl<'a> CstBuilder<'a> { } } +/// What a sort does with the trivia it moves past, set through [`PropertySort`] and [`ElementSort`]. +#[derive(Debug, Default, Clone, Copy)] +struct SortOptions { + maintain_comment_headers: bool, +} + +/// A sort of an object's properties, waiting to be told how to order them. +/// +/// Built by [`CstObject::sort_properties`]. +#[must_use = "nothing is sorted until `by` or `by_key` is called"] +pub struct PropertySort<'a> { + object: &'a CstObject, + options: SortOptions, +} + +impl PropertySort<'_> { + /// Leaves a comment that heads a group of properties where it was written. + /// + /// A comment with a blank line above it reads as a heading for the properties beneath it rather + /// than as a description of the first of them, so it stays put and the properties sort past it. + /// Without this, every comment above a property travels with that property, which carries a + /// heading off to wherever its first property happens to land. + /// + /// The blank line itself stays too, as does a blank line with no comment under it. + /// + /// # Example + /// + /// ``` + /// use jsonc_parser::ParseOptions; + /// use jsonc_parser::cst::CstRootNode; + /// + /// let json_text = r#"{ + /// "prop": 1, + /// + /// // section + /// "prop2": 2, + /// "prop1": 1 + /// }"#; + /// + /// let root = CstRootNode::parse(json_text, &ParseOptions::default()).unwrap(); + /// let root_obj = root.object_value().unwrap(); + /// root_obj + /// .sort_properties() + /// .maintain_comment_headers() + /// .by_key(|prop| prop.decoded_name()); + /// + /// assert_eq!(root.to_string(), r#"{ + /// "prop": 1, + /// + /// // section + /// "prop1": 1, + /// "prop2": 2 + /// }"#); + /// ``` + pub fn maintain_comment_headers(mut self) -> Self { + self.options.maintain_comment_headers = true; + self + } + + /// Sorts the properties with the given comparator. + /// + /// The sort is stable, so properties that compare equal keep the order they were written in. The + /// comparator must not modify the object, as changes made while it runs are discarded, and it + /// must describe a total order, as [`slice::sort_by`] panics otherwise. + pub fn by(self, mut compare: impl FnMut(&CstObjectProp, &CstObjectProp) -> Ordering) { + sort_comma_separated_children(&self.object.clone().into(), self.options, |groups| { + groups.sort_by(|left, right| { + match (left.element.as_object_prop(), right.element.as_object_prop()) { + (Some(left), Some(right)) => compare(&left, &right), + // an object holds properties, so this only happens if the tree has been manipulated into + // holding something else, in which case leaving the order alone is the safe answer + _ => Ordering::Equal, + } + }) + }); + } + + /// Sorts the properties by a key, which is worked out once per property. + /// + /// Behaves like [`PropertySort::by`] in every other respect. + pub fn by_key(self, mut key: impl FnMut(&CstObjectProp) -> K) { + sort_comma_separated_children(&self.object.clone().into(), self.options, |groups| { + groups.sort_by_cached_key(|group| group.element.as_object_prop().map(|prop| key(&prop))) + }); + } +} + +/// A sort of an array's elements, waiting to be told how to order them. +/// +/// Built by [`CstArray::sort_elements`]. +#[must_use = "nothing is sorted until `by` or `by_key` is called"] +pub struct ElementSort<'a> { + array: &'a CstArray, + options: SortOptions, +} + +impl ElementSort<'_> { + /// Leaves a comment that heads a group of elements where it was written. + /// + /// Behaves like [`PropertySort::maintain_comment_headers`]. + pub fn maintain_comment_headers(mut self) -> Self { + self.options.maintain_comment_headers = true; + self + } + + /// Sorts the elements with the given comparator. + /// + /// Behaves like [`PropertySort::by`]. + pub fn by(self, mut compare: impl FnMut(&CstNode, &CstNode) -> Ordering) { + sort_comma_separated_children(&self.array.clone().into(), self.options, |groups| { + groups.sort_by(|left, right| compare(&left.element, &right.element)) + }); + } + + /// Sorts the elements by a key, which is worked out once per element. + /// + /// Behaves like [`PropertySort::by_key`]. + pub fn by_key(self, mut key: impl FnMut(&CstNode) -> K) { + sort_comma_separated_children(&self.array.clone().into(), self.options, |groups| { + groups.sort_by_cached_key(|group| key(&group.element)) + }); + } +} + /// What sits between two elements and stays where it is, because it positions whatever comes next /// rather than belonging to either element. /// @@ -2701,7 +2785,11 @@ struct SortableGroup { /// with it and leaving the separators between them where they are. /// /// `sort` is handed the groups in the order they were written and is expected to sort them stably. -fn sort_comma_separated_children(container: &CstContainerNode, sort: impl FnOnce(&mut Vec)) { +fn sort_comma_separated_children( + container: &CstContainerNode, + options: SortOptions, + sort: impl FnOnce(&mut Vec), +) { let children = container.children(); // the surrounding tokens are what the elements sit between, so there's nothing to sort without them if children.len() < 2 || !children[0].is_token() || !children[children.len() - 1].is_token() { @@ -2723,7 +2811,7 @@ fn sort_comma_separated_children(container: &CstContainerNode, sort: impl FnOnce // what follows the last element belongs to no element and stays where it is break run_start..region.len(); } - let (separator, leading) = split_separator(region, run_start..index); + let (separator, leading) = split_separator(region, run_start..index, options); separators.push(separator); let trailing = index + 1..trailing_run_end(region, index + 1); groups.push(SortableGroup { @@ -2797,7 +2885,7 @@ fn is_sortable_element(node: &CstNode) -> bool { /// position whatever comes next, so they belong to the slot rather than to either element. What /// sits between them, such as blank lines and the comments written above the element, came with /// that element and travels with it. -fn split_separator(region: &[CstNode], run: Range) -> (Separator, Range) { +fn split_separator(region: &[CstNode], run: Range, options: SortOptions) -> (Separator, Range) { let nodes = ®ion[run.clone()]; let Some(newline) = nodes.iter().position(|n| n.is_newline()) else { // nothing indents anything on a single line, so all that is here is the space between the two @@ -2810,7 +2898,14 @@ fn split_separator(region: &[CstNode], run: Range) -> (Separator, Range { let Some(array) = root.array_value() else { return; }; - array.sort_elements_by_key(|element| element.to_string()); + array.sort_elements().by_key(|element| element.to_string()); } } @@ -71,14 +75,8 @@ fn check(random: &mut Random, shape: Shape) { // sorting what is already sorted leaves it alone match shape { - Shape::Object => reparsed - .object_value() - .unwrap() - .sort_properties_by_key(|prop| prop.decoded_name()), - Shape::Array => reparsed - .array_value() - .unwrap() - .sort_elements_by_key(|element| element.to_string()), + Shape::Object => sort_properties(&reparsed.object_value().unwrap(), maintain_headers), + Shape::Array => sort_elements(&reparsed.array_value().unwrap(), maintain_headers), } assert_eq!( reparsed.to_string(), @@ -87,6 +85,26 @@ fn check(random: &mut Random, shape: Shape) { ); } +fn sort_properties(object: &CstObject, maintain_headers: bool) { + let sort = object.sort_properties(); + let sort = if maintain_headers { + sort.maintain_comment_headers() + } else { + sort + }; + sort.by_key(|prop| prop.decoded_name()); +} + +fn sort_elements(array: &CstArray, maintain_headers: bool) { + let sort = array.sort_elements(); + let sort = if maintain_headers { + sort.maintain_comment_headers() + } else { + sort + }; + sort.by_key(|element| element.to_string()); +} + #[derive(Clone, Copy)] enum Shape { Object, From 90d8adda37dde7d34a652b409a75159e5b62038b Mon Sep 17 00:00:00 2001 From: David Sherret Date: Sat, 12 Sep 2026 16:31:48 -0400 Subject: [PATCH 4/5] feat(cst): let the caller decide what a sort moves Replaces maintain_comment_headers() with a pair that covers the whole question rather than one case of it, plus grouping. pin_comment_headers() pin_comment_headers_with(|member, comments| -> usize) within_groups() The rule is handed the comments the sorter itself worked out and says how many of them, from the top, stay where they were written; the rest travel with the member. That makes a count ordinary Rust rather than an index into a list the caller has to rebuild and hope matches, and it covers a block that is partly a heading and partly a note about the member beneath it, which a bool cannot say. within_groups() sorts each run of members between blank lines on its own. A blank line and whatever was written under it is the boundary between two groups, and a boundary stays where it is by definition, so grouping does not depend on which header rule was set. "pin" over "maintain": the axis is moves versus stays, and maintain reads first as "don't delete", which nothing here ever does. Also: a rule or comparator that adds or removes members used to be partly undone and partly kept, since a value set through the shared node survived while a removal was reverted over stale ranges. The sort now notices the container changed underneath it and leaves it alone rather than writing a corrupted tree back. has_blank_line_before() is public, since a custom rule needs it. --- src/cst/mod.rs | 403 +++++++++++++++++++++++++++++++++++++++------ tests/sort_fuzz.rs | 54 +++--- 2 files changed, 388 insertions(+), 69 deletions(-) diff --git a/src/cst/mod.rs b/src/cst/mod.rs index 148ebee..c741662 100644 --- a/src/cst/mod.rs +++ b/src/cst/mod.rs @@ -130,6 +130,11 @@ macro_rules! add_parent_info_methods { indent_text(&self.clone().into()) } + /// Whether a blank line separates this node from whatever was written before it. + pub fn has_blank_line_before(&self) -> bool { + has_blank_line_before(&self.clone().into()) + } + /// Gets the trailing comma token of the node, if it exists. pub fn trailing_comma(&self) -> Option { find_trailing_comma(&self.clone().into()) @@ -142,6 +147,26 @@ macro_rules! add_parent_info_methods { }; } +/// Whether a blank line separates the node from whatever was written before it. +fn has_blank_line_before(node: &CstNode) -> bool { + let mut ended_a_line = false; + for previous in node.previous_siblings() { + if previous.is_newline() { + if ended_a_line { + return true; + } + ended_a_line = true; + } else if previous.is_whitespace() { + // keeps looking past the indentation + } else if previous.is_comment() { + ended_a_line = false; + } else { + return false; + } + } + false +} + fn find_trailing_comma(node: &CstNode) -> Option { for next_sibling in node.next_siblings() { match next_sibling { @@ -1924,8 +1949,7 @@ impl CstObjectProp { /// Name of the object property with any escapes in it resolved. /// - /// Returns `None` if the name doesn't exist or can't be decoded, which sorts such a property - /// above every property whose name does decode. + /// Returns `None` if the name doesn't exist or can't be decoded. pub fn decoded_name(&self) -> Option { match self.name()? { ObjectPropName::String(s) => s.decoded_value().ok(), @@ -2627,22 +2651,16 @@ impl<'a> CstBuilder<'a> { } } -/// What a sort does with the trivia it moves past, set through [`PropertySort`] and [`ElementSort`]. -#[derive(Debug, Default, Clone, Copy)] -struct SortOptions { - maintain_comment_headers: bool, -} - /// A sort of an object's properties, waiting to be told how to order them. /// /// Built by [`CstObject::sort_properties`]. #[must_use = "nothing is sorted until `by` or `by_key` is called"] pub struct PropertySort<'a> { object: &'a CstObject, - options: SortOptions, + options: SortOptions<'a>, } -impl PropertySort<'_> { +impl<'a> PropertySort<'a> { /// Leaves a comment that heads a group of properties where it was written. /// /// A comment with a blank line above it reads as a heading for the properties beneath it rather @@ -2670,7 +2688,7 @@ impl PropertySort<'_> { /// let root_obj = root.object_value().unwrap(); /// root_obj /// .sort_properties() - /// .maintain_comment_headers() + /// .pin_comment_headers() /// .by_key(|prop| prop.decoded_name()); /// /// assert_eq!(root.to_string(), r#"{ @@ -2681,18 +2699,81 @@ impl PropertySort<'_> { /// "prop2": 2 /// }"#); /// ``` - pub fn maintain_comment_headers(mut self) -> Self { - self.options.maintain_comment_headers = true; + pub fn pin_comment_headers(self) -> Self { + self.pin_comment_headers_with(|prop, comments| blank_line_header_rule(&prop.clone().into(), comments)) + } + + /// Decides for each property how much of what was written above it is a heading for what + /// follows rather than part of the property. + /// + /// `rule` is handed the property and the comments written above it, in the order they appear, + /// and returns how many of them, counting from the top, stay where they were written. The rest + /// travel with the property, as does the blank line under whatever stayed. + /// + /// Returning `comments.len()` pins everything above the property and `0` pins nothing, so + /// [`PropertySort::pin_comment_headers`] is `if prop.has_blank_line_before() { comments.len() } + /// else { 0 }`. A count in between splits a block that is partly a heading and partly a note + /// about the property itself. + /// + /// The rule must not add or remove properties. Doing so leaves the sort with nothing safe to + /// write back, so it gives up and leaves the object as the rule left it. + pub fn pin_comment_headers_with(mut self, mut rule: impl FnMut(&CstObjectProp, &[CstComment]) -> usize + 'a) -> Self { + self.options.header_rule = Some(Box::new(move |element, comments| match element.as_object_prop() { + Some(prop) => rule(&prop, comments), + None => 0, + })); + self + } + + /// Sorts each run of properties between blank lines on its own, so that no property crosses one. + /// + /// A blank line, and whatever was written under it, is the boundary between two groups, and a + /// boundary stays where it is. A rule set by [`PropertySort::pin_comment_headers_with`] still + /// decides what travels with the properties inside each group. + /// + /// # Example + /// + /// ``` + /// use jsonc_parser::ParseOptions; + /// use jsonc_parser::cst::CstRootNode; + /// + /// let json_text = r#"{ + /// "m": 1, + /// + /// // section + /// "z": 2, + /// "a": 3 + /// }"#; + /// + /// let root = CstRootNode::parse(json_text, &ParseOptions::default()).unwrap(); + /// let root_obj = root.object_value().unwrap(); + /// root_obj + /// .sort_properties() + /// .within_groups() + /// .by_key(|prop| prop.decoded_name()); + /// + /// // "m" stays above the blank line and only "z" and "a" trade places + /// assert_eq!(root.to_string(), r#"{ + /// "m": 1, + /// + /// // section + /// "a": 3, + /// "z": 2 + /// }"#); + /// ``` + pub fn within_groups(mut self) -> Self { + self.options.within_groups = true; self } /// Sorts the properties with the given comparator. /// /// The sort is stable, so properties that compare equal keep the order they were written in. The - /// comparator must not modify the object, as changes made while it runs are discarded, and it - /// must describe a total order, as [`slice::sort_by`] panics otherwise. + /// comparator must describe a total order, as [`slice::sort_by`] panics otherwise, and it must + /// not add or remove properties; see [`PropertySort::pin_comment_headers_with`]. pub fn by(self, mut compare: impl FnMut(&CstObjectProp, &CstObjectProp) -> Ordering) { - sort_comma_separated_children(&self.object.clone().into(), self.options, |groups| { + let object = self.object.clone().into(); + sort_comma_separated_children(&object, self.options, |groups| { groups.sort_by(|left, right| { match (left.element.as_object_prop(), right.element.as_object_prop()) { (Some(left), Some(right)) => compare(&left, &right), @@ -2706,9 +2787,11 @@ impl PropertySort<'_> { /// Sorts the properties by a key, which is worked out once per property. /// - /// Behaves like [`PropertySort::by`] in every other respect. + /// A property whose name can't be decoded has no key, and sorts above every property that has + /// one. Behaves like [`PropertySort::by`] in every other respect. pub fn by_key(self, mut key: impl FnMut(&CstObjectProp) -> K) { - sort_comma_separated_children(&self.object.clone().into(), self.options, |groups| { + let object = self.object.clone().into(); + sort_comma_separated_children(&object, self.options, |groups| { groups.sort_by_cached_key(|group| group.element.as_object_prop().map(|prop| key(&prop))) }); } @@ -2720,15 +2803,31 @@ impl PropertySort<'_> { #[must_use = "nothing is sorted until `by` or `by_key` is called"] pub struct ElementSort<'a> { array: &'a CstArray, - options: SortOptions, + options: SortOptions<'a>, } -impl ElementSort<'_> { +impl<'a> ElementSort<'a> { /// Leaves a comment that heads a group of elements where it was written. /// - /// Behaves like [`PropertySort::maintain_comment_headers`]. - pub fn maintain_comment_headers(mut self) -> Self { - self.options.maintain_comment_headers = true; + /// Behaves like [`PropertySort::pin_comment_headers`]. + pub fn pin_comment_headers(self) -> Self { + self.pin_comment_headers_with(blank_line_header_rule) + } + + /// Decides for each element how much of what was written above it is a heading for what follows + /// rather than part of the element. + /// + /// Behaves like [`PropertySort::pin_comment_headers_with`]. + pub fn pin_comment_headers_with(mut self, rule: impl FnMut(&CstNode, &[CstComment]) -> usize + 'a) -> Self { + self.options.header_rule = Some(Box::new(rule)); + self + } + + /// Sorts each run of elements between blank lines on its own, so that no element crosses one. + /// + /// Behaves like [`PropertySort::within_groups`]. + pub fn within_groups(mut self) -> Self { + self.options.within_groups = true; self } @@ -2736,7 +2835,8 @@ impl ElementSort<'_> { /// /// Behaves like [`PropertySort::by`]. pub fn by(self, mut compare: impl FnMut(&CstNode, &CstNode) -> Ordering) { - sort_comma_separated_children(&self.array.clone().into(), self.options, |groups| { + let array = self.array.clone().into(); + sort_comma_separated_children(&array, self.options, |groups| { groups.sort_by(|left, right| compare(&left.element, &right.element)) }); } @@ -2745,20 +2845,51 @@ impl ElementSort<'_> { /// /// Behaves like [`PropertySort::by_key`]. pub fn by_key(self, mut key: impl FnMut(&CstNode) -> K) { - sort_comma_separated_children(&self.array.clone().into(), self.options, |groups| { + let array = self.array.clone().into(); + sort_comma_separated_children(&array, self.options, |groups| { groups.sort_by_cached_key(|group| key(&group.element)) }); } } +/// Decides how many of the comments written above an element stay where they are when it moves. +type HeaderRule<'a> = Box usize + 'a>; + +/// What a sort does with the trivia it moves past, set through [`PropertySort`] and [`ElementSort`]. +#[derive(Default)] +struct SortOptions<'a> { + header_rule: Option>, + within_groups: bool, +} + +impl SortOptions<'_> { + /// How many of `comments` stay where they were written rather than travelling with `element`. + fn pinned_comment_count(&mut self, element: &CstNode, comments: &[CstComment]) -> usize { + match &mut self.header_rule { + Some(rule) => rule(element, comments), + None => 0, + } + } +} + +/// The rule [`PropertySort::pin_comment_headers`] and [`ElementSort::pin_comment_headers`] use: a +/// comment with a blank line above it heads what follows rather than describing the first of them. +fn blank_line_header_rule(element: &CstNode, comments: &[CstComment]) -> usize { + if element.has_blank_line_before() { + comments.len() + } else { + 0 + } +} + /// What sits between two elements and stays where it is, because it positions whatever comes next /// rather than belonging to either element. /// /// Both parts are stretches of the container's own children, which moving elements around only /// ever copies, so they're held as ranges rather than as lists of their own. struct Separator { - /// The line break that ended the previous element line, or on a single line the space between - /// the two elements. + /// The line break that ended the previous element line, whatever of the trivia under it was + /// written as a header for what follows, and on a single line the space between two elements. before: Range, /// The indentation directly in front of the element. indent: Range, @@ -2771,7 +2902,10 @@ struct Separator { struct SortableGroup { /// Where the element was written, so that a sort changing nothing can leave the tree alone. index: usize, - /// What was written before the element and belongs to it: its comments and the blank lines above it. + /// Whether a blank line separates this element from the one before it, which is what divides a + /// container into groups. + starts_group: bool, + /// What was written before the element and belongs to it: its own comments and indentation. leading: Range, element: CstNode, /// Whatever separates the element from its comma, the comma, and any comment written after that @@ -2784,11 +2918,13 @@ struct SortableGroup { /// Reorders the elements of an object or array, moving what was written with each element along /// with it and leaving the separators between them where they are. /// -/// `sort` is handed the groups in the order they were written and is expected to sort them stably. +/// `sort` is handed the elements of one group at a time, in the order they were written, and is +/// expected to sort them stably. Without [`SortOptions::within_groups`] there is a single group +/// holding everything. fn sort_comma_separated_children( container: &CstContainerNode, - options: SortOptions, - sort: impl FnOnce(&mut Vec), + mut options: SortOptions<'_>, + mut sort: impl FnMut(&mut [SortableGroup]), ) { let children = container.children(); // the surrounding tokens are what the elements sit between, so there's nothing to sort without them @@ -2811,11 +2947,14 @@ fn sort_comma_separated_children( // what follows the last element belongs to no element and stays where it is break run_start..region.len(); } - let (separator, leading) = split_separator(region, run_start..index, options); + let run = run_start..index; + let starts_group = !groups.is_empty() && run_has_blank_line(®ion[run.clone()]); + let (separator, leading) = split_separator(region, run, ®ion[index], starts_group, &mut options); separators.push(separator); let trailing = index + 1..trailing_run_end(region, index + 1); groups.push(SortableGroup { index: groups.len(), + starts_group, leading, element: region[index].clone(), comma: region[trailing.clone()] @@ -2833,7 +2972,19 @@ fn sort_comma_separated_children( // whether the author ended the container with a comma, which the new last element takes over let ends_with_comma = groups[groups.len() - 1].comma.is_some(); - sort(&mut groups); + if options.within_groups { + // a blank line divides the container, and a divider is not something an element sorts past + for group in groups.chunk_by_mut(|_, next| !next.starts_group) { + sort(group); + } + } else { + sort(&mut groups); + } + // Adding or removing members while the sort runs would leave the ranges worked out above + // pointing at children that have moved, so writing them back would corrupt the tree. + if container.children().len() != children.len() { + return; + } if groups .iter() .enumerate() @@ -2845,10 +2996,11 @@ fn sort_comma_separated_children( // a blank line here reads as a gap under the open token rather than as something written with // the element that follows, so it doesn't travel with whatever sorted to the top let first_leading = &mut groups[0].leading; - first_leading.start += region[first_leading.clone()] + let blank_count = region[first_leading.clone()] .iter() .take_while(|n| n.is_newline()) .count(); + first_leading.start += blank_count; let last_index = groups.len() - 1; let mut new_children = Vec::with_capacity(children.len()); @@ -2877,15 +3029,38 @@ fn is_sortable_element(node: &CstNode) -> bool { !node.is_trivia() && !node.is_token() } +/// Whether a run of trivia leaves a line empty, which is what marks a group boundary and what +/// tells a comment heading a group from one describing the element beneath it. +fn run_has_blank_line(run: &[CstNode]) -> bool { + let mut ended_a_line = false; + for node in run { + if node.is_newline() { + if ended_a_line { + return true; + } + ended_a_line = true; + } else if !node.is_whitespace() { + ended_a_line = false; + } + } + false +} + /// Splits what was written between two elements into the separator, which stays where it is, and /// the trivia belonging to the element that follows. /// /// The separator is the line break that ended the previous element's line together with the /// indentation under it, or on a single line the whitespace between the two elements. Both /// position whatever comes next, so they belong to the slot rather than to either element. What -/// sits between them, such as blank lines and the comments written above the element, came with -/// that element and travels with it. -fn split_separator(region: &[CstNode], run: Range, options: SortOptions) -> (Separator, Range) { +/// sits between them came with the element that follows and travels with it, except for however +/// much of it the sort's header rule says was written as a header for what comes next. +fn split_separator( + region: &[CstNode], + run: Range, + element: &CstNode, + starts_group: bool, + options: &mut SortOptions<'_>, +) -> (Separator, Range) { let nodes = ®ion[run.clone()]; let Some(newline) = nodes.iter().position(|n| n.is_newline()) else { // nothing indents anything on a single line, so all that is here is the space between the two @@ -2898,20 +3073,21 @@ fn split_separator(region: &[CstNode], run: Range, options: SortOptions) run.start + before..run.end, ); }; - let mut leading_start = newline + 1; - // A blank line makes any comment under it read as a heading for the elements beneath rather than - // as a description of the first of them, so the whole run stays with the slot. The element's own - // indentation is in there too, which is where it needs to be either way. - let blank_line_follows = nodes.get(leading_start).map(|n| n.is_newline()).unwrap_or(false); - if options.maintain_comment_headers && blank_line_follows { - leading_start = nodes.len(); - } - let indent_len = nodes[leading_start..] + let indent_len = nodes[newline + 1..] .iter() .rev() .take_while(|n| n.is_whitespace()) .count(); let indent_start = nodes.len() - indent_len; + let leading = &nodes[newline + 1..indent_start]; + let header_len = if starts_group && options.within_groups { + // the blank line and whatever was written under it are the boundary between two groups, and a + // boundary is not something an element sorts past, so none of it travels + leading.len() + } else { + header_len(leading, element, options) + }; + let leading_start = newline + 1 + header_len; ( Separator { before: run.start..run.start + leading_start, @@ -2921,6 +3097,39 @@ fn split_separator(region: &[CstNode], run: Range, options: SortOptions) ) } +/// How much of what was written above an element was written as a header for it rather than as +/// part of it, and so stays where it is when the element moves. +/// +/// The header runs up to the line the first comment that isn't part of it begins on, so that the +/// blank line under a header stays with the header where it reads. +fn header_len(leading: &[CstNode], element: &CstNode, options: &mut SortOptions<'_>) -> usize { + let comments = leading + .iter() + .filter_map(|node| match node { + CstNode::Leaf(CstLeafNode::Comment(comment)) => Some(comment.clone()), + _ => None, + }) + .collect::>(); + let pinned = options.pinned_comment_count(element, &comments); + if pinned == 0 { + return 0; + } + if pinned >= comments.len() { + return leading.len(); + } + let mut split = leading + .iter() + .enumerate() + .filter(|(_, node)| node.is_comment()) + .map(|(index, _)| index) + .nth(pinned) + .unwrap_or(leading.len()); + while split > 0 && leading[split - 1].is_whitespace() { + split -= 1; + } + split +} + /// The end of the run after an element that was written with it: whatever separates the element /// from its comma, the comma itself, and any comment written after that on the same line. /// @@ -4679,14 +4888,14 @@ value3: true } #[test] - fn sort_properties_maintaining_comment_headers() { + fn sort_properties_pinning_comment_headers() { #[track_caller] fn run_test(json: &str, expected: &str) { let cst = build_cst(json); let root_obj = cst.object_value().unwrap(); root_obj .sort_properties() - .maintain_comment_headers() + .pin_comment_headers() .by_key(|prop| prop.decoded_name()); assert_eq!(cst.to_string(), expected); build_cst(&cst.to_string()); @@ -4726,6 +4935,104 @@ value3: true run_test("{\n \"b\": 2,\n \"a\": 1\n}", "{\n \"a\": 1,\n \"b\": 2\n}"); } + #[test] + fn sort_properties_within_groups() { + #[track_caller] + fn run_test(json: &str, expected: &str) { + let cst = build_cst(json); + let root_obj = cst.object_value().unwrap(); + root_obj + .sort_properties() + .within_groups() + .by_key(|prop| prop.decoded_name()); + assert_eq!(cst.to_string(), expected); + build_cst(&cst.to_string()); + } + + // a blank line divides the object and nothing sorts across it + // a blank line divides the object and nothing sorts across it + run_test( + "{\n \"m\": 1,\n\n // section\n \"z\": 2,\n \"a\": 3\n}", + "{\n \"m\": 1,\n\n // section\n \"a\": 3,\n \"z\": 2\n}", + ); + // every group sorts on its own + run_test( + "{\n \"d\": 4,\n \"c\": 3,\n\n \"b\": 2,\n \"a\": 1\n}", + "{\n \"c\": 3,\n \"d\": 4,\n\n \"a\": 1,\n \"b\": 2\n}", + ); + // the trailing comma still belongs to whatever ends the object + run_test( + "{\n \"b\": 2,\n\n \"d\": 4,\n \"c\": 3,\n}", + "{\n \"b\": 2,\n\n \"c\": 3,\n \"d\": 4,\n}", + ); + // a group of one has nothing to sort + run_test("{\n \"b\": 2,\n\n \"a\": 1\n}", "{\n \"b\": 2,\n\n \"a\": 1\n}"); + // an object with no blank line is one group + run_test("{\n \"b\": 2,\n \"a\": 1\n}", "{\n \"a\": 1,\n \"b\": 2\n}"); + // a comment directly under the blank line is part of the boundary and stays with it + run_test( + "{\n \"z\": 1,\n\n // section\n \"b\": 2,\n \"a\": 3\n}", + "{\n \"z\": 1,\n\n // section\n \"a\": 3,\n \"b\": 2\n}", + ); + // but one written further down the group belongs to its property and travels with it + run_test( + "{\n \"z\": 1,\n\n \"c\": 3,\n // about b\n \"b\": 2,\n \"a\": 0\n}", + "{\n \"z\": 1,\n\n \"a\": 0,\n // about b\n \"b\": 2,\n \"c\": 3\n}", + ); + } + + #[test] + fn sort_properties_pinning_some_of_the_comments() { + #[track_caller] + fn run_test(json: &str, expected: &str) { + let cst = build_cst(json); + let root_obj = cst.object_value().unwrap(); + // only the first comment above a property heads its group; the rest are its own + root_obj + .sort_properties() + .pin_comment_headers_with(|prop, _| if prop.has_blank_line_before() { 1 } else { 0 }) + .by_key(|prop| prop.decoded_name()); + assert_eq!(cst.to_string(), expected); + build_cst(&cst.to_string()); + } + + // the first comment heads the group and the second describes the property under it + run_test( + "{\n \"c\": 3,\n\n // section\n // about b\n \"b\": 2,\n \"a\": 1\n}", + "{\n \"a\": 1,\n\n // section\n // about b\n \"b\": 2,\n \"c\": 3\n}", + ); + // a blank line between the header and the note keeps the blank with the header + run_test( + "{\n \"c\": 3,\n\n // section\n\n // about b\n \"b\": 2,\n \"a\": 1\n}", + "{\n \"a\": 1,\n\n // section\n\n // about b\n \"b\": 2,\n \"c\": 3\n}", + ); + } + + #[test] + fn sort_gives_up_when_the_comparator_changes_the_object() { + let text = "{ + \"b\": 2, + \"a\": 1 +}"; + let cst = build_cst(text); + let root_obj = cst.object_value().unwrap(); + root_obj.sort_properties().by_key(|prop| { + // removing a property leaves the sort with nothing safe to write back + if prop.decoded_name().as_deref() == Some("b") { + prop.clone().remove(); + } + prop.decoded_name() + }); + + // the removal stands, but nothing was reordered on top of it + assert_eq!( + cst.to_string(), + "{ + \"a\": 1 +}" + ); + } + #[test] fn sort_keeps_line_comments_ending_their_line() { #[track_caller] diff --git a/tests/sort_fuzz.rs b/tests/sort_fuzz.rs index 3165da4..35a0af3 100644 --- a/tests/sort_fuzz.rs +++ b/tests/sort_fuzz.rs @@ -16,14 +16,18 @@ use jsonc_parser::cst::CstRootNode; fn sorting_generated_documents_preserves_them() { let mut random = Random::new(0x5eed_1234_9abc_def0); for _ in 0..20_000 { - // the option only changes which trivia travels, so both settings have to hold the same invariants - let maintain_headers = random.chance(2); - check(&mut random, Shape::Object, maintain_headers); - check(&mut random, Shape::Array, maintain_headers); + // the options only change which trivia travels and how far an element may move, so every + // combination has to hold the same invariants + let options = Options { + pin_headers: random.chance(2), + within_groups: random.chance(2), + }; + check(&mut random, Shape::Object, options); + check(&mut random, Shape::Array, options); } } -fn check(random: &mut Random, shape: Shape, maintain_headers: bool) { +fn check(random: &mut Random, shape: Shape, options: Options) { let text = generate(random, shape); let Ok(root) = CstRootNode::parse(&text, &ParseOptions::default()) else { // the generator is allowed to produce something the parser rejects; nothing to sort then @@ -75,8 +79,8 @@ fn check(random: &mut Random, shape: Shape, maintain_headers: bool) { // sorting what is already sorted leaves it alone match shape { - Shape::Object => sort_properties(&reparsed.object_value().unwrap(), maintain_headers), - Shape::Array => sort_elements(&reparsed.array_value().unwrap(), maintain_headers), + Shape::Object => sort_properties(&reparsed.object_value().unwrap(), options), + Shape::Array => sort_elements(&reparsed.array_value().unwrap(), options), } assert_eq!( reparsed.to_string(), @@ -85,23 +89,31 @@ fn check(random: &mut Random, shape: Shape, maintain_headers: bool) { ); } -fn sort_properties(object: &CstObject, maintain_headers: bool) { - let sort = object.sort_properties(); - let sort = if maintain_headers { - sort.maintain_comment_headers() - } else { - sort - }; +#[derive(Clone, Copy)] +struct Options { + pin_headers: bool, + within_groups: bool, +} + +fn sort_properties(object: &CstObject, options: Options) { + let mut sort = object.sort_properties(); + if options.pin_headers { + sort = sort.pin_comment_headers(); + } + if options.within_groups { + sort = sort.within_groups(); + } sort.by_key(|prop| prop.decoded_name()); } -fn sort_elements(array: &CstArray, maintain_headers: bool) { - let sort = array.sort_elements(); - let sort = if maintain_headers { - sort.maintain_comment_headers() - } else { - sort - }; +fn sort_elements(array: &CstArray, options: Options) { + let mut sort = array.sort_elements(); + if options.pin_headers { + sort = sort.pin_comment_headers(); + } + if options.within_groups { + sort = sort.within_groups(); + } sort.by_key(|element| element.to_string()); } From 1c7932b60991522a66d862c06e5935e702995859 Mon Sep 17 00:00:00 2001 From: David Sherret Date: Sat, 12 Sep 2026 17:07:14 -0400 Subject: [PATCH 5/5] fix(cst): close the gaps two reviews found in sorting The fuzzer was building its options and then sorting without them, so only the plain sort was ever checked; the options are applied now, and the global order assertion is skipped under grouping, where not sorting as a whole is the point. A rule or comparator that replaces a member kept the child count the same, so the guard let it through and the stale snapshot was written back, undoing the replacement and detaching whatever the caller held. The guard now asks whether each child still answers to its slot, which a removed or replaced one does not. A blank line is how a container was laid out rather than something written with the member beneath it, so it stays put whenever a header rule is set, including when no comment is pinned. The count could not say that, since "pin none of them" and "there are none" were the same answer. A blank line under the open token is a group boundary like any other; it was being skipped because there was no group before it. A header could end part way along a line holding two comments, gluing the member onto it. The split now backs up to the start of the line, so a line is pinned whole or not at all. A blank line written with spaces in it now counts as one everywhere. Also: one definition of what counts as a blank line rather than the same state machine written forwards and backwards, no node-to-property round trip for the default rule, no comment collecting when no rule is set, and docs that say what the code does about the open brace line, the line break a moved line comment gains, what has_blank_line_before measures, and what a rule must not do. --- src/cst/mod.rs | 146 ++++++++++++++++++++++++++++++--------------- tests/sort_fuzz.rs | 18 +++--- 2 files changed, 108 insertions(+), 56 deletions(-) diff --git a/src/cst/mod.rs b/src/cst/mod.rs index c741662..3dcd728 100644 --- a/src/cst/mod.rs +++ b/src/cst/mod.rs @@ -130,7 +130,8 @@ macro_rules! add_parent_info_methods { indent_text(&self.clone().into()) } - /// Whether a blank line separates this node from whatever was written before it. + /// Whether a blank line separates this node, and the comments written above it, from + /// whatever came before them. pub fn has_blank_line_before(&self) -> bool { has_blank_line_before(&self.clone().into()) } @@ -147,24 +148,10 @@ macro_rules! add_parent_info_methods { }; } -/// Whether a blank line separates the node from whatever was written before it. +/// Whether a blank line separates the node, and the comments written above it, from what came +/// before them. fn has_blank_line_before(node: &CstNode) -> bool { - let mut ended_a_line = false; - for previous in node.previous_siblings() { - if previous.is_newline() { - if ended_a_line { - return true; - } - ended_a_line = true; - } else if previous.is_whitespace() { - // keeps looking past the indentation - } else if previous.is_comment() { - ended_a_line = false; - } else { - return false; - } - } - false + has_blank_line(node.previous_siblings().take_while(|n| n.is_trivia())) } fn find_trailing_comma(node: &CstNode) -> Option { @@ -1834,10 +1821,15 @@ impl CstObject { /// Sorts the properties of the object. /// /// What was written with a property travels with it: the comments and blank lines above it, and - /// a comment written after it on the same line. What belongs to no property stays where it is, - /// which includes whatever follows the open brace and whatever precedes the close brace. Each - /// property gains or loses a comma to suit its new position, and whether the object ends with a - /// trailing comma is preserved. + /// a comment written after it on the same line. Whatever precedes the close brace, and whatever + /// shares the open brace's line, belongs to no property and stays where it is. Each property + /// gains or loses a comma to suit its new position, and whether the object ends with a trailing + /// comma is preserved. + /// + /// A blank line under the open brace travels with the property it was written above, and one + /// that would end up there instead is dropped, since a gap there reads as belonging to the + /// object. A line comment that would otherwise comment out what now follows it gains a line + /// break, which can make a single line object span several. /// /// Nothing moves until [`PropertySort::by`] or [`PropertySort::by_key`] says how to order them. /// @@ -2699,8 +2691,9 @@ impl<'a> PropertySort<'a> { /// "prop2": 2 /// }"#); /// ``` - pub fn pin_comment_headers(self) -> Self { - self.pin_comment_headers_with(|prop, comments| blank_line_header_rule(&prop.clone().into(), comments)) + pub fn pin_comment_headers(mut self) -> Self { + self.options.header_rule = Some(Box::new(blank_line_header_rule)); + self } /// Decides for each property how much of what was written above it is a heading for what @@ -2715,7 +2708,10 @@ impl<'a> PropertySort<'a> { /// else { 0 }`. A count in between splits a block that is partly a heading and partly a note /// about the property itself. /// - /// The rule must not add or remove properties. Doing so leaves the sort with nothing safe to + /// The rule is only consulted where a header could be written, which is a property on a line of + /// its own; it is not called for an object written on one line. + /// + /// The rule must not change the object's children. Doing so leaves the sort with nothing safe to /// write back, so it gives up and leaves the object as the rule left it. pub fn pin_comment_headers_with(mut self, mut rule: impl FnMut(&CstObjectProp, &[CstComment]) -> usize + 'a) -> Self { self.options.header_rule = Some(Box::new(move |element, comments| match element.as_object_prop() { @@ -2769,8 +2765,8 @@ impl<'a> PropertySort<'a> { /// Sorts the properties with the given comparator. /// /// The sort is stable, so properties that compare equal keep the order they were written in. The - /// comparator must describe a total order, as [`slice::sort_by`] panics otherwise, and it must - /// not add or remove properties; see [`PropertySort::pin_comment_headers_with`]. + /// comparator must describe a total order, as the sort may panic otherwise, and it must not + /// change the object's children; see [`PropertySort::pin_comment_headers_with`]. pub fn by(self, mut compare: impl FnMut(&CstObjectProp, &CstObjectProp) -> Ordering) { let object = self.object.clone().into(); sort_comma_separated_children(&object, self.options, |groups| { @@ -2787,8 +2783,9 @@ impl<'a> PropertySort<'a> { /// Sorts the properties by a key, which is worked out once per property. /// - /// A property whose name can't be decoded has no key, and sorts above every property that has - /// one. Behaves like [`PropertySort::by`] in every other respect. + /// A child that isn't a property, which is only possible if the tree has been manipulated into + /// holding something else, has no key and sorts above every property. Behaves like + /// [`PropertySort::by`] in every other respect. pub fn by_key(self, mut key: impl FnMut(&CstObjectProp) -> K) { let object = self.object.clone().into(); sort_comma_separated_children(&object, self.options, |groups| { @@ -2897,8 +2894,7 @@ struct Separator { /// An element of a comma separated container along with the trivia that travels with it. /// -/// Every part of it is a stretch of the container's own children, which reordering only ever -/// copies, so they're held as ranges rather than as lists of their own. +/// Held as ranges for the same reason as [`Separator`]. struct SortableGroup { /// Where the element was written, so that a sort changing nothing can leave the tree alone. index: usize, @@ -2948,7 +2944,7 @@ fn sort_comma_separated_children( break run_start..region.len(); } let run = run_start..index; - let starts_group = !groups.is_empty() && run_has_blank_line(®ion[run.clone()]); + let starts_group = has_blank_line(region[run.clone()].iter().cloned()); let (separator, leading) = split_separator(region, run, ®ion[index], starts_group, &mut options); separators.push(separator); let trailing = index + 1..trailing_run_end(region, index + 1); @@ -2980,9 +2976,15 @@ fn sort_comma_separated_children( } else { sort(&mut groups); } - // Adding or removing members while the sort runs would leave the ranges worked out above - // pointing at children that have moved, so writing them back would corrupt the tree. - if container.children().len() != children.len() { + // Changing the container while the sort runs would leave the ranges worked out above pointing + // at children that have moved, so writing them back would undo the change and detach whatever + // the caller is holding. A child that was removed or replaced no longer answers to its slot. + let unchanged = container.children().len() == children.len() + && children + .iter() + .enumerate() + .all(|(index, child)| child.parent_info().map(|info| info.child_index) == Some(index)); + if !unchanged { return; } if groups @@ -2996,11 +2998,7 @@ fn sort_comma_separated_children( // a blank line here reads as a gap under the open token rather than as something written with // the element that follows, so it doesn't travel with whatever sorted to the top let first_leading = &mut groups[0].leading; - let blank_count = region[first_leading.clone()] - .iter() - .take_while(|n| n.is_newline()) - .count(); - first_leading.start += blank_count; + first_leading.start += leading_blank_line_len(®ion[first_leading.clone()]); let last_index = groups.len() - 1; let mut new_children = Vec::with_capacity(children.len()); @@ -3029,9 +3027,33 @@ fn is_sortable_element(node: &CstNode) -> bool { !node.is_trivia() && !node.is_token() } +/// How much of the start of a run is blank lines, counting a line of nothing but whitespace as one. +/// +/// Stops at the first line holding anything, so the indentation in front of a comment is left for +/// the comment rather than counted as a blank line of its own. +fn leading_blank_line_len(run: &[CstNode]) -> usize { + let mut len = 0; + let mut index = 0; + while index < run.len() { + let mut end = index; + while end < run.len() && run[end].is_whitespace() { + end += 1; + } + if end < run.len() && run[end].is_newline() { + index = end + 1; + len = index; + } else { + break; + } + } + len +} + /// Whether a run of trivia leaves a line empty, which is what marks a group boundary and what /// tells a comment heading a group from one describing the element beneath it. -fn run_has_blank_line(run: &[CstNode]) -> bool { +/// +/// Reads the same either way round, so the run may be walked forwards or backwards. +fn has_blank_line(run: impl IntoIterator) -> bool { let mut ended_a_line = false; for node in run { if node.is_newline() { @@ -3103,6 +3125,10 @@ fn split_separator( /// The header runs up to the line the first comment that isn't part of it begins on, so that the /// blank line under a header stays with the header where it reads. fn header_len(leading: &[CstNode], element: &CstNode, options: &mut SortOptions<'_>) -> usize { + // the common sort sets no rule at all, and then nothing above an element ever stays + if options.header_rule.is_none() { + return 0; + } let comments = leading .iter() .filter_map(|node| match node { @@ -3111,23 +3137,29 @@ fn header_len(leading: &[CstNode], element: &CstNode, options: &mut SortOptions< }) .collect::>(); let pinned = options.pinned_comment_count(element, &comments); - if pinned == 0 { - return 0; - } if pinned >= comments.len() { return leading.len(); } - let mut split = leading + // A blank line is how the container was laid out rather than something written with the element, + // so it stays put whenever the caller is deciding what travels, even when no comment does. + let blank_lines = leading_blank_line_len(leading); + if pinned == 0 { + return blank_lines; + } + let first_travelling = leading .iter() .enumerate() .filter(|(_, node)| node.is_comment()) .map(|(index, _)| index) .nth(pinned) - .unwrap_or(leading.len()); - while split > 0 && leading[split - 1].is_whitespace() { + .expect("a comment past the pinned ones, since fewer were pinned than there are"); + // back up to the start of that comment's line, so that a header never ends part way along one + // and leaves what follows glued to it + let mut split = first_travelling; + while split > 0 && !leading[split - 1].is_newline() { split -= 1; } - split + split.max(blank_lines) } /// The end of the run after an element that was written with it: whatever separates the element @@ -4949,7 +4981,6 @@ value3: true build_cst(&cst.to_string()); } - // a blank line divides the object and nothing sorts across it // a blank line divides the object and nothing sorts across it run_test( "{\n \"m\": 1,\n\n // section\n \"z\": 2,\n \"a\": 3\n}", @@ -5033,6 +5064,23 @@ value3: true ); } + #[test] + fn sort_gives_up_when_the_comparator_replaces_a_member() { + let cst = build_cst("{\n \"b\": 2,\n \"a\": 1\n}"); + let root_obj = cst.object_value().unwrap(); + root_obj.sort_properties().by_key(|prop| { + let name = prop.decoded_name(); + // a replacement leaves the child count alone, so only checking that would miss it + if name.as_deref() == Some("b") { + prop.clone().replace_with("zzz", json!(9)); + } + name + }); + + // the replacement stands and nothing was reordered on top of it + assert_eq!(cst.to_string(), "{\n \"zzz\": 9,\n \"a\": 1\n}"); + } + #[test] fn sort_keeps_line_comments_ending_their_line() { #[track_caller] diff --git a/tests/sort_fuzz.rs b/tests/sort_fuzz.rs index 35a0af3..5ad9532 100644 --- a/tests/sort_fuzz.rs +++ b/tests/sort_fuzz.rs @@ -41,13 +41,13 @@ fn check(random: &mut Random, shape: Shape, options: Options) { let Some(object) = root.object_value() else { return; }; - object.sort_properties().by_key(|prop| prop.decoded_name()); + sort_properties(&object, options); } Shape::Array => { let Some(array) = root.array_value() else { return; }; - array.sort_elements().by_key(|element| element.to_string()); + sort_elements(&array, options); } } @@ -61,11 +61,15 @@ fn check(random: &mut Random, shape: Shape, options: Options) { sorted_lines(&after), "contents changed\n--- input ---\n{text}\n--- output ---\n{sorted}" ); - // only the key decides the order; members sharing one keep the order they were written in - assert!( - after.windows(2).all(|pair| pair[0].0 <= pair[1].0), - "not in order\n--- input ---\n{text}\n--- output ---\n{sorted}" - ); + // Only the key decides the order, and members sharing one keep the order they were written + // in. Grouping deliberately leaves the container unsorted as a whole, so this only holds when + // the sort was free to move a member anywhere. + if !options.within_groups { + assert!( + after.windows(2).all(|pair| pair[0].0 <= pair[1].0), + "not in order\n--- input ---\n{text}\n--- output ---\n{sorted}" + ); + } assert_eq!( keyed_order(&before), keyed_order(&after),