Fix various LastIndexOf bugs when given zero-length target values (#34616)

- string.LastIndexOf(string.Empty) shouldn't perform -1 adjustment
- MemoryExtensions.LastIndexOf(ROS<char>, string.Empty) shouldn't return 0
- Tighten up some existing unit tests

Author: GrabYourPitchforks

src/libraries/Common/tests/Tests/System/StringTests.cs

@@ -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);
@@ -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);
@@ -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);
@@ -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));
@@ -4068,8 +4070,8 @@ public static void LastIndexOf_TurkishI_EnglishUSCulture()
         }
 
         [Theory]
-        [InlineData("foo", 2)]
-        [InlineData("hello", 4)]
+        [InlineData("foo", 3)]
+        [InlineData("hello", 5)]
         [InlineData("", 0)]
         public static void LastIndexOf_EmptyString(string s, int expected)
         {
@@ -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]
@@ -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);
@@ -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);
@@ -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);
@@ -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);

src/libraries/System.Globalization/tests/CompareInfo/CompareInfoTests.LastIndexOf.cs

@@ -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 };
@@ -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 };
@@ -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;
@@ -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));
             }
         }
 

src/libraries/System.Globalization/tests/CompareInfo/CompareInfoTests.cs

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