runtime: [API Proposal]: ArgumentOutOfRangeException helper methods

UPD: Fully rewrote description after creating PR UPD2: Initially IComparisonOperators<T, T> was used instead of IComparable<T> Proposed methods (split them as mush as possible) after looking at /runtime code: (Not sure about struct constraint, everything looks working without it, but it exists in Linq.Max/Min)

namespace System;

public class ArgumentOutOfRangeException : ArgumentException
{
      public static void ThrowIfZero<T>(T value, [CallerArgumentExpression("value")] string? paramName = null) 
          where T : struct, INumberBase<T>, IComparable<T>;
      public static void ThrowIfNegative<T>(T value, [CallerArgumentExpression("value")] string? paramName = null) 
          where T : struct, INumberBase<T>, ISignedNumber<T>, IComparable<T>;
      public static void ThrowIfNegativeOrZero<T>(T value, [CallerArgumentExpression("value")] string? paramName = null) 
          where T : struct, INumberBase<T>, ISignedNumber<T>, IComparable<T>;
      
      public static void ThrowIfGreaterThan<T>(T value, T other, [CallerArgumentExpression("value")] string? paramName = null) 
          where T : struct, IComparable<T>;
      public static void ThrowIfGreaterThanOrEqual<T>(T value, T other, [CallerArgumentExpression("value")] string? paramName = null) 
          where T : struct, IComparable<T>;
      
      public static void ThrowIfLessThan<T>(T value, T other, [CallerArgumentExpression("value")] string? paramName = null)
          where T : struct, IComparable<T>;
      public static void ThrowIfLessThanOrEqual<T>(T value, T other, [CallerArgumentExpression("value")] string? paramName = null) 
          where T : struct, IComparable<T>;
      
      public static void ThrowIfNotBetween<T>(T value, T inclusiveLower, T inclusiveUpper, [CallerArgumentExpression("value")] string? paramName = null)
          where T : struct, IComparable<T>;
}

Some statistics

Actual usages count will be a little bit lower since some of them are under libraries with old targets (net 4.x or netstandard)

ThrowIfNull Usages: <10 I didn’t split this in PR, but saw a few of them with ArgumentOutOfRangeException, InvalidArgumentException and base ArgumentException. It probably makes sense to add this one for better null analysis
ThrowIfZero I personally don’t think that this one is good enough, but there is a #68339 Usages: 8
ThrowIfNegative Usages: 755
ThrowIfNegativeOrZero Usages: 100
ThrowIfGreaterThan Usages: 204
ThrowIfGreaterThanOrEqual Usages: 32
ThrowIfLessThan Usages: 174
ThrowIfLessThanOrEqual Usages: 4
ThrowIfNotBetween Usages: 255

Risks & Problems & Questions

Using casts while passing argument to helper will cause saving it to ParamName property in ArgumentException. That’s break all tests with Assert.Throw(paramName, () => {}) because they’re strictly comparing parameter name and ParamName property. I don’t think this is a problem for end users, but replacement in /runtime could be tricky
~~ThrowIf should contains additional argument for passing custom message. Should it be just string or InterpolatedStringHandler?~~ Discussed in #69637 and ThrowIf completely removed from proposal.
ThrowIfLessThan and others similar methods with IComparisonOperators constraint doesn’t work for TimeSpan, DateTime, etc. Related to #67744, but Idk why these types can’t implement IComparisonOperators that doesn’t cause any overflowing issues. Could be replaced by IComparable<T>? UPD: replaced by IComparable<T>
ThrowIfNegative{OrZero} requires too much for custom types due to IBaseNumber<> constraint (we only need T.Zero). I think most of the users will use ThrowIfLess with manual passing “zero” value instead (only IComparable<T> required for this)
I saw a lot of similar conditions with throwing base ArgumentException instead ArgumentOutOfRangeException. Should we duplicate these methods for it?
#70515
Renaming ThrowIfNotBetween -> ThrowIfOutsideRange https://github.com/dotnet/runtime/pull/69767#discussion_r906962047

About this issue

Original URL
State: closed
Created 2 years ago
Reactions: 10
Comments: 31 (30 by maintainers)

Most upvoted comments

Is there anything else to do to make this ready to review?

This is an issue where we need high confidence that it’s going to address most of our own usage; otherwise, we’ll take it through API review and either there will be a bunch of questions about what’s missing, or it’ll get approved and then when we go to use it we might find a bunch of issues, and we’ll need to flip it back to unapproved to take it back through again.

As such, my preference is that the proposal actually include doing the work to roll it out across dotnet/runtime, so that we can fully understand what issues there might be / what might be missing. That work will be necessary anyway, so it’s better to just front-load it.

Obviously some things might need to be tweaked post API-review, but hopefully it’d mostly be search-and-replace for things like naming.

stephentoub on May 23, 2022

These helper APIs are meant for the most common cases where the caller wants to use a standard error message. I don’t think these APIs should accept a message; the implementation can use the bounds it’s given to create the message, but the user shouldn’t be able to customize it. At that point, they can just write their own check.

stephentoub on May 20, 2022

@hrrrrustic, thanks for all of your work here. Do you want to revive your work from before and open a PR for this?

Yes, you can assign this one on me I’ll also look at ThrowIfNotBetween usages for some patterns except ThrowIfNotInRange(index, ICollection)

hrrrrustic on Nov 11, 2022

Thanks, @hrrrrustic. The set looks reasonable to me. A few notes:

We should use IComparison<T> instead of IComparisonOperators<T, T>.

ThrowIfNull should be removed; I assume that’s just a copy/paste error.

I assume all of these are intended to be on ArgumentOutOfRangeException. Please update the API outline to include that around the methods.

For ThrowIfNotBetween, I assume the left/right boundaries are meant to be inclusive for the range, and thus exclusive for what’s not between them, e.g. calling it with 3, 7 would throw for 2 and 8 but not 3 or 7. We should try to come up with parameter names to make that clear, maybe lowerInclusive and upperInclusive.

Why does ThrowIfZero require the T be signed?

For Martin’s proposed ThrowIfOutOfRange, I’m hesitant there as that will inevitably be slower than what many developers write for enums they know about, e.g. (uint)dayOfWeek <= (uint)DayOfWeek.Saturday is much faster than Enum.IsDefined<DayOfWeek>(day), yet by adding it we’ll be pushing developers to use it instead. Let’s keep that out of this discussion. Martin, if you’d like to add that to the other issue you opened, please feel free to do so.

Assuming that you mean IComparable<T>, udpated
Yep, that was in initial proposal, but removed later
Done
Renamed a little bit arguments and add comment above
Copy/paste error, removed
Yep, thought about enum support, but looks like there is no way to do it without losing some performance. By the way, there were something around 400 usages on enums in /runtime (can be found in linked PR)

hrrrrustic on Nov 3, 2022

I’m enthused about this too. We’re working through some planning tasks right now, and as part of that we’re crafting our “work planned” epics across our areas. I’m going to mark this as 8.0.0 so that we can include this and commit to refining these if/as needed and getting the approved set into an early preview of .NET 8.

jeffhandley on Oct 28, 2022

I was just looking for a throw helper that would help in the Console App scenario where I want to ensure I received the expected number of arguments. ArgumentOutOfRangeException.ThrowIfLessThan would be useful for that. Before I reached for that though, I was also curious if ArgumentException.ThrowIfNullOrEmpty([NotNull] string?[]? argument, ...) existed (it doesn’t).

jeffhandley on Oct 20, 2022

It’ll depend on whether the message contains any meaningful additional information. In the example you reference, the message is just “StartIndex cannot be less than zero”, which doesn’t provide anything useful beyond the default message would for a ThrowIfNegative method, so this case could be changed. Thanks.

stephentoub on May 24, 2022