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.

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

Normative: Add Logical Assignment Operators #2030

Merged
merged 1 commit into from
Jul 21, 2020

Conversation

jridgewell
Copy link
Member

@jridgewell jridgewell commented Jun 2, 2020

https://github.com/tc39/proposal-logical-assignment

This includes a few new changes:

  • NamedEvaluation is used, given today's tentative consensus
  • LogicalAssignmentOperator has been removed, and instead each @@= operator is defined individually

Outdated discussion

I need help getting it to compile correctly, though. When I run build, I get:

Could not find a production matching RHS in syntax-directed operation (undefined-nonterminal)

This is likely from defining the grammar as LeftHandSideExpression ShortCircuitAssignmentOperator AssignmentExpression, but defining individual algorithms as AssignmentExpression : LeftHandSideExpression `||=` AssignmentExpression. We could define each grammar individually, or do a bit of metaprogramming for the algorithms, like AssignmentOperator did. I just don't know how to metaprogram in spec-ese.

spec.html Outdated Show resolved Hide resolved
@bakkot
Copy link
Contributor

bakkot commented Jun 2, 2020

This is likely from defining the grammar as LeftHandSideExpression ShortCircuitAssignmentOperator AssignmentExpression, but defining individual algorithms as AssignmentExpression : LeftHandSideExpression `||=` AssignmentExpression.

That is indeed the reason! That's the linter doing its job. I'm glad the error message was clear enough for you to figure it out. (We're all going to be beta-testing it for a bit.) Apologies for not catching this during the review.

We could define each grammar individually, or do a bit of metaprogramming for the algorithms, like AssignmentOperator did. I just don't know how to metaprogram in spec-ese.

In this instance I think it will be clearest to just do away with ShortCircuitAssignmentOperator and instead add each of the three cases individually to AssignmentExpression.

@jmdyck
Copy link
Collaborator

jmdyck commented Jun 2, 2020

In this instance I think it will be clearest to just do away with ShortCircuitAssignmentOperator and instead add each of the three cases individually to AssignmentExpression.

With the existing production, you could do something like:

<emu-grammar>AssignmentExpression : LeftHandSideExpression ShortCircuitAssignmentOperator AssignmentExpression</emu-grammar>
<emu-alg>
  1. Let _lref_ be the result of evaluating |LeftHandSideExpression|.
  1. Let _lval_ be ? GetValue(_lref_).
  1. Let _opText_ be the source text matched by |ShortCircuitAssignmentOperator|.
  1. If _opText_ is `??=`, then
    1. If _lval_ is neither *undefined* nor *null*, return _lval_.
  1. Else,
    1. Let _lbool_ be ! ToBoolean(_lval_).
    1. If _opText_ is `||=`, then
      1. If _lbool_ is *true*, return _lval_.
    1. Else,
      1. Assert: _opText_ is `&amp;&amp=`.
      1. If _lbool_ is *false*, return _lval_.
  1. If IsAnonymousFunctionDefinition(|AssignmentExpression|) and IsIdentifierRef of |LeftHandSideExpression| are both *true*, then
    1. Let _rval_ be NamedEvaluation of |AssignmentExpression| with argument GetReferencedName(_lref_).
  1. Else,
    1. Let _rref_ be the result of evaluating |AssignmentExpression|.
    1. Let _rval_ be ? GetValue(_rref_).
  1. Perform ? PutValue(_lref_, _rval_).
  1. Return _rval_.
</emu-alg>

(I started it before @bakkot's comment and didn't want to just throw it away.)

@jmdyck
Copy link
Collaborator

jmdyck commented Jun 2, 2020

If you do keep ShortCircuitAssignmentOperator, you'll need to reference it from Annex A.

And in any event, HasCallInTailPosition will need to be defined for the new right-hand-side(s).

Copy link
Member Author

@jridgewell jridgewell left a comment

Choose a reason for hiding this comment

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

Updated.

spec.html Outdated Show resolved Hide resolved
@jridgewell
Copy link
Member Author

If you do keep ShortCircuitAssignmentOperator, you'll need to reference it from Annex A.

It's removed now, so I think this is done.

And in any event, HasCallInTailPosition will need to be defined for the new right-hand-side(s).

Whoops, not sure how I missed that. Updated.

spec.html Outdated Show resolved Hide resolved
@ljharb ljharb added has test262 tests normative change Affects behavior required to correctly evaluate some ECMAScript source text pending stage 4 This proposal has not yet achieved stage 4, but may otherwise be ready to merge. proposal This is related to a specific proposal, and will be closed/merged when the proposal reaches stage 4. labels Jun 2, 2020
@ljharb ljharb requested review from syg, michaelficarra and a team June 2, 2020 07:09
@jridgewell jridgewell requested a review from ljharb July 9, 2020 04:41
@jridgewell
Copy link
Member Author

This is going for Stage 4 at the July meeting, and still requires Spec Editor approvals.

/cc @syg @michaelficarra @ljharb

@jridgewell jridgewell force-pushed the logical-assignment branch from f0e23c2 to 4879950 Compare July 9, 2020 04:47
@ljharb
Copy link
Member

ljharb commented Jul 9, 2020

We plan to review it during next week's editor call, if not before.

@ljharb ljharb requested review from a team and removed request for ljharb July 9, 2020 05:14
Copy link
Contributor

@syg syg left a comment

Choose a reason for hiding this comment

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

lgtm with step linking added back

spec.html Show resolved Hide resolved
spec.html Outdated Show resolved Hide resolved
@ljharb ljharb self-assigned this Jul 15, 2020
@ljharb ljharb added has stage 4 This PR represents a proposal that has achieved stage 4, and is ready to merge. and removed pending stage 4 This proposal has not yet achieved stage 4, but may otherwise be ready to merge. labels Jul 21, 2020
@ljharb ljharb force-pushed the logical-assignment branch from 4886f4c to 740ecd6 Compare July 21, 2020 18:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
has stage 4 This PR represents a proposal that has achieved stage 4, and is ready to merge. has test262 tests normative change Affects behavior required to correctly evaluate some ECMAScript source text proposal This is related to a specific proposal, and will be closed/merged when the proposal reaches stage 4.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants