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

Strengthen assignment target definitions. #135

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

RReverser
Copy link
Member

Reopen of #134 with more precise yet backward-compatible assignment target definitions.

@mikesherov
Copy link
Contributor

👍

@@ -531,7 +531,7 @@ A unary operator token.
interface UpdateExpression <: Expression {
type: "UpdateExpression";
operator: UpdateOperator;
argument: Expression;
argument: Pattern | CallExpression;
Copy link
Member

Choose a reason for hiding this comment

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

Not just CallExpression. Any expression can be in this position in ES5 and below.

Copy link
Member Author

@RReverser RReverser Sep 9, 2016

Choose a reason for hiding this comment

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

No, see discussion in previous PRs. In ES5 specifications must reject as an early error any LHS that can't evaluate to Reference in runtime, and only CallExpression are such that could result in Reference despite being expressions. For literals, unary, binary and conditional expressions there is no way in spec they can evaluate to Reference and thus must be rejected in such position.

EDIT: On the other thought, perhaps SequenceExpression and ConditionalExpression can evaluate to Reference if their last values are CallExpression and should be allowed too, need to recheck.

EDIT(2): Rechecked, both SequenceExpression and CondtionalExpression perform GetValue on results on subexpressions, so can't be in this position, too.

Copy link
Member

Choose a reason for hiding this comment

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

Ah, I didn't know about that early error in ES5. Okay, then I am fine with this as long as we add an item to the "philopsophy" section of the README that states we do not wish to represent programs that would otherwise be syntactically valid if not for their early errors.

Copy link

Choose a reason for hiding this comment

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

@michaelficarra that would be a good note to add, but I observe that this is already the case: e.g. in es2015 the LHS of an AssignmentExpression must be a Pattern, but this is only required because of an early error.

@mikesherov
Copy link
Contributor

@RReverser seems we need to accompany this with a note in philosophy re: early errors. Care to PR that also?

@RReverser
Copy link
Member Author

@mikesherov Last time couldn't think of a good wording to specify that, I can give it another try.

@adrianheine
Copy link
Member

Quick first draft: »ESTree is intended to be able to represent valid ECMAScript scripts and modules. It is not required to support programs that follow the ECMAScript syntactic grammar, but would produce early errors during parsing. See the introduction to the syntactic grammar in the ECMAScript spec

@adrianheine
Copy link
Member

We might also want to clarify that it's obviously possible for an invalid source to be representable with ESTree.

@nzakas
Copy link
Contributor

nzakas commented Apr 9, 2020

@RReverser do you want to move forward with this or should it be closed?

@RReverser
Copy link
Member Author

I think it's still useful and valuable, but there was some confusion above as to whether ESTree should represent nodes that wouldn't pass early errors checks.

@nzakas
Copy link
Contributor

nzakas commented Apr 13, 2020

How would you like to proceed? If you’d like to push forward, I’d suggest summarizing the discussion to this point to make it easier to pick up for others.

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.

None yet

6 participants