The implementation of Array.from fails when passed a Set.
Created attachment 251423 [details] Patch
Comment on attachment 251423 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=251423&action=review Cool! I have some comments, but this looks good to me! > Source/JavaScriptCore/builtins/ArrayConstructor.js:66 > + // Since for-of loop once more looks up the @@iterator property of a given iterable, > + // it could be observable if the user defines a getter for @@iterator. > + // To avoid this situation, we define a wrapper object that @@iterator just returns a given iterator. > + var wrapper = { > + [@symbolIterator]() { > + return iterator; > + } > + }; > + > + for (var value of wrapper) { Could you drop the wrapper and use the iterator instance in the loop? for (var value of iterator) { ... } > Source/JavaScriptCore/tests/stress/array-from-with-iterable.js:29 > + shouldBe(set.has(array[i][0]), true); This is testing the Map case, so this line should be map.has(...)!
Comment on attachment 251423 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=251423&action=review Thank you for your comment! >> Source/JavaScriptCore/builtins/ArrayConstructor.js:66 >> + for (var value of wrapper) { > > Could you drop the wrapper and use the iterator instance in the loop? > > for (var value of iterator) { ... } We cannot do that because when executing `for (var value of iterator) { ... }`, `iterator[Symbol.itrator]()` is called (It's observable effect). However, this behavior is not defined in the spec... I tested it in array-from-with-iterator.js. >> Source/JavaScriptCore/tests/stress/array-from-with-iterable.js:29 >> + shouldBe(set.has(array[i][0]), true); > > This is testing the Map case, so this line should be map.has(...)! Oops! Thank you. I'll fix it soon.
(In reply to comment #3) > Comment on attachment 251423 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=251423&action=review > > Thank you for your comment! > > >> Source/JavaScriptCore/builtins/ArrayConstructor.js:66 > >> + for (var value of wrapper) { > > > > Could you drop the wrapper and use the iterator instance in the loop? > > > > for (var value of iterator) { ... } > > We cannot do that because when executing `for (var value of iterator) { ... > }`, `iterator[Symbol.itrator]()` is called (It's observable effect). > However, this behavior is not defined in the spec... > I tested it in array-from-with-iterator.js. Ahh, excellent point!
Comment on attachment 251423 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=251423&action=review >>>> Source/JavaScriptCore/builtins/ArrayConstructor.js:66 >>>> + for (var value of wrapper) { >>> >>> Could you drop the wrapper and use the iterator instance in the loop? >>> >>> for (var value of iterator) { ... } >> >> We cannot do that because when executing `for (var value of iterator) { ... }`, `iterator[Symbol.itrator]()` is called (It's observable effect). >> However, this behavior is not defined in the spec... >> I tested it in array-from-with-iterator.js. > > Ahh, excellent point! Or is it better to write like the following? :) while (true) { var next = iterator.next(); if (!@isObject(next)) throw @TypeError("..."); if (next.done) { result.length = k; return result; } var nextValue = next.value; if (mapFn) { try { @putByValDirect(result, k, thisArg === undefined ? mapFn(value, k) : mapFn.@call(thisArg, value, k)); } catch (error) { var returnMethod = iterator.return; if (returnMethod != null) { try { returnMethod.@call(iterator); } catch (watedError) { } } throw error; } } else @putByValDirect(result, k, nextValue); k += 1 } If so, I'll rewrite it with this. Maybe, builtins doesn't allow to use try-catch now, so I'll relax it when it's needed.
Created attachment 251465 [details] Patch Fixed the tests for Map
> Or is it better to write like the following? :) > > [...snip...] I think what you have with the wrapper is good. It keeps the iterator implementation is in one place (C++) instead of two (C++ and JavaScript).
(In reply to comment #7) > > Or is it better to write like the following? :) > > > > [...snip...] > > I think what you have with the wrapper is good. It keeps the iterator > implementation is in one place (C++) instead of two (C++ and JavaScript). Make sense :)
Comment on attachment 251465 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=251465&action=review > Source/JavaScriptCore/builtins/ArrayConstructor.js:49 > + throw new @TypeError("Array.from requires that the @@iterator property of the first argument, when exists, be a function"); Should @@iterator be used in the text of an exception? I’m not sure that’s terminology that we can expect JavaScript programmers to understand. > Source/JavaScriptCore/builtins/ArrayConstructor.js:91 > + var len = lengthValue > 0 ? (lengthValue < maxSafeInteger ? lengthValue : maxSafeInteger) : 0; We normally like to avoid abbreviations like "len" in WebKit code. Not sure how the JavaScriptCore folks feel about this.
Comment on attachment 251465 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=251465&action=review Thank you for your review. >> Source/JavaScriptCore/builtins/ArrayConstructor.js:49 >> + throw new @TypeError("Array.from requires that the @@iterator property of the first argument, when exists, be a function"); > > Should @@iterator be used in the text of an exception? I’m not sure that’s terminology that we can expect JavaScript programmers to understand. I've changed it to "Array.from requires that the property of the first argument, items[Symbol.iterator], when exists, be a function" >> Source/JavaScriptCore/builtins/ArrayConstructor.js:91 >> + var len = lengthValue > 0 ? (lengthValue < maxSafeInteger ? lengthValue : maxSafeInteger) : 0; > > We normally like to avoid abbreviations like "len" in WebKit code. Not sure how the JavaScriptCore folks feel about this. This `len` is derived from the spec[1]. Howeevr, "len" seems not good. I'll change it to arrayLikeLength. [1]: http://people.mozilla.org/~jorendorff/es6-draft.html#sec-array.from
Committed r183357: <http://trac.webkit.org/changeset/183357>