compat-table
compat-table copied to clipboard
kangax
Move the `Array.prototype.sort` `comparefn` test away from /es5/
https://kangax.github.io/compat-table/es5/#test-Array_methods_Array.prototype.sort:_compareFn_must_be_function_or_undefined
This test has nothing to do with ES5. It’s a formerly undefined behavior that only very recently (years after ES5 was published) got standardized. If anything, it’s an ES2018 feature.
Should this be moved from …/es5/ to another page (e.g. …/es2018/)?
It certainly is part of ES2018, technically - but it's been a defacto standard since ES5, which is why https://github.com/tc39/ecma262/pull/785 was able to be a PR, and not a proposal.
I think it should remain in ES5.
What makes you say that it’s been a de facto standard since ES5 as opposed to ES3/ES2/ES1?
Oh, and this repo has a compat table specifically for de facto standards: https://kangax.github.io/compat-table/non-standard/ This “feature” either belongs in …/non-standard/ or in …/es2018/. IMHO the latter makes the most sense at this point.
Perhaps it's been one for longer, but we don't have an older compat table :-)
It's worth noting that https://github.com/tc39/ecma262/pull/785 didn't actually standardize this behavior, but rather made it explicit - it was the original intention of the editor of ES5, it was implied by the ES5 text, and everyone implemented it that way - in other words, the PR didn't newly standardize it, it merely cleared up the spec text to reflect both the ES5 intention and the majority ES5 implementation reality. It's thoroughly an ES5 test.
everyone implemented it that way
That’s not true, as you know :) I don’t think it’s fair to blame an engine for not implementing something that was never part of a normative spec before.
lol fair, mostly everyone then :-p
A failing test isn't "blame" imo, and we shouldn't be making changes in the table so a browser can "make everything green".
In this case, it was always part of a normative spec - it was just easy to misinterpret. If an engine failed to interpret it correctly, failed to compare to other engines and ask why the interpretations were different, and also failed to ask TC39 for clarification, I'm not sure why their failing test should be swept under the rug.
A failing test isn't "blame" imo, and we shouldn't be making changes in the table so a browser can "make everything green".
That’s not what is happening. The issue has been fixed in V8 so it will be green either way.
In this case, it was always part of a normative spec
Where is this in ES5? If this was always a normative part of the spec, why did it take a normative change to clarify it?
It's a normative change because ES5 "currently vaguely implied but not explicitly required" that the TypeError be thrown.
In ES5, http://ecma-international.org/ecma-262/5.1/#sec-15.4.4.11:
- "If comparefn is not undefined and is not a consistent comparison function for the elements of this array (see below), the behaviour of sort is implementation-defined."
- Step 13/13a: "If the argument comparefn is not undefined, then If IsCallable(comparefn) is false, throw a TypeError exception."
These two are a contradiction, and https://github.com/tc39/ecma262/pull/785 solidified the latter, which was always the intention.
One can read the ES5 text as well to see that it talks about "consistent comparison functions", and says that comparison functions that are not consistent yield implementation-defined behavior, but it basically doesn't address non-function comparison functions, which is why it could be read to say that that behavior is implementation-defined. However, even from ES5, I'd have said that the explicit steps 13/13a should trump vague paragraphs of prose.
It's a normative change because ES5 "currently vaguely implied but not explicitly required" that the TypeError be thrown.
Exactly.
Per “If comparefn is not undefined and is not a consistent comparison function for the elements of this array (see below), the behaviour of sort is implementation-defined”, this test doesn’t belong in the …/es5/ compat table.
Because there's a contradiction, because of the explicit step 13, I'm claiming that it does.
Explicit algorithm steps should always trump prose.
It certainly is part of ES2018, technically - but it's been a defacto standard since ES5
@ljharb what other tests in compat-table are placed by their defacto location, rather than their "technical" spec location?
@paulirish so, again, this is a normative part of ES5 that was buggy in that it was contradictory; the recent change is to remove the contradiction. It's not just about "defacto", it's just also about that. There may not be any other similar examples in the ES5 table, but none the less, this is something ES5 mandates, and the fact that v8 interpreted it incorrectly isn't changed by the fact that the spec recently changed to reduce the chance of misinterpretation.
this is a normative part of ES5 that was buggy in that it was contradictory; the recent change is to remove the contradiction.
If you're asserting that this was normative in ES5, then I don't see why the spec change is necessary.
I'm not sure why their failing test should be swept under the rug.
@ljharb I don't see anyone doing this. I'm really happy that you personally took on this issue and addressed the contradiction in the spec. Thanks for doing that.
The spec change and the tests you added indeed incentivized V8 to fix the spec violation. That's how we all hope this process works, right?
The only thing that seems odd here is that the categorization of this test is effectively back-dated.
I don't see it as backdated; if this issue had been discovered years ago, it'd have been fixed faster, but the error still would have existed in v8 versions on the ES5 table.
I think it makes sense to include the test in ES2018; but I don't think it makes sense to exclude it from ES5.
Is there anyone not affiliated with Google/v8 who thinks this is odd? When only v8 advocates are suggesting that this is odd, it does indeed look like an attempt to make v8 "look better".
It seems strange to me that this specific test is marked as passing for Safari >= 10.1, but failing for Safari TP and Webkit. Is this correct? I don't have a Mac, so I can't verify this.
Disclosure: I'm kinda the guy who pushed this by asking in the recent Chrome AMA: https://www.reddit.com/r/webdev/comments/6zpgu1/were_the_chrome_team_here_to_answer_questions/dn1bn7l/?context=3
I'm leaning towards moving it to …/es2018/, per
It certainly is part of ES2018, technically - but it's been a defacto standard since ES5
it was the original intention of the editor of ES5, it was implied by the ES5 text, and everyone implemented it that way
Implying means there might be more than one valid state, right?
in other words, the PR didn't newly standardize it, it merely cleared up the spec text to reflect both the ES5 intention and the majority ES5 implementation reality.
Implicit now explicit with one specific, correct state.
Alternatively, as ChromeEngTeam stated on Reddit:
We use the test262 test suite as the gold standard for measuring compatibility with ECMAScript standard.
We could look at https://github.com/tc39/test262 how this specific issue was/is handled there?
@Schweinepriester "implying" means that it could be read to have more than one valid state; there was only ever one valid state. The ES2018 change ensures that nobody misreads it; it doesn't change that there was always only one valid state.
It would be useful info to determine how test262 handled it; but a) how a different project with different goals chose to handle it isn't necessarily relevant, and b) test262 doesn't tend to keep track of any form of ES except "latest", so it wouldn't really apply.
"implying" means that it could be read to have more than one valid state
I find it hard to read this and come to a different conclusion:
If
comparefnis notundefinedand is not a consistent comparison function for the elements of this array […] the behaviour ofsortis implementation-defined.
@mathiasbynens sure, but the explicit algorithm step contradicts that. Do you believe that it's reasonable, in the existence of directly contradictory steps, to a) just pick one, and b) pick the one that's implementation-defined, which is always something worth avoiding in an implementation?
I’m saying that if ES2018 is the spec that removes the contradiction, then that’s where this test belongs — not in the table for the spec that contradicts itself.
Sure, that's a defensible position to take - but one I don't agree with. ES2018 doesn't introduce a new requirement, it removes an ambiguity that engines failed to bring up back in 2009-2011 when implementing the spec, and that the spec editor and reviewers failed to notice when authoring it - the important thing is that this was the intent of ES5, so I think it belongs there.
We can certainly see what other collaborators think about it.
When I tested the behaviour nine months ago, 3 browsers out of 4 didn’t throw on [1].sort({}). The incriminated change of the spec, contrarly to what was stated in its commit message, did not reflect web reality.
(BTW, the current test on compat-table does not try arrays with less than two elements; that case should be added.)
The commit message is correct for arrays of 2 or more elements; it's only for 0 or 1 element that it's not reflective.
At any rate, what sorting should do with a two-argument comparator when there's less than two items isn't really the important part of the spec change, so my position on this test is unchanged.