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

Use early error to avoid ASI for +,-,/, and /= #18

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

rbuckton
Copy link
Collaborator

@rbuckton rbuckton commented Sep 26, 2023

This removes +, -, and / from the ThrowExpressionInvalidPunctuator list and instead bans those trailing tokens via static semantics rules to avoid ambiguity as a result of ASI for prefix-+, prefix--, and regular expressions. To verify that a ThrowExpression is not contained on the left side of AdditiveExpression and MultiplicativeExpression, we use a new SDO called ContainsThrowOnRight, which is used to check whether the right-most expression of the left-hand side of those expressions are ThrowExpression. This SDO is intended to handle the case of a ThrowExpression nessted on the right-hand side of an ExponentiationExpression:

a ** throw b 
+ c

As it is necessary to traverse the left side of the + operator (a ** throw b), and look for the right-hand side of the ** operator.

This also removes assignment operators from ThrowExpressionInvalidPunctuator as they were already illegal in the grammar.

Finally, this adds AssignmentExpression : ThrowExpresion /= AssignmentExpression so that we can produce an early error to avoid ASI as well.

This PR utilizes similar Static Semantics rules as those employed by OptionalChain to avoid ASI in cases like this:

a?.b
`c`

Fixes #19

@github-actions
Copy link

A preview of this PR can be found at https://tc39.es/proposal-throw-expressions/pr/18.

spec.emu Outdated Show resolved Hide resolved
@rbuckton
Copy link
Collaborator Author

/cc @bakkot, @waldemarhorwat

spec.emu Outdated Show resolved Hide resolved
Copy link
Member

@nicolo-ribaudo nicolo-ribaudo left a comment

Choose a reason for hiding this comment

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

I think this addresses the ASI concerns without needing too much early errors scattered all around :)

spec.emu Outdated
</emu-grammar>
<ul>
<li>
It is a Syntax Error if ContainsThrowOnRight of |MultiplicativeExpression| is *true*.
Copy link
Member

Choose a reason for hiding this comment

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

Can we assert that the operator is /? Given the the others are impossible due to the lookahead restriction.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sure, I'll fix that shortly.

a ?? throw b
/c/d
</code></pre>
<p>so that it would be interpreted as two valid statements. The purpose is to maintain consistency with similar code using |ThrowStatement|:</p>
Copy link
Member

Choose a reason for hiding this comment

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

Instead of "maintain consistency" I would say "avoid different valid behaviour", given that we are not consistent with statements because we entirely disallow this case.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Possibly, though I'd like an editor to weigh in as I think "maintain consistency" here and in OptionalChain TemplateExpression refers specifically to ASI handling, since both grammars are intended to produce an error.

@rbuckton
Copy link
Collaborator Author

This approach did not achieve consensus.

@rbuckton rbuckton closed this Jan 17, 2024
@ljharb ljharb deleted the use-early-error branch January 17, 2024 21:59
@rbuckton rbuckton restored the use-early-error branch February 7, 2024 22:11
@rbuckton
Copy link
Collaborator Author

rbuckton commented Feb 7, 2024

I've been asked to leave this open for further discussion.

@rbuckton rbuckton reopened this Feb 7, 2024
@ethanresnick
Copy link

ethanresnick commented Jun 3, 2024

@rbuckton Before you re-opened this PR, you had said:

Unfortunately, the approach leveraging syntactic restrictions was blocked from advancement. I'm not thrilled with this change either, but it's likely the only way this proposal will ever advance as I don't expect the sentiment towards the trailing syntactic restriction is likely to change.

Does re-opening this PR mean you think that some sort of syntax restrictions might be a viable solution after all?

Personally, as a developer (i.e., not a spec author or implementer), I just want to be able to avoid parentheses in the overwhelmingly-common cases of throw new Error() and throw makeSomeError(...). In all the more obscure cases, I think the optimal DX is to require parentheses around the value on the right of the throw, and provide a good error message if parentheses are omitted.

If parentheses are required in the common case, where there isn't any ambiguity, that will feel like an arbitrary, annoying restriction to almost every developer who encounters it, as they won't be steeped in the arcane details of the grammar.

One comment from the meeting notes was:

WH: I do not want early errors. This should be purely grammatical. It’s much easier to understand purely grammatical syntax than rules expressed as early errors.

I'm not totally unsympathetic to that sentiment — I get that set of possible source texts is giant and that the spec authors have to be able to reason about it to keep the language extensible and possibly even to avoid security issues.

But, purely as a developer, this sort of distinction (grammatical restriction vs early error), is not something I care about at all (or perhaps I'd even prefer an early error, if the error message can be more useful). So, I'd hope it doesn't result in parentheses being required everywhere.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ASI hazard with trailing +, -, /, and /=
3 participants