SortComparator: materialise comparator Sequence before reusing it for sort (#874)#2009
Open
wadetregaskis wants to merge 1 commit into
Open
SortComparator: materialise comparator Sequence before reusing it for sort (#874)#2009wadetregaskis wants to merge 1 commit into
Sequence before reusing it for sort (#874)#2009wadetregaskis wants to merge 1 commit into
Conversation
…or sort (swiftlang#874). The public `Sequence.sorted(using: S)` overload that takes a `Sequence` of SortComparators forwards each pair of elements through `comparators.compare(_:_:)`, which itself iterates the entire sequence. That works for `Array` (or any multi-pass collection) but `Sequence` does not promise non-destructive iteration: a single-use sequence (e.g. `AnySequence` wrapping an `AnyIterator`) is exhausted after the first comparison, so every subsequent call to `compare` walks an empty iterator and returns `.orderedSame` — silently leaving the bulk of the input unsorted. This patch fixes that by snapshotting the comparator sequence into an `Array` before sorting so it can be replayed safely.
jmschonfeld
reviewed
Jun 1, 2026
| // non-destructive multi-pass iteration. A single-use sequence (e.g. one | ||
| // backed by `AnyIterator`) would otherwise be exhausted after the first | ||
| // comparison and silently produce an unsorted result. | ||
| let comparators = Array(comparators) |
Contributor
There was a problem hiding this comment.
Have you measured the performance impact of requiring the comparators to be copied into a heap allocation for every call site, even those that could have been iterated twice?
We also don't typically wrap comments in this repo, nor do I think we need a long comment here - a short comment would suffice.
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
2 participants
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.
SortComparatorshouldn't assume it can iterate itsSequenceofSortComparators multiple times.Motivation:
The public
Sequence.sorted(using: S)overload that takes aSequenceofSortComparators forwards each pair of elements throughcomparators.compare(_:_:), which itself iterates the entire sequence. That works forArray(or any multi-pass collection) butSequencedoes not promise non-destructive iteration: a single-use sequence (e.g.AnySequencewrapping anAnyIterator) is exhausted after the first comparison, so every subsequent call tocomparewalks an empty iterator and returns.orderedSame— silently leaving the bulk of the input unsorted.I don't know how common it is to do this - I imagine the concrete type used is just an
Array99% of the time, which happens to work just fine - but this is easy to fix and the kind of bug that's quite hard to find if you stumble into it unwittingly, as a user of this API.Modifications:
This patch fixes that by snapshotting the comparator sequence into an
Arraybefore sorting so it can be replayed safely.Result:
Sorting works as the caller intended, irrespective of what type of
Sequencethey use.Testing:
New unit test added.