Replace addUndefined in serializeTypeForDeclaration with the actual declaration #58085
Conversation
typescript-bot
added
Author: Team
For Uncommitted Bug
src/compiler/checker.ts
Outdated
| * Unlike the utilities `setTextRange`, this checks if the `location` we're trying to set on `range` is within the | ||
| * same file as the active context. If not, the range is not applied. This prevents us from copying ranges across files, | ||
| * which will confuse the node printer (as it assumes all node ranges are within the current file). |
There was a problem hiding this comment.
The printer’s single-file context has been a minor nuisance for code fixes and refactors in the past. For example, in an “add missing members from an incomplete implements” fix, we might write method signatures originating in two different files. How much more trouble would it be to fix the underlying assumption that every Node’s originalNode has the same source file?
There was a problem hiding this comment.
It's not even an issue with .original, it's an issue with assumptions about the text range - two of them. One is that, for literal strings and identifiers, unless some conditions are met, the printer just copies the input range to the output for them. AFAIK, this is to save on the escaping and unescaping work... I think. @rbuckton would have stronger opinions on that. The other is the token/node range bounds checking/assertions done in many services helpers. Specifically, we often assert that the last child of a node has the end position of its' parent - even inside the formatter. It's not clear to me that this is actually a good thing to assert on constructed node trees (though it is obviously true for parsed ones), but it is obviously not a good thing to assert on a tree with node ranges from a mixed set of input files, so we'd definitely need to revisit a lot of assertions in a bunch of services methods.
| @@ -638,7 +638,7 @@ export function transformDeclarations(context: TransformationContext) { | |||
| case SyntaxKind.PropertyDeclaration: | |||
| case SyntaxKind.BindingElement: | |||
| case SyntaxKind.VariableDeclaration: | |||
| typeNode = resolver.createTypeOfDeclaration(node, enclosingDeclaration, declarationEmitNodeBuilderFlags, symbolTracker, shouldAddImplicitUndefined); | |||
| typeNode = resolver.createTypeOfDeclaration(node, enclosingDeclaration, declarationEmitNodeBuilderFlags, symbolTracker); | |||
There was a problem hiding this comment.
Do we still need shouldAddImplicitUndefined at this point? I did a diff locally forcing it to false above and it was... almost clean. So I think the answer is "yes" until we fix parameter properties and hopefully our goofy rules about optionalish/requiredish parameters...
There was a problem hiding this comment.
Do we still need shouldAddImplicitUndefined at this point?
Ultimately, I would like to remove it and the EmitResolver API backing it - for me, that was what motivated basically this whole stack of changes (quite literally: I removed it, and then started seeing what needed to be done to minimize the diffs). There's a bit more work to be done to minimize that diff, though - mostly, from what I can tell, around handling errors and duplicated declarations (with structurally-identical-but-identity-unique types) in the node builder more like the top-level declaration emitter does today.
There was a problem hiding this comment.
So I think the answer is "yes" until we fix parameter properties and hopefully our goofy rules about optionalish/requiredish parameters...
Ah, you want to lock it to true, not false, that way it always falls back to the logic within createTypeOfDeclaration (which handles optional properties/parameters), rather than the declaration emitter's visitDeclarationSubtree (which doesn't) - that'll be more representative of what needs to be fixed in the node builder to remove the API.
There was a problem hiding this comment.
I meant false in the code above (I couldn't highlight it in the diff, thanks GitHub), such that type && !shouldAddImplicitUndefined is true and we just copy the nodes (so, more reuse).
There was a problem hiding this comment.
I meant false in the code above (I couldn't highlight it in the diff, thanks GitHub), such that type && !shouldAddImplicitUndefined is true and we just copy the nodes (so, more reuse).
Yeah, exactly, at no point have I changed this case in this or any recent PR, however. The visitDeclarationSubtree codepath always copies nodes, without regard for type information that may indicate otherwise. That's right enough most of the time, without considering the shouldAddImplicitUndefined carve-out. What we care about, IMO, is if the fallback createTypeOfDeclaration calls also always copy the input nodes when they can - because if they do, the entire
if (type && !shouldAddImplicitUndefined) {
return visitNode(type, visitDeclarationSubtree, context);
}branch can be removed, as createTypeOfDeclaration will internally have all the same logic (and it needs to anyway, in order to handle copying subtrees of input nodes within the nodes it generates). After this change, the only issues blocking that, AFAIK, are that createTypeOfDeclaration over-invalidates in the presence of an error type/structurally identical types across multiple declarations, and discards input nodes it could probably safely reuse.
The small handful of non- |
| //// [b.d.ts] | ||
| export declare const g: <T>(x: T, y: globalThis.NoInfer<T>) => T; | ||
| export declare const g: <T>(x: T, y: NoInfer<T>) => T; |
|
@typescript-bot perf test this Just checkin |
|
@jakebailey Here they are:
tscComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
tsserverComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
startupComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
Developer Information: |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment.
Seems good to me so long as we're not totally throwing off @dragomirtitian :D
Definitely merge main first, as there's no way this isn't already out of date from recent test additions.
Which, given the name of the function, makes sense. This also allows the emit pipeline to prefer a specific declaration to pull type nodes from (which matters when there are duplicate declarations, for example).
This has quite a big diff to baselines (largely in the "good" direction), as we start to copy the input declaration more reliably in many situations.
In addition to just changing the API, this also has to include a fix to apply
.singleQuotepreference when copying input nodes (to minimize fourslash changes, as we reuse more nodes - this change has no baseline diff on its own, so has to be here), and to strip position information (but not origin node) from copied nodes when those positions are not in the current file (to fix a quickfix crash related to reusing nodes from outside the current file, which doesn't occur in the tests (but is definitely still a bug which could crop up) until this API change is applied).