Add support for ??=, ||=,&&= in JavaScript#72
Conversation
There was a problem hiding this comment.
Code Review
This pull request implements desugaring for logical assignment operators (??=, ||=, &&=) in the JavaScript parser. The review feedback highlights critical issues regarding semantic correctness, specifically the double evaluation of the left-hand side and the lack of proper short-circuiting behavior compared to standard JavaScript. There is also a concern regarding the reuse of UAST node instances, which could lead to corrupted tree structures.
| if (node.operator === '??=') { | ||
| return UAST.assignmentExpression(left, UAST.binaryExpression('??', left, right), '=', false); | ||
| } | ||
| if (node.operator === '||=') { | ||
| return UAST.assignmentExpression(left, UAST.binaryExpression('||', left, right), '=', false); | ||
| } | ||
| if (node.operator === '&&=') { | ||
| return UAST.assignmentExpression(left, UAST.binaryExpression('&&', left, right), '=', false); | ||
| } |
There was a problem hiding this comment.
The current implementation of logical assignment operators (??=, ||=, &&=) has two significant issues:
- Semantic Correctness (Side Effects & Short-circuiting): Desugaring
a ??= btoa = a ?? bis not equivalent in JavaScript. It causes the left-hand side to be evaluated twice (e.g.,func().prop ??= 1callsfunc()twice), which is incorrect if the expression has side effects. Furthermore, logical assignment operators in JS are short-circuiting;a ||= bonly performs an assignment ifais falsy. The desugareda = a || balways performs an assignment, which can trigger setters or proxy traps unnecessarily. - UAST Node Reuse: The
leftnode object is reused as both the assignment target and an operand in the binary expression. In most AST/UAST implementations, node objects must be unique instances within the tree. Reusing the same object instance can lead to broken parent pointers and issues during tree traversal or serialization.
If the UAST specification supports these operators, it is highly recommended to pass them directly to UAST.assignmentExpression. If not, consider updating the UAST spec to support them, as correct desugaring is complex and typically requires temporary variables to ensure single evaluation of the LHS.
Convert
a ??= btoa = a ?? bfor parsing, similar for others