Node builder reuses type predicate nodes, more often reuses return position nodes #57990
Conversation
typescript-bot
added
Author: Team
For Uncommitted Bug
weswigham
force-pushed
the
preserve-predicate-nodes
branch
from
2b9abbd
to
68b227f
Compare
| }> | Extract<ast.Node98, { | ||
| kind: NodeType; | ||
| }> | Extract<ast.Node99, { | ||
| export declare const isNodeOfType: <NodeType extends ast.SyntaxKind>(nodeType: NodeType) => (node: ast.Node | null | undefined) => node is Extract<ast.Node, { |
There was a problem hiding this comment.
This change is probably the biggest "these kinds of changes [to reuse more input nodes] are obviously improvements for everybody" demonstrator in our test suite.
There was a problem hiding this comment.
Very nice, though I wonder if the crash from this test case was related to the printing or not...
| Type '() => string | number' is not assignable to type '() => number'. | ||
| Type '() => number | string' is not assignable to type '() => number'. | ||
| Type 'string | number' is not assignable to type 'number'. |
There was a problem hiding this comment.
Kind of unfortunate that this order changes to mismatch the error above, but, that's life
There was a problem hiding this comment.
Yeah, the signature has a declaration, so can reuse its' return type node, but the union type itself has no such luck. Now, it doesn't have to be that way (intersections remember their order forever because it matters if there are overloads, unions could do the same even if it only matters for looks) - but we'd produce more type identities.
| return factory.updateParameterDeclaration(node, /*modifiers*/ undefined, node.dotDotDotToken, visitEachChild(node.name, visitExistingNodeTreeSymbols, /*context*/ undefined), node.questionToken, factory.createKeywordTypeNode(SyntaxKind.AnyKeyword), /*initializer*/ undefined); | ||
| } | ||
| if (isNamedDeclaration(node) && node.name.kind === SyntaxKind.ComputedPropertyName && !isLateBindableName(node.name)) { | ||
| return undefined; |
There was a problem hiding this comment.
This is the only new return of undefined; is there a way to avoid it to prevent all of the extra !?
There was a problem hiding this comment.
...for now. There will be more.
There was a problem hiding this comment.
(Right now the node builder's existing node tree reuse is somewhat poor at eliding erroneous nodes from the input - this is something that the top-level declaration emitter currently does more consistently than this. Honestly, ideally this function and visitDeclarationSubtree in the declaration emitter would be largely shared (since they do the same thing: walk an existing node tree and make it fit to emit), but unifying them is going to take a bit - one needs to subsume the other's feature set entirely first, rather than them having overlap, but also unique parts)
| (visited as Mutable<typeof visited>).type = factory.createKeywordTypeNode(SyntaxKind.AnyKeyword); | ||
| if (isParameter(node)) { | ||
| (visited as Mutable<ParameterDeclaration>).modifiers = undefined; |
There was a problem hiding this comment.
Are these guaranteed to be copies? This is the first time we're pulling Mutable into the checker and it feels odd to have to do this.
There was a problem hiding this comment.
Yes - right above we say "if it's not a copy, copy it so we can add a .type safely".
Honestly, this is just the bare minimum needed to keep the tests looking correct while reusing the current set of input nodes. As I mentioned in the last comment, this scenario, of transforming the node to no longer have some kinds of error states in them (erroneous modifiers, missing required type nodes), this function is actually pretty lacking in compared to the declaration emitter's visitDeclarationSubtree which is its' dual. It needs a proper cleanup in that regard, but I don't think we're quite ready for that just yet - testing it won't be easy until we're actually reusing way more nodes via the node builder (which is the plan!).
There was a problem hiding this comment.
Ah, I guess I didn't read that code that way; wasn't sure if one could return an entirely different but not synthesized node. But, makes sense.
There was a problem hiding this comment.
For additional context: visitDeclarationSubtree in declarations.ts and tryReuseExistingTypeNodeHelper in checker.ts have the same goal: take in a type node (or part thereof), and make it fit for emit, per our declaration file rules. Except tryReuseExistingTypeNodeHelper has only really seen extensive use for JS emit - so today it mostly just copies JSDoc type nodes and transforms them, while also doing some reference and visibility tracking. Meanwhile visitDeclarationSubtree doesn't handle JSDoc at all, but beacuse it's been in use longer for declaration emit, handles more edge cases around correcting erroneous input patterns when building the output (and also does some of that same reference tracking).
Eventually we'll merge the two into one implementation used in both places (ideally), but until then, I'm just fixing the things tryReuseExistingTypeNodeHelper is obviously demonstrated as lacking when it starts returning the input node in our tests.
As you can see from the baselines now, this improves how often we reuse nodes by quite a lot.