[api] Make binder-produced symbols owned by SourceFiles in the client, like the server and 6.0 - #64518
Conversation
typescript-automation
Bot
added
Author: Team
For Uncommitted Bug
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
File-record lifetime gaps can evict shared symbols, and temporary auto-import symbols may become immediately unresolvable.
Review effort: Balanced
Findings: 1
Open (3)
Resolved since last review (1)
Files not reviewed (1)
- tsc/internal/api/enum_values_generated.go: Generated file
| if symbolOwnerFile(symbol) != nil { | ||
| return newFileSymbolResponse(symbol) |
There was a problem hiding this comment.
This is actually just a more obvious symptom of an existing bug. In main, every completion symbol from the temporary snapshot get registered by the parent snapshot's snapshotData record. That means the caller can get completions, assume the returned symbols belong to the current snapshot, and use the current snapshot's checker on them. If the symbol being operated on is transient, that will probably cause incorrect results or a crash.
If an API method is going to return objects from a new snapshot, that new snapshot must be returned to the client as well, so the objects can be operated on. getCompletionsAtPosition only optionally includes symbols, and only advances the snapshot if auto-imports are both needed for the position and not already prepared. Whether or not to include auto-imports ever should be configurable, but currently is not by the API.
A proper fix needs to address this from a few angles:
- Be able to configure auto-imports in the API (perhaps per call, definitely by applying user preferences to a snapshot)
- Be able to create a snapshot that is prepared to auto-import a file
I think with both of those, you can keep getCompletionsAtPosition's return value as is and not have it return a snapshot, or bifurcate the method into one variant that returns a snapshot and one that doesn't:
- If
includeSymbolsis false, use the same logic that exists today, with auto-import retry as needed. - If
includeSymbolsis true, disable auto-import retry and return an error if the current snapshot isn't prepared. The caller either needs to disable auto-imports or explicitly create a prepared snapshot.
I will do this in another PR. cc Piotr Tomiak (@piotrtomiak)
| // GetSourceFileOfSymbol returns the owning file of a published binder symbol, or | ||
| // nil for a non-file-owned symbol, even if it borrows declarations from a file. | ||
| // Ownership recovery walks only the first declaration's AST parents. | ||
| func GetSourceFileOfSymbol(symbol *Symbol) *SourceFile { |
There was a problem hiding this comment.
Do you see this being a problem for merged declarations, e.g. globals?
There was a problem hiding this comment.
If a symbol merges cross-file, it's checker-created
There was a problem hiding this comment.
Copilot review overview
🔵 Needs a closer look
The protocol-wide identity and lifetime changes span concurrent server caches and both client implementations, warranting final human validation.
Review effort: Balanced
Findings: None
Resolved since last review (3)
Files not reviewed (1)
- tsc/internal/api/enum_values_generated.go: Generated file
This is the first follow-up to #64434. One more to come on top of this.
In short, this makes it so that when the API returns a Symbol that was produced by the binder (i.e., the Symbol belongs to a SourceFile), it gets stored in the client-side SourceFileCache, so that binder Symbols can have reference equality across different Snapshots.
This improves cache reuse, but also opens the door to getting source file responses with attached symbols, or getting the symbol of a declaration node, without any snapshot/project/checker context at all, e.g. with
api.createSourceFile(). This is the next follow-up.Symbol IDs in request params and responses now indicate whether the symbol can be looked up against a file (true for binder-created symbols) or against project/snapshot data (true for transient symbols).