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.

Sign up for GitHub

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

Jump to bottom

Incorrect tests after change to evaluation order for member assignment #3407

Open
bakkot opened this issue Feb 10, 2022 · 4 comments
Open

Incorrect tests after change to evaluation order for member assignment #3407

bakkot opened this issue Feb 10, 2022 · 4 comments

Comments

@bakkot
Copy link
Contributor

bakkot commented Feb 10, 2022 •
edited

tc39/ecma262#2267 changed the sequencing of the RequireObjectCoercible-on-base / ToString-on-property for member assignments, like a[b] = c. Some of the tests for the old behavior were updated, and new tests were added, in #2993. But that PR seems to have only covered plain =-style assignment, and there's still some tests which assert the previous behavior for compound assignment and increment/decrement. By reading these should all have been updated as well.

Specifically, the RequireObjectCoercible happens only during the eventual GetValue on the LHS, which happens in, for example,

By that point the ToString has already happened,

In fact, as I read the spec, null[{ toString: () => { console.log('hit') } }] is now supposed to print hit before throwing (by exactly the same logic, except that the GetValue call happens in the Evaluation semantics for ExpressionStatement, though of engines I have on hand only ChakraCore implements that behavior.

I've also opened an issue on ecma262 to confirm my reading: tc39/ecma262#2659

Tests which seem to me to be in error include;
@rwaldron
Copy link
Contributor

rwaldron commented Mar 1, 2022

cc @Ms2ger @romulocintra @sarahghp @ryzokuken @ptomato

Is this something any of you want to tackle?

@h2oche h2oche mentioned this issue Mar 14, 2022
Open
1 task
@h2oche
Copy link

h2oche commented Mar 20, 2022

We can also observe this behavior with our implementation ESMeta, which strictly follows the spec. Currently, spec and tests are diverged.

Additionally, following tests also seems to be related to this issue by:

@woess
Copy link
Contributor

woess commented May 23, 2022 •
edited

Related to this issue, I believe the following tests are also incorrect according to the current spec text:

Namely, the test expects that at L49: base[prop] = expr();, a DummyError is thrown from the RHS, but my reading of the spec is that ToPropertyKey(prop) in EvaluatePropertyAccessWithExpressionKey will be evaluated first and already throw.

@mstoykov mstoykov mentioned this issue Jul 27, 2022
Closed
This was referenced May 5, 2023
Closed
Closed
Closed
@woess
Copy link
Contributor

woess commented Apr 23, 2024 •
edited

I suppose tc39/ecma262#3307 fixes this issue as well, but needs to be confirmed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
4 participants
@rwaldron @bakkot @h2oche @woess