StringSegment IndexOf behavior

[hrrrrustic]

Description

Working on reopening PR for #45021 (first one was closed due to inactivity)
Found a few more strange behaviors

First:
There is no check for the right bound in IndexOf like in the IndexOfAny
IndexOfAny:

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

Lines 519 to 529 in 911640b

	if (startIndex < 0 || Offset + startIndex > Buffer.Length)
	{
	ThrowHelper.ThrowArgumentOutOfRangeException(ExceptionArgument.start);
	}
	if (count < 0 || Offset + startIndex + count > Buffer.Length)
	{
	ThrowHelper.ThrowArgumentOutOfRangeException(ExceptionArgument.count);
	}
	index = Buffer.IndexOfAny(anyOf, Offset + startIndex, count);

IndexOf:

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

Lines 457 to 467 in 911640b

	if (!HasValue || start < 0 || (uint)offset > (uint)Buffer.Length)
	{
	ThrowHelper.ThrowArgumentOutOfRangeException(ExceptionArgument.start);
	}
	if (count < 0)
	{
	ThrowHelper.ThrowArgumentOutOfRangeException(ExceptionArgument.count);
	}
	int index = AsSpan().Slice(start, count).IndexOf(c);

Anyway, exception will be thrown in Span<T>.Slice method (exception message will be a little different), but I don't think this is by design 😄

Second:
Check for the right bound above (IndexOfAny) is quite weird, it allows to get index from original string
Example:

public static void Main(string[] args)
        {
            StringSegment segment = new StringSegment("12345", 0, 1);
            var index = segment.IndexOfAny(new[] { '5' }, 2, 3);
            Console.WriteLine(index); // Prints 4 
        }

I believe that correct value here should be -1, am I missing something?
UPD: OutOfRangeException should be thrown
So it seems like Buffer.Length have to be replaced with Length property

Regression?

Fixes will be breaking changes, but for the original issue (#45021) it was ok #46432 (comment)

[dotnet-issue-labeler]

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.

[hrrrrustic]

@gfoidl @tarekgh @davidfowl (You reviewed/commented PR #46432)
Btw, is it possible to unlock old PR or I have to create new one?

[ghost]

Tagging subscribers to this area: @eerhardt, @maryamariyan
See info in area-owners.md if you want to be subscribed.

Issue Details

---

Description

Working on reopening PR for #45021 (first one was closed due to inactivity)
Found a few more strange behaviors

First:
There is no check for the right bound in IndexOf like in the IndexOfAny
IndexOfAny:

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

Lines 519 to 529 in 911640b

	if (startIndex < 0 || Offset + startIndex > Buffer.Length)
	{
	ThrowHelper.ThrowArgumentOutOfRangeException(ExceptionArgument.start);
	}
	if (count < 0 || Offset + startIndex + count > Buffer.Length)
	{
	ThrowHelper.ThrowArgumentOutOfRangeException(ExceptionArgument.count);
	}
	index = Buffer.IndexOfAny(anyOf, Offset + startIndex, count);

IndexOf:

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

Lines 457 to 467 in 911640b

	if (!HasValue || start < 0 || (uint)offset > (uint)Buffer.Length)
	{
	ThrowHelper.ThrowArgumentOutOfRangeException(ExceptionArgument.start);
	}
	if (count < 0)
	{
	ThrowHelper.ThrowArgumentOutOfRangeException(ExceptionArgument.count);
	}
	int index = AsSpan().Slice(start, count).IndexOf(c);

Anyway, exception will be thrown in Span<T>.Slice method (exception message will be a little different), but I don't think this is by design 😄

Second:
Check for the right bound above (IndexOfAny) is quite weird, it allows to get index from original string
Example:

public static void Main(string[] args)
        {
            StringSegment segment = new StringSegment("12345", 0, 1);
            var index = segment.IndexOfAny(new[] { '5' }, 2, 3);
            Console.WriteLine(index); // Prints 4 
        }

I believe that correct value here should be -1, am I missing something?
So it seems like Buffer.Length have to be replaced with Length property

Regression?

Fixes will be breaking changes, but for the original issue (#45021) it was ok #46432 (comment)

Author:	hrrrrustic
Assignees:	-
Labels:	area-Extensions-Primitives, untriaged
Milestone:	-

[gfoidl]

exception will be thrown in Span.Slice method (exception message will be a little different), but I don't think this is by design

You are right. In #53463 (comment) it was similar, and the argument validation was kept (to throw the correct exception with the right paramName).

[tarekgh]

CC @GrabYourPitchforks

[tarekgh]

@hrrrrustic

Btw, is it possible to unlock old PR or I have to create new one?

Could you please open a new PR if it is handy? Thanks!

[villarst]

Very weird. I tried testing with var index1 = segment.IndexOfAny(new[] { '5' }, 0, 5); and var index2 = segment.IndexOfAny(new[] { '5' }); which resulted in 4 and -1 respectively. The only way you will get 4 as an output while having '5' as your char argument is if you have the index of '5' be included in the two int arguments for IndexOfAny (char[] anyOf, int startIndex, int count);. Essentially you have to let it be hit or passed for a 4 to be outputted.

My guess for why var index1 = segment.IndexOfAny(new[] { '5' }, 0, 5); is outputting a 4 is because when the IndexOfAny method is being called it is referring to the original string that is being passed in when we call StringSegment segment = new StringSegment("12345", 0, 1);

I also tested var index3 = segment.IndexOfAny(new[] { '4' }, 1, 3); to see if maybe it was because the '5' was at the end of the string but I was still able to get a number other than -1 which was 3.