Fix a crash in an anonymous buffer that copilot is writing to. #60377
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Fix a crash in an unbacked buffer that copilot is writing to. LineNode.walk reads the first element out of LineNode.children without checking that there are any elements to read. Of course, it's supposed to be called only on nodes that have children. Why no tests? ------------- - This failure is flaky -- I never managed to reproduce it. - scriptVersionCache is barely maintained. The last two fixes in this particular code are 3.5 and 6.5 years ago. - #21924 has no tests to fix an assert - af47ddbc3b0c6421c38b18e56f8da88434a473c8d has an exasperated comment about the difficulty of learning invariants and an apology for an only-possibly-relevant test. - So, I didn't think the effort of a complete fix was worthwhile in a case where a small, local fix will stop the crashes.
typescript-bot
added
Author: Team
For Milestone Bug
sandersn
changed the title
iisaduan
reviewed
Oct 30, 2024
| @@ -632,6 +632,7 @@ export class LineNode implements LineCollection { | |||
|
|
|||
| walk(rangeStart: number, rangeLength: number, walkFns: LineIndexWalker): void { | |||
| // assume (rangeStart < this.totalChars) && (rangeLength <= this.totalChars) | |||
| if (this.children.length === 0) return; | |||
There was a problem hiding this comment.
Is this.children always defined?
There was a problem hiding this comment.
Yes, the type is LineCollection[] and the constructor defaults it to [] if it's not provided.
Edit: in fact, there are no explicit assignments to it; just the implicit private readonly children: LineCollection[] = [] in the constructor.
iisaduan
approved these changes
Oct 30, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fix #60118
LineNode.walk reads the first element out of LineNode.children without checking that there are any elements to read. Of course, it's supposed to be called only on nodes that have children.
Why no tests?
ScriptInfo.positionToLineOffset#44746 has an exasperated comment about the difficulty of learning invariants and an apology for an only-possibly-relevant test.