Skip to content
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

Proposal: Adding a concise API for throwing exceptions #18907

Closed
jamesqo opened this issue Oct 10, 2016 · 9 comments
Closed

Proposal: Adding a concise API for throwing exceptions #18907

jamesqo opened this issue Oct 10, 2016 · 9 comments
Labels
api-needs-work API needs work before it is approved, it is NOT ready for implementation area-System.Runtime design-discussion Ongoing discussion about design without consensus
Milestone

Comments

@jamesqo
Copy link
Contributor

jamesqo commented Oct 10, 2016

Background

Argument validation is done a lot in .NET code. So much, in fact, that people almost always find that writing throw new ArgumentNullException(...) et al. each time is too verbose, and end up writing their own helper classes to throw exceptions. Some random examples I picked from GitHub: Octokit.NET, xUnit, System.Collections.Immutable.

Proposal

We should have an API in the BCL for this so people don't have to keep writing their own helper classes. Here's what an initial prototype could look like (similar to the S.C.I class I linked to above):

public static class Requires
{
    public static void NotNull<T>(T value, string paramName); // no T : class constraint to allow nullables
    // parameter names are same as ArgumentNullException, to allow easy conversion
    // for people using named parameters
    public static void NotNull<T>(T value, string paramName, string message);

    public static void Range(bool condition, string paramName);
    public static void Range(bool condition, string paramName, string message);

    public static void Argument(bool condition, string message); // message comes first for ArgumentException
    public static void Argument(bool condition, string message, string paramName);
}

How it would be used:

T Single<T>(this IList<T> list)
{
    Requires.NotNull(list, nameof(list));
    Requires.Argument(list.Count == 1, "The input list needs to have only 1 item.");

    return list[0];
}

Additional Notes

In addition to providing a nice unified API, this may also lead to better performance / decreased memory consumption for .NET apps. Typically when people write their own helper APIs for this, they write it so that the throw is in the same clause as the check for the condition, i.e.

void NotNull<T>(T value, string name)
{
    if (value == null)
    {
        throw new ArgumentNullException(name);
    }
}

or, alternatively, they have a method that returns an exception and throw that from the caller:

ArgumentNullException ArgumentNull(string name)
{
    return new ArgumentNullException(name);
}

if (value == null) throw Error.ArgumentNull(nameof(value));

Code snippet 1 is not likely to be inlined because of the throw new ..., while ArgumentNull in code snippet 2 does get inlined, resulting in generated code bloat for a throw path that will very rarely get called.

If we wrote our own API for throwing exceptions, we could structure it so that the check gets inlined, but the throwing code doesn't:

void NotNull<T>(T value, string name)
{
    if (value == null)
    {
        ThrowArgumentNullException(name);
    }
}

void ThrowArgumentNullException(string name) => throw new ArgumentNullException(name);

Thoughts?

cc @svick @benaadams @jkotas @GSPP @mellinoe @KrzysztofCwalina

@prajaybasu
Copy link

I believe CodeContracts has a somewhat similar functionality ?

@danmoseley
Copy link
Member

@terrajobst thoughts? is this api ready for review?

@danmoseley
Copy link
Member

Code snippet 1 is not likely to be inlined because of the throw new ...

If I understand this change correctly, this is happily no longer true:
dotnet/coreclr@e76da56

@AlexGhiondea
Copy link
Contributor

@jamesqo with the change @danmosemsft mentions, do you feel like adding such a 'throw helper' class is still something we need to consider?

@danmoseley
Copy link
Member

danmoseley commented Dec 27, 2016

There are some potential other advantages of throwing helpers (which is why I used them in other projects even though I was unaware they broke inlining then). Examples

  1. ability to put a breakpoint without breaking on all such exceptions. It's easier to set a breakpoint than to go to the exceptions UI, also you can potentially set a conditional breakpoint.
  2. ability to add tracing/logging in a single place for example in debug builds possibly with extra diagnostic information or checking of self consistency
  3. ability to look up a localized string for the message
  4. conditionally throw for example as a check for consistency or other check that is verbose to repeat inline or slow so you only want in debug builds

Having said that if the helpers are in the framework these points may be less relevant unless there is something like static callbacks you can set up.

Presumably the main motivation is to reduce code size and potentially help inlining, just as ThrowHelper still has value to corelib, it would have the same value to user code.

@jamesqo
Copy link
Contributor Author

jamesqo commented Dec 28, 2016

@AlexGhiondea @danmosemsft I am deciding to go in a different direction with this proposal. C# 7 introduces throw exprs where you can write `_name = name ?? throw new ArgumentNullException(nameof(name))`, so I feel like the current shape kind of conflicts with that since you can't use it with throw exprs.

@danmosemsft You mentioned in an earlier comment that methods with throw can now be inlined, so it might be better to have an API that returns exceptions instead:

public static class Error
{
    public static ArgumentNullException ArgumentNull(string name) =>
        new ArgumentNullException(name);

    public static ArgumentOutOfRangeException ArgumentOutOfRange(string name) =>
        new ArgumentOutOfRangeException(name);
}

_name = name ?? throw Error.ArgumentNull(nameof(name));

There would still be some code size benefit, albeit less, because the exception allocation would not happen inline in the caller.

@AlexGhiondea
Copy link
Contributor

@jamesqo I like this approach!

I would rename the class to Exception or ExceptionFactory since we are basically building an exception factory.

And I am sure we can somehow auto-generate this for all exception types in the framework.

@jkotas
Copy link
Member

jkotas commented Jan 4, 2017

_name = name ?? throw Error.ArgumentNull(nameof(name));

The outlining codesize optimization like this would be best done by the compilation toolchain ... works for billions lines of existing code, and you do not need to train million developers to write different code to take advantage of it.

@jamesqo
Copy link
Contributor Author

jamesqo commented Mar 13, 2017

@jkotas 👍 . I am deciding to retarget this again as a convenience-oriented API rather than a perf-oriented one, and also add bulk validation for common patterns, but I feel like I've flip flopped too many times in one proposal. I'll close this and open another one with the new shape I have in mind

@jamesqo jamesqo closed this as completed Mar 13, 2017
@msftgits msftgits transferred this issue from dotnet/corefx Jan 31, 2020
@msftgits msftgits added this to the 2.0.0 milestone Jan 31, 2020
@ghost ghost locked as resolved and limited conversation to collaborators Dec 28, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-needs-work API needs work before it is approved, it is NOT ready for implementation area-System.Runtime design-discussion Ongoing discussion about design without consensus
Projects
None yet
Development

No branches or pull requests

7 participants