feat(cst): sort an object's properties or an array's elements - #88
Merged
Merged
Conversation
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.
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.
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.
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.
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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Adds CST reordering that carries each member's comments along with it — the thing the AST plus a position-keyed comment map can't express. Motivated by dprint/dprint-plugin-json#40, which wants to write a
package.json's properties in a conventional order; doing it on the AST there forced two compromises (files with comments skipped entirely, and a reordered object losing its blank lines) that this removes.A sort is started with
sort_properties()/sort_elements()and told how to order things withbyorby_key.by_keyworks the key out once per member rather than once per comparison, which matters when the key is a decoded name. Both are stable.What travels
A container's children read as
open [ sep lead element trail ]... tail close.Commas are re-decided per position, carrying over whichever of a trailing comma or none the container was written with, and one is added where the new order needs it even if the author left it out. The comma is claimed wherever it was written, including on a later line, so leading-comma style can't strand one in front of the member that follows.
Two normalizations keep the result meaning what it did: a blank line landing 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 member or the closing token.
Ownership of a same-line comment follows what
remove_comma_separatedalready does: everything up to and including the comma was written with the member, and a comment after the comma is the member's only when nothing else shares the line. So{ "b": 2, /* between */ "a": 1 }treats the comment as written above"a", the same answerremovegives.Deciding what travels
The default above is right for a comment describing the member beneath it and wrong for one heading a group, so the caller can say which is which:
pin_comment_headers()— a comment with a blank line above it heads what follows and stays where it was written, while members sort past it. Without it a heading is carried off to wherever its first member lands:pin_comment_headers_with(rule)— the rule is handed the member and the comments written above it, and returns how many of them, counting from the top, stay put; the rest travel.pin_comment_headers()isif member.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 member itself, which a boolean can't say:Handing the rule the comments the sorter itself computed is deliberate: a count against a list the caller rebuilds would disagree with the sorter exactly in the interesting cases (a comment after the previous member's comma, a block comment sharing the member's line), and an off-by-one there is silent.
within_groups()— each run of members between blank lines sorts on its own and no member crosses one. A blank line and whatever was written under it is the boundary between two groups, and a boundary stays where it is, so this doesn't depend on which header rule was set; a rule still decides what travels with members inside each group.has_blank_line_before()is public on every node, since a custom rule needs it.name_decoded→ publicdecoded_nameIt was private and gated behind the
serde_jsonfeature, and it's the natural sort key, so it had to become public. Renamed while it's still free to rename:decoded_value()is the crate's existing spelling for this operation onCstStringLitandObjectPropName, andname_decodedinverted that word order.A rule or comparator that mutates the container
This used to be partly undone and partly kept — a value set through the shared node survived while a removal was reverted over stale ranges, which could write a corrupted tree. The sort now notices the container changed underneath it and leaves it alone instead.
Performance
Splitting a container was building four small
Vecs per member. Each is a stretch of the container's own children that reordering only ever copies, so they're ranges now and the nodes are copied straight out at write-back. The one edited run — the trailing one, because the comma has to suit the new position — records where its comma sits and settles it while writing out, which removed the copy and a whole mutating pass.by_keyusessort_by_cached_key.Testing
Unit tests cover field order, trailing-comma styles, single-line and compact containers, word and escaped names, duplicate names (stability), CRLF, missing commas, leading-comma style, comments in every position, blank lines on both sides of the rule, partial header splits, group boundaries vs comments inside a group, the mutation guard, and that handles taken before a sort still resolve and remain editable.
tests/sort_fuzz.rsgenerates 40,000 documents — trivia in every slot, blank lines, CRLF, missing and trailing commas, duplicate and escaped names, nested values — across all four option combinations, asserting each result re-parses, holds the same members in the comparator's order, preserves stability for equal keys, keeps every comment, and sorts idempotently. ~2.7s, and CI already runs--all-features.That fuzzer is not decoration. It and the review passes found six bugs the hand-written tests missed: a line comment swallowing the member after it, leading-comma style emitting a container that starts with a comma, a resulting doubled
,,, a replaced member being silently reverted and detached, a blank line riding along instead of staying put, and a header splitting part way along a line holding two comments. It also caught a bug in itself — for a while it was building its options and then sorting without them, so only the default sort was really being checked.Everything is additive, so a patch release covers it. 160 lib tests, 14 doctests and the fuzz test pass; no new clippy warnings (the two in
src/serde.rsare pre-existing).