TypeError is expected when reassigning to const during destructuring in for statement

[hyjorc1]

c228e7c

TypeError is expected when reassigning to `const` during destructuring in for statement
https://bugs.webkit.org/show_bug.cgi?id=247432
rdar://102064568

Reviewed by Yusuke Suzuki.

Running this JavaScript program should throw TypeError.
```
class x { static #y = { x = 0 } = 0 ; }
```

V8
```
class x { static #y = { x = 0 } = 0 ; }
                        ^
TypeError: Assignment to constant variable.
```

GraalJS
```
TypeError: Assignment to constant "x"
```

To fix this, we should use AssignmentContext::AssignmentExpression
to construct destructuring pattern in for...in/of statement. In this case,
symbolTablePut will check for read-only variable without ignoring.

* JSTests/stress/destructuring-for-in-of.js: Added.
(shouldThrow):
* Source/JavaScriptCore/parser/Parser.cpp:
(JSC::Parser<LexerType>::parseForStatement):

Canonical link: https://commits.webkit.org/256580@main

e5d27c1

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
✅ 🛠 🧪 merge	✅ 🛠 watch	🧪 mac-AS-debug-wk2	✅ 🛠 jsc-mips
	✅ 🛠 watch-sim	🧪 mac-wk2-stress	✅ 🧪 jsc-mips-tests

[webkit-early-warning-system]

EWS run on previous version of this PR (hash 2bc8820)

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

[webkit-early-warning-system]

EWS run on previous version of this PR (hash 95134aa)

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

[rkirsling]

How do we know what's expected though? I would expect a patch like this to resolve a test262 failure; if such a test doesn't exist, then (we should contribute one, and) the commit message should include other engines' behavior / spec text.

[Constellation]

r=me

[hyjorc1]

How do we know what's expected though? I would expect a patch like this to resolve a test262 failure; if such a test doesn't exist, then (we should contribute one, and) the commit message should include other engines' behavior / spec text.

Hi @rkirsling, thanks for your comments. The bug report https://bugs.webkit.org/show_bug.cgi?id=247432 already contains the problem and the expectation behavior with other engines. Do we need to confirm that in the commit message?

we should contribute one

Should we add the test to the original test262 repository or the test262 test cases in WebKit?

[webkit-early-warning-system]

EWS run on current version of this PR (hash e5d27c1)

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
✅ 🛠 🧪 merge	✅ 🛠 watch	🧪 mac-AS-debug-wk2	✅ 🛠 jsc-mips
	✅ 🛠 watch-sim	🧪 mac-wk2-stress	✅ 🧪 jsc-mips-tests

[webkit-commit-queue]

Committed 256580@main (c228e7c): https://commits.webkit.org/256580@main

Reviewed commits have been landed. Closing PR #6390 and removing active labels.

[rkirsling]

Hi @rkirsling, thanks for your comments. The bug report https://bugs.webkit.org/show_bug.cgi?id=247432 already contains the problem and the expectation behavior with other engines. Do we need to confirm that in the commit message?

Ah, I had no idea that there was a bug report; bugzilla is mostly just PR placeholders at this point, so I'm no longer expecting it to contain useful information. 😅 In this case, the bug report doesn't link to the spec, doesn't mention SM's behavior, and doesn't have a representative test262 case, so it would require further investigation before handling—we might find that V8 and SM disagree, or that V8 and SM agree but differ from the spec. This information is what we should have in the commit message, in order to justify the correctness of the change. (test262 cases are convenient because "this patch fixes a test262 failure" is usually an adequate justification on its own.)

Should we add the test to the original test262 repository or the test262 test cases in WebKit?

The former. We regularly sync JSTests/test262 with upstream test262, so we can't modify those test cases directly.