_.keys returns incorrect keys for arrays that have gaps

[wheresrhys]

Running the following code:
var myArray = [1, 2, 3];
delete myArray[0];
_.keys(myArray) == [0,1, 2] // true
_.keys(myArray) == [1, 2] // false

Shouldn't _.keys ignore undefined array values?

[michaelficarra]

Yes, it definitely should (for certain definitions of undefined). From your example, Object.prototype.hasOwnProperty.call(myArray,'0') is false and Object.keys(myArray) is ["1","2"]. However, I am not able to reproduce your observed behaviour, and judging by its source, I doubt that you are able to either.

[wheresrhys]

I was a bit lazy in writing out the example (does == perform a good array comparison?? probably not), but console.log( _.keys(myArray)) does give a value of [0,1,2], as keys works (for arrays) by checking the array length, which is always 1+[the array's greatest key], and then uses _.range, which assumes all the keys from 0 to myArray.length-1 exist.

[michaelficarra]

Oh, sorry, I completely missed line 501 last night. I must have been very tired. I agree, that looks like it could be a problem. I believe line 501 could just be removed to fix it.

edit: but I am still unable to reproduce your described behaviour... looking into it

[wheresrhys]

maybe try
myArray = [];
myArray[2] = "defined";
That's may have been what I tested on (there was a delay of a day or two between finding the bug and reporting it), although there shouldn't be any difference

[michaelficarra]

I see. Since I was using a browser with native Object.keys support, I was unable to reproduce the problem. Once I forced the alternative function to be loaded, I am able to duplicate your results with both of your examples. Looks like a bug to me.

[ghost]

Yep, the problem is that the Object.keys implementation for browsers without native support delegates to Underscore's range method for arrays. While this avoids a costly for...in loop and an additional hasOwnProperty check for each property, it unfortunately produces incorrect results for sparse arrays. As michaelficarra mentioned, removing line 501 (in Underscore 1.1.4) will fix the problem. Incidentally, this will also make the fallback implementation ECMAScript 5-compliant.

[wheresrhys]

How would this perform
.keys = nativeKeys || function(obj) {
var keys = [];
if (.isArray(obj)) {
for (var key in obj) if ([key>=0]) keys[keys.length] = key;
} else {
for (var key in obj) if (hasOwnProperty.call(obj, key)) keys[keys.length] = key;
}
return keys;
};

The >=0 returns true for 0, 1, "1", but false for "string"

[wheresrhys]

Or (bearing in mind that >=0 also returns true for "" (which is a valid object key I think))

_.keys = nativeKeys || function(obj) {
  var keys = [];
  var helper = _.isArray(obj) ? function(str) { return str>0 || str=== 0;} : hasOwnProperty; 
// may need to check for === "0" too if some browsers return a string rather than an integer as the array key
  for (var key in obj) if (helper.call(obj, key)) keys[keys.length] = key;
  return keys;
};

[michaelficarra]

@wheresrhys: the simple solution mentioned above works fine. Line 501 was an improper attempt at optimizing _.keys when called upon an array. That line just needs to be removed and arrays need to be treated like all other objects. An additional problem with the current _.keys (and your suggested fixes) is that it does not return properties/methods that are added to an array instance (myArray.methodName = function(){} should include "methodName" in its key list). The removal of line 501 will correct this behaviour.

[jashkenas]

Thanks for researching this, everyone. I've removed the offending false optimization at SHA: 2f369f8. It'll go out with the next release.