dotnet / runtime

.NET is a cross-platform runtime for cloud, mobile, desktop, and IoT apps.
https://docs.microsoft.com/dotnet/core/
MIT License
14.95k stars 4.65k forks source link

[API Proposal]: Add `ThrowIfOutOfRange` helper method to `System.ArgumentOutOfRangeException` #96522

Open Corey-M opened 8 months ago

Corey-M commented 8 months ago

Background and motivation

Currently there are a number of helper methods on System.ArgumentOutOfrangeException to cover a variety of common usages. Currently these helpers target one end of a range only: ThrowIfNegativeOrZero, ThrowIfGreaterThan and so on. Since ranges have two ends by definition, it would be nice to be able to hit both ends in one call during argument validation.

API Proposal

This is modeled after the existing helper methods.

namespace System;

public partial class ArgumentOutOfRangeException
{
    [DoesNotReturn]
    private static void ThrowOutOfRange(T value, T minimum, T maximum, string? paramName) =>
        throw new ArgumentOutOfRangeException(paramName, value, SR.Format(SR.ArgumentOutOfRange_Generic_OutOfRange, value, minimum, maximum);

    public static void ThrowIfOutOfRange<T>(T value, T minimum, T maximum, [CallerArgumentExpression(nameof(value))] string? paramName = null)
        where T : IComparable<T>
    {
        if (value.CompareTo(minimum) < 0 || value.CompareTo(maximum) > 0)
            ThrowOutOfRange(value, minimum, maximum, paramName);
    }
}

API Usage

// Date example:
DateTime weekStart = GetWeekStart();
DateTime weekEnd = GetWeekEnd();
DateTime date = DateTime.Today.AddDays(2);
ArgumentOutOfRangeException.ThrowIfOutOfRange(date, weekStart, weekEnd);

// Array range checking:
int[] data = new int[10];

int GetDataPoint(int index)
{
    ArgumentOutOfRangeException.ThrowIfOutOfRange(index, 0, data.Length - 1);
    return data[index];
}

// Non-zero-based range check
int GetDataPointFromWindow(int index, int windowStart, int windowLength)
{
    ArgumentOutOfRangeException.ThrowIfOutOfRange(index, windowStart, windowStart + windowLength - 1);
    return data[index];
}

Alternative Designs

Any other method to check both the start and end of the range would be suitable, but this seems the simplest option with the broadest application.

Risks

As the proposed method is an addition only and so risk for existing code bases approaches zero.

The supplied values are inclusive start and end points which works for me conceptually, but may be out of sync with broader API consistency. It may be perferable to use a non-inclusive end value (start <= value && value < end style range check) to slightly reduce the workload of computing the end value from start and length, etc. That would change the array checking example to:

ArgumentOutOfRangeException(index, 0, data.Length);
ghost commented 8 months ago

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

Issue Details
### Background and motivation Currently there are a number of helper methods on `System.ArgumentOutOfrangeException` to cover a variety of common usages. Currently these helpers target one end of a range only: `ThrowIfNegativeOrZero`, `ThrowIfGreaterThan` and so on. Since ranges have two ends by definition, it would be nice to be able to hit both ends in one call during argument validation. ### API Proposal This is modeled after the existing helper methods. ```csharp namespace System; public partial class ArgumentOutOfRangeException { [DoesNotReturn] private static void ThrowOutOfRange(T value, T minimum, T maximum, string? paramName) => throw new ArgumentOutOfRangeException(paramName, value, SR.Format(SR.ArgumentOutOfRange_Generic_OutOfRange, value, minimum, maximum); public static void ThrowIfOutOfRange(T value, T minimum, T maximum, [CallerArgumentExpression(nameof(value))] string? paramName = null) where T : IComparable { if (value.CompareTo(minimum) < 0 || value.CompareTo(maximum) > 0) ThrowOutOfRange(value, minimum, maximum, paramName); } } ``` ### API Usage ```csharp // Date example: DateTime weekStart = GetWeekStart(); DateTime weekEnd = GetWeekEnd(); DateTime date = DateTime.Today.AddDays(2); ArgumentOutOfRangeException.ThrowIfOutOfRange(date, weekStart, weekEnd); // Array range checking: int[] data = new int[10]; int GetDataPoint(int index) { ArgumentOutOfRangeException.ThrowIfOutOfRange(index, 0, data.Length - 1); return data[index]; } // Non-zero-based range check int GetDataPointFromWindow(int index, int windowStart, int windowLength) { ArgumentOutOfRangeException.ThrowIfOutOfRange(index, windowStart, windowStart + windowLength - 1); return data[index]; } ``` ### Alternative Designs Any other method to check both the start and end of the range would be suitable, but this seems the simplest option with the broadest application. ### Risks As the proposed method is an addition only and so risk for existing code bases approaches zero. The supplied values are inclusive start and end points which works for me conceptually, but may be out of sync with broader API consistency. It may be perferable to use a non-inclusive end value (`start <= value && value < end`) to slightly reduce the workload of computing the end value from start and length, etc. That would change the array checking example to: ```csharp ArgumentOutOfRangeException(index, 0, data.Length); ```
Author: Corey-M
Assignees: -
Labels: `api-suggestion`, `area-System.Runtime`, `untriaged`, `needs-area-label`
Milestone: -
tannergooding commented 8 months ago

The problem with "range" is that there are two definitions

This may be simpler if you just made it the following and noting you could also consider having some ICollection<T> or other overloads if appropriate:

ThrowIfOutOfRange<T>(int index, ReadOnlySpan<T> array);

But it's also worth calling out that .NET (especially when considering range checks with regards to arrays, spans, collections, etc) is most typically "exclusive" because the Count/Length property represents an exclusive upper bound. There is then the InsertRange (such as for List<T>.Insert) which treats it as inclusive.

IntelliSense documentation can also help clear up some of the confusion.

I, personally, feel it better to just use two throw checks:

This is less efficient and would be one of the reasons why providing an explicit API is beneficial.

Corey-M commented 8 months ago

The problem with "range" is that there are two definitions: inclusive or exclusive end?[a] C#'s Range type takes the exclusive end form (written in math notation as [a,b)), but inclusive end forms are still relatively common.

I agree, which is why I pointed that out in the 'Risks' section. I personally prefer inclusive ranges, but end-exclusive ranges make sense too. I don't have a firm preference in which gets used.

So, when I would see ArgumentOutOfRangeException.ThrowIfNotInRange(val, a, b), it's not necessarily clear (without documentation) which form it would be in.

Well sure, if your argument names are that uninformative of course it's going to be difficult to understand what's going on. Adding document comments to give Intellisense something useful to display resolves a lot of the issue.

I, personally, feel it better to just use two throw checks:

And you can continue to do so since I'm not proposing that we scrap the other helper functions. I just think that it was an oversight to not have a helper that tests for a full range while adding helpers for all sorts of other things. Tanner pointed out it's less efficient, and I'd like to add that not only is it a lot more typing it's also unnecessary visual clutter.

[a]: Technically there's four if you account for an exclusive start, but they're even less common.

Exclusive starts on ranges is up there with 1-based array indices. Just saying.

stephentoub commented 8 months ago

For reference, such range methods were part of https://github.com/dotnet/runtime/issues/69590#issuecomment-1307687726, and after lengthy discussion weren't added.

Corey-M commented 8 months ago

@stephentoub Not entirely surprised, but considering the large number of existing APIs with similar concerns - System.Range was confusing as heck when it first came out - I'm a little bemused. I guess I'll have to stick with my own ThrowHelper classes.

stephentoub commented 8 months ago

I'm actually personally in favor of an api for this. I'm just pointing out it was discussed at length.

Daniel-Svensson commented 8 months ago

How about assing 2 method one for exclusive and one for inclusive? Or just add an inclusive version where inclusive is added to the name?

(Exclusive is a bit trickier since postfix might imply exclusive lower bound and changing parameter to range type could lose some performance since i guess it must handle "from end " indexing).

I like this proposal in general (i've missed it when the other methods were added).

It might be nice to optimize the implementation for integers to be branchless. You might even want consider optimize/limit the implementation to integer types (or primitive types) in order to not have to reason about floating point types and how to handle nan +0, -0 etc.

julealgon commented 3 months ago

@Daniel-Svensson

How about assing 2 method one for exclusive and one for inclusive? Or just add an inclusive version where inclusive is added to the name?

The problem with that is that there are actually 4 variations:

Attempting to cover all of these with overloads alone would be clunky.


@stephentoub

I'm just pointing out it was discussed at length.

Do you recall if the idea of introducing an actual Interval<T> (or Range<T> although that could be misleading now...) type was discussed back then? I've run into the issue of needing to model a range of values in multiple projects now. If such type was available natively, methods like this could just take an instance of that class and it would have ways to construct it using inclusive or exclusive values:

    public static void ThrowIfOutOfRange<T>(T value, Range<T> range, [CallerArgumentExpression(nameof(value))] string? paramName = null)
        where T : IComparable<T>

Honestly... I feel like the existing Range class should be renamed to IndexRange or something similar because it is incredibly misleading to me and it would be nice to be able to reuse Range for this new much more general-purpose type.