Prevent detached diagnostics from running off the end of the file - #55381
Merged
Conversation
typescript-bot
added
Author: Team
For Milestone Bug
jakebailey
commented
Aug 15, 2023
Comment on lines
+8153
to
+8155
| if ((start + length) > sourceText.length) { | ||
| length = sourceText.length - start; | ||
| } |
Member
Author
There was a problem hiding this comment.
This is the actual fix.
jakebailey
commented
Aug 15, 2023
| addRelatedInfo( | ||
| lastError, | ||
| createDetachedDiagnostic(fileName, openPosition, 1, Diagnostics.The_parser_expected_to_find_a_1_to_match_the_0_token_here, tokenToString(openKind), tokenToString(closeKind)) | ||
| createDetachedDiagnostic(fileName, sourceText, openPosition, 1, Diagnostics.The_parser_expected_to_find_a_1_to_match_the_0_token_here, tokenToString(openKind), tokenToString(closeKind)) |
Member
Author
There was a problem hiding this comment.
This set of three detached diagnostics are the trouble; openPosition can totally be at the end of the file.
andrewbranch
approved these changes
Aug 15, 2023
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
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
3 participants
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.
Fixes #55296
This is similar to #52450 and #53674; we can create diagnostics with hardcoded range lengths, but if the parse error starts at the end of the file, we'll crash.
I could band-aid this by special casing the one crash, but as you can see in the change, we have quite a few other places which assume it's okay to use a
length=1diagnostic, when it's possible that it may not work.So, just set the length to the maximum available (or zero) when this happens.
To make this more obvious, I've restructured things a little so that we check the ranges earlier. The stack trace in #55296 happens late because the diagnostics weren't checked until they were finally attached to the file, after parse was complete. It's possible that I could find a different way to do this that doesn't look so noisy, but I feel like this is more robust.
Realistically, I could just move
createDetachedDiagnosticintoparser.ts; it's only used there so that'd avoid some parameters. But, all of its related helpers are still in this utilities file.