Skip to content

Fix various LastIndexOf bugs when dealing with zero-length target substrings #34616

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 1 commit into from
Apr 7, 2020
Merged

Fix various LastIndexOf bugs when dealing with zero-length target substrings #34616

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
32 changes: 15 additions & 17 deletions src/libraries/Common/tests/Tests/System/StringTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3379,7 +3379,6 @@ public static void IndexOfSequenceLengthOneValue_Char()
Assert.Equal(2, index);
Assert.Equal(index, s1.IndexOf(s2, StringComparison.Ordinal));

// A zero-length value is always "found" at the start of the span.
ReadOnlySpan<char> span = s1.AsSpan();
ReadOnlySpan<char> value = s2.AsSpan();
index = span.IndexOf(value);
Expand All @@ -3396,7 +3395,6 @@ public static void IndexOfSequenceLengthOneValueAtVeryEnd_Char()
Assert.Equal(5, index);
Assert.Equal(index, s1.IndexOf(s2, StringComparison.Ordinal));

// A zero-length value is always "found" at the start of the span.
ReadOnlySpan<char> span = s1.AsSpan();
ReadOnlySpan<char> value = s2.AsSpan();
index = span.IndexOf(value);
Expand All @@ -3413,7 +3411,6 @@ public static void IndexOfSequenceLengthOneValueJustPasttVeryEnd_Char()
Assert.Equal(-1, index);
Assert.Equal(index, s1.IndexOf(s2, StringComparison.Ordinal));

// A zero-length value is always "found" at the start of the span.
ReadOnlySpan<char> span = s1.AsSpan();
ReadOnlySpan<char> value = s2.AsSpan();
index = span.IndexOf(value);
Expand Down Expand Up @@ -3887,18 +3884,23 @@ public static void LastIndexOf_AllSubstrings(string s, string value, int startIn

if (value.Length == 0)
{
int expectedIndex = s.Length > 0 ? s.Length - 1 : 0;
int expectedStartIndex = startIndex == s.Length ? startIndex - 1 : startIndex;
int expectedStartIndex = startIndex;
if (s.Length == 0 && (startIndex == -1 || startIndex == 0))
expectedStartIndex = (value.Length == 0) ? 0 : -1;
Assert.Equal(expectedIndex, s.LastIndexOf(value, comparison));
expectedStartIndex = 0; // empty string occurs at beginning of search space
if (s.Length > 0 && startIndex < s.Length)
expectedStartIndex = startIndex + 1; // empty string occurs just after the last char included in the search space

Assert.Equal(s.Length, s.LastIndexOf(value, comparison));
Assert.Equal(expectedStartIndex, s.LastIndexOf(value, startIndex, comparison));
Assert.Equal(expectedIndex, s.AsSpan().LastIndexOf(value.AsSpan(), comparison));
Assert.Equal(s.Length, s.AsSpan().LastIndexOf(value.AsSpan(), comparison));
return;
}

if (s.Length == 0)
{
// unit test shouldn't have passed a weightless string to this routine
Assert.NotEqual(value, string.Empty, StringComparer.FromComparison(comparison));

Assert.Equal(-1, s.LastIndexOf(value, comparison));
Assert.Equal(-1, s.LastIndexOf(value, startIndex, comparison));
Assert.Equal(-1, s.AsSpan().LastIndexOf(value.AsSpan(), comparison));
Expand Down Expand Up @@ -4068,8 +4070,8 @@ public static void LastIndexOf_TurkishI_EnglishUSCulture()
}
Copy link
Member

@adamsitnik adamsitnik Apr 7, 2020

Choose a reason for hiding this comment

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

not related to your changes: IMHO this test (IndexOf_AllSubstrings) is very complex and is trying to test too many test cases at the same time. It would be better to split it into few smaller tests with self-describing names

Copy link
Member Author

@GrabYourPitchforks GrabYourPitchforks Apr 7, 2020

Choose a reason for hiding this comment

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

Agreed, it's a bit of a bear. Do you have recommendations for how it might naturally be split?

Copy link
Member

@adamsitnik adamsitnik Apr 8, 2020

Choose a reason for hiding this comment

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

I think that the two if blocks could become separate tests: value.Length == 0, s.Length == 0


[Theory]
[InlineData("foo", 2)]
[InlineData("hello", 4)]
[InlineData("foo", 3)]
[InlineData("hello", 5)]
[InlineData("", 0)]
public static void LastIndexOf_EmptyString(string s, int expected)
{
Expand Down Expand Up @@ -4325,13 +4327,13 @@ public static void LastIndexOfSequenceZeroLengthValue_Char()
string s1 = "0172377457778667789";
string s2 = string.Empty;
int index = s1.LastIndexOf(s2);
Assert.Equal(s1.Length - 1, index);
Assert.Equal(s1.Length, index);

// A zero-length value is always "found" at the start of the span.
// A zero-length value is always "found" at the end of the span.
ReadOnlySpan<char> span = s1.AsSpan();
ReadOnlySpan<char> value = s2.AsSpan();
index = span.LastIndexOf(value);
Assert.Equal(0, index);
Assert.Equal(span.Length, index);
}

[Fact]
Expand All @@ -4356,7 +4358,6 @@ public static void LastIndexOfSequenceLengthOneValue_Char()
int index = s1.LastIndexOf(s2);
Assert.Equal(2, index);

// A zero-length value is always "found" at the start of the span.
ReadOnlySpan<char> span = s1.AsSpan();
ReadOnlySpan<char> value = s2.AsSpan();
index = span.LastIndexOf(value);
Expand All @@ -4371,7 +4372,6 @@ public static void LastIndexOfSequenceLengthOneValueAtVeryEnd_Char()
int index = s1.LastIndexOf(s2);
Assert.Equal(5, index);

// A zero-length value is always "found" at the start of the span.
ReadOnlySpan<char> span = s1.AsSpan();
ReadOnlySpan<char> value = s2.AsSpan();
index = span.LastIndexOf(value);
Expand All @@ -4386,7 +4386,6 @@ public static void LastIndexOfSequenceLengthOneValueMultipleTimes_Char()
int index = s1.LastIndexOf(s2);
Assert.Equal(5, index);

// A zero-length value is always "found" at the start of the span.
ReadOnlySpan<char> span = s1.AsSpan();
ReadOnlySpan<char> value = s2.AsSpan();
index = span.LastIndexOf(value);
Expand All @@ -4401,7 +4400,6 @@ public static void LastIndexOfSequenceLengthOneValueJustPasttVeryEnd_Char()
int index = s1.LastIndexOf(s2);
Assert.Equal(-1, index);

// A zero-length value is always "found" at the start of the span.
ReadOnlySpan<char> span = s1.AsSpan();
ReadOnlySpan<char> value = s2.AsSpan();
index = span.LastIndexOf(value);
Expand Down
47 changes: 30 additions & 17 deletions src/libraries/System.Globalization/tests/CompareInfo/CompareInfoTests.LastIndexOf.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ public class CompareInfoLastIndexOfTests
public static IEnumerable<object[]> LastIndexOf_TestData()
{
// Empty strings
yield return new object[] { s_invariantCompare, "foo", "", 2, 3, CompareOptions.None, 2 };
yield return new object[] { s_invariantCompare, "foo", "", 2, 3, CompareOptions.None, 3 };
yield return new object[] { s_invariantCompare, "", "", 0, 0, CompareOptions.None, 0 };
yield return new object[] { s_invariantCompare, "", "a", 0, 0, CompareOptions.None, -1 };
yield return new object[] { s_invariantCompare, "", "", -1, 0, CompareOptions.None, 0 };
Expand All @@ -30,8 +30,8 @@ public static IEnumerable<object[]> LastIndexOf_TestData()
yield return new object[] { s_invariantCompare, "Hello", "b", 5, 5, CompareOptions.None, -1 };
yield return new object[] { s_invariantCompare, "Hello", "l", 5, 0, CompareOptions.None, -1 };

yield return new object[] { s_invariantCompare, "Hello", "", 5, 5, CompareOptions.None, 4 };
yield return new object[] { s_invariantCompare, "Hello", "", 5, 0, CompareOptions.None, 4 };
yield return new object[] { s_invariantCompare, "Hello", "", 5, 5, CompareOptions.None, 5 };
yield return new object[] { s_invariantCompare, "Hello", "", 5, 0, CompareOptions.None, 5 };

// OrdinalIgnoreCase
yield return new object[] { s_invariantCompare, "Hello", "l", 4, 5, CompareOptions.OrdinalIgnoreCase, 3 };
Expand Down Expand Up @@ -157,6 +157,32 @@ public void LastIndexOf_String(CompareInfo compareInfo, string source, string va
// Use LastIndexOf(string, string, int, int, CompareOptions)
Assert.Equal(expected, compareInfo.LastIndexOf(source, value, startIndex, count, options));

// Fixup offsets so that we can call the span-based APIs.

ReadOnlySpan<char> sourceSpan;
int adjustmentFactor; // number of chars to add to retured index from span-based APIs

if (startIndex == source.Length - 1 && count == source.Length)
{
// This idiom means "read the whole span"
sourceSpan = source;
adjustmentFactor = 0;
}
else if (startIndex == source.Length)
{
// Account for possible off-by-one at the call site
sourceSpan = source.AsSpan()[^(Math.Max(0, count - 1))..];
adjustmentFactor = source.Length - sourceSpan.Length;
}
else
{
// Bump 'startIndex' by 1, then go back 'count' chars
sourceSpan = source.AsSpan()[..(startIndex + 1)][^count..];
adjustmentFactor = startIndex + 1 - count;
}

if (expected < 0) { adjustmentFactor = 0; } // don't modify "not found" (-1) return values

if ((compareInfo == s_invariantCompare) && ((options == CompareOptions.None) || (options == CompareOptions.IgnoreCase)))
{
StringComparison stringComparison = (options == CompareOptions.IgnoreCase) ? StringComparison.InvariantCultureIgnoreCase : StringComparison.InvariantCulture;
Expand All @@ -165,20 +191,7 @@ public void LastIndexOf_String(CompareInfo compareInfo, string source, string va
Assert.Equal(expected, source.LastIndexOf(value, startIndex, count, stringComparison));

// Use int MemoryExtensions.LastIndexOf(this ReadOnlySpan<char>, ReadOnlySpan<char>, StringComparison)
// Filter differences betweeen string-based and Span-based LastIndexOf
// - Empty value handling - https://github.com/dotnet/runtime/issues/13382
// - Negative count
if (value.Length == 0 || count < 0)
return;

if (startIndex == source.Length)
{
startIndex--;
if (count > 0)
count--;
}
int leftStartIndex = (startIndex - count + 1);
Assert.Equal((expected == -1) ? -1 : (expected - leftStartIndex), source.AsSpan(leftStartIndex, count).LastIndexOf(value.AsSpan(), stringComparison));
Assert.Equal(expected - adjustmentFactor, sourceSpan.LastIndexOf(value.AsSpan(), stringComparison));
}
}

Expand Down
27 changes: 17 additions & 10 deletions src/libraries/System.Globalization/tests/CompareInfo/CompareInfoTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -310,7 +310,7 @@ public static IEnumerable<object[]> SortKey_TestData()

public static IEnumerable<object[]> IndexOf_TestData()
{
yield return new object[] { s_invariantCompare, "foo", "", 0, 0, 0 };
yield return new object[] { s_invariantCompare, "foo", "", 0, 0, 1 };
yield return new object[] { s_invariantCompare, "", "", 0, 0, 0 };
yield return new object[] { s_invariantCompare, "Hello", "l", 0, 2, -1 };
yield return new object[] { s_invariantCompare, "Hello", "l", 3, 3, 3 };
Expand All @@ -321,10 +321,14 @@ public static IEnumerable<object[]> IndexOf_TestData()

public static IEnumerable<object[]> IsSortable_TestData()
{
yield return new object[] { "", false, false };
yield return new object[] { "abcdefg", false, true };
yield return new object[] { "\uD800\uDC00", true, true };
yield return new object[] { "\uD800\uD800", true, false };
yield return new object[] { "", false };
yield return new object[] { "abcdefg", true };
yield return new object[] { "\uD800\uDC00", true };

// VS test runner for xunit doesn't handle ill-formed UTF-16 strings properly.
// We'll send this one through as an array to avoid U+FFFD substitution.

yield return new object[] { new char[] { '\uD800', '\uD800' }, false };
}

[Theory]
Expand Down Expand Up @@ -413,27 +417,30 @@ public void SortKeyMiscTest()
public void IndexOfTest(CompareInfo compareInfo, string source, string value, int startIndex, int indexOfExpected, int lastIndexOfExpected)
{
Assert.Equal(indexOfExpected, compareInfo.IndexOf(source, value, startIndex));
if (value.Length > 0)
if (value.Length == 1)
{
Assert.Equal(indexOfExpected, compareInfo.IndexOf(source, value[0], startIndex));
}

Assert.Equal(lastIndexOfExpected, compareInfo.LastIndexOf(source, value, startIndex));
if (value.Length > 0)
if (value.Length == 1)
{
Assert.Equal(lastIndexOfExpected, compareInfo.LastIndexOf(source, value[0], startIndex));
}
}

[Theory]
[MemberData(nameof(IsSortable_TestData))]
public void IsSortableTest(string source, bool hasSurrogate, bool expected)
public void IsSortableTest(object sourceObj, bool expected)
{
string source = sourceObj as string ?? new string((char[])sourceObj);
Assert.Equal(expected, CompareInfo.IsSortable(source));

bool charExpectedResults = hasSurrogate ? false : expected;
// If the string as a whole is sortable, then all chars which aren't standalone
// surrogate halves must also be sortable.

foreach (char c in source)
Assert.Equal(charExpectedResults, CompareInfo.IsSortable(c));
Assert.Equal(expected && !char.IsSurrogate(c), CompareInfo.IsSortable(c));
}

[Fact]
Expand Down
Loading