Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Array.sort arguments order changed in Node.js 11 #24294

Closed
hudochenkov opened this issue Nov 10, 2018 · 20 comments

Comments

@hudochenkov
Copy link

@hudochenkov hudochenkov commented Nov 10, 2018

  • Version: 11.1.0
  • Platform: Darwin hubert.local 18.2.0 Darwin Kernel Version 18.2.0: Fri Oct 5 19:41:49 PDT 2018; root:xnu-4903.221.2~2/RELEASE_X86_64 x86_64 (macOS 10.14.1)
  • Subsystem:

Hi!

I've noticed a change with Array.sort(). In Node.js 11 order of arguments passed to compare function is reversed. While in most cases it's not a problem, in some cases it causes problems.

In my project I have quite large compare function for .sort(): https://github.com/hudochenkov/postcss-sorting/blob/944da947a628192b54448368c197f586fbbe0c10/lib/sorting.js#L10-L62. It relies on arguments order. It works in Node.js 4—10. I don't expect anyone to figure out what my function does, and I can't simplify it for this issue report yet.

I created a gist, which shows how arguments get to compare function in Node.js 10 and Node.js 11.

$ npx -p node@10 npx https://gist.github.com/hudochenkov/29b739f8dbdb4aa00a46b953f62dd0a6

a: 1, b: 2
a: 2, b: 3
a: 3, b: 4
[ 1, 2, 3, 4 ]

$ npx -p node@11 npx https://gist.github.com/hudochenkov/29b739f8dbdb4aa00a46b953f62dd0a6

a: 2, b: 1
a: 3, b: 2
a: 4, b: 3
[ 1, 2, 3, 4 ]

Is it a regression or intentional change?

I tried to understand how V8 7.0 changed .sort(), but it's to complex for me :(

@richardlau

This comment has been minimized.

Copy link
Member

@richardlau richardlau commented Nov 10, 2018

It's intentional: #22754 (comment)

@refack

This comment has been minimized.

Copy link
Member

@refack refack commented Nov 10, 2018

Hello @hudochenkov, and thank you for the report.
As you mention, this is a change node get by way of a update V8. There was some discussion on the implication of the new algorithm #22754 (comment), but AFAIK this specific issue was reported for node

/CC @nodejs/v8 (esp. @targos @hashseed @mathiasbynens)

@refack

This comment has been minimized.

Copy link
Member

@refack refack commented Nov 10, 2018

P.S. @hudochenkov can you provide some data that using your code sorts differently?

@devsnek

This comment has been minimized.

Copy link
Member

@devsnek devsnek commented Nov 10, 2018

@hudochenkov stable sort means what if elements in the unsorted array were already in the correct order, they are guaranteed to be in the same order after sorting.

in any case, as long as you always return the correct number for two elements, no matter the order, the engine will properly handle it. if the engine switches the order of the arguments, it will also handle the switched return value.

@hudochenkov

This comment has been minimized.

Copy link
Author

@hudochenkov hudochenkov commented Nov 10, 2018

Thank you for quick replies!

I'll simplify my sorting function as much as possible and will provide examples of how it sorts differently. I'll do it tomorrow.

@refack

This comment was marked as outdated.

Copy link
Member

@refack refack commented Nov 10, 2018

I've forked your gist - https://gist.github.com/refack/8fc2bbdf084f9ad28f900ad5a4f80a14 to uncover the regression:

$ npx -p node@10 npx https://gist.github.com/refack/8fc2bbdf084f9ad28f900ad5a4f80a14

a: { p: 'mottob', id: 1 }, b: { p: 'bottom', id: 2 } - 0
a: { p: 'bottom', id: 2 }, b: { p: 'mottob', id: 3 } - -1
a: { p: 'mottob', id: 3 }, b: { p: 'bottom', id: 4 } - 0
[ { p: 'mottob', id: 1 },
  { p: 'bottom', id: 2 },
  { p: 'mottob', id: 3 },
  { p: 'bottom', id: 4 } ]


$ npx -p node@11 npx https://gist.github.com/refack/8fc2bbdf084f9ad28f900ad5a4f80a14

a: { p: 'bottom', id: 2 }, b: { p: 'mottob', id: 1 } - -1
a: { p: 'mottob', id: 3 }, b: { p: 'bottom', id: 2 } - 0
a: { p: 'mottob', id: 3 }, b: { p: 'mottob', id: 1 } - 0
a: { p: 'bottom', id: 4 }, b: { p: 'mottob', id: 1 } - -1
a: { p: 'bottom', id: 4 }, b: { p: 'bottom', id: 2 } - -1
[ { p: 'bottom', id: 4 },
  { p: 'bottom', id: 2 },
  { p: 'mottob', id: 1 },
  { p: 'mottob', id: 3 } ]
@refack refack added the regression label Nov 10, 2018
@refack

This comment has been minimized.

Copy link
Member

@refack refack commented Nov 10, 2018

simpler code:

const declarations = [
  {p: 'mottob', id: 1},
  {p: 'bottom', id: 2},
  {p: 'mottob', id: 3},
  {p: 'mottob', id: 4},
];

if (process.argv[2] === 'anti' ) {
  declarations.sort((b, a) => {
    const ret = sortDeclarations(a, b);
    console.log(`a: %o, b: %o - %d`, a, b, ret);

    return -ret
  });
} else {
  declarations.sort((a, b) => {
    const ret = sortDeclarations(a, b);
    console.log(`a: %o, b: %o - %d`, a, b, ret);

    return ret
  });
}

console.log(declarations);

function sortDeclarations(a, b) {
  if (b.p === 'bottom') {
    return 1;
  }
  return a.id - b.id;
}

Get me:

D:\code\prws>d:\bin\dev\node\node10.9.0.exe t.js
a: { p: 'mottob', id: 1 }, b: { p: 'bottom', id: 2 } - 1
a: { p: 'mottob', id: 1 }, b: { p: 'mottob', id: 3 } - -2
a: { p: 'mottob', id: 3 }, b: { p: 'mottob', id: 4 } - -1
[ { p: 'bottom', id: 2 },
  { p: 'mottob', id: 1 },
  { p: 'mottob', id: 3 },
  { p: 'mottob', id: 4 } ]

D:\code\prws>d:\bin\dev\node\node11.0.0.exe t.js
a: { p: 'bottom', id: 2 }, b: { p: 'mottob', id: 1 } - 1
a: { p: 'mottob', id: 3 }, b: { p: 'bottom', id: 2 } - 1
a: { p: 'mottob', id: 4 }, b: { p: 'mottob', id: 3 } - 1
[ { p: 'mottob', id: 1 },
  { p: 'bottom', id: 2 },
  { p: 'mottob', id: 3 },
  { p: 'mottob', id: 4 } ]

D:\code\prws>d:\bin\dev\node\node11.0.0.exe t.js anti
a: { p: 'mottob', id: 1 }, b: { p: 'bottom', id: 2 } - 1
a: { p: 'bottom', id: 2 }, b: { p: 'mottob', id: 3 } - -1
a: { p: 'mottob', id: 1 }, b: { p: 'mottob', id: 3 } - -2
a: { p: 'mottob', id: 1 }, b: { p: 'mottob', id: 4 } - -3
a: { p: 'mottob', id: 3 }, b: { p: 'mottob', id: 4 } - -1
[ { p: 'bottom', id: 2 },
  { p: 'mottob', id: 1 },
  { p: 'mottob', id: 3 },
  { p: 'mottob', id: 4 } ]
@mathiasbynens

This comment has been minimized.

Copy link
Contributor

@mathiasbynens mathiasbynens commented Nov 11, 2018

  • Array#sort is returning the correct result, just like before the change in V8. This report is not about a spec correctness issue.

  • The order in which the elements are compared during sorting (and similarly, how many times the comparison function is called in total) is intentionally unspecified, and depends on the exact implementation. When the V8 implementation changed from QuickSort to TimSort, this order changed. But that’s the thing: this order wildly varies across implementations, and even within the same engine it can change at any point; developers shouldn’t rely on it. This is explicitly called out in https://v8.dev/blog/array-sort#accessors-prototype.

This is not a regression; it’s an implementation detail that shouldn’t be relied upon.

@mathiasbynens

This comment has been minimized.

Copy link
Contributor

@mathiasbynens mathiasbynens commented Nov 11, 2018

My above comment was in response to the gist in the OP, i.e. https://gist.github.com/hudochenkov/29b739f8dbdb4aa00a46b953f62dd0a6.

@refack, let me respond to your test case now. This program demonstrates something separate from the issue OP reported. The sort callback in this code example is what the spec calls an inconsistent comparison function. For example, assume the following values:

  • a = { p: 'x', id: 3 }
  • b = { p: 'bottom', id: 4 }

Now, given the way sortDeclarations is written, we can observe the following:

  • if you compare a with b, sortDeclarations returns 1
  • if you compare b with a, sortDeclarations returns 4 - 3 which is… also 1

The result of a sort with an inconsistent comparison function is implementation-defined, and cannot be relied upon. In other words, this too is WAI.

@devsnek devsnek added invalid and removed regression labels Nov 11, 2018
@hudochenkov

This comment has been minimized.

Copy link
Author

@hudochenkov hudochenkov commented Nov 11, 2018

Thank you for investigation, @refack! And thank you for clarification, @mathiasbynens!

this order wildly varies across implementations, and even within the same engine it can change at any point; developers shouldn’t rely on it.

I guess I was just lucky with my sorting function all these years :) I'm going to rewrite it to be independent of arguments order.

@refack

This comment has been minimized.

Copy link
Member

@refack refack commented Nov 11, 2018

inconsistent comparison function.

I agree on that. But we still have a user-land regression, due to a dependency on unspecified behaviour.

If timsort can call the the compare function in an anti-commutative way, and that will get some percentage of such code to work again, while not breaking the spec, IMO it's a win-win.

@refack refack added regression and removed invalid labels Nov 11, 2018
@refack refack reopened this Nov 11, 2018
@refack refack added regression and removed regression labels Nov 11, 2018
@refack

This comment has been minimized.

Copy link
Member

@refack refack commented Nov 11, 2018

I'm a bit ambivalent on this, but I'm still marking this regression since the array.sort implementation did not go through a deprecation cycle, and defacto we have user code that broke without enough notification. as per our policy - https://github.com/nodejs/node/blob/master/COLLABORATOR_GUIDE.md#when-breaking-changes-actually-break-things

P.S. in #22754 (comment) I did not object to landing this, even though there was indication that user code was going to break (at least 300 lines in ~50 npm packages) due to using a comparison function that returns Boolean, because IMHO that was misuse of the public API.
Here we have a much subtler situation, where the user code looks like it's correct (i.e. would pass compilation if it was strongly typed), and probably passes tests. Admittedly because it's relying on unspecified behaviour.

@mathiasbynens

This comment has been minimized.

Copy link
Contributor

@mathiasbynens mathiasbynens commented Nov 11, 2018

in #22754 (comment) I did not object to landing this, even though there was indication that user code was going to break (lt least 300 line in ~50 npm packages) due to using a comparison function that returns Boolean, because IMHO that was misuse of the public API.

Here we have a much subtler situation, where the user code looks like it's correct (i.e. would pass compilation if it was strongly typed), and probably passes tests. Admittedly because it's relying on unspecified behaviour.

I don’t see how these situations are different. They’re both examples of problems caused by inconsistent comparison functions. It’s not Node.js’s responsibility to provide support for something that’s explicitly unspecified behavior, IMHO.

A comparison function that returns a boolean true or false is no worse than a comparison function that returns only 1 or 0 (but never -1) — in fact, it’s equivalent per spec, since ToNumber is implicitly called on the result. Both situations are just as bad; they’re both inconsistent comparison functions.

@devsnek devsnek added the wontfix label Nov 11, 2018
@refack

This comment has been minimized.

Copy link
Member

@refack refack commented Nov 11, 2018

they’re both inconsistent comparison functions.

True.
But bottom line we have users hurting, and I'm asking, can we mitigate the situation? IMO if calling the comparison in anti-commutative way has only benefits, we should consider that. If that's not possible, we need to think is there's something else we can do. If we can't do anything, then we admit it's a known-limitation/wontfix situation.

Anyway I think we should do a publicity push to get users aware of this situation, and how they should solve issue with inconsistent comparison functions.

@mathiasbynens

This comment has been minimized.

Copy link
Contributor

@mathiasbynens mathiasbynens commented Nov 11, 2018

IMO if calling the comparison in anti-commutative way has only benefits, we should consider that.

I don’t understand what you mean by this suggestion.

@refack

This comment has been minimized.

Copy link
Member

@refack refack commented Nov 11, 2018

In https://github.com/v8/v8/blob/78f2610345fdd14ca401d920c140f8f461b631d1/third_party/v8/builtins/array-sort.tq#L362

instead of:

const result: Number = sortCompare(context, userCmpFn, x, y);

do

const result: Number = -sortCompare(context, userCmpFn, y, x);
@mathiasbynens

This comment has been minimized.

Copy link
Contributor

@mathiasbynens mathiasbynens commented Nov 11, 2018

AFAICT, that just shifts the problem to other inconsistent comparison functions.

@hashseed

This comment has been minimized.

Copy link
Member

@hashseed hashseed commented Nov 11, 2018

Imo it's a slippery slope if we started to cater to bugs in user code. Relevant xkcd: https://xkcd.com/1172/

@refack

This comment has been minimized.

Copy link
Member

@refack refack commented Nov 11, 2018

If there's an XKCD for this I have to concede (praise be to Randall Munroe seer of truth)

@refack refack closed this Nov 11, 2018
@targos targos added this to Closed issues in v11.x Nov 18, 2018
eliperkins added a commit to eliperkins/relay that referenced this issue Mar 7, 2019
This will highlight the issue posed on facebook#2675 and nodejs/node#24294
vsiakka added a commit to vsiakka/gherkin-lint that referenced this issue Mar 31, 2019
The rules broke because the behavior os Array.sort() changed in node 11
as described in nodejs/node#24294
vsiakka added a commit to vsiakka/gherkin-lint that referenced this issue Mar 31, 2019
The rules broke because the behavior os Array.sort() changed in node 11
as described in nodejs/node#24294
@NeerajDana

This comment has been minimized.

@cudr cudr referenced this issue Apr 9, 2019
@devsnek devsnek added regression and removed regression labels May 25, 2019
@jayco jayco referenced this issue Sep 7, 2019
@eliascodes eliascodes referenced this issue Oct 16, 2019
1 of 1 task complete
astroash added a commit to TwinePlatform/twine-monolith that referenced this issue Oct 17, 2019
Changes made as part of adding projects chart (see #335)

### Changes
- Fix typo `abreviated` -> `abbreviated`
- Add `sortFormatted` function to sort formatted date strings chronologically
- Add `Strings.truncate` function
- Truncate labels at 10 chars if they are not date strings
- Upgrade to node v12 on travis to prevent inconsistent sorting behavious between local & travis (see nodejs/node#24294)

### Testing Requirements
- [x] Dashboard
  - Check dashboard landing page statistics make sense and are formatted right
  - Check order of months is chronological on all charts

### Release Requirements
- Not yet.

### Manual Deployment
- N/A
eliascodes added a commit to TwinePlatform/twine-monolith that referenced this issue Nov 1, 2019
Changes made as part of adding projects chart (see #335)

### Changes
- Fix typo `abreviated` -> `abbreviated`
- Add `sortFormatted` function to sort formatted date strings chronologically
- Add `Strings.truncate` function
- Truncate labels at 10 chars if they are not date strings
- Upgrade to node v12 on travis to prevent inconsistent sorting behavious between local & travis (see nodejs/node#24294)

### Testing Requirements
- [x] Dashboard
  - Check dashboard landing page statistics make sense and are formatted right
  - Check order of months is chronological on all charts

### Release Requirements
- Not yet.

### Manual Deployment
- N/A
dxg added a commit to dresende/node-orm2 that referenced this issue Nov 4, 2019
Had to fix a sorting test - see nodejs/node#24294 for details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
v11.x
  
Closed issues
8 participants
You can’t perform that action at this time.