Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Normative: Suppress GetMethod errors in IteratorClose #1408

Merged
merged 1 commit into from
Apr 30, 2020

Conversation

zenparsing
Copy link
Member

@zenparsing zenparsing commented Jan 14, 2019

Fixes #1398

Current behavior:

If the "return" property of an iterator is not callable, a TypeError will be thrown from IteratorClose, regardless of the completion provided as an argument.

Proposed behavior:

If a "throw" completion is provided to IteratorClose, then it will rethrow that error even if the return property is not callable.

Test case:

let iter = {
  [Symbol.iterator]() { return this; },
  next() { return { value: 1, done: false }; },
  return: 0, // Not a function
};
try {
  for (let i of iter)
    throw 1; // Throw and trigger IteratorClose
} catch (e) {
  // Does this print "1" or "TypeError: 0 is not a function"?
  print(e);
}

Current engine behavior:

eshost -e '(() => { try { for (let i of { [Symbol.iterator]() { return this }, next() { return { value: 1, done: false }; }, return: 0 }) throw 1; } catch (e) { print(e) } })()'

#### ch
1

#### jsc
1

#### v8
TypeError: The iterator's 'return' method is not callable

#### sm
TypeError: property 'return' of iterator is not callable

#### xs
1

@zenparsing zenparsing added normative change Affects behavior required to correctly evaluate some ECMAScript source text needs consensus This needs committee consensus before it can be eligible to be merged. labels Jan 14, 2019
@ljharb ljharb added the needs test262 tests The proposal should specify how to test an implementation. Ideally via github.com/tc39/test262 label Jan 14, 2019
@ljharb ljharb requested review from bterlson and a team January 14, 2019 19:12
@littledan
Copy link
Member

littledan commented Jan 28, 2019

cc @LeszekSwirski @kmiller68

@ljharb ljharb removed the request for review from bterlson February 28, 2019 19:53
@ljharb ljharb added has consensus This has committee consensus. and removed needs consensus This needs committee consensus before it can be eligible to be merged. labels Mar 26, 2019
@shvaikalesh
Copy link
Member

Tests PR: tc39/test262#2565

spec.html Outdated Show resolved Hide resolved
@ljharb ljharb added has test262 tests editor call to be discussed in the next editor call and removed needs test262 tests The proposal should specify how to test an implementation. Ideally via github.com/tc39/test262 labels Apr 6, 2020
@ljharb ljharb self-assigned this Apr 6, 2020
@ljharb ljharb force-pushed the iterator-close-get-method branch from f83865d to f8bfba2 Compare April 6, 2020 07:05
ljharb pushed a commit to zenparsing/ecma262 that referenced this pull request Apr 6, 2020
@ljharb ljharb removed the editor call to be discussed in the next editor call label Apr 8, 2020
spec.html Show resolved Hide resolved
spec.html Show resolved Hide resolved
Copy link
Contributor

@bakkot bakkot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM other than the typographical issue.

xeonchen pushed a commit to xeonchen/gecko that referenced this pull request Apr 15, 2020
…mpletions. r=arai

Implements the changes from: tc39/ecma262#1408

The spec PR requires to start the non-syntactic `try` block before retrieving
the "return" property and checking whether or not the "return" property is
callable. As part of this change we can also reorder the other byte code
instructions, which enables us to make the code more similar to normal JS code.
The equivalent JS code is documented in the added comments. Furthermore these
changes allow us to remove the manual stack depth fixups.

Depends on D70819

Differential Revision: https://phabricator.services.mozilla.com/D70820
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this pull request Apr 15, 2020
…mpletions. r=arai

Implements the changes from: tc39/ecma262#1408

The spec PR requires to start the non-syntactic `try` block before retrieving
the "return" property and checking whether or not the "return" property is
callable. As part of this change we can also reorder the other byte code
instructions, which enables us to make the code more similar to normal JS code.
The equivalent JS code is documented in the added comments. Furthermore these
changes allow us to remove the manual stack depth fixups.

Depends on D70819

Differential Revision: https://phabricator.services.mozilla.com/D70820

--HG--
extra : moz-landing-system : lando
Copy link
Contributor

@syg syg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm with convention nits fixed.

spec.html Show resolved Hide resolved
spec.html Show resolved Hide resolved
@ljharb ljharb force-pushed the iterator-close-get-method branch from f8bfba2 to 0b89915 Compare April 30, 2020 03:42
@ljharb ljharb merged commit 0b89915 into tc39:master Apr 30, 2020
ryanhaddad pushed a commit to WebKit/WebKit that referenced this pull request Dec 22, 2020
https://bugs.webkit.org/show_bug.cgi?id=212378

Reviewed by Keith Miller.

JSTests:

* stress/custom-iterators.js:
* stress/iterator-return-abrupt-lookup-builtins.js: Added.
* test262/expectations.yaml: Mark 4 test cases as passing.

Source/JavaScriptCore:

This patch implements recent spec change [1] that prevents "return" method lookup error
from overriding outer exception, aligning JSC with V8 and SpiderMonkey.

It is accomplished by moving pushTry() before emitGetById() in BytecodeGenerator.cpp
(covered by test262 suite) and removal of RETURN_IF_EXCEPTION in IteratorOperations.cpp
(added a stress test).

Before this patch, JSC partly implemented the spec change [1] by suppressing TypeError
if "return" method of iterator was not callable.

BytecodeGenerator::emitDelegateYield() is intentionally left unchanged.
Also, this patch utilizes emitIteratorGenericClose() to avoid code duplication.
for/of microbenchmarks are neutral.

[1]: tc39/ecma262#1408

* bytecompiler/BytecodeGenerator.cpp:
(JSC::BytecodeGenerator::emitGenericEnumeration):
(JSC::BytecodeGenerator::emitEnumeration):
* runtime/IteratorOperations.cpp:
(JSC::iteratorClose):


Canonical link: https://commits.webkit.org/225225@main
git-svn-id: https://svn.webkit.org/repository/webkit/trunk@262165 268f45cc-cd09-0410-ab3c-d52691b4dbfc
buttercookie42 pushed a commit to buttercookie42/Waterfox-Classic that referenced this pull request Jun 2, 2022
…mpletions. r=arai

Implements the changes from: tc39/ecma262#1408

The spec PR requires to start the non-syntactic `try` block before retrieving
the "return" property and checking whether or not the "return" property is
callable. As part of this change we can also reorder the other byte code
instructions, which enables us to make the code more similar to normal JS code.
The equivalent JS code is documented in the added comments. Furthermore these
changes allow us to remove the manual stack depth fixups.

Depends on D70819

Differential Revision: https://phabricator.services.mozilla.com/D70820
buttercookie42 pushed a commit to buttercookie42/Waterfox-Classic that referenced this pull request Jun 2, 2022
…mpletions. r=arai

Implements the changes from: tc39/ecma262#1408

The spec PR requires to start the non-syntactic `try` block before retrieving
the "return" property and checking whether or not the "return" property is
callable. As part of this change we can also reorder the other byte code
instructions, which enables us to make the code more similar to normal JS code.
The equivalent JS code is documented in the added comments. Furthermore these
changes allow us to remove the manual stack depth fixups.

Depends on D70819

Differential Revision: https://phabricator.services.mozilla.com/D70820
buttercookie42 pushed a commit to buttercookie42/Waterfox-Classic that referenced this pull request Jun 3, 2022
…mpletions. r=arai

Implements the changes from: tc39/ecma262#1408

The spec PR requires to start the non-syntactic `try` block before retrieving
the "return" property and checking whether or not the "return" property is
callable. As part of this change we can also reorder the other byte code
instructions, which enables us to make the code more similar to normal JS code.
The equivalent JS code is documented in the added comments. Furthermore these
changes allow us to remove the manual stack depth fixups.

Depends on D70819

Differential Revision: https://phabricator.services.mozilla.com/D70820
buttercookie42 pushed a commit to buttercookie42/Waterfox-Classic that referenced this pull request Jun 13, 2022
…mpletions. r=arai

Implements the changes from: tc39/ecma262#1408

The spec PR requires to start the non-syntactic `try` block before retrieving
the "return" property and checking whether or not the "return" property is
callable. As part of this change we can also reorder the other byte code
instructions, which enables us to make the code more similar to normal JS code.
The equivalent JS code is documented in the added comments. Furthermore these
changes allow us to remove the manual stack depth fixups.

Depends on D70819

Differential Revision: https://phabricator.services.mozilla.com/D70820
buttercookie42 pushed a commit to buttercookie42/Waterfox-Classic that referenced this pull request Jun 17, 2022
…mpletions. r=arai

Implements the changes from: tc39/ecma262#1408

The spec PR requires to start the non-syntactic `try` block before retrieving
the "return" property and checking whether or not the "return" property is
callable. As part of this change we can also reorder the other byte code
instructions, which enables us to make the code more similar to normal JS code.
The equivalent JS code is documented in the added comments. Furthermore these
changes allow us to remove the manual stack depth fixups.

Depends on D70819

Differential Revision: https://phabricator.services.mozilla.com/D70820
buttercookie42 pushed a commit to buttercookie42/Waterfox-Classic that referenced this pull request Jun 17, 2022
…mpletions. r=arai

Implements the changes from: tc39/ecma262#1408

The spec PR requires to start the non-syntactic `try` block before retrieving
the "return" property and checking whether or not the "return" property is
callable. As part of this change we can also reorder the other byte code
instructions, which enables us to make the code more similar to normal JS code.
The equivalent JS code is documented in the added comments. Furthermore these
changes allow us to remove the manual stack depth fixups.

Depends on D70819

Differential Revision: https://phabricator.services.mozilla.com/D70820
buttercookie42 pushed a commit to buttercookie42/Waterfox-Classic that referenced this pull request Jun 17, 2022
…mpletions. r=arai

Implements the changes from: tc39/ecma262#1408

The spec PR requires to start the non-syntactic `try` block before retrieving
the "return" property and checking whether or not the "return" property is
callable. As part of this change we can also reorder the other byte code
instructions, which enables us to make the code more similar to normal JS code.
The equivalent JS code is documented in the added comments. Furthermore these
changes allow us to remove the manual stack depth fixups.

Depends on D70819

Differential Revision: https://phabricator.services.mozilla.com/D70820
buttercookie42 pushed a commit to buttercookie42/Waterfox-Classic that referenced this pull request Jun 21, 2022
…mpletions. r=arai

Implements the changes from: tc39/ecma262#1408

The spec PR requires to start the non-syntactic `try` block before retrieving
the "return" property and checking whether or not the "return" property is
callable. As part of this change we can also reorder the other byte code
instructions, which enables us to make the code more similar to normal JS code.
The equivalent JS code is documented in the added comments. Furthermore these
changes allow us to remove the manual stack depth fixups.

Depends on D70819

Differential Revision: https://phabricator.services.mozilla.com/D70820
buttercookie42 pushed a commit to buttercookie42/Waterfox-Classic that referenced this pull request Jun 23, 2022
…mpletions. r=arai

Implements the changes from: tc39/ecma262#1408

The spec PR requires to start the non-syntactic `try` block before retrieving
the "return" property and checking whether or not the "return" property is
callable. As part of this change we can also reorder the other byte code
instructions, which enables us to make the code more similar to normal JS code.
The equivalent JS code is documented in the added comments. Furthermore these
changes allow us to remove the manual stack depth fixups.

Depends on D70819

Differential Revision: https://phabricator.services.mozilla.com/D70820
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
has consensus This has committee consensus. has test262 tests normative change Affects behavior required to correctly evaluate some ECMAScript source text
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Suppress GetMethod(iterator.return) errors in IteratorClose
6 participants