LSP.Range may request beyond length of file #68081

beccamc · 2023-05-03T22:18:03Z

There has been a persistent issue with ArgumentException at ProtocolConversions.RangeToTextSpan. Issue 66258.

Cyrus added more logging and it seems the problem is triggered when a line beyond the end of the document is requested. Here is an example error logged: System.ArgumentException: Range={ Start={ Line=0, Character=0 }, End={ Line=31, Character=0 } }. text.Length=811. text.Lines.Count=31

Per the LSP Range spec

A range in a text document expressed as (zero-based) start and end positions. A range is comparable to a selection in an editor. Therefore, the end position is exclusive. If you want to specify a range that contains a line including the line ending character(s) then use an end position denoting the start of the next line.

Edit: End range outside of the length of the file should throw an error.
~~Using this definition, it seems that for a document with n lines, requesting line n +1 is a valid way to specify you'd like the line ending characters included.~~

beccamc · 2023-05-03T22:19:25Z

src/Features/LanguageServer/Protocol/Extensions/ProtocolConversions.cs

+
+                        var editedEndLinePosition = new LinePosition(indexOfLastLine, endLineCharacterPosition);
+                        linePositionSpan = new LinePositionSpan(linePositionSpan.Start, editedEndLinePosition);
+                    }


@dibarbet Just realized you recommended this be in RangeToLinePosition.

Looks like it's not used (unless there is a specification I don't know about) so I've removed it.

ryzngard · 2023-05-03T22:31:08Z

@allisonchou do we need to do the same in razor?

Also, for the spec can the n+1 line have a column value > 0? My interpretation is not but that's just me recently reading it.

sharwell · 2023-05-03T22:35:09Z

I'm guessing this stems from the handling of documents without a final new line. If you have 31 lines and the 31st line does not have any EOL characters at the end, it seems like it should not be possible to request the next line.

beccamc · 2023-05-03T22:43:59Z

@sharwell This is a good point. I read it as you can always request the start of the next line. However, if there is no next line there shouldn't be end of line characters. How do I determine intent of the spec?

jasonmalinowski · 2023-05-03T23:16:00Z

@beccamc We can always chat with the LSP spec owners and get a clarification if needed.

beccamc · 2023-05-04T15:16:34Z

@jasonmalinowski That might be necessary. How do I do that?

beccamc · 2023-05-04T17:59:14Z

@dbaeumer Question here on the LSP spec for Range.

A range in a text document expressed as (zero-based) start and end positions. A range is comparable to a selection in an editor. Therefore, the end position is exclusive. If you want to specify a range that contains a line including the line ending character(s) then use an end position denoting the start of the next line.

For a document with n lines, is it valid to request line n + 1 as the end position? Or, given that line n has no line-ending-characters, is it not valid?

Let me know if you need any other details. Thanks!

CyrusNajmabadi · 2023-05-04T18:57:27Z

From my reading, it seems like that would be illegal. Specifically, it says that to get the line ending for a line, you start at teh next position (i.e. the next line). That seems totally reasonable given that there is a line-ending, and the next line exists (but has zero length). But absent a line-ending, it's non-sensical to be asking for the next line (which does not exist). So i would view this as a client bug.

dbaeumer · 2023-05-05T11:54:24Z

To easy the programming model we allow this (and I would actually put it that way into the spec). Otherwise the API would require you to first check whether there is a next line and then construct the getText accordingly. Allowing something like document.getText(new Range(11,0, 12, 0)) were the document only has 12 lines makes the programming easy since you simple get a line without ant new line characters.

sharwell · 2023-05-05T13:51:57Z

To easy the programming model we allow this (and I would actually put it that way into the spec).

I do not think it's unreasonable to require an IDE extensibility author to understand how lines and line endings work. I would discourage artificially relaxing the specification in a way that would allow a client to request a location that does not exist.

The simpler model here is representing documentation locations as character positions. LSP opted to not use that approach, which is fine, but opting out of the simpler model does not make it reasonable to allow a client to start making up non-existent ranges.

Allowing something like document.getText(new Range(11,0, 12, 0)) were the document only has 12 lines

Is there not a document.getText() method that can just handle this all internally? It seems strange to require users specify a range when getting the complete text of a document.

beccamc · 2023-05-05T20:37:02Z

Here is something I'm getting hung up on. Per the spec, the end position is exclusive. When I request line n +1, I'm requesting up to but not including n + 1. I am not requesting the non-existent line but rather everything up to it. I don't think the client is requesting something that doesn't exist.

CyrusNajmabadi · 2023-05-05T20:42:19Z

an exclusive endpoint though for a line without a new newline would be (to me) { Line=ThatLineNumZeroBased, Col=ThatLineLengthZeroBased }.

beccamc · 2023-05-05T21:49:12Z

@CyrusNajmabadi Yes, I see you're point. It seems reasonable to me that the client doesn't want this one case to act differently than the rest of the file.

beccamc · 2023-05-05T21:51:31Z

@dbaeumer Can you provide us with any more context on how often you're using this call in your code? I'm making an assumption it streamlines calls to not have to write a special case for the last line. Any sort of "data" to back that up?

dbaeumer · 2023-05-09T07:04:26Z

@sharwell you can call document.getText() without any range which gives you the whole content of the file including all line delimeters. If users want to get the whole document this is what they usually use

@beccamc I can't tell since the case is not coded special (that is the whole idea) and we don't have telemetry about getText calls with a range.

src/Compilers/Core/Portable/Text/TextLineCollection.cs

cston · 2023-05-10T16:09:16Z

src/Compilers/Core/Portable/Text/TextLineCollection.cs

@@ -62,6 +62,11 @@ public LinePositionSpan GetLinePositionSpan(TextSpan span)
        /// </summary>
        public int GetPosition(LinePosition position)
        {
+            if (position.Line >= this.Count)
+            {
+                throw new ArgumentOutOfRangeException(nameof(position.Line), string.Format(CodeAnalysisResources.LineCannotBeGreaterThanEnd, position.Line, this.Count));


string.Format(CodeAnalysisResources.LineCannotBeGreaterThanEnd, position.Line, this.Count)

When position.Line == this.Count, it looks like the exception message will be something like "The requested line number 3 is greater than the number of lines 3."

Should the message be "The requested line number 3 must be less than ..." instead?

Great point. I updated it like you suggested.

The requested line number {0} must be less than the number of lines {1}.

Does that seem descriptive enough to understand what to fix?

cston

Compiler changes LGTM.

cston · 2023-05-10T18:10:01Z

@dotnet/roslyn-compiler for a second review.

cston · 2023-05-10T19:47:01Z

src/Compilers/Core/Portable/Text/TextLineCollection.cs

@@ -62,6 +62,11 @@ public LinePositionSpan GetLinePositionSpan(TextSpan span)
        /// </summary>
        public int GetPosition(LinePosition position)
        {
+            if (position.Line >= this.Count)
+            {
+                throw new ArgumentOutOfRangeException(nameof(position.Line), string.Format(CodeAnalysisResources.LineCannotBeGreaterThanEnd, position.Line, this.Count));


throw new ArgumentOutOfRangeException(nameof(position.Line), string.Format(CodeAnalysisResources.LineCannotBeGreaterThanEnd, position.Line, this.Count))

Is the 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. We could use throw new ArgumentOutOfRangeException(nameof(position.Line), actualValue: position.Line, message: null); here.

cston · 2023-05-10T19:48:51Z

src/Compilers/Core/Portable/Text/TextLineCollection.cs

@@ -62,6 +62,11 @@ public LinePositionSpan GetLinePositionSpan(TextSpan span)
        /// </summary>
        public int GetPosition(LinePosition position)
        {
+            if (position.Line >= this.Count)


position.Line >= this.Count

Let's also check position.Line < 0.

cston · 2023-05-10T20:00:17Z

Compiler changes typically require two sign-offs. If the suggestions above for the compiler change seem reasonable, perhaps we could simply open a new PR to update the change and get two sign-offs there.

allow range beyond length of file

be8088a

beccamc requested a review from a team as a code owner May 3, 2023 22:18

dotnet-issue-labeler bot added Area-IDE untriaged Issues and PRs which have not yet been triaged by a lead labels May 3, 2023

beccamc commented May 3, 2023

View reviewed changes

remove range to line position

299fd06

davidwengier mentioned this pull request May 4, 2023

Fix OmniSharp formatting issue dotnet/razor#8669

Merged

cleanup tests

251d341

davidwengier mentioned this pull request May 5, 2023

Support mapping document positions after the end of the file dotnet/razor#8673

Closed

throw error when out of range

8198c29

beccamc requested a review from a team as a code owner May 9, 2023 22:52

beccamc commented May 9, 2023

View reviewed changes

src/Compilers/Core/Portable/Text/TextLineCollection.cs Outdated Show resolved Hide resolved

remove unused using

0897ee4

cston reviewed May 10, 2023

View reviewed changes

update resource

85e6335

beccamc enabled auto-merge May 10, 2023 17:48

cston approved these changes May 10, 2023

View reviewed changes

beccamc merged commit b521599 into dotnet:main May 10, 2023

ghost added this to the Next milestone May 10, 2023

cston reviewed May 10, 2023

View reviewed changes

beccamc mentioned this pull request May 11, 2023

Cleanup ArgumentOutOfRange exception for LSP.Range #68168

Closed

sharwell mentioned this pull request May 12, 2023

Fix detection of nested symbols in Document Outline #68107

Merged

allisonchou mentioned this pull request May 12, 2023

[Automated] PRs inserted in VS build main-33712.304 #68184

Closed

Cosifne modified the milestones: Next, 17.7 P2 May 31, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

LSP.Range may request beyond length of file #68081

LSP.Range may request beyond length of file #68081

beccamc commented May 3, 2023 •

edited

Loading

beccamc May 3, 2023

beccamc May 3, 2023

ryzngard commented May 3, 2023

sharwell commented May 3, 2023

beccamc commented May 3, 2023

jasonmalinowski commented May 3, 2023

beccamc commented May 4, 2023

beccamc commented May 4, 2023

CyrusNajmabadi commented May 4, 2023

dbaeumer commented May 5, 2023 •

edited by arunchndr

Loading

sharwell commented May 5, 2023 •

edited by beccamc

Loading

beccamc commented May 5, 2023

CyrusNajmabadi commented May 5, 2023

beccamc commented May 5, 2023

beccamc commented May 5, 2023 •

edited

Loading

dbaeumer commented May 9, 2023

cston May 10, 2023

beccamc May 10, 2023

cston left a comment

cston commented May 10, 2023

cston May 10, 2023 •

edited

Loading

cston May 10, 2023

cston commented May 10, 2023

LSP.Range may request beyond length of file #68081

LSP.Range may request beyond length of file #68081

Conversation

beccamc commented May 3, 2023 • edited Loading

beccamc May 3, 2023

Choose a reason for hiding this comment

beccamc May 3, 2023

Choose a reason for hiding this comment

ryzngard commented May 3, 2023

sharwell commented May 3, 2023

beccamc commented May 3, 2023

jasonmalinowski commented May 3, 2023

beccamc commented May 4, 2023

beccamc commented May 4, 2023

CyrusNajmabadi commented May 4, 2023

dbaeumer commented May 5, 2023 • edited by arunchndr Loading

sharwell commented May 5, 2023 • edited by beccamc Loading

beccamc commented May 5, 2023

CyrusNajmabadi commented May 5, 2023

beccamc commented May 5, 2023

beccamc commented May 5, 2023 • edited Loading

dbaeumer commented May 9, 2023

cston May 10, 2023

Choose a reason for hiding this comment

beccamc May 10, 2023

Choose a reason for hiding this comment

cston left a comment

Choose a reason for hiding this comment

cston commented May 10, 2023

cston May 10, 2023 • edited Loading

Choose a reason for hiding this comment

cston May 10, 2023

Choose a reason for hiding this comment

cston commented May 10, 2023

beccamc commented May 3, 2023 •

edited

Loading

dbaeumer commented May 5, 2023 •

edited by arunchndr

Loading

sharwell commented May 5, 2023 •

edited by beccamc

Loading

beccamc commented May 5, 2023 •

edited

Loading

cston May 10, 2023 •

edited

Loading