LSP.Range may request beyond length of file

[beccamc]

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]

@dibarbet Just realized you recommended this be in RangeToLinePosition.

[beccamc]

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

[ryzngard]

@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]

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]

@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]

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

[beccamc]

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

[beccamc]

@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]

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]

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]

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]

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]

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

[beccamc]

@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]

@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]

@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.

[cston]

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?

[beccamc]

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]

@dotnet/roslyn-compiler for a second review.

[cston]

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]

position.Line >= this.Count

Let's also check position.Line < 0.

[cston]

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.