Skip to content

Conversation

@andreabergia
Copy link
Contributor

@andreabergia andreabergia commented Dec 2, 2025

Code like this:

for (var {};;) {}

was parsed correctly, but then failed during the code generation with various exceptions because the destructuring expression after var doesn't have a right-hand side.

This change implements a check at parse time, to throw a SyntaxError, coherently with what node or spidermonkey or quickjs do.

I'm a bit surprised that this doesn't seem to show any improvement in test262 coverage. Perhaps a test there might be warranted?

Fixes #1922

andrea.bergia added 2 commits December 2, 2025 16:44
Code like this:

```js
for (var {};;) {}
```

was parsed correctly, but then failed during the code generation
with various exceptions because the destructuring expression after
`var` doesn't have a right-hand side.

This change implements a check at parse time, to throw a `SyntaxError`,
coherently with what `node` or `spidermonkey` or `quickjs` do.

I'm a bit surprised that this doesn't seem to show any improvement in
test262 coverage. Perhaps a test there might be warranted?

Fixes mozilla#1922
@andreabergia andreabergia marked this pull request as draft December 2, 2025 16:11
Copy link
Contributor

@aardvark179 aardvark179 left a comment

Choose a reason for hiding this comment

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

Looks goodActually, seems to be causing some failures, and does make me wonder, are there other places that might need similar checks?

@aardvark179
Copy link
Contributor

Ah, scratch that, it's a problem with a Date test.

@andreabergia andreabergia marked this pull request as ready for review December 2, 2025 20:25
@andreabergia
Copy link
Contributor Author

are there other places that might need similar checks?

From what I can tell, this problem should only happen in for loops currently. Variable declarations were already fine, function arguments do allow that syntax, catch ({a}) should allow it but it's not supported (created #2194). For what it's worth, Claude also thinks we don't need new places for this fix.

@aardvark179
Copy link
Contributor

Okay looking at the spec and MDN I agree that the only places that allow destructuring binding are loops, functions, and catch blocks.

public void errorOnInvalidDestructuringDeclaration() {
expectParseErrors(
"for(var {};;) {}",
new String[] {"Missing = in destructuring declaration", "syntax error"});
Copy link
Collaborator

Choose a reason for hiding this comment

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

As a reader of error messages i like this form more - not sure why the french version has the ' and the english not.

Missing '=' in destructuring declaration

@gbrail
Copy link
Collaborator

gbrail commented Dec 7, 2025

Great, thanks!

@gbrail gbrail merged commit 425cad9 into mozilla:master Dec 7, 2025
11 checks passed
@andreabergia andreabergia deleted the fix-issue-1922 branch December 9, 2025 10:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

java.lang.IllegalStateException: FAILED ASSERTION

4 participants