Test case: delete Promise.resolve; Promise.all({ [Symbol.iterator]() { return this; }, return() { console.log("iterator closed"); }, }); Expected: Promise rejected with TypeError, "iterator closed" logged Actual: Promise rejected with TypeError, no logs ECMA262: https://tc39.es/ecma262/#sec-promise.all (step 6.a) Test262: https://test262.report/browse/built-ins/Promise/all/invoke-resolve-get-error-close.js
Created attachment 393622 [details] Patch
Created attachment 393648 [details] Patch Inline @getIteratorAndPromiseResolve and drop @isObject check.
Comment on attachment 393648 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=393648&action=review > Source/JavaScriptCore/builtins/PromiseConstructor.js:64 > + var promiseResolve = this.resolve; > + if (typeof promiseResolve !== "function") > + @throwTypeError("Promise.resolve is not a function"); I wish we could stick the try-catch for iterator.return under this `if` to avoid the rethrow, but I guess a this.resolve getter might throw in an arbitrary way... > Source/JavaScriptCore/builtins/PromiseConstructor.js:67 > + iterator.return(); Shouldn't we check whether iterator.return is undefined (per step 5)? > Source/JavaScriptCore/builtins/PromiseConstructor.js:81 > + var wrapper = {}; > + wrapper.@@iterator = function() { return iterator; }; > + > + for (var value of wrapper) { I wonder if this is really better than operating the iterator manually in a while loop?
Imo, the spec is doing the wrong thing here... If I were a web-dev asked to polyfill this I would do what JSC does today. I think it makes more sense for the spec to do the intuitive thing unless we have a compelling reason otherwise. Also, FWIW, looking at test262 report at one point every engine failed this test... https://test262.report/browse/built-ins/Promise/all/invoke-resolve-get-error-close.js?date=2019-07-06.
Comment on attachment 393648 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=393648&action=review >> Source/JavaScriptCore/builtins/PromiseConstructor.js:81 >> + for (var value of wrapper) { > > I wonder if this is really better than operating the iterator manually in a while loop? It's definitely better once we start optimizing the builtin iterators as the front end knows what for-of does in a way it can't for manual iteration. Also getting the details of iterating right is a PITA with a while loop. Trust me... 🤮
(In reply to Ross Kirsling from comment #3) > > Source/JavaScriptCore/builtins/PromiseConstructor.js:67 > > + iterator.return(); > > Shouldn't we check whether iterator.return is undefined (per step 5)? If `iterator.return` is null or undefined, calling it will throw a TypeError that will be ignored due to empty `catch` clause. This patch would not bring 100% spec compliance though: `this.resolve` should be looked up before `iterator.next` (step 6 of https://tc39.es/ecma262/#sec-getiterator), not after. I hope that spec change (https://github.com/tc39/ecma262/pull/1912) will be approved during next TC-39 meeting. Great job on the fix, Keith!
(In reply to Alexey Shvayka from comment #6) > I hope that spec change (https://github.com/tc39/ecma262/pull/1912) will be > approved during next TC-39 meeting. Great job on the fix, Keith! The change has gained consensus and is already implemented in SpiderMonkey. r262544 synced updated test262 coverage.
(In reply to Alexey Shvayka from comment #7) > (In reply to Alexey Shvayka from comment #6) > > I hope that spec change (https://github.com/tc39/ecma262/pull/1912) will be > > approved during next TC-39 meeting. Great job on the fix, Keith! > > The change has gained consensus and is already implemented in SpiderMonkey. > r262544 synced updated test262 coverage. Oh you changed test262 for me, thanks a bunch! I was about to look into it now that TC-39 is done.