Skip to content

Improve ESTree compliance#383

Merged
adams85 merged 6 commits intomainfrom
improve-estree-compliance
Apr 11, 2023
Merged

Improve ESTree compliance#383
adams85 merged 6 commits intomainfrom
improve-estree-compliance

Conversation

@adams85
Copy link
Collaborator

@adams85 adams85 commented Apr 9, 2023

This PR aims to improve compliance with the ESTree specification.

Besides what have been discussed here, I made an attempt to address a few further issues:

  • In our AST model we don't have separate BinaryOperator/LogicalOperator and UnaryOperator/UpdateOperator enums as specified by the standard but have the separation w.r.t. the corresponding node types (BinaryExpression/LogicalExpression and UnaryExpression/UpdateExpression). However, there were no checks in place to prevent instantiation of these types with a wrong operator type (e.g. a LogicalExpression with a multiplication operator.) Thus, checks have been added to prevent invalid combinations.
  • The standard defines special cases (subclasses) for regex and bigint literals. In the case of bigints, I don't think we'd gain anything from a subclass since we don't really have additional data to store (the bigint property defined by the standard can be replaced with a simple literal.BigIntValue.ToString() call in our current model). However, adding a subclass for the regex case seems beneficial as it's a cleaner design and reduces the memory footprint of the AST a bit. (As a matter of fact, we may also consider changing RegexValue and TemplateElementValue to readonly struct records to save a few further allocations.) I'm not sure about this change though, its impact on existing codebases might be more than acceptable. Please let me know how do you feel about this.

@adams85 adams85 requested a review from lahma April 9, 2023 19:02
@adams85 adams85 linked an issue Apr 9, 2023 that may be closed by this pull request
Copy link
Collaborator

@lahma lahma left a comment

Choose a reason for hiding this comment

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

LGTM, tested migrating Jint and changes were straightforward. Tests green on that side too.

@adams85 adams85 merged commit b6edeea into main Apr 11, 2023
@adams85 adams85 deleted the improve-estree-compliance branch April 11, 2023 18:58
@adams85
Copy link
Collaborator Author

adams85 commented Apr 11, 2023

@lahma Thanks for checking the PR out and verifying it in Jint! 👍

I'd probably release this along with #381 once time is ripe for merging that. But please do as you see fit.

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.

Inconsistencies with ESTree

2 participants