Fixed more ghost errors after LSP requests caching wrong contextual and parameter types#62184
Conversation
…nd parameter types
There was a problem hiding this comment.
Pull Request Overview
This PR fixes ghost errors that occur after LSP requests (like signature help and completions) cache wrong contextual and parameter types. The fix ensures that cached types/signatures are not incorrectly carried over from LSP requests to regular type checking scenarios.
- Replaces complex ancestry-based cache invalidation with simpler source file-based approach
- Introduces separate cache arrays for nodes/symbols during LSP requests to avoid contaminating regular caches
- Adds comprehensive test cases to verify the fix works for both signature help and completion scenarios
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/compiler/checker.ts | Implements the core fix by using separate cache arrays for LSP requests and checking node types within the source file |
| tests/cases/fourslash/noErrorsAfterSignatureHelpRequestWithinGenericFunction1.ts | Test case verifying no errors after signature help requests in complex generic scenarios |
| tests/cases/fourslash/noErrorsAfterCompletionsRequestWithinGenericFunction4.ts | Test case verifying no errors after completion requests in generic function contexts |
| return symbolLinksWithoutResolvedSignatureCaching[id] ??= new SymbolLinks(); | ||
| } | ||
| return symbolLinks[id] ??= new SymbolLinks(); | ||
| } |
There was a problem hiding this comment.
This condition is complex and difficult to read. Consider extracting it into a well-named helper function like shouldUseSeparateSymbolLinks to improve readability and maintainability.
| } | |
| function getSymbolLinks(symbol: Symbol): SymbolLinks { | |
| if (symbol.flags & SymbolFlags.Transient) return (symbol as TransientSymbol).links; | |
| const id = getSymbolId(symbol); | |
| if (shouldUseSeparateSymbolLinksForSymbol(symbol, sourceFileWithoutResolvedSignatureCaching)) { | |
| symbolLinksWithoutResolvedSignatureCaching ??= []; | |
| return symbolLinksWithoutResolvedSignatureCaching[id] ??= new SymbolLinks(); | |
| } | |
| return symbolLinks[id] ??= new SymbolLinks(); | |
| } | |
| function shouldUseSeparateSymbolLinksForSymbol(symbol: Symbol, sourceFileWithoutResolvedSignatureCaching: SourceFile | undefined): boolean { | |
| return !!(sourceFileWithoutResolvedSignatureCaching && | |
| symbol.valueDeclaration && | |
| (isFunctionExpressionOrArrowFunction(symbol.valueDeclaration) || tryGetRootParameterDeclaration(symbol.valueDeclaration)) && | |
| getSourceFileOfNode(symbol.valueDeclaration) === sourceFileWithoutResolvedSignatureCaching); | |
| } |
| } | ||
| return nodeLinks[nodeId] ??= new (NodeLinks as any)(); | ||
| } | ||
|
|
There was a problem hiding this comment.
This condition is also complex and nearly identical to the one in getSymbolLinks. Consider extracting both conditions into shared helper functions to reduce duplication and improve maintainability.
|
@jakebailey could you build a playground here? |
|
@typescript-bot pack this |
|
Hey @jakebailey, I've packed this into an installable tgz. You can install it for testing by referencing it in your and then running There is also a playground for this build and an npm module you can use via |
…too error-prone" This reverts commit a3ce662.
|
I don't know if this is going to be portable to the new codebase; over there |
…ks-in-whole-source-files-during-lsp
|
With 6.0 out as the final release vehicle for this codebase, we're closing all PRs that don't fit the merge criteria for post-6.0 patches. If you think this was a mistake and this PR fits the post-6.0 patch criteria, please post to the 6.0 iteration issue with details (specifically, which PR and which patch criteria it satisfies). Next steps for PRs:
|
fixes #58351
fixes #62183
supersedes #58378
It's not enough to ignore cached types/signatures on the ancestry path of the location node of the LSP request. All nodes under that ancestry path can potentially cache wrong types and those types should not be carried over to regular checking scenarios.
This PR chooses a simple solution of ignoring node/symbol links on certain node kinds within the source file of the location node. It might ignore more caches than it has to but the implementation stays fairly straighforward thanks to that.