GeezerVison

I assume that if we need to update this, we'll know because some other code won't be able to compile?

To add a method to visitEachChild (and potentially forEachChild), we'd have to add the node to this union.

This is maybe a silly question, but are we sure the perf boost here isn't only because the new code never calls Debug.type?

Debug.type() had almost no impact on perf when I added it. It's a noop, so inlining pretty much results in elimination. It definitely wouldn't account for a 3% difference in emit time.

Fair enough, just checking!

What happens if someone adds a new syntax kind and forgets to update this? Seems like their new node might not emit its children?

AFAIK that's what would have happened in the old code too, because it'd hit the default case to just do nothing.

If both this and #50225 use HasChildren, then you wouldn't be able to define a forEachChild branch or visitEachChild branch without adding an entry, and if you add an entry to HasChildren you would have compile time errors if you forget to add branches to forEachChild and visitEachChild.

Isn't the input possibly unrelated to the output? Jake Bailey (@jakebailey) noticed something like this while converting to SFT.

It totally is. In this case, the implementation fails under SFT because it doesn't overlap with its overloads, except that we don't properly check that without SFT which is why this weird narrower implementation works.

Changing the signature to match the overload is totally fine; no worse than what we have now and users won't see the difference. Actual SFT support and a corrected signature will have to come later when we're ready for a break.

I'm asserting visitEachChildTable[node.kind] as VisitEachChildFunction<T> | undefined. It's difficult to prove this in the type system, but VisitEachChildTable[T["kind"]] should be VisitEachChildFunction<T> | undefined. This isn't perfect, but its better than any.

We also don't have a great way of expressing the existing overloads in the type system. visitEachChild<BinaryExpressionButWithExtraTypeInfo>(...) would say it produces a BinaryExpressionButWithExtraTypeInfo, but would actually produce a BinaryExpressionButWithExtraTypeInfo | BinaryExpression depending on whether we updated the node. Unless we introduce a big ADT union of all possible Node subtypes, I don't think this can be improved.

Hm, unless I'm mistaken, the transform API is used heavily to completely and entirely replace nodes when visiting with... whatever, even nodes of differing types. So, I would expect that a fixed signature would instead have an overload with a more specific return first, but also a more generic "it's just a node" when the result can't be inferred.

But, that's offtopic.

visitEachChild always returns the same type of node passed as the first argument. You can only update child nodes using the supplied visitor.

Ah, oops, I think Daniel and I are thinking of visitNode and visitNodes, which do have a bogus signature.

It is and it isn't. visitNode is really supposed to be used with a test parameter, such that you validate the result is a T when visiting a class of nodes such as Statement, Expression, etc. The test is optional, but really shouldn't be. Unfortunately, making test a required argument would probably be a breaking change, and we'd have to expose some of the higher-level node tests (i.e., isStatement, isExpression, isClassElement, etc.) in the public API that are currently internal.