Call for Stage 3 Reviews

[jridgewell]

Hi All,

I'm seeking Stage 3 advancement at the March meeting (< 3 weeks away), and in order to do that, I need a spec review.

• Reviewers:
  • [x] @bakkot
  • [x] @DanielRosenwasser
• Spec Editors
  • [x] @bakkot
  • [x] @ljharb
  • [x] @michaelficarra
  • [x] @syg

Please add any comments here.

[ljharb]

var a = 1; (a) += 4; a; is valid; can someone confirm if var a = 1; (a) &&= 4; a; is also valid? If so, it seems like perhaps we don't want to add more parenthesized LHS's to the language?

[rkirsling]

You explained in https://github.com/tc39/proposal-logical-assignment/issues/15#issuecomment-596054653 that LogicalAssignmentPunctuator is just for viewability within the proposal, but could we call this out in a Section 1 note, so it's understood that we don't mean to merge it that way?

(Otherwise LGTM from the sidelines. :smile:)

[jridgewell]

can someone confirm if var a = 1; (a) &&= 4; a; is also valid?

It is, and this is covered by the AssignmentTargetType static error in https://tc39.es/proposal-logical-assignment/#sec-assignment-operators-static-semantics-early-errors. The AssignmentTargetType of the (a) in (a) &&= 4 is simple, due to:

• https://tc39.es/ecma262/#sec-grouping-operator-static-semantics-assignmenttargettype
• https://tc39.es/ecma262/#sec-identifiers-static-semantics-assignmenttargettype

If so, it seems like perhaps we don't want to add more parenthesized LHS's to the language?

It'd actually take considerably more work to change this behavior, we'd have to plumb an entirely new AssignmentTargetType through the spec.

---

could we call this out in a Section 1 note, so it's understood that we don't mean to merge it that way?

Sure.

[bakkot]

It'd actually take considerably more work to change this behavior, we'd have to plumb an entirely new AssignmentTargetType through the spec.

Without offering an opinion on whether this change should be made, I don't think this is true: you could just have an Early Error to the effect of

AssignmentExpression : LeftHandSideExpression LogicalAssignmentOperator AssignmentExpression

It is a Syntax Error if LeftHandSideExpression is PrimaryExpression : CoverParenthesizedExpressionAndArrowParameterList.

(Similar wording is used in the Early Errors for delete.)

Of course, it might be better to have a new AssignmentTargetType instead of this.

[bakkot]

On reflection, I think it'd be best to allow these parenthesized LHSes (i.e., keep the current spec). Banning weird syntax whenever there is any new syntax involved, even though that syntax is very similar to existing syntax, causes trouble for implementations and does not obviously benefit anyone.

For example, we recently removed the restriction that you could not have try {} catch (e) { for (var e of foo); } (note the duplicate e), which strikes me as a pretty similar situation.

[ljharb]

In that case, that's because that's a reasonable thing to want to do :-) when would you actually want to put parens around the LHS?

[bakkot]

That was not why we relaxed that restriction. It was explicitly because the restriction caused difficulty for implementations and did not obviously benefit anyone. The same is true here.

Also, that is definitely not a reasonable thing to want to do, at least not with the semantics it has. (The var e declares a variable in the outer function scope, but the loop assigns to the e created by the catch binding.)

[DanielRosenwasser]

Review approval pending https://github.com/tc39/proposal-logical-assignment/pull/20

[ljharb]

LGTM

[syg]

I agree that we should allow parenthesized LHSes.

LGTM editorially. Only suggestion is when preparing the PR for merge, please make sure the strict mode note isn't duplicated for every single production.

[michaelficarra]

LGTM. I think we could've described the error behaviour inline in the algorithm steps instead of in a big note that references step numbers (gross), but I see that it's just consistent with what's already there, so it's fine for now.

[rkirsling]

One more point I almost forgot: in the same way as https://github.com/tc39/proposal-nullish-coalescing/pull/50, it would be good to rename LogicalAssignmentOperator to ShortCircuitAssignmentOperator before this reaches Stage 4.

(I know the name of the proposal is a foregone thing but we can still editorialize the spec text. :joy:)

[ljharb]

My understanding is that "LogicalAssignmentOperator" won't actually be in the spec PR; it was just a device to show the diff in this repo?

[rkirsling]

@ljharb That's LogicalAssignmentPunctuator, but the similarity is in fact why I forgot to mention this earlier. 😅

[ljharb]

whoops, good call then :-)

[jridgewell]

Update: I've opened https://github.com/tc39/proposal-logical-assignment/issues/23 to discuss adding a NamedEvaluation to anonymous functions that are the RHS.