Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Cleanup ArgumentOutOfRange exception for LSP.Range #68168

Closed
wants to merge 1 commit into from
Closed

Cleanup ArgumentOutOfRange exception for LSP.Range #68168

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions src/Features/LanguageServer/Protocol/Extensions/ProtocolConversions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Copy link
Contributor Author

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

Copy link
Member

@cston cston May 11, 2023 •
edited
Loading

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.

Copy link
Contributor Author

@beccamc beccamc May 11, 2023 •
edited
Loading

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.

Copy link
Contributor Author

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.

throw new ArgumentException($"Range={RangeToString(range)}. text.Length={text.Length}. text.Lines.Count={text.Lines.Count}", ex);
}
Expand Down
10 changes: 10 additions & 0 deletions src/Features/LanguageServer/ProtocolUnitTests/ProtocolConversionsTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -105,6 +105,16 @@ public void RangeToTextSpanEndAfterStartError()
Assert.Throws<ArgumentException>(() => ProtocolConversions.RangeToTextSpan(range, sourceText));
}

[Fact]
beccamc marked this conversation as resolved.
Show resolved Hide resolved
public void RangeToTextSpanNegativeValueError()
{
var markup = GetTestMarkup();
var sourceText = SourceText.From(markup);

var range = new Range() { Start = new Position(-1, 0), End = new Position(0, 0) };
Assert.Throws<ArgumentOutOfRangeException>(() => ProtocolConversions.RangeToTextSpan(range, sourceText));
}

private static string GetTestMarkup()
{
// Markup is 31 characters long. Line break (\n) is 2 characters
Expand Down