Repository navigation
Reuse diagnostics checker for semantic tokens of checked files - #65411
Andrea Cappuccio (hood) wants to merge 2 commits into
Conversation
Semantic tokens ran on a cold query checker and re-inferred every variable, parameter and property type that diagnostics had just computed. Once a diagnostics request has fully checked a file and finished, lend the idle diagnostics checker to semantic token requests for that file instead. They never wait for it and never run before the file is checked, so the diagnostics checker's walk order is unchanged.
There was a problem hiding this comment.
🟡 Changes recommended
A critical request-affinity tracking gap allows checker reuse before supplemental-file declaration diagnostics finish, and semantic-token reuse still lacks integration regression coverage.
2 open issues
What changed in this PR
This PR addresses #65386 by letting semantic-token requests reuse an idle diagnostics checker for fully checked files, reducing repeated type inference while preserving query-checker fallback.
Changes:
- Adds a checked-file query lifetime for full and range semantic tokens.
- Gates checker reuse on completed checks, availability, and in-flight diagnostics tracking.
- Adds pool tests covering reuse and fallback behavior.
| File | Description |
|---|---|
tsc/internal/project/checkerpool.go |
Implements guarded diagnostics-checker lending and request tracking. |
tsc/internal/project/checkerpool_test.go |
Tests checker reuse eligibility and fallback. |
tsc/internal/ls/semantictokens.go |
Requests checked-file reuse for full and range tokens. |
tsc/internal/core/context.go |
Defines the checked-file query lifetime. |
tsc/internal/checker/exports.go |
Exposes whether a file has completed checking without cancellation. |
Review effort: Balanced
| if file != nil { | ||
| p.diagnosticsRequests[requestID] = file |
There was a problem hiding this comment.
Good catch clanker, the affinity path returned before recording anything, so only the canonical file was tracked. Instead of tracking files per request I made it stricter: the pool now keeps the set of in-flight diagnostics request IDs (cleared by the same ctx AfterFunc as requestAssociations) and won't lend the checker at all while any of them is in flight. That covers supplemental files for free and kinda simplifies things, also reverts the getDiagnosticsChecker signature change.
TestCheckerPoolCheckedFileQuery now checks two files under one diagnostics request and asserts neither can borrow the checker until the request is done (fails if the guard is removed). Fixed in 9afed7b.
| } | ||
|
|
||
| func (l *LanguageService) ProvideSemanticTokens(ctx context.Context, documentURI lsproto.DocumentUri) (lsproto.SemanticTokensResponse, error) { | ||
| ctx = core.WithCheckerLifetime(ctx, core.CheckerLifetimeCheckedFileQuery) |
There was a problem hiding this comment.
Added TestSemanticTokensReuseCheckedDiagnosticsChecker in 9afed7b. It runs the #51661 repro through the real session + language service: semantic tokens on a query checker, then diagnostics, then semantic tokens again. It asserts the second request actually took the lending path (hooked on the pool's log, so it can't pass by falling back), that the tokens are identical to the query-checker ones, and that diagnostics are still empty and unchanged afterwards.
Range tokens go through the exact same GetChecker path, so I didn't duplicate the test for them tbh, happy to add it if you prefer.
|
This is generally unsafe due to ordering issues, hence diag checkers being separated. Somewhat what I'm trying to address once and for all in #65064 |
Track in-flight diagnostics requests by ID instead of by first file, so supplemental files checked under the same request are covered too, and add an end-to-end test that semantic tokens reuse the checker without changing tokens or diagnostics.
|
Yeah that's fair, tbh I couldn't fully convince myself either: even with the guard, classification still creates a handful of types/symbols on the borrowed checker (~6/32 per request on recharts' Converting this to a draft and parking it behind #65064 (happy to just close it if you prefer). If order independence lands, the payoff is roughly: in VS Code's request order, semantic tokens go 41 → 3 ms per request and ~41% less CPU while typing, since today each edit type-checks the file twice. If it's useful, I've got a small LSP typing harness (real keystrokes on a real repo, pulls diagnostics/semantic tokens/completions per key, dumps responses for byte-identity diffs): https://gist.github.com/hood/506e87027c83bbec1b44c1f687273349. Happy to run it against #65064 to help shake out ordering differences. |
Fixes #65386
Semantic tokens used to run on a cold query checker and re-infer basically every variable, parameter and property type in the file, the same work diagnostics had just done on the diagnostics checker. With this PR, once a diagnostics request has fully checked a file and is done, semantic tokens for that file borrow the idle diagnostics checker instead.
The rules are kinda strict so the diagnostics checker's walk order doesn't change:
diagSem), the file is fully checked (typeChecked+unusedChecked, not cancelled) and no diagnostics request for that file is still in flight (they acquire it three times: semantic, suggestion, declaration);That ordering is what avoids stuff like #51661, where a query asked for a type before the normal walk and you'd get an order-dependent false error in the editor.
VSCode pulls diagnostics right on change (
diagnosticPullOptions.onChange) and only asks for semantic tokens ≥300 ms later (REQUEST_MIN_DELAY), so in practice the reuse path is the common one.src/cartesian/Line.tsx, 120 keystrokes, 3 runsmainsemanticTokens/fullmediantextDocument/diagnosticmedianTests:
TestCheckerPoolCheckedFileQuerycovers no reuse before the file is checked, no reuse while the diagnostics request is still in flight, reuse after, and fallback while the checker is lent out. It fails if the in-flight guard gets removed.Line.tsxand three other open files (TS2883 type printing, unused suggestions, the transient errors you get mid-typing) are byte-identical tomainacross 600 responses, both sequential and concurrent.go test ./...,npx hereby lintandnpx hereby check:formatpass.One thing I'm not 100% sure about: after the file is checked, classification still creates a few types/symbols on the diagnostics checker (~6 types and 32 symbols per request here, looks like throwaway re-checks of expressions). I didn't see any diagnostics change, but if keeping that checker totally free of query work is a hard requirement this approach doesn't fit, so happy to go another way.
AI disclosure: I polished my solution, my breakdown of it and -most importantly- did most of the measurements with help from an AI agent. I've gone 2x through everything and I'll handle any follow-up myself ofc.