-
Notifications
You must be signed in to change notification settings - Fork 4.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[API Proposal]: ArgumentOutOfRangeException helper methods #69590
Comments
I couldn't figure out the best area label to add to this issue. If you have write-permissions please help me learn by adding exactly one area label. |
@tannergooding, were you looking to add an IsBetween method to one of the generic math interfaces? |
Can you elaborate? Do you mean the message needs to include information about the bounds being checked? |
Yes. Most of the usages in dotnet/runtime use prepared messages from runtime/src/libraries/System.Net.Sockets/src/System/Net/Sockets/NetworkStream.cs Line 131 in 6ca8c9b
But I think it is fine to do something like this sometimes if (arg < someMinValue) throw ArgumentOutOfRangeException(nameof(arg), $"value has to be at least {someMinValue}) and in the new API laziness of interpolation will be lost without interpolation handler |
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. |
Yep. I'll be getting a proposal up later today most likely. |
This one needs a different name. In the realm of |
I'd likely recommend the other APIs have slightly different names as well:
|
Agree, but not sure how can Updated proposal for other methods. |
This also "fits" the naming convention we'd presumably do if we had |
Uhh, make sense, updated proposal |
Tagging subscribers to this area: @dotnet/area-system-runtime Issue DetailsBackground and motivationSimilar to #62628, #58047 and #51700 (also #68339) Tried to collect all possible helpers for
Originally posted by @stephentoub in #68339 (comment) API ProposalAll methods below are inside Group 1: public static void ThrowIfNegative<T>(T value, [CallerArgumentExpression("value")] string? paramName = null)
where T: ISignedNumber<T> {}
public static void ThrowIfNegativeOrZero<T>(T value, [CallerArgumentExpression("value")] string? paramName = null)
where T: INumber<T> {} Group 2: // Note that there are no 'INumber<T>' constraint! This methods can be used for any types, not only for numbers.
// Also possible to extend usages for user defined types by implementing 'IComparisonOperators'
public static void ThrowIfGreaterThan<T>(T value, T other, [CallerArgumentExpression("value")] string? paramName = null)
where T: IComparisonOperators<T, T> {}
public static void ThrowIfLowerThan<T>(T value, T other, [CallerArgumentExpression("value")] string? paramName = null)
where T: IComparisonOperators<T, T> {}
public static void ThrowIfNotBetween<T>(T value, T left, T right, [CallerArgumentExpression("value")] string? paramName = null)
where T: IComparisonOperators<T, T> {} API UsageGroup 1:
runtime/src/libraries/System.Private.CoreLib/src/System/Version.cs Lines 28 to 39 in 6ca8c9b
There is also kind of this helper in runtime/src/libraries/System.Private.CoreLib/src/System/Random.cs Lines 118 to 122 in 0c6d412
Lines 21 to 23 in 0c6d412
Group 2:
runtime/src/libraries/System.Collections/src/System/Collections/Generic/SortedList.cs Lines 446 to 448 in 6ca8c9b
Alternative Designs
Risks
|
Is there anything else to do to make this ready to review? @stephentoub @tannergooding
I don't think |
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 runtime/src/libraries/System.Private.CoreLib/src/System/String.cs Lines 88 to 89 in 6ca8c9b
ArgumentOutOfRangeException.ThrowIfNegative(startIndex); There are only 79 files that uses ArgumentOutOfRangeException only with paramName overload |
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 I created a PR with changes across /runtime and updated the proposal See also #69767 (comment) |
@stephentoub @tannergooding Is there anything else to do here to make this one ready to review or at least get some thoughts? As I see review meetings started processing issues for net8 |
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. |
@stephentoub @tannergooding Just met another copy paste helper class with these methods, any chance to make progress on this? 😢 |
The proposal looks reasonable to me. I'll let Stephen comment as well and then we can look at getting this into API review. |
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. |
Please consider this addition to validate enum values: /// <summary>
/// Throws an <see cref="System.ArgumentOutOfRangeException"/> if the enum value is not valid.
/// </summary>
/// <param name="argument">The argument to evaluate.</param>
/// <param name="paramName">The name of the parameter being checked.</param>
/// <typeparam name="T">The type of the enumeration.</typeparam>
/// <returns>The original value of <paramref name="argument"/>.</returns>
public static T ThrowIfOutOfRange<T>(T argument, [CallerArgumentExpression("argument")] string paramName = "")
where T : struct, Enum
{
#if NET5_0_OR_GREATER
if (!Enum.IsDefined<T>(argument))
#else
if (!Enum.IsDefined(typeof(T), argument))
#endif
{
throw new ArgumentOutOfRangeException(paramName, $"{argument} is an invalid value for enum type {typeof(T)}");
}
return argument;
} |
Thanks, @hrrrrustic. The set looks reasonable to me. A few notes:
|
|
It's a shame to not support enums, it's a fairly common use case. Then perhaps an overload which has the developer specify th lower bound and upper bound of the enum value explicitly? |
I don't remember any way to compare generic enums without boxing. We only have I guess there is a #17456 for that |
namespace System;
public partial class ArgumentOutOfRangeException
{
public static void ThrowIfZero<T>(T value, [CallerArgumentExpression("value")] string? paramName = null)
where T : INumberBase<T>;
public static void ThrowIfNegative<T>(T value, [CallerArgumentExpression("value")] string? paramName = null)
where T : INumberBase<T>;
public static void ThrowIfNegativeOrZero<T>(T value, [CallerArgumentExpression("value")] string? paramName = null)
where T : INumberBase<T>;
public static void ThrowIfGreaterThan<T>(T value, T other, [CallerArgumentExpression("value")] string? paramName = null)
where T : IComparable<T>;
public static void ThrowIfGreaterThanOrEqual<T>(T value, T other, [CallerArgumentExpression("value")] string? paramName = null)
where T : IComparable<T>;
public static void ThrowIfLessThan<T>(T value, T other, [CallerArgumentExpression("value")] string? paramName = null)
where T : IComparable<T>;
public static void ThrowIfLessThanOrEqual<T>(T value, T other, [CallerArgumentExpression("value")] string? paramName = null)
where T : IComparable<T>;
} |
@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 |
Fixed by #78222 |
UPD: Fully rewrote description after creating PR
UPD2: Initially
IComparisonOperators<T, T>
was used instead ofIComparable<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)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)
Usages: <10
I didn't split this in PR, but saw a few of them with
ArgumentOutOfRangeException
,InvalidArgumentException
and baseArgumentException
. It probably makes sense to add this one for better null analysisI personally don't think that this one is good enough, but there is a [API Proposal]: Add ArgumentOutOfRangeException.ThrowIfZero() #68339
Usages: 8
Usages: 755
Usages: 100
Usages: 204
Usages: 32
Usages: 174
Usages: 4
Usages: 255
Risks & Problems & Questions
ParamName
property inArgumentException
. That's break all tests withAssert.Throw(paramName, () => {})
because they're strictly comparing parameter name andParamName
property. I don't think this is a problem for end users, but replacement in /runtime could be trickyDiscussed in [API Proposal]: More helper methods for exception types #69637 andThrowIf
should contains additional argument for passing custom message. Should it be juststring
orInterpolatedStringHandler
?ThrowIf
completely removed from proposal.UPD: replaced byThrowIfLessThan
and others similar methods withIComparisonOperators
constraint doesn't work for TimeSpan, DateTime, etc. Related to Determine recommended behavior of checked/unchecked operators for DateTime and similar types #67744, but Idk why these types can't implement IComparisonOperators that doesn't cause any overflowing issues. Could be replaced by IComparable<T>?IComparable<T>
ThrowIfNegative{OrZero}
requires too much for custom types due toIBaseNumber<>
constraint (we only need T.Zero). I think most of the users will useThrowIfLess
with manual passing "zero" value instead (onlyIComparable<T>
required for this)ArgumentException
insteadArgumentOutOfRangeException
. Should we duplicate these methods for it?ThrowIfNotBetween
->ThrowIfOutsideRange
ArgumentOutOfRangeException throw helpers #69767 (comment)The text was updated successfully, but these errors were encountered: