Proposal: Convenience API for throwing exceptions

[jamesqo]

Background

It's quite verbose to validate arguments because you have to write an if-statement and throw new XXXException for each validation. Even with C# 7's new throw-expressions, you still have to write out the latter part. This makes people write their own helper classes for validating arguments, e.g. here or here. It would be nice if we had such a helper class as part of the framework.

Proposal

namespace System
{
    public static class Verify
    {
        public static void Argument(bool condition, string message, string argumentName);

        public static void InRange(bool condition, string argumentName);
        public static void InRange(string s, int index, int count, string sName, string indexName, string countName);
        public static void InRange<T>(T[] array, int index, int count, string arrayName, string indexName, string countName);

        public static void NotEmpty<T>(IEnumerable<T> source, string sourceName);

        public static void NotNegative(int argument, string argumentName);

        public static void NotNull(bool condition, string argumentName);
        public static void NotNull<T>(T argument, string argumentName) where T : class;

        public static void Positive(int argument, string argumentName);
    }
}

// Sample usage:
T MyGenericMethod<T>(T[] array, int index, int count)
{
    // Note: All of this is equivalent to Verify.InRange(array, index, count, nameof(array), nameof(index), nameof(count)).
    // The arguments are validated manually for demo purposes.
    Verify.NotNull(array, nameof(array));
    Verify.NotNegative(index, nameof(index));
    Verify.NotNegative(count, nameof(count));
    Verify.InRange(array.Length - index >= count, nameof(index));
}

A sample implementation can be found here.

Remarks

• In my experience, it's common to validate things like a signed integer being positive/nonnegative, so those patterns will get their own Positive / NotNegative methods. This will enable us to provide a better error message if such a check fails. These methods throw the same exception type as InRange.

  • Same applies for InRange, NotEmpty

• The class will work nicely with using static:

using static System.Verify;

T MyGenericMethod<T>(T[] array, int index, int count)
{
    NotNull(array, nameof(array));
    NotNegative(index, nameof(index));
    NotNegative(count, nameof(count));
    InRange(array.Length - index >= count, nameof(index));
}

• The extra NotNull overload taking a bool covers the rare cases when people are writing generic code, and the parameter might be null but the compiler can't guarantee that it's a class. e.g. Like here. It also covers the rare times when someone would want to verify a nullable is non-null.
  • I didn't include a NotNull<T>(T?, string) where T : struct nullable overload since if someone thinks a nullable is non-null they would likely 1) not accept a nullable parameter in the first place, or 2) if they're calling some method they know returns a non-null nullable, they're likely to cast the T? to a T or call .Value, which do the validation automatically, instead of bothering to validate themselves.

---

This is a follow-up to https://github.com/dotnet/corefx/issues/12509 after putting some thought into the API.

[jamesqo]

/cc @KrzysztofCwalina; would such an API be a good idea for corefxlab?

[karelz]

BTW: I don't think using corefxlab would bring too much benefit in this case.

[jamesqo]

@karelz Why not?

[karelz]

Let me turn it around: What kind of benefits do you expect from corefxlab?

[KrzysztofCwalina]

There is a need for APIs like that in corfxlab and in the platfrom. For example, System.Slices already duplicates this: https://github.com/dotnet/corefxlab/blob/master/src/System.Slices/System/Runtime/Contract.cs

I never turned the Contract.cs APIs into a package as there are several interesting trade-offs here, and I did not have time to think how to resolve them.

Also, I think this is related to a feature that @justinvp was pushing at some point (and I liked a lot), i.e. the ability to throw various common exceptions conveniently without the need to deal with message resources.

Lastly, I would love to have a set of attributes for identifying exceptions that propagate out of APIs. The attributes would divide errors into usage errors (roughly preconditions) and runtime errors (errors that need to be handled). Usage errors would show in VS tooltips and runtime error attributes would be used by analyzers to detect code that does not handle them properly.

[jamesqo]

Also, I think this is related to a feature that @justinvp was pushing at some point (and I liked a lot), i.e. the ability to throw various common exceptions conveniently without the need to deal with message resources.

Nice. Do you or @justinvp have a link to the discussion?

Lastly, I would love to have a set of attributes for identifying exceptions that propagate out of APIs. The attributes would divide errors into usage errors (roughly preconditions) and runtime errors (errors that need to be handled).

You mean things like annotating parameters with [NotNull] like in EF Core for usage errors, and Java-style checked exceptions for runtime errors?

[jamesqo]

There is a need for APIs like that in corfxlab and in the platfrom.

@KrzysztofCwalina BTW, if you like the shape of this API, there is something that would make it even sweeter. A C# language proposal that I neglected to mention earlier is related to this API: dotnet/csharplang#205. If that were implemented, it would allow users to forego having to pass in the argument names at all, and still get descriptive error strings.

static class Verify
{
    public static void NotNull<T>(T argument, [CallerArgumentExpression("argument")] string argumentName = null)
    {
        if (argument == null) throw new ArgumentNullException(argumentName);
    }

    public static void NotNegative(int argument, [CallerArgumentExpression("argument")] string argumentName = null)
    {
        if (argument < 0) throw new ArgumentOutOfRangeException(argumentName);
    }
}

T MyGenericMethod<T>(T[] array, int index, int count)
{
    // No nameof, the names of each param are passed to the methods by the compiler.
    Verify.NotNull(array);
    Verify.NotNegative(index);
    Verify.NotNegative(count);
    Verify.InRange(array.Length - index >= count);
}

Note that if we added the API in its current shape to the framework and then that language feature was implemented, it would not be breaking to use the new feature because we'd just mark the last parameter of each method optional.

[justinvp]

Do you or @justinvp have a link to the discussion?

#14487

[svick]

public static void InRange(string s, int index, int count, string sName, string indexName, string countName);
public static void InRange<T>(T[] array, int index, int count, string arrayName, string indexName, string countName);

What is the point of all those name parameters here? I get how specifying the string/array name is useful, but I think the index and count parameters might be obvious or they might not be parameters at all. So I think this mostly just makes the API harder to use.

---

public static void NotNegative(int argument, string argumentName);
public static void Positive(int argument, string argumentName);

Should there be overloads for other numeric types, at least long?

[PhilipDaniels]

This is a common need, I have a similar class, though the names are different. One suggestion: the Verify method should return the object being verified, because this value often gets assigned to a member variable. This enables compact statements of the form

this.foo = NotNull(foo, nameof(foo));

[jamesqo]

@svick

What is the point of all those name parameters here? I get how specifying the string/array name is useful, but I think the index and count parameters might be obvious or they might not be parameters at all. So I think this mostly just makes the API harder to use.

Not everyone uses the same index/count names; some places in the framework it's offset, some arrayIndex, and some destinationIndex. IMO, it would be confusing if an exception was thrown with a different parameter name. We should wait to see the fate of dotnet/csharplang#287; if that is approved, then we can receive all of the argument names for free.

Should there be overloads for other numeric types, at least long?

Depends on how common that would be. I do think long would be a reasonable addition here because it is used in IO APIs, but I wanted to leave that to another proposal to not scare reviewers off.

@PhilipDaniels

This is a common need, I have a similar class, though the names are different. One suggestion: the Verify method should return the object being verified, because this value often gets assigned to a member variable

I had thought of that too. I have mixed feelings about that because it would lead to inconsistencies where some people would write NotNull(foo, nameof(foo)); this.foo = foo; and others would do it your way. However, my main concern is perf-- I think it could lead to poorer code being generated by the JIT, even when the value isn't actually used.

[wekempf]

I think everyone has a version of this, so having an "official" one to use would certainly be nice. In my own, I use [MethodImpl(MethodImplOptions.AggressiveInlining)] for performance. Also, as constructive feedback, I think the naming here is poor. Verify sounds like a generic assertion class (plus, it's a verb and the FDG tells us classes should be nouns). I can see people using Verify.InRange(...) with non-arguments, in which case the exception thrown would be confusing. My own class is called Argument for these reasons, so it's Argument.InRange(...). That said, I also then turn around and use a static using, so all my code says is InRange(...) which has it's own set of problems (it's not a verb and has lost the Argument name at the call site). Naming is hard, to say the least, but I still see people being confused by the exception types being thrown by this helper class without some naming changes.

There's also other argument assertions that are missing here that should be considered. For instance, NotNullOrEmpty and ValidEnumValue are common ones I've used. I've got some others but would have to look them up, but I bet searching in GitHub would find you several more. In fact, this seems to be the only valid reason to reject a proposal like this... there's no way to include assertions for all argument validation needs. I don't think that's enough of a reason not to move forward with this, but is something worth thinking about during design.

[jamesqo]

@wekempf Concerning the naming, do you think calling the class Require or Requires would be more acceptable? e.g. Require.NotNull, Requires.NotNull

[benaadams]

It would likely need to be something like NuGet "source" package when used; rather than a built in to runtime as assembly, so the tests will inline across assembly boundaries when compiled AoT/R2R

[wekempf]

@jamesqo For someone familiar with contracts, probably. For the "masses", I honestly don't know. There are two hard things in programming....

Edit: Oh, forgot this bit. Require/Requires is still not a noun, and thus in violation of the FDG. No idea how rigid that should be followed, however. There's the same problem with the methods in that case, where the methods should be verbs, like IsNotNull.

@benaadams For the inline suggestion, probably, yes. There's several of these already available in NuGet, like https://www.nuget.org/packages/netfx-Guard/. I use source links in my projects for this very reason. Provided as part of the BCL, this obviously wouldn't work. And maybe it's not a big concern? I actually do it not just for the perf, though, it also produces better stack traces when it gets inlined. Maybe there's another solution for that, though?

[wekempf]

So, I've had time to think about it. We already have Debug and Trace, so the concern about strictly following the FDG with regards to using verbs for class names is probably not a concern here. So Require is probably an "OK" name. However, I still believe Argument to be superior.

Verify.NotNull(list, nameof(list)); // Probably confusing
Require.NotNull(list, nameof(list)); // Probably not confusing
Argument.NotNull(list, nameof(list)); // Absolutely not confusing

I like the fact that with Argument as the name the assertions read exactly the same as the exceptions they throw: Argument.NotNull throws ArgumentNullException.

I also just noticed in your sample implementation you have overloads of InRange for validating indexes, but they throw ArgumentException. I've always thrown ArgumentOutOfRange exception which made more sense to me. However, looking at List.BinarySearch it looks like precedent says otherwise? List.BinarySearch also throws ArgumentOutOfRangeException but that's for validation that index and count are non-negative, while it just throws ArgumentException for range violations (confusing to me). You also don't assign a ParamName to the exception, which is something that really bothers me, but, again, this is the behavior of List.BinarySearch as well. So, while you've followed precedent, maybe we should consider changing some of this behavior?

Also you only have overloads for string and arrays when you should probably just have a single generic overload constrained for IList types. Also, it might be nice to have an overload that doesn't take the count parameter and instead just validates index is in the range of valid indexes for the collection.

Argument.InRange(index, array, nameof(array), nameof(index));

BTW, while I'd like to see something like this regardless, these helpers really don't shine until we get your language proposal. I vote +1 for both.

[JonHanna]

While these verification methods are handy (I use similar often myself), there is something to be said about the benefit of a throw within a method for static analysis.

public bool SomeMethod(string someArg)
{
    if (someArg == null) throw new ArgumentNullException();
    // Clearly someArg is not null at this point.
    if (someArg.length < 3)
    {
        if (someArg.length == 0)
        {
            throw new ArgumentException("Empty");
        }
        else
        {
            throw new ArgumentException("Too short");
        }
        // Clearly unreachable. A helper method that threw the exception itself
        // above would produce CS0161 here.
    }
    return true
}

Changing ErrorContext.Error in Microsoft.CSharp from throwing to returning an exception to throw, and passing that change up to any method that always threw, made quite a few chunks of code not only more clearly unreachable, but demonstrably unreachable so the compiler would complain about their being there. It was a big jump in how easy it was to reason about a lot of the methods.

This is going to become even more pertinent when C#8 introduces nullability checks as promised:

public bool IsEmpty(string? someArg)
{
    if (someArg == null) throw new ArgumentNullException();
    return someArg.length  == 0;
}

or even

public bool IsEmpty(string? someArg)
{
    if (someArg == null) throw ErrorHelper.ArgumentNull();
    return someArg.length  == 0;
}

vs

public bool IsEmpty(string? someArg)
{
    Requires.NotNull(someArg);
    return someArg.length  == 0; // Compiler warning someArg could be null.
}

Unless we get a way to flag to the compiler that Requres.NotNull() will never succeed with with a null argument, and hence that someArg is demonstrably not null after it, using it will lose the nullability checking of the language.

How do we get to have both?

[JonHanna]

One possibility for the above is to return the argument.

public static T NotNull<T>(T? value, string paramName = null) =>
    value ?? throw new ArgumentNullException(paramName);

Then we could do:

public bool IsEmpty(string? someArg)
{
    someArg = Prover.NotNull(someArg); // "Prover" is part of what it does now,
                                       // but not an actual name suggestion
    return someArg.length  == 0; // Compiler knows someArg could not be null.
}

Or even the concision (YMMV on the value of concision, as always) of

public bool IsEmpty(string? someArg) => Prover.NotNull(someArg).length  == 0;

Annoyingly, I'm unclear what NotNull would do with a value-type T and how the ? of Nullable<T> will work or not with the T of C#8 nullable reference types. Ideally I'd like to be able to use this with a generic type that could potentially be a non-nullable value type, so I could get a null check when valuable and have the jitter skip it when not. I guess I'm going to have to read up on C#8 beyond high-level blogs earlier than I suspected.

Also, this perhaps solves the particular C# 8 nullability check issue. It doesn't make places that should always have thrown unreachable, though perhaps they aren't as applicable anyway.

[JonHanna]

Returning the argument would also allow chaining if done as extensions: value.VerifyNotNull().VerifyNotEmpty()

[PhilipDaniels]

Strongly support returning the argument, my own extensions do this. Results in fewer lines of code being required, which is always good (less vertical space needed).

[JonHanna]

From @wekempf:

In my own, I use [MethodImpl(MethodImplOptions.AggressiveInlining)] for performance.

There's a downside to this, when one considers the effect of pulling in the extra code into the method for an exceptional case.

Perhaps the best balance might be something like:

[MethodImpl(MethodImplOptions.AggressiveInlining)]
public T SomeVerifyingMethod<T>(T someArgument)
{
    if (!VerifyCodeHere(someArgument)
    {
        DoThrow();
    }

    return someArgument;
}

[MethodImpl(MethodImplOptions.NoInlining)]
public void DoThrow() => throw new Exception("blah blah");

Then only the check gets inlined, which is the part we'd like to get past quickly, but not the throw if it happens.

I recall one of the corefx team saying something on the performance impacts of validation and/or throw-helper methods that had been on the back of investigating the impact of different approaches, but can't remember who.

Personally my own instincts are to avoid AggressiveInlining until it becomes time to ignore my instincts (because it's in something identified as a hotspot, and it's time to start profiling and putting numbers ahead of instincts).

[benaadams]

Don't want to no-inline the throw; gives the jit the wrong signals - better as a non-returning method; and the exception create as a non-inline (for older runtimes) - and keep any preparation of it to the right hand side of the throw - so its cold code.

Want to inline the check; so the Jit can elide checks (e.g. if var is guaranteed not null or range is predefined then that whole chunk can be removed); also because its always run.

I recall one of the corefx team saying something on this that had been on the back of investigating the impact of different approaches...

@mikedn introduced non-returning methods to the Jit "Do not inline methods that never return" dotnet/corefx#6103, from which a new pattern emerged:

I did a little bit in a recent talk about it: Slides 19-22 What's new for performance in .NET Core 2.0

Non-Returning methods are better than direct throws; other than the extra entry in call stack; as driect throws can push out the chance to inline (itself an überoptimization; which can lead to a cascade of other optimizations)

Also move the throw out of generic methods and classes; to reduce repetition from each specialization of the generic method - all of which the ThrowHelper in coreclr is doing

The essentials are below:

Would be nice if the C# compiler could make use of this in some way; as the Jit doesn't split methods (non-crossgen/ngen/AoT)

[mikedn]

@mikedn introduced non-returning methods to the Jit "Do not inline methods that never return" dotnet/corefx#6103, from which a new pattern emerged:

The pattern existed before, it was used as a code size optimization. I just changed the JIT to handle it properly. It's likely that older JITs did not inline such methods and that's how the pattern appeared. Granted, not inlining without also propagating "no return" information can cause CQ issues but probably nobody noticed that in the past.

Would be nice if the C# compiler could make use of this in some way; as the Jit doesn't split methods (non-crossgen/ngen/AoT)

Something like this was mentioned before, I think. The C# compiler or some kind of IL optimizer could outline throw blocks into assembly internal methods.

[jnm2]

Might this have a place here? I'm using this helper in argument validation as a non-exception-throwing cast:

/// <summary>
/// Casts to a value of the given type if possible.
/// If <paramref name="obj"/> is <see langword="null"/> and <typeparamref name="T"/>
/// can be <see langword="null"/>, the cast succeeds just like the C# language feature.
/// </summary>
/// <param name="obj">The object to cast.</param>
/// <param name="value">The value of the object, if the cast succeeded.</param>
public static bool TryCast<T>(object obj, out T value)
{
    if (obj is T)
    {
        value = (T)obj;
        return true;
    }

    value = default(T);
    return obj == null && default(T) == null;
}

Direct cast would require catching and rethrowing as ArgumentException with param name. Safe cast would conflate mismatching types with valid nulls.

[jamesqo]

@jnm2 I personally find a direct cast to be enough; it throws an exception for you and gives you info about the object type/cast type. It doesn't give you the parameter name, but in most cases that info isn't necessary to determine where the error occurred, in my experience.

@JonHanna You make a good point about static analysis. The solution is to let the compiler specially recognize these validation methods so it won't raise false positives. Even if that isn't implemented, though, I don't think it'll be much of an issue. If someone wants a non-null string, they shouldn't be accepting a string? in the first place.

[jnm2]

I personally find a direct cast to be enough; it throws an exception for you and gives you info about the object type/cast type. It doesn't give you the parameter name, but in most cases that info isn't necessary to determine where the error occurred, in my experience.

I prefer ArgumentException, whether it's a NRE, IndexOutOfRangeException, or InvalidCastException, or anything else. I just see it as helpfulness in the same category as using my turn signal. But beyond that, there have been cases where no exception should be thrown at all.

[JonHanna]

If someone wants a non-null string, they shouldn't be accepting a string? in the first place.

We can't force non-nullability back on API users since it's opt-in and not supported by all languages (or language versions, for that matter). Indeed one could say if someone wants a non-null string they shouldn't be throwing on null strings in the first place.

In the case where null might be passed (because it's part of the public API) but is wrong, and hence will throw, we want to start with a possibly null string, but then know it won't be null after we've validated.

Validation methods that return the argument for valid input would allow this, as per above, as well as allowing for chaining, and won't force that use on people who don't want it (since they can just ignore the return).

[wekempf]

"Chaining" is problematic. The example given of value.VerifyNotNull().VerifyNotEmpty() requires these verifiers to be extension methods that work on all types. I find that repugnant, to be honest. It pollutes IntelliSense. There is a solution to this, which I use in my Testify unit testing library. The above can be coded as Argument.Verify(value).NotNull().NotEmpty(), where Argument.Verify returns some unique wrapper type, such as an ArgumentValue<T> type, and the extension methods are based on that type. This also has the benefit of making it easy to add assertions and even to limit IntelliSense to valid types (so, for instance, NotNull won't be in IntelliSense for value types). The downside is in performance and possibly memory/gc overhead if ArgumentValue<T> is a reference type. Also, for this to work with the concept of returning the value the API is more complex with the need to chain to a Value property, or to rely on implicit conversions.

All that said, however, you could always take the functional POV to chaining and code it as NotNull(NotEmpty(value)) (with lots more syntactical noise without the compiler feature to get at the caller expression and without a static using).

[olivier-spinelli]

Seems that the focus here is on arguments. However there's also InvalidOperationExceptions that are also somehow "sent" to the caller of the method.

public bool CanRun => ...;

public void Run()
{
   if( !CanRun ) throw new InvalidOperationException( nameof(CanRun) );
   ...
}

This is "State" rather than "Argument" (there is no argument at all). And one can add another "group" that turns around the InvalidDataException (that is not Argument, nor directly State but external data that must participate to the process AND must be valid by design).
(This is just my 2 cents: this Argument/State/Data distinction is often a good guide for me.)

Whatever helpers exist they ideally should be able to work on (at least) these 3 fundamental aspects.

[huoyaoyuan]

We are now having throw helpers in the exception classes: #58047 #62628 #69590 #77749

I believe this can be closed and follow the latest designs.