Array.prototype.reverse should call [[HasProperty]] on elements before [[Get]]

src/array.js

 // Copyright 2012 the V8 project authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 var $arrayConcat;
 var $arrayPush;
 var $arrayPop;
 var $arrayShift;
 var $arraySlice;
 var $arraySplice;
@@ skipping 549 matching lines @@
       high = j;
       while (keys[--high_counter] == j) { }
       low = j_complement;
     }
     if (j_complement >= i) {
       low = i;
       while (keys[++low_counter] == i) { }
       high = len - i - 1;
     }
 
-    var current_i = array[low];
-    if (!IS_UNDEFINED(current_i) || low in array) {

[adamk, 2015/07/16 22:08:13]

As discussed offline, I don't think SparseReverse needs to be changed at all,
since it's guaranteed that [[Get]] and [[HasProperty]] are both side-effect free
in here.

[Dan Ehrenberg, 2015/07/16 22:16:49]

Done.

-      var current_j = array[high];
-      if (!IS_UNDEFINED(current_j) || high in array) {
+    if (low in array) {
+      var current_i = array[low];
+      if (high in array) {
+        var current_j = array[high];
         array[low] = current_j;
         array[high] = current_i;
       } else {
         array[high] = current_i;
         delete array[low];
       }
     } else {
-      var current_j = array[high];
-      if (!IS_UNDEFINED(current_j) || high in array) {
+      if (high in array) {
+        var current_j = array[high];
         array[low] = current_j;
         delete array[high];
       }
     }
   }
 }
 
+function PackedArrayReverse(array, len) {
+  var j = len - 1;
+  for (var i = 0; i < j; i++, j--) {
+    var current_i = array[i];
+    var current_j = array[j];
+    array[i] = current_j;
+    array[j] = current_i;
+  }
+  return array;
+}
+
 
 function InnerArrayReverse(array, len) {
   var j = len - 1;
   for (var i = 0; i < j; i++, j--) {
-    var current_i = array[i];
-    if (!IS_UNDEFINED(current_i) || i in array) {
-      var current_j = array[j];
-      if (!IS_UNDEFINED(current_j) || j in array) {
+    if (i in array) {
+      var current_i = array[i];
+      if (j in array) {
+        var current_j = array[j];
         array[i] = current_j;
         array[j] = current_i;
       } else {
         array[j] = current_i;
         delete array[i];
       }
     } else {
-      var current_j = array[j];
-      if (!IS_UNDEFINED(current_j) || j in array) {
+      if (j in array) {
+        var current_j = array[j];
         array[i] = current_j;
         delete array[j];
       }
     }
   }
   return array;
 }
 
 
 function ArrayReverse() {
   CHECK_OBJECT_COERCIBLE(this, "Array.prototype.reverse");
 
   var array = TO_OBJECT_INLINE(this);
   var len = TO_UINT32(array.length);
 
   if (UseSparseVariant(array, len, IS_ARRAY(array), len)) {
     %NormalizeElements(array);
     SparseReverse(array, len);
     return array;
+  } else if (%_HasFastPackedElements(array)) {

[adamk, 2015/07/16 22:08:13]

This needs an IS_ARRAY check (you can cache this above). Please add a test
covering this. This object violates the contract PackedArrayReverse() is
expecting:

{ length: 2, 0: 'foo', __proto__: { get 1() { delete this[1] } } }

Arrays can't have this problem because of their exotic 'length'.

[Toon Verwaest, 2015/07/16 22:15:32]

Do we have objects with packed elements that are not arrays?

[Dan Ehrenberg, 2015/07/16 22:16:49]

Done.

+    return PackedArrayReverse(array, len);
+  } else {
+    return InnerArrayReverse(array, len);

[adamk, 2015/07/16 22:08:13]

how about "GenericArrayReverse" as the name for this instead?

[Dan Ehrenberg, 2015/07/16 22:16:49]

Done.

   }
-
-  return InnerArrayReverse(array, len);
 }
 
 
 function ObservedArrayShift(len) {
   var first = this[0];
 
   try {
     $observeBeginPerformSplice(this);
     SimpleMove(this, 0, 1, len, 0);
     this.length = len - 1;
@@ skipping 1042 matching lines @@
   to.ArrayToString = ArrayToString;
   to.InnerArrayEvery = InnerArrayEvery;
   to.InnerArrayFilter = InnerArrayFilter;
   to.InnerArrayForEach = InnerArrayForEach;
   to.InnerArrayIndexOf = InnerArrayIndexOf;
   to.InnerArrayJoin = InnerArrayJoin;
   to.InnerArrayLastIndexOf = InnerArrayLastIndexOf;
   to.InnerArrayMap = InnerArrayMap;
   to.InnerArrayReduce = InnerArrayReduce;
   to.InnerArrayReduceRight = InnerArrayReduceRight;
-  to.InnerArrayReverse = InnerArrayReverse;
   to.InnerArraySome = InnerArraySome;
   to.InnerArraySort = InnerArraySort;
   to.InnerArrayToLocaleString = InnerArrayToLocaleString;
+  to.PackedArrayReverse = PackedArrayReverse;
 });
 
 $arrayConcat = ArrayConcatJS;
 $arrayPush = ArrayPush;
 $arrayPop = ArrayPop;
 $arrayShift = ArrayShift;
 $arraySlice = ArraySlice;
 $arraySplice = ArraySplice;
 $arrayUnshift = ArrayUnshift;
 
 });