[API Proposal]: Consider changing return types of new throw helpers

[stephentoub]

In .NET 6 we introduced:

class ArgumentNullException
{
+    public static void ThrowIfNull([NotNull] object? argument, [CallerArgumentExpression(nameof(argument))] string? paramName = null);
}

In .NET 7 we introduced:

class ArgumentNullException
{
+    public static void ThrowIfNull([NotNull] void* argument, [CallerArgumentExpression(nameof(argument))] string? paramName = null);
}
class ArgumentException
{
+    public static void ThrowIfNullOrEmpty([NotNull] string? argument, [CallerArgumentExpression(nameof(argument))] string? paramName = null);
}

Now in .NET 8, we've introduced:

class ArgumentException
{
+    public static void ThrowIfNullOrWhiteSpace([NotNull] string? argument, [CallerArgumentExpression(nameof(argument))] string? paramName = null);
}
class ArgumentOutOfRangeException
{
+    public static void ThrowIfEqual<T>(T value, T other, [CallerArgumentExpression(nameof(value))] string? paramName = null) where T : IEquatable<T>?;
+    public static void ThrowIfGreaterThan<T>(T value, T other, [CallerArgumentExpression(nameof(value))] string? paramName = null) where T : IComparable<T>;
+    public static void ThrowIfGreaterThanOrEqual<T>(T value, T other, [CallerArgumentExpression(nameof(value))] string? paramName = null) where T : IComparable<T>;
+    public static void ThrowIfLessThan<T>(T value, T other, [CallerArgumentExpression(nameof(value))] string? paramName = null) where T : IComparable<T>;
+    public static void ThrowIfLessThanOrEqual<T>(T value, T other, [CallerArgumentExpression(nameof(value))] string? paramName = null) where T : IComparable<T>;
+    public static void ThrowIfNegative<T>(T value, [CallerArgumentExpression(nameof(value))] string? paramName = null) where T : INumberBase<T>;
+    public static void ThrowIfNegativeOrZero<T>(T value, [CallerArgumentExpression(nameof(value))] string? paramName = null) where T : INumberBase<T>;
+    public static void ThrowIfNotEqual<T>(T value, T other, [CallerArgumentExpression(nameof(value))] string? paramName = null) where T : IEquatable<T>?;
+    public static void ThrowIfZero<T>(T value, [CallerArgumentExpression(nameof(value))] string? paramName = null) where T : INumberBase<T>;
}

We can't change the 3 methods we shipped in .NET 6 and .NET 7, but we still have time to tweak the new 10 methods we're shipping in .NET 8. In particular, we've had some requests for returning the input being validated, e.g. that would make them instead be:

class ArgumentException
{
+    public static string ThrowIfNullOrWhiteSpace([NotNull] string? argument, [CallerArgumentExpression(nameof(argument))] string? paramName = null);
}
class ArgumentOutOfRangeException
{
+    public static T ThrowIfEqual<T>(T value, T other, [CallerArgumentExpression(nameof(value))] string? paramName = null) where T : IEquatable<T>?;
+    public static T ThrowIfGreaterThan<T>(T value, T other, [CallerArgumentExpression(nameof(value))] string? paramName = null) where T : IComparable<T>;
+    public static T ThrowIfGreaterThanOrEqual<T>(T value, T other, [CallerArgumentExpression(nameof(value))] string? paramName = null) where T : IComparable<T>;
+    public static T ThrowIfLessThan<T>(T value, T other, [CallerArgumentExpression(nameof(value))] string? paramName = null) where T : IComparable<T>;
+    public static T ThrowIfLessThanOrEqual<T>(T value, T other, [CallerArgumentExpression(nameof(value))] string? paramName = null) where T : IComparable<T>;
+    public static T ThrowIfNegative<T>(T value, [CallerArgumentExpression(nameof(value))] string? paramName = null) where T : INumberBase<T>;
+    public static T ThrowIfNegativeOrZero<T>(T value, [CallerArgumentExpression(nameof(value))] string? paramName = null) where T : INumberBase<T>;
+    public static T ThrowIfNotEqual<T>(T value, T other, [CallerArgumentExpression(nameof(value))] string? paramName = null) where T : IEquatable<T>?;
+    public static T ThrowIfZero<T>(T value, [CallerArgumentExpression(nameof(value))] string? paramName = null) where T : INumberBase<T>;
}

at which point we would also do so for all new throw helpers of the same ilk.

Should we?

[ghost]

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

Issue Details

---

In .NET 6 we introduced:

class ArgumentNullException
{
+    public static void ThrowIfNull([NotNull] object? argument, [CallerArgumentExpression(nameof(argument))] string? paramName = null);
}

In .NET 7 we introduced:

class ArgumentNullException
{
+    public static void ThrowIfNull([NotNull] void* argument, [CallerArgumentExpression(nameof(argument))] string? paramName = null);
}
class ArgumentException
{
+    public static void ThrowIfNullOrEmpty([NotNull] string? argument, [CallerArgumentExpression(nameof(argument))] string? paramName = null);
}

Now in .NET 8, we've introduced:

class ArgumentException
{
+    public static void ThrowIfNullOrWhiteSpace([NotNull] string? argument, [CallerArgumentExpression(nameof(argument))] string? paramName = null);
}
class ArgumentOutOfRangeException
{
+    public static void ThrowIfEqual<T>(T value, T other, [CallerArgumentExpression(nameof(value))] string? paramName = null) where T : IEquatable<T>?;
+    public static void ThrowIfGreaterThan<T>(T value, T other, [CallerArgumentExpression(nameof(value))] string? paramName = null) where T : IComparable<T>;
+    public static void ThrowIfGreaterThanOrEqual<T>(T value, T other, [CallerArgumentExpression(nameof(value))] string? paramName = null) where T : IComparable<T>;
+    public static void ThrowIfLessThan<T>(T value, T other, [CallerArgumentExpression(nameof(value))] string? paramName = null) where T : IComparable<T>;
+    public static void ThrowIfLessThanOrEqual<T>(T value, T other, [CallerArgumentExpression(nameof(value))] string? paramName = null) where T : IComparable<T>;
+    public static void ThrowIfNegative<T>(T value, [CallerArgumentExpression(nameof(value))] string? paramName = null) where T : INumberBase<T>;
+    public static void ThrowIfNegativeOrZero<T>(T value, [CallerArgumentExpression(nameof(value))] string? paramName = null) where T : INumberBase<T>;
+    public static void ThrowIfNotEqual<T>(T value, T other, [CallerArgumentExpression(nameof(value))] string? paramName = null) where T : IEquatable<T>?;
+    public static void ThrowIfZero<T>(T value, [CallerArgumentExpression(nameof(value))] string? paramName = null) where T : INumberBase<T>;
}

We can't change the 3 methods we shipped in .NET 6 and .NET 7, but we still have time to tweak the new 10 methods we're shipping in .NET 8. In particular, we've had some requests for returning the input being validated, e.g. that would make them instead be:

class ArgumentException
{
+    public static string ThrowIfNullOrWhiteSpace([NotNull] string? argument, [CallerArgumentExpression(nameof(argument))] string? paramName = null);
}
class ArgumentOutOfRangeException
{
+    public static T ThrowIfEqual<T>(T value, T other, [CallerArgumentExpression(nameof(value))] string? paramName = null) where T : IEquatable<T>?;
+    public static T ThrowIfGreaterThan<T>(T value, T other, [CallerArgumentExpression(nameof(value))] string? paramName = null) where T : IComparable<T>;
+    public static T ThrowIfGreaterThanOrEqual<T>(T value, T other, [CallerArgumentExpression(nameof(value))] string? paramName = null) where T : IComparable<T>;
+    public static T ThrowIfLessThan<T>(T value, T other, [CallerArgumentExpression(nameof(value))] string? paramName = null) where T : IComparable<T>;
+    public static T ThrowIfLessThanOrEqual<T>(T value, T other, [CallerArgumentExpression(nameof(value))] string? paramName = null) where T : IComparable<T>;
+    public static T ThrowIfNegative<T>(T value, [CallerArgumentExpression(nameof(value))] string? paramName = null) where T : INumberBase<T>;
+    public static T ThrowIfNegativeOrZero<T>(T value, [CallerArgumentExpression(nameof(value))] string? paramName = null) where T : INumberBase<T>;
+    public static T ThrowIfNotEqual<T>(T value, T other, [CallerArgumentExpression(nameof(value))] string? paramName = null) where T : IEquatable<T>?;
+    public static T ThrowIfZero<T>(T value, [CallerArgumentExpression(nameof(value))] string? paramName = null) where T : INumberBase<T>;
}

at which point we would also do so for all new throw helpers of the same ilk.

Should we?

Author:	stephentoub
Assignees:	-
Labels:	area-System.Runtime, api-ready-for-review
Milestone:	8.0.0

[jkotas]

Should we?

Do you have some good examples of how we would change the code in this repo to take advantage of the return value?

[stephentoub]

Do you have some good examples of how we would change the code in this repo to take advantage of the return value?

I don't think we would.

It ends up being a stylistic request for folks that prefer the style of doing things today like:

int value = input >= 0 ? input : throw new ArgumentOutOfRangeException(...);

and would like to instead do:

int value = ArgumentOutOfRangeException.ThrowIfNegative(input);

rather than:

ArgumentOutOfRangeException.ThrowIfNegative(input);
int value = input;

I'm personally not a fan of that style, but I've opened this to represent those who are :) (we discussed it briefly when adding ThrowIfNullOrWhiteSpace and said we'd have a follow-on conversation after I opened an issue for it)

The one place this currently has meaningful "I couldn't otherwise use the API" impact is in places where only expressions are usable, such as when delegating to another this/base ctor:

public Blah(int input) : base(ArgumentOutOfRangeException.ThrowIfNegative(input)) { ... }

It's possible we'd use that somewhere in dotnet/runtime, but I can't think of any off the top of my head. I do know of some places where we might have done that for ArgumentNullException.ThrowIfNull, but only if ThrowIfNull returned the T input, and what we shipped neither returns anything nor is generic. For inlining/size reasons we made it non-generic, and once it's non-generic returning the instance as object isn't helpful. Such use might also be addressed in a future version of C#.

[Sergio0694]

Maybe also worth noting (other than that assignment style Stephen already showed, which I also personally really dislike), making these APIs return a value would trigger a ton of warnings for everyone having warnings enabled for implicitly discarding return values. All those developers would have to always use _ = ThrowHelperThrowIf.Whatever(...) every single time if we made those APIs return values. Full disclosure I'm also in that group, as I've enabled that in all my repos, as I found that to be an extremely valuable too to help spot errors 🥲

[jkotas]

It ends up being a stylistic request for folks that prefer the style of doing things today like:
int value = ArgumentOutOfRangeException.ThrowIfNegative(input);

Why not just use input in the rest of the method in this example?

It is not just a style. Creating extra unused return values and extra locals has potential negative JIT throughput and code quality impact. The JIT should be able to clean it up for the simple cases, but complex methods can see some hit.

[tannergooding]

I'm also not a huge fan of this style; but the other place it comes up is in places like switch patterns where the thrower can only be used if it returns something (ideally the language would improve that instead, but its hard in the case the function actually returns, since the compiler can't "prove" it doesn't)

[stephentoub]

Why not just use input in the rest of the method in this example?

That was just me quickly writing something out. Often the thing being assigned to is, e.g., a field:

_value = ArgumentOutOfRangeException.ThrowIfNegative(input);

Again, I'm not actually pushing for this, just trying to represent the viewpoint.

[MihaZupan]

Personally, I would have liked ThrowIfNull to have a return value for use in ctors (RIP !!)

public Foo(string arg)
{
    _field = ArgumentNullException.ThrowIfNull(arg);
}

But if changing ThrowIfNull is out of the question due to compat, having the option to use such a pattern for some validation, but not for null feels worse than just sticking to the current pattern.

[danmoseley]

I also agree we shouldn't change this. But just curious wrt the shipped ones - would we consider changing void return type a breaking change? (To reflection I guess?) I thought we did not.

[teo-tsirpanis]

Changing the return type of a method would break all compiled code using it; the return type of a method is part of the IL signature.

[danmoseley]

Changing the return type of a method would break all compiled code using it; the return type of a method is part of the IL signature.

You're right, and we document it I see.

runtime/docs/coding-guidelines/breaking-change-rules.md

Line 250 in 3747edb

* Changing the type of a property, field, parameter or return value

[hez2010]

Should we?

Do you have some good examples of how we would change the code in this repo to take advantage of the return value?

Without this change, it is impossible to use those helpers in a switch expression.

I already started to define my own ThrowHelpers with T as the return value instead of using the built-in void version of ThrowHelpers because of this.

Foo x = obj switch
{
    null => Throw<Foo>()
}

But I still don't see the value to do this in the BCL. C# doesn't have bottom types (never type which is the subtype of every type) today, so we have to work around this issue with the above approach. If C# is planning to bring bottom types in the future, it could be a potential blocker because C# doesn't support overload on return types.

[Sergio0694]

"Without this change, it is impossible to use those helpers in a switch expression."

Worth noting, the way throw helpers are generally used in switch expressions (eg. to throw on the default case and whatnot) is to have a branch that just returns a throw helper return. That is, in this scenario you'd use a throw helper that throws unconditionally. That is not the case for the API whose signature is being proposed for change here, as they all have conditions. So I'm not sure the example of switch expressions could really be used in this case either 🤔

[wertzui]

I've seen a lot of discussion and feelings whether these should return T or void. But why not have the cake and eat it at the same time with new methods?
So all ThrowIf... methods could return void. They could be inlined and would be good for those high performance scenarios.
At the same time there could be methods with another naming scheme like Ensure... which would return T.
That way everyone gets to choose which way to go.

class ArgumentNullException
{
    public static void ThrowIfNull([NotNull] object? argument, [CallerArgumentExpression(nameof(argument))] string? paramName = null);
    public static void ThrowIfNull([NotNull] void* argument, [CallerArgumentExpression(nameof(argument))] string? paramName = null);

+    public static T EnsureNotNull<T>(T? argument, [CallerArgumentExpression(nameof(argument))] string? paramName = null);
}

class ArgumentException
{
    public static void ThrowIfNullOrEmpty([NotNull] string? argument, [CallerArgumentExpression(nameof(argument))] string? paramName = null);
+    public static void ThrowIfNullOrWhiteSpace([NotNull] string? argument, [CallerArgumentExpression(nameof(argument))] string? paramName = null);

+    public static string EnsureNotNullOrEmpty(string? argument, [CallerArgumentExpression(nameof(argument))] string? paramName = null);
+    public static string EnsureNotNullOrWhiteSpace(string? argument, [CallerArgumentExpression(nameof(argument))] string? paramName = null);
}

class ArgumentOutOfRangeException
{
+    public static void ThrowIfEqual<T>(T value, T other, [CallerArgumentExpression(nameof(value))] string? paramName = null) where T : IEquatable<T>?;
+    public static void ThrowIfGreaterThan<T>(T value, T other, [CallerArgumentExpression(nameof(value))] string? paramName = null) where T : IComparable<T>;
+    public static void ThrowIfGreaterThanOrEqual<T>(T value, T other, [CallerArgumentExpression(nameof(value))] string? paramName = null) where T : IComparable<T>;
+    public static void ThrowIfLessThan<T>(T value, T other, [CallerArgumentExpression(nameof(value))] string? paramName = null) where T : IComparable<T>;
+    public static void ThrowIfLessThanOrEqual<T>(T value, T other, [CallerArgumentExpression(nameof(value))] string? paramName = null) where T : IComparable<T>;
+    public static void ThrowIfNegative<T>(T value, [CallerArgumentExpression(nameof(value))] string? paramName = null) where T : INumberBase<T>;
+    public static void ThrowIfNegativeOrZero<T>(T value, [CallerArgumentExpression(nameof(value))] string? paramName = null) where T : INumberBase<T>;
+    public static void ThrowIfNotEqual<T>(T value, T other, [CallerArgumentExpression(nameof(value))] string? paramName = null) where T : IEquatable<T>?;
+    public static void ThrowIfZero<T>(T value, [CallerArgumentExpression(nameof(value))] string? paramName = null) where T : INumberBase<T>;

+    public static T EnsureNotEqual<T>(T value, T other, [CallerArgumentExpression(nameof(value))] string? paramName = null) where T : IEquatable<T>?;
+    public static T EnsureLessThanOrEqual<T>(T value, T other, [CallerArgumentExpression(nameof(value))] string? paramName = null) where T : IComparable<T>;
+    public static T EnsureLessThan<T>(T value, T other, [CallerArgumentExpression(nameof(value))] string? paramName = null) where T : IComparable<T>;
+    public static T EnsureGreaterThanOrEqual<T>(T value, T other, [CallerArgumentExpression(nameof(value))] string? paramName = null) where T : IComparable<T>;
+    public static T EnsureGreaterThan<T>(T value, T other, [CallerArgumentExpression(nameof(value))] string? paramName = null) where T : IComparable<T>;
+    public static T EnsurePositive<T>(T value, [CallerArgumentExpression(nameof(value))] string? paramName = null) where T : INumberBase<T>;
+    public static T EnsurePositiveOrZero<T>(T value, [CallerArgumentExpression(nameof(value))] string? paramName = null) where T : INumberBase<T>;
+    public static T EnsureNegative<T>(T value, [CallerArgumentExpression(nameof(value))] string? paramName = null) where T : INumberBase<T>;
+    public static T EnsureNegativeOrZero<T>(T value, [CallerArgumentExpression(nameof(value))] string? paramName = null) where T : INumberBase<T>;
+    public static T EnsureEqual<T>(T value, T other, [CallerArgumentExpression(nameof(value))] string? paramName = null) where T : IEquatable<T>?;
+    public static T EnsureNotZero<T>(T value, [CallerArgumentExpression(nameof(value))] string? paramName = null) where T : INumberBase<T>;
+    public static T EnsureZero<T>(T value, [CallerArgumentExpression(nameof(value))] string? paramName = null) where T : INumberBase<T>;
}

[danmoseley]

That way everyone gets to choose which way to go.

In general this is something we try to avoid in API design, as it is more cognitive load. You hit dot, see the list, and now have to check docs to figure out which you want.

[wertzui]

But isn't that also the case with all those Parse() and TryParse()methods?

[teo-tsirpanis]

Parse does not return the input argument unchanged, it processes it.

[wertzui]

I was referring to the cognitive load mentioned by @danmoseley, not to the return value.

[tannergooding]

The Try* pattern is an existing and well-understood pattern that occurs across many languages and throughout the BCL.

Ensure would be a relatively unique thing for these APIs in particular and isn't necessarily "obvious" that around what exactly it does.

[terrajobst]

Video

• It seems a bit noisy because it would require a naming conventions for the already shipped APIs
  • The hypothetical "binary breaking change" proposal by Jared might help here
• The language is looking at a proposal to allow enable void returning expressions in switch expressions
• This seems "neat" but labor intense
• For now, we'd rather reject this

namespace System;

public partial class ArgumentException
{
    public static string ThrowIfNullOrWhiteSpace([NotNull] string? argument, [CallerArgumentExpression(nameof(argument))] string? paramName = null);
}

public partial class ArgumentOutOfRangeException
{
    public static T ThrowIfEqual<T>(T value, T other, [CallerArgumentExpression(nameof(value))] string? paramName = null) where T : IEquatable<T>?;
    public static T ThrowIfGreaterThan<T>(T value, T other, [CallerArgumentExpression(nameof(value))] string? paramName = null) where T : IComparable<T>;
    public static T ThrowIfGreaterThanOrEqual<T>(T value, T other, [CallerArgumentExpression(nameof(value))] string? paramName = null) where T : IComparable<T>;
    public static T ThrowIfLessThan<T>(T value, T other, [CallerArgumentExpression(nameof(value))] string? paramName = null) where T : IComparable<T>;
    public static T ThrowIfLessThanOrEqual<T>(T value, T other, [CallerArgumentExpression(nameof(value))] string? paramName = null) where T : IComparable<T>;
    public static T ThrowIfNegative<T>(T value, [CallerArgumentExpression(nameof(value))] string? paramName = null) where T : INumberBase<T>;
    public static T ThrowIfNegativeOrZero<T>(T value, [CallerArgumentExpression(nameof(value))] string? paramName = null) where T : INumberBase<T>;
    public static T ThrowIfNotEqual<T>(T value, T other, [CallerArgumentExpression(nameof(value))] string? paramName = null) where T : IEquatable<T>?;
    public static T ThrowIfZero<T>(T value, [CallerArgumentExpression(nameof(value))] string? paramName = null) where T : INumberBase<T>;
}