Skip to content
New issue

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

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

Already on GitHub? # to your account

Defer evaluation order of computed properties #238

Merged

Conversation

bnoordhuis
Copy link
Contributor

The evaluation order is observable. Align with what test262 expects.

@bnoordhuis
Copy link
Contributor Author

Hm, that failing test262/test/language/expressions/assignment/S11.13.1_A7_T3.js test seems to be almost 100% at odds with the two fixed tests... guess I'll have to think about this a bit more.

@bnoordhuis
Copy link
Contributor Author

For posterity, the fixed tests both have this in their description (and is what this pull request currently implements):

Assignment Operator evaluates the value prior validating a MemberExpression's reference

Whereas the now failing test says this:

The left-hand side expression is evaluated before the right-hand side.

Looking at the three tests, the expected evaluation order in o[k] = f() seems to be:

  1. f(), o, k when o is null or undefined, but

  2. k, o, f() when o is an object. Seriously.

@bnoordhuis bnoordhuis force-pushed the defer-eval-order-computed-props branch from 246586e to b6b42e8 Compare December 29, 2023 10:32
@bnoordhuis
Copy link
Contributor Author

I think I have it working although it's not very pretty or efficient... PTAL.

The evaluation order is observable. Align with what test262 expects.
@bnoordhuis bnoordhuis force-pushed the defer-eval-order-computed-props branch from b6b42e8 to 6986963 Compare December 30, 2023 09:50
@bnoordhuis
Copy link
Contributor Author

@saghul your thoughts on this approach por favor?

@saghul saghul merged commit e995085 into quickjs-ng:master Jan 16, 2024
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants