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

Fix order between initializing x and this.x in strict mode #6438

Closed
wants to merge 1 commit into from

Conversation

hyjorc1
Copy link
Contributor

@hyjorc1 hyjorc1 commented Nov 13, 2022

6b1504a

Fix order between initializing x and this.x in strict mode
https://bugs.webkit.org/show_bug.cgi?id=247493
rdar://102064848

Reviewed by NOBODY (OOPS!).

Given following JavaScript program:
```
"use strict";
x = this.x = 0;
```

It should throw ReferenceError since

https://tc39.es/ecma262/2022/multipage/ecmascript-language-expressions.html#sec-assignment-operators-runtime-semantics-evaluation
```
AssignmentExpression : LeftHandSideExpression = AssignmentExpression
1. If LeftHandSideExpression is neither an ObjectLiteral nor an ArrayLiteral, then
    ...
    e. Perform ? PutValue(lref, rval).
```

and

https://tc39.es/ecma262/2022/multipage/ecmascript-data-types-and-values.html#sec-putvalue
```
4. If IsUnresolvableReference(V) is true, then
    a. If V.[[Strict]] is true, throw a ReferenceError exception
```

* JSTests/stress/put-to-scope-reference-error.js: Added.
(shouldThrow):
(shouldNotThrow):
* Source/JavaScriptCore/llint/LLIntSlowPaths.cpp:
(JSC::LLInt::LLINT_SLOW_PATH_DECL):

6b1504a

Misc iOS, tvOS & watchOS macOS Linux Windows
✅ 🧪 style ✅ 🛠 ios ✅ 🛠 mac ✅ 🛠 wpe ❌ 🛠 🧪 win
✅ 🛠 ios-sim ✅ 🛠 mac-debug ✅ 🛠 gtk ✅ 🛠 wincairo
✅ 🧪 webkitperl ❌ 🧪 ios-wk2 ✅ 🛠 mac-AS-debug ❌ 🧪 gtk-wk2
✅ 🧪 api-ios ✅ 🧪 api-mac ✅ 🧪 api-gtk
❌ 🛠 🧪 jsc ✅ 🛠 tv ❌ 🧪 mac-wk1 ✅ 🛠 jsc-armv7
✅ 🛠 tv-sim ❌ 🧪 mac-wk2 ❌ 🧪 jsc-armv7-tests
✅ 🛠 watch ❌ 🧪 mac-AS-debug-wk2 ✅ 🛠 jsc-mips
✅ 🛠 watch-sim ✅ 🧪 mac-wk2-stress ❌ 🧪 jsc-mips-tests

https://bugs.webkit.org/show_bug.cgi?id=247493
rdar://102064848

Reviewed by NOBODY (OOPS!).

Given following JavaScript program:
```
"use strict";
x = this.x = 0;
```

It should throw ReferenceError since

https://tc39.es/ecma262/2022/multipage/ecmascript-language-expressions.html#sec-assignment-operators-runtime-semantics-evaluation
```
AssignmentExpression : LeftHandSideExpression = AssignmentExpression
1. If LeftHandSideExpression is neither an ObjectLiteral nor an ArrayLiteral, then
    ...
    e. Perform ? PutValue(lref, rval).
```

and

https://tc39.es/ecma262/2022/multipage/ecmascript-data-types-and-values.html#sec-putvalue
```
4. If IsUnresolvableReference(V) is true, then
    a. If V.[[Strict]] is true, throw a ReferenceError exception
```

* JSTests/stress/put-to-scope-reference-error.js: Added.
(shouldThrow):
(shouldNotThrow):
* Source/JavaScriptCore/llint/LLIntSlowPaths.cpp:
(JSC::LLInt::LLINT_SLOW_PATH_DECL):
@hyjorc1 hyjorc1 self-assigned this Nov 13, 2022
@hyjorc1 hyjorc1 added JavaScriptCore For bugs in JavaScriptCore, the JS engine used by WebKit, other than kxmlcore issues. WebKit Local Build labels Nov 13, 2022
@hyjorc1 hyjorc1 marked this pull request as ready for review November 13, 2022 08:26
@hyjorc1 hyjorc1 requested a review from a team as a code owner November 13, 2022 08:26
@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Nov 13, 2022
Copy link
Member

@Constellation Constellation left a comment

Choose a reason for hiding this comment

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

I think you need to fix the same thing in Baseline JIT, DFG, and FTL.

@rkirsling
Copy link
Member

rkirsling commented Nov 13, 2022

Hmm, I'm not actually certain whether this change is web compatible, as it appears that no other browsers pass the test case in question either (though engine262 does, which confirms the reporter's reading of the spec).

The same reporter filed issues with V8 and SM; SM closed theirs as a duplicate of a 12(!)-year-old bug, but it doesn't give a reason for not fixing it.

We definitely need to investigate this situation further; I'll discuss this on Matrix with other TC39 folks as to how to proceed.

@rkirsling
Copy link
Member

rkirsling commented Nov 14, 2022

Sure enough, this is a web reality issue to be fixed spec-side; it is not something that a single implementation can do anything about at present: tc39/ecma262#2205

(There is a test262 case for the currently spec'ed behavior, but it will be updated in the corresponding tc39/test262#2891.)

@Constellation
Copy link
Member

@rkirsling Oh, this is nice catch!

@hyjorc1 hyjorc1 closed this Nov 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
JavaScriptCore For bugs in JavaScriptCore, the JS engine used by WebKit, other than kxmlcore issues. merging-blocked Applied to prevent a change from being merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants