1035122 - Polyfill of Array.prototype.forEach incorrect

Status: RESOLVED INVALID

(Developer Documentation Graveyard :: JavaScript, defect, P5)

Description

[christian bodart]

:: Developer Documentation Request

      Request Type: Correction
     Gecko Version: unspecified
 Technical Contact: 

:: Details

Since the array can be modified during for each the 'cacheing' of the length value in the method would result in different behaviour since the function would be called for more or less elements

Comment 1

[Florian Scholz (Open Web Docs)]

Rick, can you have a look the Array.prototype.forEach polyfill on MDN? 

You are maintaining the others quite well, too :-)

Comment 2

[Rick Waldron [:rwaldron]]

Will do

Comment 3

[Rick Waldron [:rwaldron]]

This report is invalid. As the ES5 spec states: 

15.4.4.18

  The range of elements processed by forEach is set before the first call to callbackfn. Elements which are appended to the array after the call to forEach begins will not be visited by callbackfn. If existing elements of the array are changed, their value as passed to callback will be the value at the time forEach visits them; elements that are deleted after the call to forEach begins and before being visited are not visited.


If more elements are added, they are ignored. If elements are removed, they are skipped by steps 7.B and 7.C

Comment 4

[christian bodart]

 elements that are deleted after the call to forEach begins and before being visited are not visited.
However in the current polyfill they will be visited (the value passed will be undefined)...

Comment 5

[Rick Waldron [:rwaldron]]

Attachment: foreach-visit.js

Comment 6

[Rick Waldron [:rwaldron]]

Please see the attached test case file that illustrates the correct behaviour of the polyfill.

Comment 7

[christian bodart]

Thanks gonna write some tests so I'm happy

Comment 8

[christian bodart]

Attachment: array.forEach.spec.js

Comment 9

[christian bodart]

Okay your interpretation of the spec seems to be way better than mine (funnily enough the poly fill I wrote behaves the same way as you and the browsers (but includes string support for IE) - it was only when I checked the spec (which I clearly misinterpreted I hopped to check if yours was better!)

IE6/7/ and 8 (the browsers we need the polyfill for) failures listed below - also in other browsers the first two failures occur but this may be Jasmine...

PLEASE NOTE: Jasmine does not support IE6/7 (think 8 is ok) I run with a sloppily patched version for them (basically it uses querySelector and createTextNode which aren't supported - oh and the CSS sucks!)

Anyway here are the failures

24 specs, 5 failures

Spec List | Failures
Spec List | Failures
Array.prototype.forEach tests throws an error with null
Expected function to throw an exception.
Array.prototype.forEach tests throws an error with undefined
Expected function to throw an exception.
Array.prototype.forEach tests it works on literal strings
Expected 0 to be 6.
Array.prototype.forEach tests works on String objects with length and members
Expected 0 to be 3.
Array.prototype.forEach tests visits explicitly set to undefined elements
Expected 0 to equal 1.

Comment 10

[christian bodart]

you can fix the last failure (which is the only important using the 'in' solution I suggested when the index is undefined)

Comment 11

[Rick Waldron [:rwaldron]]

>  Array.prototype.forEach tests throws an error with null
>  Expected function to throw an exception.
  
>  Array.prototype.forEach tests throws an error with undefined
>  Expected function to throw an exception.

Not sure how to reasonably cover these two without strict mode, which isn't implemented in IE 6, 7, 8 or 9. See: http://es5.github.io/#x15.3.4.4 specifically, read the NOTE section were null and undefined are automatically replaced by the global object. This means that: 

  Array.prototype.forEach.call(null, function() { console.log(this); });

That will log the `window` object in those browsers. 


> Array.prototype.forEach tests it works on literal strings
> Expected 0 to be 6.

> Array.prototype.forEach tests works on String objects with length and members
> Expected 0 to be 3.


The "generic" semantics that Array.prototype.forEach has are defined for an ES5 compliant environment, which IE6, 7 and 8 are not. The reason this can't be supported is because bracket notation "property" access doesn't work on strings in those browsers. Try the following: 

  var foo = "foo";

  alert(foo);
  alert(foo[0]);
  alert(foo.charAt(0));

The results will be: 

  "foo"
  undefined
  "f"



The last test is not fixable, that caused by a bug in IE6, 7, 8 and would require omitting the `k in O` check altogether and treating the array as "always dense" instead of "possibly sparse", which breaks the polyfill. Holes must be skipped, per spec. 

Further reading: https://github.com/es-shims/es5-shim/issues/190 https://github.com/es-shims/es5-shim/pull/131 These specifically: https://github.com/es-shims/es5-shim/issues/190#issuecomment-34931058 https://github.com/es-shims/es5-shim/issues/190#issuecomment-34933840

Comment 12

[christian bodart]

Yeah I just ran the tests against my code and it failed too :(


Here's my code (bit simpler than yours but passes all the tests except the undefined one (which I thought it did!) - could do while loops but scoping the variables to the for seems to be faster (guess thats an engine/interpreter thing) 

 Array.prototype.forEach = function(callback, thisArg) {
      if (this == window) {
        throw new Error(".forEach called on null or undefined")
      }
      if (this.constructor == String || this instanceof String) {
        for (var i = 0,l=this.length||0; i != l; i++) {
              callback.call(thisArg, this.charAt(i), i, this);
        }
      } else {
        for (var i = 0,l=this.length||0; i != l; i++) { 
          if (i in this) {
              callback.call(thisArg, this[i], i, this);
          }
        }
      }
    }

Comment 13

[christian bodart]

Mind you I dont check the length is a number :( darn it and of course I suppose you could call it on the window object too so there's another oversight on my part)
Oh well
thanks for your time and I'll do unit tests first before I start misinterpreting the spec
:)