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

Added Mozilla-compliant AST specification #14

Closed
wants to merge 3 commits into from
Closed

Added Mozilla-compliant AST specification #14

wants to merge 3 commits into from

Conversation

RReverser
Copy link
Contributor

Generic ECMAScript+JSX parsers and some other tools use extended Mozilla AST format as intermediate representation.

This PR describes those extensions for new spec consumers that want to work with AST.

@sebmarkbage
Copy link
Contributor

We should probably change these to use the JSX prefix instead of the XJS prefix. The XJS prefix was really just a legacy artifact.

@sebmarkbage
Copy link
Contributor

Do you think that we should make this a stand-alone document that we refer to as normative? It's technically not necessary to comply with this AST to comply with the spec. It is a normative recommendation though.

argument: Expression;
}

interface XJSSpreadAttribute <: SpreadElement {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure we should extend the SpreadElement here. The React JSX transform currently treats this as a SpreadProperty as proposed here: https://github.com/sebmarkbage/ecmascript-rest-spread/blob/master/Spread.md

It is intentionally vague whether this should be an array spread or an object spread. A transform can chose which semantics this spread should have.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

SpreadElement is general interface for destructuring multiple elements to pattern (...expression syntax), whether it's inside ArrayPattern, CallExpression or, as you wish, ObjectPattern.

Copy link
Contributor

Choose a reason for hiding this comment

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

The spread in CallExpression is not called SpreadElement in the ES6 spec. It doesn't have a name. The destructuring cases are called BindingRestElement or AssignmentRestElement.

I think it was a poor decision to conflate these in the Mozilla AST since they may need to diverge at some point.

In ES terminology an Element is the term for an item in a list where as a Property is an item in an object.

I think that the property case will likely need to diverge at some point when it gets decorators and stuff. This is why I chose to call them SpreadProperty in our esprima AST: https://github.com/facebook/esprima/blob/fb-harmony/esprima.js#L179

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The spread in CallExpression is not called SpreadElement in the ES6 spec.

Looks like here comes root of our misunderstanding.

ES6 spec !== Mozilla AST format spec (and type names are not even supposed to match - check out acornjs/acorn#113, there were some discussions and explanations on this).

Reflect.parse, Esprima#harmony (incl. Esprima-FB) and Acorn all use SpreadElement as representing for argument in call expression, array literal, whatever. So I think it makes sense to reuse it as parent class in SpreadAttribute, too.

Copy link
Contributor

Choose a reason for hiding this comment

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

The question is, will they use it for ES7 SpreadProperty? We don't and doing that seems bad for future compatibility.

Copy link
Contributor

Choose a reason for hiding this comment

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

Well, my thinking is that you can add things like decorators to properties.

{ @foo f: x, @bar ...y }

or other syntax that gets added. That could be wrapped, or you can expand the existing node with decorators.

I guess that thing could extend SpreadElement but then SpreadElement can't change independently. I guess I'm fine keeping this as XJSSpreadAttribute <: SpreadElement until someone settles on what to do with SpreadProperty in the Mozilla AST.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed. So do you want me to fix merge conflicts in order to merge this PR?

Copy link
Contributor

Choose a reason for hiding this comment

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

Let make the AST a stand-alone document in the same repo that we link to, from the other one. We can use strong words to encourage everyone to follow. If/when the Mozilla's AST is in ECMAScript we can merge the documents. I think for now it's clearer that README.md extends ECMA-262 and AST.md extends Mozilla's AST.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@sebmarkbage I'd like to create it as separate document, too, but I wasn't sure how to create such file that could be visible on site as separate page, so @vjeux recommended to just add this to main page for now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@sebmarkbage Got same problem as was in #10 (fork lost link to original repo), so moved to separate file as per request, rebased and submitted as #16.

@benjamn
Copy link

benjamn commented Sep 3, 2014

I'd like to keep this in sync with https://github.com/benjamn/ast-types/blob/master/def/fb-harmony.js, if possible. Even better, we might be able to generate the documentation automatically (see benjamn/ast-types#37).

@RReverser
Copy link
Contributor Author

@sebmarkbage: Do you think that we should make this a stand-alone document that we refer to as normative? It's technically not necessary to comply with this AST to comply with the spec. It is a normative recommendation though.

It can be published as separate document, yes, but I disagree that it's not necessary to treat is as spec.

If we don't, we can get same mess than was in JS world before Mozilla AST became de-facto standard for AST format and a lot of tools started building around it.

Of course, there might be some transpilers or other tools that might not need to use it, or there might be parsers for other languages that have different AST format. However, for ECMAScript-based parsers and tools this should be strict so any adapted or newly created tool (incl. parsers) could refer to same AST specification and their outputs/inputs would be interoperable.

@RReverser
Copy link
Contributor Author

@sebmarkbage: We should probably change these to use the JSX prefix instead of the XJS prefix. The XJS prefix was really just a legacy artifact.

I don't like that prefix as well, but changing it requires a lot of changes in ast-types, react, esprima-fb, acorn-jsx, estraverse-fb and maybe other existing tools. Are you sure it's a right time for that?

@RReverser
Copy link
Contributor Author

@benjamn I'd like to keep this in sync with https://github.com/benjamn/ast-types/blob/master/def/fb-harmony.js, if possible. Even better, we might be able to generate the documentation automatically (see benjamn/ast-types#37).

I like the idea if that's possible. But I don't get XJSText vs Literal battle - why did you write TODO Esprima should return XJSText instead.? Wouldn't it be better to reuse as much types from standard ECMAScript as possible? Or is it just for isString check?

@RReverser
Copy link
Contributor Author

@benjamn How about adding it as hand-written for now and later just replacing section with auto-generated content from your scripts? (tools section will remain handwritten anyway)

@sebmarkbage
Copy link
Contributor

@RReverser everyone responsible to update those tools are already on this thread.
We can just decide to do it right now. A quick search and replace. :)

That way we don't confuse the many other contributors that may come into this later as this grows.

I think we should keep it handwritten so that contributors don't have to get ramped up on another tool chain to contribute to the spec. Generally tooling is downstream from the spec. One less thing to bikeshed.

If the Mozilla AST isn't codegen:ed neither, it seems like it might be useful to write a tool that converts either spec to JS calls.

@RReverser
Copy link
Contributor Author

@sebmarkbage It's not complete list of tools that depend on existing format. Just a bit more examples:

And it's without projects that contain copies of esprima / JSXTransformer inside.

If we do this change, let's at least submit it as major update for our packages so dependencies won't suddenly break.

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.

3 participants