1242043 - {Array,%TypedArray%}.prototype.{i,lastI}ndexOf should never return -0

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jason Orendorff [:jorendorff]]

Section 5.2:

"Mathematical operations such as addition, subtraction, negation, multiplication, division, and the mathematical functions defined later in this clause should always be understood as computing exact mathematical results on mathematical real numbers, which unless otherwise noted do not include infinities and do not include a negative zero that is distinguished from positive zero."

"The mathematical function min(x1, x2, ..., xN) produces the mathematically smallest of x1 through xN."

22.1.3.15: https://tc39.github.io/ecma262/#sec-array.prototype.lastindexof

"If n ≥ 0, let k be min(n, len - 1)."

At this point, even if n is -0, k is set to 0. Our implementation could be fixed by adding

    if (k === 0)
        k = 0;

at the right point.

Pretty awful.

Comment 1

[Jeff Walden [:Waldo]]

Simple testcase:

  assertEq([0].lastIndexOf(0, -0), +0);

Comment 2

[Jeff Walden [:Waldo]]

Attachment: {Array,%TypedArray%}.prototype.{i,lastI}ndexOf should never return -0

And also

js> assertEq([0].indexOf(0, -0), +0);
typein:2:1 Error: Assertion failed: got -0, expected 0
Stack:
  @typein:2:1

...except, not.  That algorithm simply says, "If n >= 0, then let k be n."  That doesn't pass the number through a mathematical operation, so it never converts to +0.  Spec bug?  Seems like the two functions should be consistent in never returning -0.  And indeed, bterlson notes https://github.com/tc39/ecma262/pull/316 just now in #jslang.

Here's a patch making the methods consistent.  We could avoid adding 0 until very slightly later, for some of this stuff, but I think it makes things slightly less scrutable overall.

Comment 3

[Jason Orendorff [:jorendorff]]

Comment on attachment 8711256 [details] [diff] [review]
{Array,%TypedArray%}.prototype.{i,lastI}ndexOf should never return -0

Review of attachment 8711256 [details] [diff] [review]:
-----------------------------------------------------------------

All right!

Comment 4

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/aa58a439e77a

Comment 5

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/aa58a439e77a

Comment 6

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/aa58a439e77a

Comment 7

[Florian Scholz (Open Web Docs)]

Mentioned in Firefox 47 for devs:
https://developer.mozilla.org/en-US/Firefox/Releases/47#JavaScript

Small compat note on the reference pages:
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/indexOf
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/lastIndexOf
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/TypedArray/indexOf
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/TypedArray/lastIndexOf