Ensure no-overlap between Flow and TS node types

[jamiebuilds]

Resolves #709

[xtuc]

LGTM for Babylon. We need to change babel-generator with the new Node names.

Do you think some users could already rely on these nodes?

[hzoo]

@xtuc no it's alpha period so definetely would be part of the try it out/warning don't rely on this kinda of deal (in terms of using the preset not using the nodes). It's not something to worry about right now 🙂

[jamiebuilds]

Just to note: Without this change it is very difficult to build tools that support both Flow and TypeScript, which I'm currently trying to do so it would be good to get this change in sooner rather than later.

[danez]

I don't want to argue against this, as I think it might make sense, but in your case couldn't you simply check if either the typescript or the flow plugin is active? As far as I remember they can't be active at the same time anyway.

[jamiebuilds]

Yes, as I mentioned in the original issue, if you are working in the context of babel-traverse (which is properly set up), you can check path.hub.file.opts.parserOpts.plugins. But that is brittle and you often do not have access to a NodePath, let alone the parser options.

[JamesHenry]

I would be 👎 as it stands. I will elaborate in a comment so it is easier to reply.

[JamesHenry]

Andy and I worked together to align on using existing node type precedents from Flow where possible, and where they are syntactically consistent across Flow and TypeScript.

This was something that @nzakas was also very keen to do right from the get go when introducing type information into ESTree related parsers.

Of particular note is the fact that TypeAnnotation nodes have officially been accepted as part of an extension to ESTree:

https://github.com/estree/estree/blob/master/extensions/type-annotations.md

I realise Babylon is no longer exactly ESTree, but it is still within that "family", and shouldn't diverge any more than is absolutely necessary. Therefore, at the very least, it would not make sense to introduce a TSTypeAnnotation IMO.

If two things are syntactically equivalent across Flow and TypeScript, why would we need a fundamentally different AST? If there were differences in semantics for the same syntax, that would be applied later by the relevant parser/tooling within that context.

The more the ASTs diverge, the harder it would be to assess pieces of consistent syntax and look to standardise them in future, like was done with TypeAnnotation, for example.

Just to note: Without this change it is very difficult to build tools that support both Flow and TypeScript, which I'm currently trying to do so it would be good to get this change in sooner rather than later.

Please can you elaborate on specific issues you are facing? We have managed to do exactly this with Prettier. I am not saying the overlap in node types made it easier to achieve, but I would be interested to know the exact problems you are facing.

Perhaps with specific cases to hand, it may turn out that there are currently cases where we have the same AST where we fundamentally shouldn't, and the solution would be to just diverge/namespace those specific instances rather than apply it to absolutely everything.

[jamiebuilds]

using existing node type precedents from Flow where possible, and where they are syntactically consistent across Flow and TypeScript.

Seeing that there is no shared standard between the two grammars, and no guarantee that both will remain aligned, it does not make sense to try uniting the two grammars.

The TypeAnnotation node in ESTree is hardly a shared standard, it's currently defined as:

interface TypeAnnotation <: Node { }

The most you can really say about type annotations in ESTree is that there's some reserved space for them which is the typeAnnotation field on patterns and returnType on Functions. Which is fine, I'm not suggesting that we use different fields on JavaScript node types.

As it stands, when you are given a raw Babylon AST, there is no way to know which grammars are contained within it except for when you discover a node that belongs to a particular grammar (which is a thing worth preserving in the AST as it means you don't need context besides maybe the parent node to understand a node).

Perhaps with specific cases to hand, it may turn out that there are currently cases where we have the same AST where we fundamentally shouldn't, and the solution would be to just diverge/namespace those specific instances rather than apply it to absolutely everything

We don't have a lot of opportunities to make changes to the Babylon AST, we should not be changing things frequently.

If we can't look at an individual node and say which grammar it came from, we should treat that as a bug.

I am not saying the overlap in node types made it easier to achieve

So why even attempt at merging types?

Please can you elaborate on specific issues you are facing?

Type annotations within Flow and TypeScript have different behavior, I need to mimic that behavior in tooling that I am building. Which I'd get into further, but based on previous interactions with TypeScript team members I'm not going to.

[danez]

What if we add a field to program indicating which typing grammar was used?

{
"program": {
    "type": "Program",
    "sourceType": "module",
    "typeExtension": "flow/ts",
    "body": []
}

[xtuc]

I agree with @JamesHenry, having a generic TypeAnnotation node is very powerful.

It seems that the solution proposed by @danez could solve @thejameskyle issue? It wouldn't work with mixed TS and Flow annotations but that doesn't work anyway.

If we want to have the information at a node granularity, we can store the typeExtension in the node.

[jamiebuilds]

No, we should not require context outside of an individual node or its parent to determine what the node is. Scenarios like that make an AST crap to work with, it's already super annoying the places in ESTree/Babylon where you need to look at the surrounding nodes to know where you are in the AST. This is (in part) why the Shape AST is so great to work with and why ESTree is annoying.

There is no reason we should be adding these hacks into our AST to work around bad design if we have the opportunity to do it well from the start.

[JamesHenry]

Excuse the really, really long comment, but I just feel some further background to the status quo would be useful:

typescript-eslint-parser has been around for quite a long time and effectively was the first tool to try and define TypeScript nodes in an ESTree (@thejameskyle you wrote ESLint a few times, where I think you meant ESTree) compliant way.

Naturally, there is a lot of syntax which does not have precedent in ESTree, but there was, however, a lot of identical syntax in Flow already and so its ESTree-like node structure made a lot of sense to use as a template. In cases where there was no precedent in ESTree or Flow, we simply had to create something from scratch which was ESTree-like.

Fast forward a bit and I used typescript-eslint-parser to allow for TypeScript support to be added to Prettier. It is naturally currently built to look for and reprint the TypeScript nodes set by typescript-eslint-parser. (@azz would be a good person to comment on the pros and cons of supporting TypeScript and Flow nodes in the same codebase as he has done far more work on the actual printing of nodes than I have.)

Fast forward even further and the TypeScript team decided to dedicate time and energy into parsing TypeScript code directly with Babylon, Andy has done a great job on this. Of course, it made sense for the couple of years of existing precedent from typescript-eslint-parser not to be ignored during this process, and I was keen to work with Andy to align on the AST that babylon and typescript-eslint-parser produce and we are now very close.

My long-term hope is that this effort will allow for typescript-eslint-parser to use babylon behind the scenes, instead of parsing with the TypeScript compiler and converting the AST itself and that babylon would then power the TypeScript parsing in Prettier as well. It will involve throwing a lot of my own code away, but I am committed to avoiding competing standards wherever possible and I think that is the best way.

The reason for providing this backstory is just to illustrate that this PR has a big impact on all of that alignment work and future integration work into Prettier and ESLint plugins, so it would be great to take some time to discuss it.

---

We don't have a lot of opportunities to make changes to the Babylon AST, we should not be changing things frequently.

I definitely agree, and we should not rush the changes through either.

Seeing that there is no shared standard between the two grammars, and no guarantee that both will remain aligned, it does not make sense to try uniting the two grammars.

The TypeAnnotation node in ESLint is hardly a shared standard

I feel like this is a bit of a chicken and egg situation - before this very recent effort by Andy and me, had anybody actually taken the time to try and identify commonalities in syntax across the full "languages"? That initial extension to ESTree was very small by design, because there was not enough information to hand to standardise more things. I believe we now have that information and some buy-in from Flow and TypeScript team members could actually make standardisation of overlapping syntax a possibility.

Type annotations within Flow and TypeScript have different behavior, I need to mimic that behavior in tooling that I am building. Which I'd get into further, but based on previous interactions with TypeScript team members I'm not going to.

I am a bit confused by this, as you are currently not interacting with any TypeScript team members on this PR and I don't believe you and I have ever directly interacted before. I think requesting specifics about the issues you are facing is a reasonable thing to do on an important PR like this. There may be other solutions to the problem, such as the one suggested by @danez.

You are a far more important member/contributor to the Babel ecosystem than I am, and I have huge respect for the work you do. I am not trying to hold up your PR for the sake of it, I am just providing all the context I can (which you would not necessarily be expected to know about) and trying to help ensure we consider all the options.

All of the tools (typescript-eslint-parser, ESLint plugins, Prettier etc) can naturally all be updated for the right solution. I am not at all against making further changes, but so far all the context you have provided is that you are building a tool and that you would like this to be the solution to some issues you are facing. It would be great to get a bit more info from you.

[jamiebuilds]

@hzoo can you tell me what I need to do to get the tests in CI running with an updated copy of Babel with these changes applied?

[rstacruz]

hey, somewhat off-topic, but:

Without this change it is very difficult to build tools that support both Flow and TypeScript, which I'm currently trying to do so it would be good to get this change in sooner rather than later.

May I ask what you're building? Sounds awesome!

[azz]

For what it's worth, as an AST consumer, so far I have preferred it when nodes are shared between Flow and TypeScript. For the most part, once a TypeScript AST has been converted via typescript-eslint-parser, we can print it in prettier with the same code path as Flow. There are very few cases (only one from a quick search, relating to directives) where we've had to check if the name of the parser is "typescript" to make a decision on how to re-print the AST.

In contrast, if new AST nodes are introduced, we'd have to add extra ||s or cases that line up with the existing flow nodes, and we'd inevitably miss a few cases in the future.

In a sense I look at the node re-use as a form of polymorphism. If it is possible to provide the context required to perform edge-case decisions, why not go for that option?

No, we should not require context outside of an individual node or its parent to determine what the node is.

node.type tells you what the node is. A TypeAnnotation is a type annotation. What you're after is what language the node comes from, which seems outside the scope of a node to provide.

Sometimes you need more than the node and its parent. You might need to know the source type, if the current scope is strict mode, even in some cases you might need the extension (TypeScript acts differently depending on .ts and .d.ts).

[jamiebuilds]

If people are going to respond to this thread, please take the time to understand what has already been said.

1. Just because two nodes are the same syntactically does not mean they are semantically the same
2. Just because two nodes are semantically the same today does not mean they will always be, there is no shared standard between TypeScript and Flow, and Flow has explicitly said they don't want such a standard
3. These places where the nodes are merged are exceptions to the rule. In 99% of the Flow/TS grammars you will need to check different node types. What we have here is highly inconsistent
4. Nodes should not require additional context (besides maybe their parent node) in order to determine what they are
5. We should be optimizing to avoid future breaking changes to the AST
6. We should not try to optimize for a standard that does not exist, has no roadmap, has no buy in, is done by two people on their own without any process
7. There are tools that cannot be built without additional information about the grammar existing in the AST
   a. We should not add something to the root of the AST as that requires additional context (See 4)
   b. We should not add an additional property to the nodes which are shared, because that is highly inconsistent with the rest of the AST
8. ESTree does not formalize the TypeAnnotation definition, it just reserves space for it.
9. ESTree says nothing about the other node types that I have updated in this PR, those decisions were made by TypeScript people by themselves
10. We already have a pattern for how to handle node types that are related to one another in babel-types, we already have node types like t.isFunction which are aliases to more precise node types
11. Having more information in our AST does not get in the way of building tools like Prettier or ESLint, this "inconvenience" is like 3 lines you have to add to manually handle each case. It's silly to optimize in such a small way

This is the right decision and I'll be pushing it forward. I'll update babel-generator next and anything else we need to do

[hzoo]

aside: to fix the babel tests, it links the PR's version of babylon with the master version of babel. If it's an intentional breaking change we would need to either merge a fix in babel master first, or just understand that the failing tests are intentional and merge/release. Maybe it should create a branch off of master each PR so that you could make a change to that branch instead of having to merge master.

And ideally none of this would need to be an issue because I want us to merge this package back in to the monorepo (having to deal with this is one the reasons) https://github.com/babel/notes/blob/master/2017-08/aug-30.md#discussion-mostly-maintainence-related

[loganfsmyth]

From my point of view, I think it does make sense to have these be different node types.

@JamesHenry For your points:

Andy and I worked together to align on using existing node type precedents from Flow where possible, and where they are syntactically consistent across Flow and TypeScript.

This jumps out as unexpected for me. Syntactic similarity on its own, in my mind anyway, is an argument for similarly structured nodes and fields, but not necessarily for using the same node types. We have UnionTypeAnnotation instead of adding a Union BinaryExpression. We have BooleanLiteral and also BooleanLiteralTypeAnnotation.

We all certainly draw the line somewhere, but to me I tend to favor @thejameskyle's approach here. Our goal with the AST is the make it easy to analyze the code itself, and that means the AST should contain all of the information needed to serialize it back to original source code.

A TypeAnnotation is a type annotation. What you're after is what language the node comes from, which seems outside the scope of a node to provide.

Would you be able to dig into what makes you feel this way? To me it seems entirely contrary to the goal of having the AST. If the AST on its own can't represent the file without additional sidechannel data, that seems like a massive problem.

Of particular note is the fact that TypeAnnotation nodes have officially been accepted as part of an extension to ESTree

I don't think this specific argument holds up because doing

interface FlowTypeAnnotation <: TypeAnnotation {}
interface TSTypeAnnotation <: TypeAnnotation {}

would still fit inside that, similar to how ESTree defines Pattern. There's no Pattern node type, but now anything that implements that interface can be used at that location.

If two things are syntactically equivalent across Flow and TypeScript, why would we need a fundamentally different AST? If there were differences in semantics for the same syntax, that would be applied later by the relevant parser/tooling within that context.

I'll second @thejameskyle's point here:

Just because two nodes are semantically the same today does not mean they will always be, there is no shared standard between TypeScript and Flow, and Flow has explicitly said they don't want such a standard

@azz

Sometimes you need more than the node and its parent. You might need to know the source type, if the current scope is strict mode, even in some cases you might need the extension (TypeScript acts differently depending on .ts and .d.ts).

You are right that to properly validate the behavior of some code, you need sourceType and strict. To me it's a balancing act of avoiding an explosion of combinatorial types, while also making the node type distinct within reason. As for .ts vs .d.ts, I can't say I'm familiar. What differences are we talking? I personally would have assumed that .d.ts files would create type-declaration based node types like Flow's DeclareClass and such, which then clearly represents the behavioral differences, making the file extension unnecessary.

@thejameskyle

Type annotations within Flow and TypeScript have different behavior, I need to mimic that behavior in tooling that I am building.

I think a clarifying example for this would help this discussion move along. I'm curious myself.

@JamesHenry What are your thoughts on this and James' other comments.

I love that the two are converging in syntax, but without an official understanding saying that won't deviate, this seems like if anything it would add confusion. It's much clearer to have them be two separate things instead of two almost similar things that deviate when each one decides why not. I'd probably be even happier of this PR included prefixing all of the Flowtype node types with Flow.

[JamesHenry]

@loganfsmyth Thanks for sharing your thoughts, sorry for the delayed response.

I definitely appreciate where you and @thejameskyle are coming from with this, and it has been really useful for me to learn in this thread that the Flow team has explicitly stated they are not open to aligning on things like this. That is vital context. We obviously haven't yet heard any official response from the TypeScript team on this idea, but if Flow is not open to it, then it is effectively dead in the water.

@loganfsmyth At one point when (seemingly) addressing me (with the question Would you be able to dig into what makes you feel this way?), you are quoting @azz, so I won't attempt to answer that question as that seems to just be a mix-up.

I will remove my objection to this PR and lead the efforts to accommodate the knock-on effects in typescript-eslint-parser and prettier.

I would also love to see the Flow nodes updated with a Flow prefix.

[jamiebuilds]

I would also love to see the Flow nodes updated with a Flow prefix.

I would too, I'm afraid of making that big of a breaking change with all the tools that depend on the AST as it is today. I know personally this would cost me several days of working time. I'm personally happy to do that, but there's a lot more people than me.

[jamiebuilds]

For the record I've been working on a tool that internally unifies the Flow and TypeScript ASTs into another more verbose AST designed after the Shift AST. So that could always be exposed if you don't care about the semantic differences between types in Flow and TypeScript.

[DanielRosenwasser]

I think several of us on our team acknowledge that distinct nodes can make some things easier, but most folks on our team would prefer the node shapes continue to align for the benefit of downstream tool authors.

All that said, @andy-ms and I spoke and we're alright with the change. We'll defer to @azz and @JamesHenry who seem to be the other primary consumers at this point. If it's manageable for them, then great, but I know they've put a lot of work into this stuff.

[azz]

I'm fine with it, @thejameskyle's point

We should be optimizing to avoid future breaking changes to the AST

Is the most compelling. It'll take some work on the prettier side to adopt this change, and it may lead to some bugs here and there, but if it prevents breaking changes in the future that's always a win.

We may well end up using @thejameskyles unified AST as prettier doesn't really care about semantics of the TS/Flow AST.

[hzoo]

Ok thanks so much for the discussions everyone, I know hard to balance wanting to keep the same AST, node names, extra code etc but I think it's worth doing too. And shoutout if help is needed for prettier integration after the next release and all that. Btw anyone is free to join in on the Babel meetings to bring up something, etc.

Going to fix the tests locally in Babel and then merge in a bit once I get that figured out.

[existentialism]

I'll do a PR for prettier, have some of it started already.

[hzoo]

Ok babel tests are going to fail until merge in master but it's ok.

[JamesHenry]

@existentialism typescript-eslint-parser will need to be updated before prettier, it does not use babylon for TypeScript parsing.

I have created an issue here: eslint/typescript-eslint-parser#386

[existentialism]

@JamesHenry 👍, I was going to update that (sorry wasn't clear), but if you got it... go for it :)

[JamesHenry]

Working on it now, I'll let you know

[JamesHenry]

The PR is ready eslint/typescript-eslint-parser#388, you can use the PR commit in your WIP for prettier, let me know how you get on!