Skip to content

Fix SubText ctor parameter verification. #78989

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

Merged
merged 2 commits into from
Jun 17, 2025
Merged

Fix SubText ctor parameter verification. #78989

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
6c9a7ab
Fix SubText ctor parameter verification.
ToddGrun Jun 16, 2025
1b2e53b
Add WorkItem attribute to test
ToddGrun Jun 17, 2025
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
23 changes: 23 additions & 0 deletions src/Compilers/Core/CodeAnalysisTest/Text/TextChangeTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,29 @@ public void TestSubTextSpanMid()
Assert.Equal("o W", subText.ToString());
}

[Fact]
public void TestSubTextSubTextStart()
{
var text = SourceText.From("Hello World");
var subText = text.GetSubText(new TextSpan(0, 5));
var subSubText = subText.GetSubText(new TextSpan(0, 0));

Assert.Equal("Hello", subText.ToString());
Assert.Equal(0, subSubText.Length);
}

[Fact]
[WorkItem(78989, "https://github.com/dotnet/roslyn/pull/78989")]
public void TestSubTextSubTextEnd()
{
var text = SourceText.From("Hello World");
var subText = text.GetSubText(6);
var subSubText = subText.GetSubText(subText.Length);

Assert.Equal("World", subText.ToString());
Assert.Equal(0, subSubText.Length);
}

[Fact]
public void TestChangedText()
{
Expand Down
4 changes: 2 additions & 2 deletions src/Compilers/Core/Portable/Text/SubText.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,9 +22,9 @@ public SubText(SourceText text, TextSpan span)
throw new ArgumentNullException(nameof(text));
}

// span.Start and span.End are valid wrt eachother by nature of being passed in as a TextSpan,
// so there is no need to verify span.Start against text.Length or span.End against zero.
Copy link
Member

@333fred 333fred Jun 16, 2025

Choose a reason for hiding this comment

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

I don't follow how this is true. I can absolutely construct a bad TextSpan and pass it in, what prevents me from doing that?

Copy link
Member

@CyrusNajmabadi CyrusNajmabadi Jun 16, 2025

Choose a reason for hiding this comment

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

a textspan itself prevents the end from being before the start. so a text span's start/end always represent at least a legal range where end >= start. given that. you only ened to validate that the start of this text span is not before the start of the text, and the end of the text span is not after the end of the text.

Copy link
Member

@333fred 333fred Jun 16, 2025

Choose a reason for hiding this comment

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

Thanks for the clarification!

Copy link
Contributor Author

@ToddGrun ToddGrun Jun 16, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

In that case I might need to change this. I'm curious as to what mechanism would allow creation of the object such that the TextSpan ctor lets the object creation proceed while violating these constraints. Is it through serialization which sets the backing Start/Length properties through reflection?

*** Edit: this comment was made before I saw Cyrus's response

if (span.Start < 0
|| span.Start >= text.Length
|| span.End < 0
|| span.End > text.Length)
{
throw new ArgumentOutOfRangeException(nameof(span));
Expand Down