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.

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

LSP.Range may request beyond length of file #68081

Merged
merged 6 commits into from
May 10, 2023
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
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
3 changes: 3 additions & 0 deletions src/Compilers/Core/Portable/CodeAnalysisResources.resx
Original file line number Diff line number Diff line change
Expand Up @@ -756,4 +756,7 @@
<data name="InvalidInstrumentationKind" xml:space="preserve">
<value>Invalid instrumentation kind: {0}</value>
</data>
<data name="LineCannotBeGreaterThanEnd" xml:space="preserve">
<value>The requested line number {0} is greater than the number of lines {1}.</value>
</data>
</root>
6 changes: 6 additions & 0 deletions src/Compilers/Core/Portable/Text/TextLineCollection.cs
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
using System.Collections;
using System.Collections.Generic;
using System.Diagnostics.CodeAnalysis;
using Microsoft.CodeAnalysis.PooledObjects;
beccamc marked this conversation as resolved.
Show resolved Hide resolved
using Microsoft.CodeAnalysis.Text;

namespace Microsoft.CodeAnalysis.Text
Expand Down Expand Up @@ -62,6 +63,11 @@ public LinePositionSpan GetLinePositionSpan(TextSpan span)
/// </summary>
public int GetPosition(LinePosition position)
{
if (position.Line >= this.Count)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

position.Line >= this.Count

Let's also check position.Line < 0.

{
throw new ArgumentOutOfRangeException(nameof(position.Line), string.Format(CodeAnalysisResources.LineCannotBeGreaterThanEnd, position.Line, this.Count));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

@cston cston May 10, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}

return this[position.Line].Start + position.Character;
}

Expand Down

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Original file line number Diff line number Diff line change
Expand Up @@ -196,23 +196,20 @@ public static LSP.VersionedTextDocumentIdentifier DocumentToVersionedTextDocumen
public static LinePosition PositionToLinePosition(LSP.Position position)
=> new LinePosition(position.Line, position.Character);

public static LinePositionSpan RangeToLinePositionSpan(LSP.Range range)
=> new LinePositionSpan(PositionToLinePosition(range.Start), PositionToLinePosition(range.End));

public static TextSpan RangeToTextSpan(LSP.Range range, SourceText text)
{
var linePositionSpan = RangeToLinePositionSpan(range);
var linePositionSpan = new LinePositionSpan(PositionToLinePosition(range.Start), PositionToLinePosition(range.End));

try
{
try
{
return text.Lines.GetTextSpan(linePositionSpan);
}
catch (ArgumentException)
catch (ArgumentException ex)
{
// Create a custom error for this so we can examine the data we're getting.
throw new ArgumentException($"Range={RangeToString(range)}. text.Length={text.Length}. text.Lines.Count={text.Lines.Count}");
throw new ArgumentException($"Range={RangeToString(range)}. text.Length={text.Length}. text.Lines.Count={text.Lines.Count}", ex);
}
}
// Temporary exception reporting to investigate https://github.com/dotnet/roslyn/issues/66258.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -92,4 +92,19 @@
<PublicAPI Include="PublicAPI.Unshipped.txt" />
</ItemGroup>

<ItemGroup>
<Compile Update="LanguageServerResources.Designer.cs">
<DesignTime>True</DesignTime>
<AutoGen>True</AutoGen>
<DependentUpon>LanguageServerResources.resx</DependentUpon>
</Compile>
</ItemGroup>

<ItemGroup>
<EmbeddedResource Update="LanguageServerResources.resx" GenerateSource="true" Namespace="Microsoft.CodeAnalysis.LanguageServer">
<Generator>ResXFileCodeGenerator</Generator>
<LastGenOutput>LanguageServerResources.Designer.cs</LastGenOutput>
</EmbeddedResource>
</ItemGroup>

</Project>
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,12 @@
// The .NET Foundation licenses this file to you under the MIT license.
// See the LICENSE file in the project root for more information.

using System;
using System.Linq;
using Microsoft.CodeAnalysis.Text;
using Microsoft.VisualStudio.LanguageServer.Protocol;
using Xunit;
using Range = Microsoft.VisualStudio.LanguageServer.Protocol.Range;

namespace Microsoft.CodeAnalysis.LanguageServer.UnitTests
{
Expand All @@ -19,5 +22,105 @@ public void CompletionItemKind_DoNotUseMethodAndFunction()

Assert.False(containsFunction && containsMethod, "Don't use Method and Function completion item kinds as it causes user confusion.");
}

[Fact]
public void RangeToTextSpanStartWithNextLine()
{
var markup = GetTestMarkup();

var sourceText = SourceText.From(markup);
var range = new Range() { Start = new Position(0, 0), End = new Position(1, 0) };
var textSpan = ProtocolConversions.RangeToTextSpan(range, sourceText);

// End should be start of the second line
Assert.Equal(0, textSpan.Start);
Assert.Equal(10, textSpan.End);
}

[Fact]
public void RangeToTextSpanMidLine()
{
var markup = GetTestMarkup();
var sourceText = SourceText.From(markup);

// Take just "x = 5"
var range = new Range() { Start = new Position(2, 8), End = new Position(2, 12) };
var textSpan = ProtocolConversions.RangeToTextSpan(range, sourceText);

Assert.Equal(21, textSpan.Start);
Assert.Equal(25, textSpan.End);
}

[Fact]
public void RangeToTextSpanLineEndOfDocument()
{
var markup = GetTestMarkup();
var sourceText = SourceText.From(markup);

var range = new Range() { Start = new Position(0, 0), End = new Position(3, 1) };
var textSpan = ProtocolConversions.RangeToTextSpan(range, sourceText);

Assert.Equal(0, textSpan.Start);
Assert.Equal(30, textSpan.End);
}

[Fact]
public void RangeToTextSpanLineEndOfDocumentWithEndOfLineChars()
{
var markup =
@"void M()
{
var x = 5;
}
"; // add additional end line

var sourceText = SourceText.From(markup);

var range = new Range() { Start = new Position(0, 0), End = new Position(4, 0) };
var textSpan = ProtocolConversions.RangeToTextSpan(range, sourceText);

// Result now includes end of line characters for line 3
Assert.Equal(0, textSpan.Start);
Assert.Equal(32, textSpan.End);
}

[Fact]
public void RangeToTextSpanLineOutOfRangeError()
{
var markup = GetTestMarkup();
var sourceText = SourceText.From(markup);

var range = new Range() { Start = new Position(0, 0), End = new Position(sourceText.Lines.Count, 0) };
Assert.Throws<ArgumentException>(() => ProtocolConversions.RangeToTextSpan(range, sourceText));
}

[Fact]
public void RangeToTextSpanEndAfterStartError()
{
var markup = GetTestMarkup();
var sourceText = SourceText.From(markup);

// This start position will be beyond the end position
var range = new Range() { Start = new Position(2, 20), End = new Position(3, 0) };
Assert.Throws<ArgumentException>(() => ProtocolConversions.RangeToTextSpan(range, sourceText));
}

private static string GetTestMarkup()
{
// Markup is 31 characters long. Line break (\n) is 2 characters
/*
void M() [Line = 0; Start = 0; End = 8; End including line break = 10]
{ [Line = 1; Start = 10; End = 11; End including line break = 13]
var x = 5; [Line = 2; Start = 13; End = 27; End including line break = 29]
} [Line = 3; Start = 29; End = 30; End including line break = 30]
*/

var markup =
@"void M()
{
var x = 5;
}";
return markup;
}
}
}