Always call the sort comparator, even for identical values - #1645
Open
andreasrosdal wants to merge 4 commits into
Open
Always call the sort comparator, even for identical values#1645andreasrosdal wants to merge 4 commits into
andreasrosdal wants to merge 4 commits into
Conversation
js_array_cmp_generic() skips the comparator when the two JSValues are
bitwise identical, on the assumption that a comparator returns 0 for
identical objects. SortCompare makes no such allowance: if comparefn is
not undefined it is called for every compared pair, and its return value
is the only thing that decides the order.
The assumption also breaks code that uses the comparator for its side
effects. jQuery's uniqueSort() sorts a node list with a comparator that
sets a `hasDuplicate` flag whenever it is handed the same node twice, then
strips the duplicates afterwards. With the shortcut in place the flag is
never set and duplicates survive:
const o = {};
let dups = false;
[o, o].sort((a, b) => { if (a === b) dups = true; return 0; });
dups // false, V8/JSC/SpiderMonkey: true
V8, JavaScriptCore and SpiderMonkey all call the comparator here. Drop the
shortcut; the `cmp_same` label goes with it, since the stable-sort
tie-break it guarded is the normal fallthrough.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_014mv33YvfHz7t9mmkituBnn
Contributor
|
LGTM but please add a test. |
Covers the behaviour restored by "Always call the sort comparator, even for identical values": SortCompare has no allowance for skipping comparefn when the two values happen to be the same value, and code that uses the comparator for its side effects depends on the call happening. Every block fails without the fix: the jQuery uniqueSort() flag pattern, identical primitives, toSorted(), an exception thrown from the comparator for an identical pair, and an all-identical array large enough to leave the insertion sort cutoff. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KZ4EJPvK2gevMjpx1xdY21
andreasrosdal
force-pushed
the
fix-sort-always-call-comparator
branch
from
August 7, 2026 08:25
e099d5f to
34a187c
Compare
The removed shortcut compared the two JSValues bit for bit, so it fired for every type whose value is the payload itself or a shared pointer: identical strings, NaN, bigints, symbols, booleans and null all skipped the comparator as well. Pin each of them, and the array-like path through Array.prototype.sort.call(). Also pin the two things that must not change: undefined values and holes still sort to the end without reaching the comparator, which is SortCompare's own rule, and the separate typed array comparison function keeps calling it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01K6eRbuuuCujKgQkrHgvMrc
Every pair now reaches user code, including the pairs the shortcut used to answer without a call, so there are strictly more points at which the comparator can reach back into the array. Added a comparator that shrinks, grows, deletes from, reverses and re-sorts the array under the sort's feet, the coercion of the comparator's return value and of a comparator that is not callable, and a two thousand element run checked for order and stability, once with distinct elements and once where every element is the same object.
saghul
approved these changes
Aug 7, 2026
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
3 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.
js_array_cmp_generic()skips the comparator when the twoJSValues are bitwise identical, on the assumption that a comparator returns 0 for identical objects. SortCompare makes no such allowance: ifcomparefnis not undefined it is called for every compared pair, and its return value is the only thing that decides the order.The assumption also breaks code that uses the comparator for its side effects. jQuery's
uniqueSort()sorts a node list with a comparator that sets ahasDuplicateflag whenever it is handed the same node twice, then strips the duplicates afterwards. With the shortcut in place the flag is never set and duplicates survive:V8, JavaScriptCore and SpiderMonkey all call the comparator here. Drop the shortcut; the
cmp_samelabel goes with it, since the stable-sort tie-break it guarded is the normal fallthrough.🤖 Generated with Claude Code
https://claude.ai/code/session_014mv33YvfHz7t9mmkituBnn
Generated by Claude Code