-
Notifications
You must be signed in to change notification settings - Fork 4k
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
Cleanup ArgumentOutOfRange exception for LSP.Range #68168
Conversation
@@ -206,6 +206,7 @@ public static TextSpan RangeToTextSpan(LSP.Range range, SourceText text) | |||
} | |||
catch (ArgumentException ex) | |||
{ | |||
// Remove after ArgumentException investigation - https://github.com/dotnet/roslyn/issues/66258 | |||
// Create a custom error for this so we can examine the data we're getting. |
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.
Chuck made a Comment in the other PR. Is the new argument resource string necessary? It looks like the caller in ProtocolConversions will catch the exception and wrap the exception in an exception that includes the line count.
That exception catching code is here, and was added by Cyrus. I've added an issue for removing it when this investigation is done. Without this code catching the exception and adding the line numbers, I think its useful to still throw an exception saying The requested line number {0} must be less than the number of lines {1}
Open to feedback
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 agree, it seems useful to throw an ArgumentOutOfRange
exception from TextLineCollection.LinePosition(), but I don't think we need a dedicated resource string for that case, particularly since the caller here is catching and wrapping the exception and including the line count.
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.
The catching code here will be removed when the investigation into these ArgumentOutOfRange exceptions is finished. When that code is removed there will not be logging for the total number of lines in the document.
Is there a cost to having a resource string here? It seems like an easy thing to add that will bring clarity to something that has been a point of confusion in the LSP spec.
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.
Basically this catching code on line 211 should not stay here permanently because it hides the true cause of the ArgumentException. It should be removed once we round up the ArgumentOutOfRange exceptions.
We've already proven that for the case of end.Line == lines.Count it is difficult to determine the source of the error. We needed the logging on line 211 to do it. Additionally, given that the case of end.Line == lines.Count might not intuitively be seen an error, I believe that we should throw a very clear error message as to what has been done wrong.
This is a correction to PR 68081, which went in without appropriate approval. PR 68081 added an error message when requesting a line beyond the length of the file.
Two things here...