Fix bugs in AST description

[reverofevil]

ForOfStatement should not inherit from ForInStatement due to incompatible types. It was at some point of time fixed for NewExpression which inherited from CallExpression and had incompatible type's type.
Same for ArrowFunctionExpression that extends Function and has incompatible body type.

Function should be renamed not to clash with Function type of Typescript and Flow.

CallExpression has property named arguments which is a reserved word
MethodDefinition has property named static which is a reserved word
Consider deprecating these names in favor of args and isStatic.

(At least) Class, Function, Node, Pattern, Expression, Declaration and Statement are not AST nodes, and it's not marked in any way. One could have thought that AST nodes are distinguished by having a type, but Node has type and is not an AST node. Consider deprecating these in favor of unions:

type Expression = ThisExpression | ThatExpression | ...;

AssignmentProperty incorrectly inherits from Property. Even though it has type: 'Property', it's a different AST node that accidentally got the same name, and has to be distinguished by context. It can have ObjectPattern in its value, but Pattern requires value to be an Expression. They should probably have common ancestor.

I've attempted to fix a legacy version of estree used for estree/formal project by renaming non-AST nodes to have Abstract in their names.

[loganfsmyth]

It seems like universally my comments below are that you're trying to make assumptions about the ESTree spec that aren't things that have been goals of this project. This project began as a way of taking an existing implementation and specifying it so other projects could aim to conform to it. It is not aimed at being a statically-typed context-free structure, and is not aimed at Flow or TS.

While those certainly are things that it could have aimed for, at this point it would require major breaking changes.

ForOfStatement should not inherit from ForInStatement due to incompatible types. It was at some point of time fixed for NewExpression which inherited from CallExpression and had incompatible type's type.

They are not incompatible because the type is overridden by the ForOfStatement interface.

Same for ArrowFunctionExpression that extends Function and has incompatible body type.

Same here.

Function should be renamed not to clash with Function type of Typescript and Flow.

It is not clear what you mean here. Where do Typescript and Flow come into this picture?

CallExpression has property named arguments which is a reserved word
MethodDefinition has property named static which is a reserved word
Consider deprecating these names in favor of args and isStatic.

Why is this an issue? Even in JS, reserved words are allowed as property names.

(At least) Class, Function, Node, Pattern, Expression, Declaration and Statement are not AST nodes, and it's not marked in any way. One could have thought that AST nodes are distinguished by having a type, but Node has type and is not an AST node. Consider deprecating these in favor of unions:

While the DSL could probably be clearer, I think it's pretty easy to say that any type with an explicit string type: "Foo" is a node type.

AssignmentProperty incorrectly inherits from Property. Even though it has type: 'Property', it's a different AST node that accidentally got the same name, and has to be distinguished by context. It can have ObjectPattern in its value, but Pattern requires value to be an Expression. They should probably have common ancestor.

Context is required in several ways when processing patterns. For instance, var { foo: this.bar } = {}; is a syntax error, but ({ foo: this.bar } = {}); is valid. We would need entirely separate types for binding vs assignment patterns as well. As mentioned above though, this project does not aim to be a full static type system.

[reverofevil]

make assumptions about the ESTree spec that aren't things that have been goals of this project

I didn't go far to look up that it aims to be

a community standard for people involved in building and using these tools

and the fact that developing tools with this standard in mind is a pain is not an assumption. Then I created this issue.

While those certainly are things that it could have aimed for, at this point it would require major breaking changes.

While renaming static to isStatic might be a breaking change, I don't see how any of the mentioned fixes could break anything. The AST is the same, typing is different. Such kind of fixes were already applied.

They are not incompatible because the type is overridden by the ForOfStatement interface.

It's only valid to override a type with a supertype in this covariant context. Current override breaks LSP. If common definition of inheritance is discarded, it should be mentioned in documentation. The proper approach is to have a supertype, the same way as with Class and its descendants.

Same here.

Same here.

It is not clear what you mean here. Where do Typescript and Flow come into this picture?

Where estree becomes practically viable. It's common to build JS processing tools in typed programming languages. It takes no effort to rename Function to something else, given the format won't become any less interoperable with all previous versions. Example here.

Why is this an issue? Even in JS, reserved words are allowed as property names.

This is another unnecessary issue of tool implementation. You cannot name argument or local variable arguments, but you most likely would want to do it once if you had an object with a field arguments.

While the DSL could probably be clearer, I think it's pretty easy to say that any type with an explicit string type: "Foo" is a node type.

If this phrase was normative, I could have arbitrary value {type: 'foo'} in my tree and expect it to be read somehow by any of the tools, because Node also falls into this category (especially for automated tools). There should have been a list of things considered a node, not inheritance from Node. The example was attached.

Context is required in several ways when processing patterns.
this project does not aim to be a full static type system.

Inapplicable here. It's fine to reuse node types. This exact case requires a 1-deep context. Take a look at the example.

[loganfsmyth]

It's only valid to override a type with a supertype in this covariant context. Current override breaks LSP. If common definition of inheritance is discarded, it should be mentioned in documentation. The proper approach is to have a supertype, the same way as with Class and its descendants.

This repository uses a simple DSL to define structural types for the AST. The inheritance described here is purely structural inheritance for the sake of avoiding repetition. It is not a definition of a class hierarchy for AST node types themselves.

There should have been a list of things considered a node, not inheritance from Node. The example was attached.

What specifically would be gained by enumerating this in the specification rather than having it be implicit based on the presence of a string-literal type?

I could have arbitrary value {type: 'foo'} in my tree and expect it to be read somehow by any of the tools, because Node also falls into this category

Again, this assumes that the types in the spec are meant to behave as a type hierarchy for user code, which is not the case. type: string in Node is simply meant as an indication that all nodes will have a type field.

Inapplicable here. It's fine to reuse node types. This exact case requires a 1-deep context. Take a look at the example.

I don't follow what you mean by this.

[reverofevil]

The inheritance described here is purely structural inheritance for the sake of avoiding repetition.

It would be quite unfortunate if people had to find this issue to figure it out. Could you please add description of "structural inheritance" into standard?

What specifically would be gained by enumerating this in the specification rather than having it be implicit based on the presence of a string-literal type?

Ability to use estree in automated tools. How do you generate babel-types code from the grammar in its current state? Node and Class don't help it at all.

Again, this assumes that the types in the spec are meant to behave as a type hierarchy for user code

No, it doesn't assume anything except for what you previously said: a node should have a type. Node is a node that has arbitrary type, thus {type: 'foo'} is a valid node. It has nothing to do with implementation, it's exactly the meaning of what's right now in standard.

type: string in Node is simply meant as an indication that all nodes will have a type field.

It means so only with your interpretation of it. You cannot give automated tools human-readable interpretation and expect it to figure out correct code somehow.

I don't follow what you mean by this.

I mean that in this exact case there has to be a superclass AbstractProperty and its subclasses AssignmentProperty and Property. The same way it's already done with Class, mentioned three times previously and explicitly shown in linked example.

[RReverser]

It would be quite unfortunate if people had to find this issue to figure it out.

The README already says that it uses a custom syntax to describe its structures, so I don't think it's reasonable to expect it to behave like TypeScript or Flow or some other specific language.

Ability to use estree in automated tools.

That's what autogenerated https://github.com/estree/formal is intended for. The ESTree spec itself, like ECMAScript syntax spec, is not intended for machine-readable consumers but rather for implementers (humans).

And if you're looking for just TypeScript checks, then it's best to use version from DefinitelyTyped which tries to address most (all?) of your mentioned concerns in the best way possible for TypeScript specifically.

How do you generate babel-types code from the grammar in its current state?

babel-types couldn't be generated from the grammar either way because Babel doesn't follow ESTree (it uses a custom fork of spec with somewhat different rules).

{type: 'foo'} is a valid node

Yep, generally that's the intention. Whether the tool can handle specific type of node or not is a separate matter, but in practice it's fine to expect anything containing type: string to be an ESTree node. In particular, doing so can help with future-compatibility of tools against new node types.