Runtime: Passing default SequencePosition to ReadOnlySequence.Slice shouldn't throw NullRef

[ahsonkhan]

```C#
string jsonString = "a";

ReadOnlyMemory dataMemory = Text.Encoding.UTF8.GetBytes(jsonString);

var firstSegment = new BufferSegment(dataMemory.Slice(0, 1));
ReadOnlyMemory secondMem = dataMemory.Slice(0, 0);
BufferSegment secondSegment = firstSegment.Append(secondMem);

// A two segment sequence where the second one is empty: [a]-> []
var sequence = new ReadOnlySequence(firstSegment, 0, secondSegment, secondMem.Length);
sequence = sequence.Slice(default(SequencePosition));

```text
System.NullReferenceException : Object reference not set to an instance of an object.
        Stack Trace:
          E:\GitHub\Fork\corefx\src\System.Memory\src\System\Buffers\ReadOnlySequence.Helpers.cs(243,0): at System.Buffers.ReadOnlySequence`1.BoundsCheck(SequencePosition& position)
          E:\GitHub\Fork\corefx\src\System.Memory\src\System\Buffers\ReadOnlySequence.cs(416,0): at System.Buffers.ReadOnlySequence`1.Slice(SequencePosition start)

cc @davidfowl, @pakrym

[devsko]

@ahsonkhan Should default(SequencePosition) just throw ArgumentOutOfRangeException in this case or does it have any special meaning when used for slicing?

[ahsonkhan]

Maybe, but I think one of the following makes more sense:

1. Return an empty sequence (you have sliced past the end)
2. Return the original sequence (you haven't sliced at all)

I think (1) makes more sense to me. @davidfowl, @pakrym - what do you think?

[pakrym]

I'm torn between exception and not slicing. What happens when you use a default(Range) with array?

[ahsonkhan]

What happens when you use a default(Range) with array?

cc @tarekgh - can you help answer that?

I'm torn between exception and not slicing.

I see. You don't think slicing till the end and returning an empty sequence is viable?

[pakrym]

You don't think slicing till the end and returning an empty sequence is viable?

I couldn't come up with a scenario where it would be useful.

[tarekgh]

What happens when you use a default(Range) with array?

Default Range will have 0 start index and 0 End index. The start index is inclusive and end index is exclusive. using this range with array should produce empty array as the length of the range will be 0.

CC @agocke

[pakrym]

@ahsonkhan I'm leaning towards treating default(Position) as "0" position.

So ros.Slice(default) would return the whole thing and ros.Slice(0, default) would return empty.

[davidfowl]

That seems fine and more friendly

[devsko]

I'm not sure if default(SequencePostion) should be equal to position 0. There is a different representation of position 0: SequencePosition(fistSegment, 0).
For me throwing an ArgumentOutOfRangeException is the right thing.
If I had to give it a meaning, I find most intuitive when default behaves as kind of a neutral position. When used as start it is the first and when used as last it is the end of the sequence.
c# ros.Slice(default) // whole sequence ros.Slice(10, default) // from 10 up to the end ros.Slice(default, default) // whole sequence

[pakrym]

I think Slice on ros.Slice(0, default) should behave the same way as span.Slice(0, default) or memory.Slice(0, default) or s.Substring(0, default).

Assigning contextual meaning to default(SP) doesn't feel right.

[devsko]

The 2nd parameter of the .Slice() overloads of Span/Memory/string always determines the length of the result. Therefore the result for a length of default(int) is always empty there.

On the other hand this overload of ROS.Slice() takes a SequencePosition end as parameter which points to the position of the end of the resulting slice. These two operations look similar but are not really comparable. ros.Slice(0, default) doesn't even compile because default is ambiguous between default(int) and default(SequencePosition).

I totally agree with you that ros.Slice(0, default(int)) should return an empty result - what it does.
The question is: which position in the input points default(SequencePosition) to? As mentioned above, I consider this position as invalid and thus would throw a ArgumentOutOfRangeException when it is passed as start or end.

I understood that throwing in this case should always be avoided (maybe I misunderstood?). What I tried in dotnet/corefx#35766 was to match the behavior of ros.Slice(…, default(SequencePosition)) with the overloads without a second parameter. It seemed reasonable to me that default(SequencePosition) has the same meaning as the absence of a position. But I'm also fine with defining default(SequencePosition) always as "position 0".

[pakrym]

@davidfowl @KrzysztofCwalina any preference here?

[KrzysztofCwalina]

I think default(SequencePosition) should be "position 0", and so:
Slice(default, default) returns empty sequence
Slice(default, x) returns sequence from beginning to x
Slice(x, default) return empty if x == 0 and throws "end has to be after start" ix x > 0

[ahsonkhan]

I think default(SequencePosition) should be "position 0"

That sounds good to me as well and this matches what @pakrym said earlier.

@devsko, I understand that ROS.Slice() takes SequencePosition end while Span.Slice() takes int length, but to me, matching the semantics closely makes it easy to reason about.

But I'm also fine with defining default(SequencePosition) always as "position 0".

Great :)

The clarification to that would be what @KrzysztofCwalina mentioned. If start != 0, and end = default, we should throw ArgumentOutOfRangeException

[pgovind]

Ugh, random clicked on "Close". I'm looking at this at the moment.

[pgovind]

The null dereference was because we now allow nullable objects in SequencePosition. Once I fixed that, there's still the issue of what default(SequencePosition)._object should be. If the expectation is that the default start position is 0, it makes sense that default(SequencePosition)._object = Start?