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 |
| if (sourceFileWithoutResolvedSignatureCaching && symbol.valueDeclaration && (isFunctionExpressionOrArrowFunction(symbol.valueDeclaration) || tryGetRootParameterDeclaration(symbol.valueDeclaration)) && getSourceFileOfNode(symbol.valueDeclaration) === sourceFileWithoutResolvedSignatureCaching) { | ||
| symbolLinksWithoutResolvedSignatureCaching ??= []; | ||
| 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); | |
| } |
Copilot uses AI. Check for mistakes.
| nodeLinksWithoutResolvedSignatureCaching ??= []; | ||
| return nodeLinksWithoutResolvedSignatureCaching[nodeId] ??= new (NodeLinks as any)(); | ||
| } | ||
| 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.
Copilot uses AI. Check for mistakes.
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.