Skip to content

Prevent detached diagnostics from running off the end of the file #55381

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

Merged
merged 2 commits into from
Aug 15, 2023

Conversation

jakebailey
Copy link
Member

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=1 diagnostic, 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 createDetachedDiagnostic into parser.ts; it's only used there so that'd avoid some parameters. But, all of its related helpers are still in this utilities file.

@typescript-bot typescript-bot added Author: Team For Milestone Bug PRs that fix a bug with a specific milestone labels Aug 15, 2023
Comment on lines +8153 to +8155
if ((start + length) > sourceText.length) {
length = sourceText.length - start;
}
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the actual fix.

@@ -2470,7 +2470,7 @@ namespace Parser {
if (lastError) {
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))
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This set of three detached diagnostics are the trouble; openPosition can totally be at the end of the file.

@jakebailey jakebailey merged commit f37d2ad into microsoft:main Aug 15, 2023
@jakebailey jakebailey deleted the fix-55296 branch August 15, 2023 15:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Author: Team For Milestone Bug PRs that fix a bug with a specific milestone
Projects
None yet
Development

Successfully merging this pull request may close these issues.

attachFileToDiagnostics: Debug Failure crash
3 participants