-
Notifications
You must be signed in to change notification settings - Fork 215
Allow graceful handling of ArgumentException when trying to apply changes to SourceText in LspTextChangesLoader #11727
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
Allow graceful handling of ArgumentException when trying to apply changes to SourceText in LspTextChangesLoader #11727
Conversation
…nges to SourceText in LspTextChangesLoader
return TextAndVersion.Create(newText, version.GetNewerVersion()); | ||
} | ||
} | ||
catch (ArgumentException ex) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I left it as isolated as possible but should we just catch all exceptions? We would still want to report a fault (needs follow up with EA changes) but it can be handled
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, IMO we should catch all exceptions except OperationCancellationException
. We should let that bubble up to the caller on the Roslyn side.
return TextAndVersion.Create(newText, version.GetNewerVersion()); | ||
} | ||
} | ||
catch (ArgumentException ex) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, IMO we should catch all exceptions except OperationCancellationException
. We should let that bubble up to the caller on the Roslyn side.
|
||
// Validate the checksum information so the edits are known to be correct | ||
if (IsSourceTextMatching(sourceText)) | ||
try |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems possible that the failure is on line 41 as well? I couldn't see line numbers in the stack trace.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It shouldnt be possible but here we are so may be worth wrapping it all.
…nges to SourceText in LspTextChangesLoader (dotnet#11727) Helps mitigate dotnet/vscode-csharp#8147
…nges to SourceText in LspTextChangesLoader (dotnet#11727) Helps mitigate dotnet/vscode-csharp#8147
…nges to SourceText in LspTextChangesLoader (dotnet#11727) Helps mitigate dotnet/vscode-csharp#8147
Helps mitigate dotnet/vscode-csharp#8147