add Logical Assignment Operators #212
Conversation
| ### LogicalAssignmentExpression | ||
|
|
||
| ```js | ||
| interface LogicalAssignmentExpression <: Expression { |
There was a problem hiding this comment.
Why do we need a new node type for this rather than extending AssignmentOperator in AssignmentExpression?
There was a problem hiding this comment.
Ah, sorry, I see it's explained below.
There was a problem hiding this comment.
Just to make sure I understand this: a new node type is necessary because logical assignment expressions are short-circuited?
There was a problem hiding this comment.
As far as I understood, it's for symmetry with BinaryExpression vs LogicalExpression (which were initially separate due to short-circuiting, I suppose).
Personally, I'd rather have former to be a single node type as well, but that ship has sailed.
As for new node types, I don't have a strong opinion and fine with either a new node type or reusing AssignmentExpression. Would like to hear opinions of others.
There was a problem hiding this comment.
Yes, it's for symmetry with BinaryExpression vs LogicalExpression, and I thought that's because of short-circuit.
There was a problem hiding this comment.
@bradzacher Yeah, as I said above
Personally, I'd rather have former to be a single node type as well, but that ship has sailed.
This has been there from the beginning of SpiderMonkey AST and I can only assume that it was for encoding of short-circuiting behaviour, but personally I'd also just use a single node type and encourage consumers to check the operator.
Not sure whether to follow the precedent or do something simpler for the new node types here. Side-note: this is why I said I'm very cautious of accidentally setting new precedents in #204 - they can affect design of nodes far away from the original addition.
There was a problem hiding this comment.
I don't have any strong opinion about the choice of following the precedent or using AssignmentExpression simply.
There was a problem hiding this comment.
This is why I asked, as I’m not sure if we want to encode short-circuiting operations separately. My preference would be to reuse AssignmentExpression. If the intent is to execute based on the AST, you’ll always need to look at the operator anyway, and that seems like the logical time to apply any short circuiting behavior.
There was a problem hiding this comment.
I have updated this PR to extending AssignmentOperator because the separation of nodes by short-circuiting looks not important to ESTree.
|
We have approvals from all ESC members on this pull request, so it is approved and so I’m merging. |
Recently, Logical Assignment Operators arrived at Stage 3.
Considering AST before Stage 4 is nice.