Skip to content

Implement logical assigment operators ||= and &&= #1597

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
Merged
merged 1 commit into from
Sep 5, 2024
Merged

Implement logical assigment operators ||= and &&= #1597

merged 1 commit into from
Sep 5, 2024

Conversation

andreabergia
Copy link
Contributor

@andreabergia andreabergia commented Aug 30, 2024 •
edited by p-bakker
Loading

Turned out to be pretty simple 🙂

I had to re-number all the entries in Token because the number for the new tokens has to land between FIRST_ASSIGN and LAST_ASSIGN

Also, we get two regression in test262 (lgcl-and-assignment-operator-non-simple-lhs.js and lgcl-or-assignment-operator-non-simple-lhs.js), where the tests does something like this:

// It is a Syntax Error if AssignmentTargetType of LeftHandSideExpression is not simple.
function test() {}
test() ||= 1;

Before, the code was throwing a SyntaxError because it did not recognize the ||= operator. Now, it does, and it does not handle correctly the fact that there is a non-simple expression on the left-hand side. However, this behavior is shared with all other assignments operator (e.g., test() += 32 does not throw a SyntaxError either), so I don't think this is a real "regression".

Partially resolves #936

@andreabergia andreabergia force-pushed the logical_assignment_operators branch from cd22dca to 75c8b71 Compare August 30, 2024 10:23
@andreabergia andreabergia mentioned this pull request Aug 30, 2024
Merged
@p-bakker
Copy link
Collaborator

Great stuff!

What about ??= through? That is also part of the same proposal, see #938

As for the 'regression': Can you create a case for that (assuming you don't (directly) go off and provide a PR for that one as well :-)

@p-bakker
Copy link
Collaborator

BEsides the 'regression': seems not all lgcl-or/and-assignment-opearator-* are passing.

I just looked at language/expressions/logical-assignment/lgcl-or-assignment-operator-lhs-before-rhs.js for example

Aren't those fixable?

@andreabergia
Copy link
Contributor Author

What about ??= through? That is also part of the same proposal, see #938

I know, but IMHO that is better implemented after #1591 is merged. So, I'd prefer opening a new PR after this and that one are merged.

BEsides the 'regression': seems not all lgcl-or/and-assignment-opearator-* are passing.

I just looked at language/expressions/logical-assignment/lgcl-or-assignment-operator-lhs-before-rhs.js for example

All of these problems happen also for existing operators, like +=. They should be tracked in proper issues (I'll try to open some), but they are not strictly related to the new operators that this PR adds.

@andreabergia
Copy link
Contributor Author

Opened #1598 #1599 and #1600 which should track the test262 failures for the new operators

@gbrail
Copy link
Collaborator

gbrail commented Aug 31, 2024

I'm OK with merging this and opening new issues and PRs for the other features if others think this is a good PR. (looks good to me, BTW). Out of time today but it won't be long. Thanks!

@p-bakker
Copy link
Collaborator

This PR looks good to me as well: have no clue how the limited set of changes actually made things work 🤔, but the relevant tests in test262 pass where they can, so apparently they work 🙂 so I'm all for merging this!

@gbrail
Copy link
Collaborator

gbrail commented Sep 5, 2024

Thanks for doing this!

@gbrail gbrail merged commit 1fb4321 into mozilla:master Sep 5, 2024
3 checks passed
@p-bakker p-bakker mentioned this pull request Sep 5, 2024
Closed
@andreabergia
Copy link
Contributor Author

This PR looks good to me as well: have no clue how the limited set of changes actually made things work 🤔, but the relevant tests in test262 pass where they can, so apparently they work 🙂 so I'm all for merging this!

Well, very simply: we already have a lot of logic for operators such as += or |=. It was super easy to extend it to ||= because it has the same semantic.

@andreabergia andreabergia deleted the logical_assignment_operators branch September 6, 2024 14:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Support ES2021 Nullish Assignment Operator
3 participants
@andreabergia @p-bakker @gbrail