-
Notifications
You must be signed in to change notification settings - Fork 12.8k
Fixed crash when cross-file reusing nodes for class member snippet completions #58216
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fixed crash when cross-file reusing nodes for class member snippet completions #58216
Conversation
Uhhhhh, kinda nervous about walking the tree on every literal. @typescript-bot perf test this |
I think the right thing to do is do a clone of the node from wherever this comes from. Especially since there are other bugs that often come up like when a node is reused within the tree and the formatter tries to overwrite it. |
Yep, if the underlying problem is that node re-use messes up the formatter calls in the services layer, we seem to have many occurrences of that. @armanio123 is working on a fix for #57924 (comment), which is an instance of this, although that crash is caused by re-use of a question token instead of a literal. |
@DanielRosenwasser Here they are:
tscComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
tsserverComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
startupComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
Developer Information: |
@typescript-bot perf test this |
@gabritto Here they are:
tscComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
tsserverComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
startupComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
Developer Information: |
So I thought that the node reuse would be desired here. If I understand correctly you are saying that it should never happen here, right? Cloning the original node would likely make it synthesized or something and that would prevent reuse altogether. |
The current state of the formatter is such that we should never pass it an AST that has node reuse, as that will cause a crash. Other parts of the compiler don't suffer from this problem. I think the emitter doesn't care about node reuse, for the most part. I think the problem is that the formatter was written to handle actual parse trees, coming from actual files, but we've been using it to format trees we generate for completions. |
Yeah, and I believe the formatter actually mutates the parse trees, so that complicates things too. |
I didn't see (yet?) any mutation like that happening. TypeScript/src/services/utilities.ts Lines 3205 to 3211 in 21b5c96
TypeScript/src/compiler/factory/nodeFactory.ts Line 1266 in 21b5c96
and that gets used here by the emitter in TypeScript/src/compiler/emitter.ts Line 5150 in 21b5c96
Given all of that It still feels like using the correct source file is the easiest here - but I can change the location where it's looked for because only the code that uses |
@typescript-bot perf test this |
Some notes for the future:
|
@gabritto Here they are:
tscComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
tsserverComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
startupComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
Developer Information: |
fixes #58205