StringSegment.EndsWith inconsistent (buggy?) behavior

[adams85]

Description

Console.WriteLine("/".StartsWith("/", StringComparison.Ordinal));
// prints True

Console.WriteLine(new StringSegment("/").StartsWith("/", StringComparison.Ordinal));
// prints True

Console.WriteLine("/".EndsWith("/", StringComparison.Ordinal));
// prints True

Console.WriteLine(new StringSegment("/").EndsWith("/", StringComparison.Ordinal));
// prints False !!!

Is this by design or simply a bug?

Configuration

.NET Core 3.1.5, Windows 10 x64

[Dotnet-GitSync-Bot]

I couldn't figure out the best area label to add to this issue. If you have write-permissions please help me learn by adding exactly one area label.

[GrabYourPitchforks]

FYI to whoever addresses this bug: using string.Compare may give incorrect results, especially when dealing with linguistic comparisons (e.g., StringComparison.InvariantCulture). Use CompareInfo.IsPrefix and CompareInfo.IsSuffix instead within the implementation, as it will always give the correct results.

Tagging subscribers to this area: @eerhardt, @maryamariyan
Notify danmosemsft if you want to be subscribed.

[maryamariyan]

Here's the EndsWith logic on StringSegment:

runtime/src/libraries/Microsoft.Extensions.Primitives/src/StringSegment.cs

Lines 333 to 352 in bd6cbe3

	/// </exception>
	[MethodImpl(MethodImplOptions.AggressiveInlining)]
	public bool EndsWith(string text, StringComparison comparisonType)
	{
	if (text == null)
	{
	ThrowHelper.ThrowArgumentNullException(ExceptionArgument.text);
	}
	var result = false;
	var textLength = text.Length;
	var comparisonLength = Offset + Length - textLength;
	if (HasValue && comparisonLength > 0)
	{
	result = string.Compare(Buffer, comparisonLength, text, 0, textLength, comparisonType) == 0;
	}
	return result;
	}

The problem is on the comparisonLength.

@adams85 would you like to push up a PR to fix this?

[adams85]

@maryamariyan No, I'd pass on this. I guess it's not too much work to fix the method itself but then - if one wants to be thorough - the full codebase needs to be scrutinized to make sure the fix doesn't break anything. Unfortunately, I won't have the time for this in the near future.

[GrabYourPitchforks]

Reiterating my previous comment:

The index needs to be fixed, sure. But this code should not use string.Compare. It should instead use CompareInfo.IsPrefix or CompareInfo.IsSuffix, as mentioned earlier.

(Or, even better, just use the StartsWith or EndsWith extensions already on ROS<char>.)