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

Fix destructuring assignment errors #1529

Merged
merged 2 commits into from
Jul 22, 2024
Merged

Fix destructuring assignment errors #1529

merged 2 commits into from
Jul 22, 2024

Conversation

tuchida
Copy link
Contributor

@tuchida tuchida commented Jul 18, 2024 •
edited
Loading

Closes #1187

js> var a = []; var b = 0; [a[b+1]] = [123];
Exception in thread "main" java.lang.NullPointerException
	at org.mozilla.javascript.optimizer.BodyCodegen.generateExpression(BodyCodegen.java:967)
	at org.mozilla.javascript.optimizer.BodyCodegen.generateExpression(BodyCodegen.java:1233)
	at org.mozilla.javascript.optimizer.BodyCodegen.visitSetElem(BodyCodegen.java:4143)

This error has been corrected in this commit.

Closes #406

js> (1 ? {} : 490) = 1
Exception in thread "main" java.lang.ClassCastException: class org.mozilla.javascript.Node cannot be cast to class org.mozilla.javascript.ast.ObjectLiteral (org.mozilla.javascript.Node and org.mozilla.javascript.ast.ObjectLiteral are in unnamed module of loader 'app')
	at org.mozilla.javascript.Parser.destructuringAssignmentHelper(Parser.java:4046)
	at org.mozilla.javascript.Parser.createDestructuringAssignment(Parser.java:4016)
	at org.mozilla.javascript.IRFactory.createAssignment(IRFactory.java:2056)

This error has been corrected in this commit.

@p-bakker
Copy link
Collaborator

Nice!

tuchida
tuchida commented Jul 18, 2024
View reviewed changes
Copy link
Contributor Author

@tuchida tuchida left a comment

Choose a reason for hiding this comment

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

rhino/rhino/src/main/java/org/mozilla/javascript/IRFactory.java

Line 1112 in 5f55f04

Node d = parser.createDestructuringAssignment(node.getType(), left, right);

Because a variable declaration cannot be an expression, no transform is passed here.

var [a.b] = []; // SyntaxError

rhino/src/main/java/org/mozilla/javascript/IRFactory.java
@@ -1476,7 +1476,9 @@ private Node createForIn(
Node newBody = new Node(Token.BLOCK);
Node assign;
if (destructuring != -1) {
assign = parser.createDestructuringAssignment(declType, lvalue, id);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

var a;
for ([a] of [[1], [2], [3]]) {
  //
}

rhino/src/main/java/org/mozilla/javascript/IRFactory.java
@@ -2053,7 +2055,8 @@ private Node createAssignment(int assignType, Node left, Node right) {
parser.reportError("msg.bad.destruct.op");
return right;
}
return parser.createDestructuringAssignment(-1, left, right);
return parser.createDestructuringAssignment(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

var a;
[a] = [1];

@tuchida tuchida marked this pull request as ready for review July 18, 2024 17:08
gbrail
gbrail reviewed Jul 18, 2024
View reviewed changes
tests/testsrc/test262.properties
@@ -3186,6 +3186,206 @@ language/expressions/arrow-function 209/333 (62.76%)
scope-paramsbody-var-close.js
scope-paramsbody-var-open.js

language/expressions/assignment 200/468 (42.74%)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm trying to refresh my memory on test262.properties -- is this a long list of tests that we now have to disable becuase we broke them, or were these tests never run and we're actually able to pass 58% of them?

In other words, do these test262 changes represent an improvement, or a regression?

Copy link
Collaborator

Choose a reason for hiding this comment

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

The test262Suite only looks at tests in directories that are already included in trst262.properties

So what you're seeing here in inclusion of a new directory full of tests, some of which already pass

Ran into this recently, tried to create a PR that will include ALL directories automatically, because this is a gap in our 'security net'. Didn't get around to finish it though....

Copy link
Contributor Author

Choose a reason for hiding this comment

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

f772166 First I added language/expressions/assignment tests to test262.properties in the first commit.
1fd026f And two of those tests were improved.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added all directories to test262.properties. ref. #1531
I would like to merge that PR first.

@gbrail
Copy link
Collaborator

gbrail commented Jul 21, 2024

This looks good, but I merged your other 262.properties change and I'd feel better if you looked it over and resolved before merging. Thanks!

@tuchida tuchida force-pushed the fix-destructuring-assignment-errors branch from 5f55f04 to d89002d Compare July 21, 2024 21:33
@tuchida
Copy link
Contributor Author

tuchida commented Jul 21, 2024 •
edited
Loading

f772166 Add assignment tests of test262

I deleted the commit. No other changes have been made.

@gbrail
Copy link
Collaborator

gbrail commented Jul 22, 2024

Thanks for working on this!

@gbrail gbrail merged commit 7525f81 into mozilla:master Jul 22, 2024
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gbrail gbrail gbrail left review comments

@p-bakker p-bakker p-bakker left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
3 participants
@tuchida @p-bakker @gbrail