Skip to content

Stop building invalid type syntax for hover and code fixes - #65502

Draft
Jake Bailey (jakebailey) wants to merge 7 commits into
microsoft:mainfrom
jakebailey:compiler-fixes/hover-display-metadata
Draft

Jake Bailey (jakebailey) wants to merge 7 commits into
microsoft:mainfrom
jakebailey:compiler-fixes/hover-display-metadata

Conversation

@jakebailey

Copy link
Copy Markdown
Member

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.

Copilot AI balanced review requested due to automatic review settings November 14, 2026 06:58
@github-project-automation github-project-automation Bot moved this to Not started in PR Backlog Nov 14, 2026
@typescript-automation typescript-automation Bot added Author: Team For Uncommitted Bug PR for untriaged, rejected, closed or missing bug labels Nov 14, 2026

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 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 typeof annotations.
  • 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

Comment thread tsc/internal/checker/nodebuilderimpl.go Resolved
Comment thread tsc/internal/checker/nodebuilderimpl.go Resolved
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.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 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

Comment thread tsc/internal/checker/nodebuilderimpl.go Resolved
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.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Needs a closer look

Symbol formatting still uses a cached builder, causing repeated hovers to accumulate display trees that remain reachable for the checker’s lifetime.

No open issues 🎉

1 resolved since last review

Review effort: Balanced

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.
@jakebailey
Jake Bailey (jakebailey) marked this pull request as draft November 14, 2026 17:01
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.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

Symbol-name formatting still uses the cached builder, causing repeated generic-member hovers to accumulate display metadata in an emitNodes map that ReleaseArenas() does not clear.

1 open issue

Review effort: Max

Comment thread tsc/internal/checker/nodebuilder.go Resolved
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.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 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 branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Author: Team For Uncommitted Bug PR for untriaged, rejected, closed or missing bug

Projects

Status: Not started

Development

Successfully merging this pull request may close these issues.

2 participants

Sponsor
SponsoredKunjungi sekarang
Promo