Bug 1155900 - Make frontend-triggered GetIterator perform ToObject before doing GetMethod(..., @@iterator), and add tests. (There's no behavioral change, except in terms of error messages, because the presence of bug 603201 causes this ToObject call to happen anyway -- but if that bug had been fixed before this one, these changes would have fixed a bug.) r=shu

--HG--
extra : rebase_source : aa1a9e88b6c45f1e8b0918a9620a788de261e9db

js/src/frontend/BytecodeEmitter.cpp

@@ -4973,14 +4973,22 @@ BytecodeEmitter::emitToObject()
 bool
 BytecodeEmitter::emitIterator()
 {
-    // Convert iterable to iterator.
-    if (!emit1(JSOP_DUP))                                 // OBJ OBJ
+    // Convert iterable to iterator, consistent with GetIterator.
+    //
+    // Note that what we call VAL, the spec calls |obj|.  The value isn't
+    // necessarily an object!  |GetMethod(obj, @@iterator)| implies a ToObject
+    // call during the effective |obj[@@iterator]()|, but that object-for-real
+    // is discarded after the method lookup.  So we call the iterator function
+    // with the originally-provided value.
+    if (!emit1(JSOP_DUP))                                 // VAL VAL
         return false;
-    if (!emit2(JSOP_SYMBOL, jsbytecode(JS::SymbolCode::iterator))) // OBJ OBJ @@ITERATOR
+    if (!emitToObject())                                  // VAL OBJ
         return false;
-    if (!emitElemOpBase(JSOP_CALLELEM))                   // OBJ ITERFN
+    if (!emit2(JSOP_SYMBOL, jsbytecode(JS::SymbolCode::iterator))) // VAL OBJ @@ITERATOR
         return false;
-    if (!emit1(JSOP_SWAP))                                // ITERFN OBJ
+    if (!emitElemOpBase(JSOP_CALLELEM))                   // VAL ITERFN
+        return false;
+    if (!emit1(JSOP_SWAP))                                // ITERFN VAL
         return false;
     if (!emitCall(JSOP_CALL, 0))                          // ITER
         return false;

js/src/jit-test/tests/basic/destructuring-toobject-before-iterator-lookup.js

@@ -0,0 +1,80 @@
+// Test that looking up a supposedly-iterable value's @@iterator property
+// first boxes up the value into an object via ToObject, then gets the
+// @@iterator using the object as receiver.  Cover most of the cases where the
+// frontend effects a GetIterator call.  (The remaining frontend cases are
+// handled in other tests in the revision that introduced this file.
+// Non-frontend cases aren't tested here.)
+"use strict";
+
+load(libdir + 'asserts.js');
+load(libdir + 'eqArrayHelper.js');
+
+Object.defineProperty(Boolean.prototype, Symbol.iterator,
+{
+  get() {
+    assertEq(typeof this, "object",
+             "GetMethod(obj, @@iterator) internally first performs ToObject");
+    assertEq(this !== null, true,
+             "this is really an object, not null");
+
+    function FakeBooleanIterator()
+    {
+      assertEq(typeof this, "boolean",
+               "iterator creation code must perform ToObject(false) before " +
+               "doing a lookup for @@iterator");
+
+      var count = 0;
+      return { next() {
+                 if (count++ > 0)
+                   throw new Error("unexpectedly called twice");
+                 return { done: true };
+               } };
+
+    }
+
+    return FakeBooleanIterator;
+  },
+  configurable: true
+});
+
+function destructuringAssignment()
+{
+  ([] = false);
+  ([,] = false);
+}
+for (var i = 0; i < 10; i++)
+  destructuringAssignment();
+
+function spreadElements()
+{
+  var arr1 = [...false];
+  assertEqArray(arr1, []);
+
+  var arr2 = [1, ...false];
+  assertEqArray(arr2, [1]);
+
+  var arr3 = [1, ...false, 2];
+  assertEqArray(arr3, [1, 2]);
+}
+for (var i = 0; i < 10; i++)
+  spreadElements();
+
+function spreadCall()
+{
+  var arr1 = new Array(...false);
+  assertEqArray(arr1, []);
+
+  var arr2 = new Array(0, ...false);
+  assertEqArray(arr2, []);
+
+  var arr3 = new Array(1, 2, ...false);
+  assertEqArray(arr3, [1, 2]);
+}
+for (var i = 0; i < 10; i++)
+  spreadCall();
+
+function destructuringArgument([])
+{
+}
+for (var i = 0; i < 10; i++)
+  destructuringArgument(false);

js/src/jit-test/tests/basic/expression-autopsy.js

@@ -111,27 +111,31 @@ check("o[- (o)]");
 // A few one off tests
 check_one("6", (function () { 6() }), " is not a function");
 check_one("0", (function () { Array.prototype.reverse.call('123'); }), " is read-only");
-check_one("(intermediate value)[Symbol.iterator](...).next(...).value",
-          function () { ieval("let (x) { var [a, b, [c0, c1]] = [x, x, x]; }") }, " is undefined");
 check_one("void 1", function() { (void 1)(); }, " is not a function");
 check_one("void o[1]", function() { var o = []; (void o[1])() }, " is not a function");
 
-// Manual testing for this case: the only way to trigger an error is *not* on
-// an attempted property access during destructuring, and the error message
-// invoking ToObject(null) is different: "can't convert {0} to object".
-try
+// Manual testing for a few isolated cases.  In these instances, the only way
+// to trigger an error is *not* on an attempted property access during
+// destructuring, but during a preceding ToObject() call on the value to be
+// iterated.  And the error message for failed ToObject(...) is different:
+// "can't convert {0} to object".
+function checkCantConvert(f, valstr)
 {
-  (function() {
-    var [{x}] = [null, {}];
-   })();
-  throw new Error("didn't throw");
-}
-catch (e)
-{
-  assertEq(e instanceof TypeError, true,
-           "expected TypeError, got " + e);
-  assertEq(e.message, "can't convert null to object");
+    try
+    {
+        f();
+        throw new Error("didn't throw");
+    }
+    catch (e)
+    {
+        assertEq(e instanceof TypeError, true,
+                 "expected TypeError, got " + e + " for function " + f);
+        assertEq(e.message, "can't convert " + valstr + " to object");
+    }
 }
 
+checkCantConvert(function() { var [{x}] = [null, {}]; }, "null");
+checkCantConvert(function () { ieval("let (x) { var [a, b, [c0, c1]] = [x, x, x]; }") }, "undefined");
+
 // Check fallback behavior
 assertThrowsInstanceOf(function () { for (let x of undefined) {} }, TypeError);

js/src/jit-test/tests/for-of/for-of-primitive-with-iterator-hooked.js

@@ -0,0 +1,32 @@
+// Test that looking up a supposedly-iterable value's @@iterator property
+// first boxes up the value into an object via ToObject, then gets the
+// @@iterator using the object as receiver.  Cover only the for-of case: other
+// cases are handled elsewhere in the revision that introduced this test.
+"use strict";
+
+load(libdir + 'asserts.js');
+load(libdir + 'iteration.js');
+
+Object.defineProperty(Boolean.prototype, Symbol.iterator,
+{
+  get() {
+    assertEq(typeof this, "object",
+             "GetMethod(obj, @@iterator) internally first performs ToObject");
+    assertEq(this !== null, true,
+             "this is really an object, not null");
+
+    function FakeBooleanIterator()
+    {
+      assertEq(typeof this, "boolean",
+               "iterator creation code must perform ToObject(false) before " +
+               "doing a lookup for @@iterator");
+      return { next() { return { done: true }; } };
+    }
+
+    return FakeBooleanIterator;
+  },
+  configurable: true
+});
+
+for (var i of false)
+  assertEq(true, false, "not reached");

js/src/tests/ecma_6/Generators/delegating-yield-getiterator-does-toobject.js

@@ -0,0 +1,58 @@
+// With yield*, the GetIterator call on the provided value looks up the
+// @@iterator property on ToObject(value), not on a possibly-primitive value.
+"use strict";
+
+Object.defineProperty(Boolean.prototype, Symbol.iterator,
+{
+  get() {
+    assertEq(typeof this, "object",
+             "GetMethod(obj, @@iterator) internally first performs ToObject");
+    assertEq(this !== null, true,
+             "this is really an object, not null");
+
+    function FakeBooleanIterator()
+    {
+      assertEq(typeof this, "boolean",
+               "iterator creation code must perform ToObject(false) before " +
+               "doing a lookup for @@iterator");
+
+      var count = 0;
+      return { next() {
+                 if (count++ > 0)
+                   throw new Error("unexpectedly called twice");
+                 return { done: true };
+               } };
+
+    }
+
+    return FakeBooleanIterator;
+  },
+  configurable: true
+});
+
+function* f()
+{
+  yield 1;
+  yield* false;
+  yield 2;
+}
+
+for (var i = 0; i < 10; i++)
+{
+  var gen = f();
+
+  var first = gen.next();
+  assertEq(first.done, false);
+  assertEq(first.value, 1);
+
+  var second = gen.next();
+  assertEq(second.done, false);
+  assertEq(second.value, 2);
+
+  var last = gen.next();
+  assertEq(last.done, true);
+  assertEq(last.value, undefined);
+}
+
+if (typeof reportCompare === "function")
+  reportCompare(true, true);

js/src/tests/js1_8_5/extensions/decompile-for-of.js

@@ -9,7 +9,7 @@ function check(code) {
         s = exc.message;
     }
 
-    assertEq(s, `x[Symbol.iterator] is not a function`);
+    assertEq(s, "(intermediate value)(...)[Symbol.iterator] is not a function");
 }
 
 x = {};