Repository navigation
Stop building invalid type syntax for hover and code fixes - #65502
Jake Bailey (jakebailey) wants to merge 7 commits into
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
Repeated generic-symbol hovers retain display declarations in the cached builder, and ordinary qualified generic types lose module/class classification in hover text.
2 open issues
What changed in this PR
This PR separates hover-only display notation from the native compiler’s syntax trees, keeping nodes valid for language-service code fixes and API consumers.
Changes:
- Adds display metadata for the printer instead of embedding noncanonical syntax in type nodes.
- Uses temporary builders for display-oriented type formatting and constructs valid qualified names for
typeofannotations. - Tightens type-node classification and adds checker, code-fix, and API regression tests.
| File | Description |
|---|---|
tsc/internal/printer/printer.go |
Prints display substitutions and signature type arguments. |
tsc/internal/printer/factory.go |
Converts access expressions into valid entity names. |
tsc/internal/printer/emitcontext.go |
Stores and copies display-only metadata. |
tsc/internal/ls/codeactions_fixmissingtypeannotation.go |
Builds canonical names for typeof annotations. |
tsc/internal/fourslash/tests/codeFixMissingTypeAnnotationOnExports34-object-spread_test.go |
Tests annotations for qualified object spreads. |
tsc/internal/checker/printer.go |
Selects display-formatting builders and preserves valid API signatures. |
tsc/internal/checker/nodebuilderimpl.go |
Separates display notation from canonical type nodes. |
tsc/internal/checker/nodebuilder.go |
Adds operation-local builders for display-oriented type formatting. |
tsc/internal/checker/checker_test.go |
Tests display-only generic parameter notation. |
tsc/internal/ast/utilities.go |
Excludes instantiation expressions from type-node classification. |
tsc/internal/ast/precedence.go |
Removes precedence handling for noncanonical type syntax. |
packages/typescript/test/sync/api.test.ts |
Adds synchronous API syntax-validity tests. |
packages/typescript/test/async/api.test.ts |
Adds asynchronous API syntax-validity tests. |
packages/typescript/src/ast/is.ts |
Aligns JavaScript type-node classification with the compiler. |
Review effort: Balanced
The node builder and the missing-type-annotation code fix could build type nodes containing syntax that is not valid in those positions: expressions in place of types, property-access expressions as typeof names, type parameter declarations in type argument lists, and type arguments in type parameter lists. These trees reach API consumers and every caller that expects valid syntax, so the printer accepted property accesses as entity names, and instantiation expressions were classified as type nodes to let them through. Most of this came from notation that hover shows but that is not valid type syntax: f<string>.C for a class local to a generic function, outer type parameters with their constraints and defaults in names such as C<out T extends string = "a">.prototype, and instantiated arguments in place of a signature's type parameters, as in <string>(value: string). Build valid syntax instead, and record the hover notation as emit-context metadata that the printer substitutes. An instantiated signature still declares its remaining type parameters, but those declarations are not printed, so they do not count toward the truncation length. Hover formatting uses a builder local to the operation so that its display metadata is not retained by the checker's cached builder.
977dabf to
70f413c
Compare
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved findings identify accumulating retained emit nodes during repeated generic-symbol hovers and lost symbol mappings in cloned display subtrees, which degrade classified hover output.
1 open issue
2 resolved since last review
Review effort: Balanced
Classified hover colors each name by looking up the symbol recorded for its identifier. Display notation such as foo<void>.X.m<number, boolean>.(Anonymous function)<Date>.Y<string> is assembled from copies of earlier display expressions, and those copies had no recorded symbols, so most of the names printed as plain text. Copy the symbols along with every identifier in a copied display expression.
Constrained and defaulted outer type parameters, as in D<out T extends string = "a">.prototype, appear only in hover text, so check that text directly. The removed API and checker tests checked internal tree structure, and the hover baselines already cover it.
Display expressions are synthesized and only printed, so they can share nodes with each other and with the type tree. Sharing keeps each identifier's recorded symbol for classified hover without copying. Instantiated signatures used for display print their arguments in place of the type parameters, so build the arguments alone for them. Hover is the only consumer; the checker's public entry points clear the hover-only flags. The printer prints a display type parameter through its type reference, so type argument lists need no separate hook.
isTypeNode rejects instantiation expressions, so inlay hints no longer need to exclude them separately. symbolToTypeParameterDeclarations is the only caller of its helper, so it takes the helper's body.
The checker reuses one node builder for type, symbol and signature formatting. Display entries recorded on its emit context stayed for the checker's lifetime, so repeated hovers kept adding to it. Store display entries in their own table and clear it when the outermost operation releases the builder. Serialized types that use display notation are not cached, because a cached copy would outlive those entries. Every formatting path can then share the builder.
There was a problem hiding this comment.
🔵 Needs a closer look
Human assessment is needed because unpublished findings identify a critical closure-narrowing regression, retained hover metadata, and omitted generic type arguments in hover text.
No open issues 🎉
1 resolved since last review
Review effort: Max

This gets rid of all the places in the LS that we break the AST rules and smuggle in data for nicer printing.
Now, there's a consolidated way to do this on the emit resolver.
To make this not leak memory on the checker's node builder, the LS uses a separate temporary builder. It turns out that the caching that the checker's node builder did was pointless because things like hover verbosity meant that the queries were uncached anyway.