Simplify range validation in Memory.Span

[xtqqczze]

• desiredStartIndex cannot be negative as high order bit of _index has been removed.
• desiredLength cannot be negative as all public constructors validate _length is not negative.
• lengthOfUnderlyingSpan cannot be negative as _object.Length as the length property of an array, string, or span cannot be negative.

[dotnet-policy-service]

Tagging subscribers to this area: @dotnet/area-system-memory
See info in area-owners.md if you want to be subscribed.

[EgorBo]

if this + overflows the check is passed, isn't ?

[xtqqczze]

desiredLength should never be negative, this could do with an assert

runtime/src/libraries/System.Private.CoreLib/src/System/Memory.cs

Line 330 in 8b10891

int desiredLength = _length;

[Clockwork-Muse]

Yes - it's why the 32-bit target check is written the way it was below (which is a common overflow-avoiding pattern).

We can't "simplify" this, though - the 64 and 32 bit targets are different for a reason; they're taking advantage of different bit widths (or not able to) on different platforms.

[tannergooding]

This is the kind of thing where the logic is getting even more dense and hard to comprehend

I would much rather we do such transforms in the JIT when we know that a given invariant is held, so that the high level managed code can remain easy to understand.

Otherwise, this needs a significant number of comments and asserts covering those invariants and why the various overflow considerations are "safe" before it could be accepted.

[xtqqczze]

The check here is only to prevent undefined behaviour if the struct is torn, if it wasn't for this it could be removed completely.

[xtqqczze]

I would much rather we do such transforms in the JIT when we know that a given invariant is held, so that the high level managed code can remain easy to understand.

Related issue: #118587

[xtqqczze]

desiredLength should never be negative, this could do with an assert

runtime/src/libraries/System.Private.CoreLib/src/System/Memory.cs

Line 330 in 8b10891

int desiredLength = _length;

We already assume the invariant _length >= 0 holds

runtime/src/libraries/System.Private.CoreLib/src/System/Memory.cs

Lines 225 to 230 in 1e322fd

	public Memory<T> Slice(int start)
	{
	if ((uint)start > (uint)_length)
	{
	ThrowHelper.ThrowArgumentOutOfRangeException(ExceptionArgument.start);
	}

[tannergooding]

The check here is only to prevent undefined behaviour if the struct is torn, if it wasn't for this it could be removed completely.

Yes, which is necessary/important. The team has a firm stance that we can't rely on users doing safe threading and that we need to do the "right things".

It's the same general reason we have both sets of lengths checks when dealing with List<T> (CC. @GrabYourPitchforks)

We already assume the invariant _length >= 0 holds

It's still something where adding an assert is beneficial as it explicitly documents the expectation.

But, I'd generally rather we have the JIT doing these types of optimizations. The managed code should prefer being readable/understandable first. If something is truly perf critical, then making it less readable with added comments/asserts is ok. However, the JIT automatically recognizing the critical pattern and doing the right thing is even better, as then the code stays readable and other paths likely benefit as well.

[xtqqczze]

@MihuBot

[MihaZupan]

Changes here overlap with #115275

[Clockwork-Muse]

Yes - it's why the 32-bit target check is written the way it was below (which is a common overflow-avoiding pattern).

We can't "simplify" this, though - the 64 and 32 bit targets are different for a reason; they're taking advantage of different bit widths (or not able to) on different platforms.