paulmillr / es6-shim

ECMAScript 6 compatibility shims for legacy JS engines
http://paulmillr.com
MIT License
3.11k stars 387 forks source link

Wrappers for methods of Array.prototype broken #341

Closed zloirock closed 9 years ago

zloirock commented 9 years ago

See https://github.com/kangax/compat-table/issues/531#issuecomment-100466634

[].map.call({length: -1}, isNaN);  // => undefined, should be []
[].some.call({length: -1}, isNaN); // => undefined, should be false

it can be simple fixed, but not so simple

var c = 0;
[].map.call({get length(){c++;return 0}}, isNaN);
c; // => 2, should be 1 

var O = {length: Math.pow(2, 32) + 1};
O[Math.pow(2, 32)] = 42;
[].forEach.call(O, console.log, console); // => nothing, should be 42, 4294967296, O
ljharb commented 9 years ago

Repeating my comment here:

For the first one, there's simply no solution for array-like objects with getters to avoid calling it twice besides, as you said, redefining the entire method. I suspect that's not worth it for the obscure tiny edge case of an array-like object with a getter for the length.

For the second one, it goes through https://people.mozilla.org/~jorendorff/es6-draft.html#sec-tolength to https://people.mozilla.org/~jorendorff/es6-draft.html#sec-arraysetlength to https://people.mozilla.org/~jorendorff/es6-draft.html#sec-touint32 - which means Math.pow(2, 32) + 1 becomes 1 and so your Math.pow(2, 32) key should never be visited - so I think this isn't actually a bug.

ljharb commented 9 years ago

If we can shim the ES5 array methods in the es6-shim without taking a performance hit, that's not a bad option, but that also means we'll be repeating es5-shim code here. If we ever move to a build model where all the shims are npm-installed, then the es5-shim's methods could be pulled in with virtually no impact, but until then, it'd be a lot of extra weight.

zloirock commented 9 years ago

About second - nope, by spec there is should not be toUint32 conversion, only toLength - it's array-like object, not array.

About first - not so many cases requires negative array length too :)

ljharb commented 9 years ago

Ah, good call.

It seems like fixing the "overflowed 32 bit length" issue though would require the same shim as the first problem.

So, I'm going to leave this open, but these particular bugs will have to wait until we can require in the es5-shim's implementations.

The first issues I'll definitely fix ASAP.