Improve ArgumentOutOfRange exception in Math.Clamp

[pangeorg]

Replaces e.g. "System.ArgumentException: '0' cannot be greater than -2." with
System.ArgumentException: 'minValue = 0' cannot be greater than maxValue = -2
This follows the behavior of System.Math.Random

Fix #115337

[dotnet-policy-service]

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

[xtqqczze]

Should the same changes be made in BigInteger.Clamp?

runtime/src/libraries/System.Runtime.Numerics/src/System/Numerics/BigInteger.cs

Lines 3967 to 3970 in 10bc776

	static void ThrowMinMaxException<T>(T min, T max)
	{
	throw new ArgumentException(SR.Format(SR.Argument_MinMaxValue, min, max));
	}

[kasperk81]

better replace ThrowMinMaxException with

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

Line 111 in ed96395

private static void ThrowLess<T>(T value, T other, string? paramName) =>

[jeffhandley]

@tannergooding I'm going to merge this improvement to the exception message and I'll file a new issue recommending we relax Math.Clamp to have the logic you described instead of throwing.

[xtqqczze]

Looks like there are large regressions in code quality for floating point types, see: https://csharp.godbolt.org/z/K4P4xvWdK.

[tannergooding]

That'd be because it uses IComparable which is not the same as x < y due to having to handle and normalize NaN

[xtqqczze]

That'd be because it uses IComparable which is not the same as x < y due to having to handle and normalize NaN

Yeah, this PR also changes behaviour in relation to NaN, see sharplab.

[jkotas]

It sounds like that this PR should be reverted?

[xtqqczze]

It sounds like that this PR should be reverted?

Yeah, Math.Clamp now has different behaviour to INumber.Clamp:

runtime/src/libraries/System.Private.CoreLib/src/System/Numerics/INumber.cs

Lines 25 to 38 in 4b8a478

	static virtual TSelf Clamp(TSelf value, TSelf min, TSelf max)
	{
	if (min > max)
	{
	Math.ThrowMinMaxException(min, max);
	}
	TSelf result = value;
	result = TSelf.Max(result, min);
	result = TSelf.Min(result, max);
	return result;
	}

[xtqqczze]

That'd be because it uses IComparable which is not the same as x < y due to having to handle and normalize NaN

@tannergooding It is quite unintuitive that ThrowIfLessThan behaves as if it was a hypothetical ThrowIfBeforeWhenSorted. If this cannot be changed, it should be documented.

[tannergooding]

It is quite unintuitive that ThrowIfLessThan behaves as if it was a hypothetical

It's that there are multiple types of relational comparisons, just like there are multiple types of "equality" (Equals vs == can differ). This is just a standard part of .NET and many different ecosystems.

You have different considerations based on what the spec may require and what may be required for sorting or for use in hash sets, etc. You may need to even customize this which is why we also define https://learn.microsoft.com/en-us/dotnet/api/system.numerics.totalorderieee754comparer-1?view=net-9.0.

There is no single right answer and its not something we can document absolutely everywhere that takes IComparable.