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

Lift restrictions on the left-hand-side in the ES5 parser #40

Closed
achudnov opened this issue Sep 6, 2013 · 7 comments
Closed

Lift restrictions on the left-hand-side in the ES5 parser #40

achudnov opened this issue Sep 6, 2013 · 7 comments
Labels

Comments

@achudnov
Copy link
Member

achudnov commented Sep 6, 2013

Right now the expressions that are used as targets of assignments are validated against the validLHS function. While there is reason in that (all other expressions would raise a ReferenceError at run-time), the specification is less restrictive on this matter, and we'd like to follow it closely.

Thus, assignmentExpressionGen should be rewritten to expect any LeftHandSideExpression (as defined by the spec) instead of just a variable or a field reference.

When issue #41 is closed we could relax the restriction in the spec to allow error recovery.

@philipnilsson
Copy link

assignmentExpressionGen should (and can not) be re-written to use LeftHandSideExpression due to the fact that Parsec does not deal with left-recursive grammars. Doing so would require unnecessary backtracking. The implementation actually looked liked that at some point, and at that point the parser choked on parsing jQuery (used up all memory).

Instead what we need is to re-write the validLHS function so that it corresponds correctly with the producitons generated by LeftHandSideExpression, possibly causing it to generate errors and continue parsing as described in #41. Keep in mind that assignmentExpressionGen currently uses logicalOrExpressionGen which is less restrictive than leftHandSideExpression. Any unnecessary restrictions that currently exist in the parser is singularly due to the validLHS function.

@philipnilsson
Copy link

Actually I might have misunderstood what you mean above, you probably have the right idea, but yeah, just to be clear, we should not change assignmentExpressionGen to use (parser production from the codebase) leftHandExpression, and instead change validLHS to correspond to (ecmascript spec production) LeftHandSideExpression (capital L)

@achudnov
Copy link
Member Author

achudnov commented Sep 6, 2013

Understood. I'll be careful with the parser. The thing is, restricting the left-hand-side expression by the AST constructor is not an option, because even non-LeftHandSide expressions can be targets of an assignment, provided they are in parentheses.

Speaking of left-recursion and choking up on parsing jQuery: performance is, indeed, an important characteristic of a parser, though I haven't paid too much attention to it before. @philipnilsson, I understand that you have done some performance benchmarking for language-ecmascript before. Do you think it makes sense to add some performance benchmarks to the library? I'm not sure what's the best way to integrate it in the package (as another test-suite) and which library to use (I hear criterion is real good, but never used it), but if you think you could contribute your benchmarks to language-ecmascript, I think it would be very helpful.

@philipnilsson
Copy link

Not really benchmarks though. The parser used to choke with an out-of-memory error. The performance has been "tuned" to not do that, but not any more.

@achudnov
Copy link
Member Author

achudnov commented Mar 1, 2014

I have finally gotten my head around the solution space for the issue. In short, precise determination of LeftHandSidedness of an expression by pattern matching of the constructor is impossible (see validLHS). That's because there are parentheses that "convert" any expression to a PrimaryExpression and, hence, LeftHandSideExpression. So, validLHS needs additional information: whether the expression was parenthesized in the source text. I think that information should best come from AST annotations, and Expressions produced by parsers (with the exception of the top-level expression parser) should have extended annotations that include that information. I've started refactoring the code to accommodate that.

Specifically, the code in ParserState needs to be refactored. Need to abstract from the type of the annotation in withPos and similar. I thought of using lens to scrap all the boilerplate with datatypes, but decided against it as it's quite a heavyweight library. I'll probably just bake a few simple lenses myself.

achudnov added a commit that referenced this issue May 3, 2014
…etermine whether they were parenthesized. Also started abstracting withPos and similar to accept rich annotations
@achudnov
Copy link
Member Author

I'm not sure fixing this makes sense anymore: the ECMAScript 6 standard now imposes additional syntactic restrictions (IsValidSimpleAssignmentTarget in Static Semantics) that seem to be equivalent to the restrictions we already have. While we are not working on a extending the parser to ECMAScript 6, being too pedantic about the 5th edition of the specification makes no sense if the newer edition already imposes those restrictions. I'll give a few weeks for comments. If noone opposes, I'll close this issue as a WONTFIX.

@philipnilsson
Copy link

Agreed.

@achudnov achudnov removed this from the ECMAScript 5 support milestone Dec 1, 2015
@achudnov achudnov closed this as completed Dec 1, 2015
achudnov added a commit that referenced this issue Dec 1, 2015
restrictions. So, the test case is no longer valid.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants