Incorrect `VisitorKeys`

[fisker]

These seems impossible

• ArrowFunctionExpression.id
• FunctionExpression.predicate

https://github.com/prettier/prettier/blob/7df3ed6e3a6d171f7eeaa610b655ac64cd71992c/src/language-js/traverse/visitor-keys.evaluate.js#L33-L35

And these are missing

• NeverTypeAnnotation
• SatisfiesExpression
• TupleTypeAnnotation.elementTypes
• TypePredicate.asserts
• UndefinedTypeAnnotation
• UnknownTypeAnnotation

https://github.com/prettier/prettier/blob/7df3ed6e3a6d171f7eeaa610b655ac64cd71992c/src/language-js/traverse/visitor-keys.evaluate.js#L23-L29

[gkz]

Thanks for the report, we'll take a look

[gkz]

• I'm adding proper parsing support for NeverTypeAnnotation, UndefinedTypeAnnotation, UnknownTypeAnnotation
• I'm changing the prop name for TupleTypeAnnotation from types to elementTypes to match Flow/TS parsers
• @panagosg7 is looking at FunctionExpression.predicate and TypePredicate.asserts
• For ArrowFunctionExpression.id, I believe ArrowFunctionExpression extends Function, so having an id field in the definition makes sense (?), even if it is always null: https://github.com/estree/estree/blob/master/es2015.md#arrowfunctionexpression
• SatisfiesExpression isn't a feature we actually support, we parse some stuff in Flow so that we can have a better error message/quickfix in the IDE - Hermes Parser isn't expected to parse this.

[fisker]

For ArrowFunctionExpression.id,

I'm not sure, but it's not in eslint-visitor-keys, @babel/type, and @typescript-eslint/visitor-keys. I'm fine with keeping it if you think it makes sense.

SatisfiesExpression isn't a feature we actually support, we parse some stuff in Flow so that we can have a better error message/quickfix in the IDE - Hermes Parser isn't expected to parse this.

Good to know.

[gkz]

We generate the visitors from the C++ AST node type defs. For ArrowFunctionExpression.id I'll test out the change in removing the id field and see if the Hermes team wants to make that change (I am on the Flow team) - they have the final call

[fisker]

Can't it be excluded on the JS side? (Or the generate script, not sure how it works, just saying)

[tmikov]

I don't remember why there is an id at all - probably an oversight. I don't think we object to removing it from C++.

[gkz]

@fisker Out of curiosity, for what are you using these visitor keys in Prettier so that an issue appeared?

[fisker]

Long story, we started using them since Prettier v3.

We added support for async parser, to support that, the embed format needs to be async too, then the whole print process needs to be async.
I first made these changes, turns out the performance gets worth.

So we changed embed print to print ahead the main print process, so it can remain sync.

To do so, we need to traverse the AST to know which node should be printed (with another async parser).

Instead of traversing all the properties to know where the children are.
To prevent processing non-node properties such as location, tokens, of cause we can exclude them, but not a generic solution.
I was insistent on using visitor keys back then, and the solution accepted.

Now the visitor keys also help us to find which children no longer exist, already cleaned up lots of outdated code.

[fisker]

You may noticed that I added many comments about possible node types to the print function, same purpose to track what the print function is doing, which part is dead.

[fisker]

Is this 80359d4 removing id from tree? I think we should only remove from visitor keys, otherwise it's not following the ESTree spec.

[gkz]

Is this 80359d4 removing id from tree? I think we should only remove from visitor keys, otherwise it's not following the ESTree spec.

I don't think it always being undefined vs. always being null is a big deal.

Either the field exists and should be possibly visited, or it does not exist.

[gkz]

For FunctionExpression.predicate, we do parse it:

const fn = function f(x: mixed): %checks  {
  return typeof x === "number";
}

But it is a deprecated feature, so it is a Flow error to use. In the future we may remove parsing.

For TypePredicate.asserts, we don't have that field in our definitions anymore, it was replaced with TypePredicate.kind