[API Proposal]: Throw helpers should return the valid value back

[julealgon]

Background and motivation

I've been using the newly introduced ArgumentNullException.ThrowIfNull method quite a bit to simplify my code and reduce the reliance on nameof for passing the argument name.

However, I'm running into the following situation a lot:

public SomeClass(string value)
{
    ArgumentNullException.ThrowIfNull(value);
    this.value = value;    
}

Most often, this happens on constructors, where we want to validate the value and then assign it to a field in the class.

Prior to the helper method, I used to use throw expressions for this:

public SomeClass(string value)
{
    this.value = value ?? throw new ArgumentNullException(nameof(value));
}

I think code would be a lot simpler if we could combine both of these like this:

public SomeClass(string value)
{
    this.value = ArgumentNullException.ThrowIfNull(value);
}

By making it so that the exception validation helpers always returned the value on exit (instead of being void methods.

This is how most exception guard classes/libraries work, exactly because one of the main cases is to assign the "validated" value to some backing field after validation happens.

The "validate and return" pattern guarantees I'll only ever need to use one line to initialize any value and that there will be no need to repeat the value multiple times (once on validation, and another time on assignment).

This is also particularly important when dealing with base classes, where we need inline validation to happen but still pass the value forward:

public abstract Base
{
    public Base(string value)
    {
        ArgumentNullException.ThrowIfNull(value);
        this.value = value;
    }
}

public class Inheritor : Base
{
    public Inheritor(string value)
        // : base(ArgumentNullException.ThrowIfNull(value)) <- this won't work
        : base(value ?? throw new ArgumentNullException(nameof(value))) // <- fallback to throw expressions :(
    {
    }
}

API Proposal

Current:

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

New:

public class ArgumentNullException : ArgumentException
{
    public static T ThrowIfNull<T>([NotNull] T? argument, [CallerArgumentExpression("argument")] string? paramName = null)

    public static T ThrowIfNull<T>([NotNull] T? argument, [CallerArgumentExpression("argument")] string? paramName = null)
        where T : struct
}

This would apply to other such helpers, like ArgumentException.ThrowIfNullOrEmpty proposed here:

• [API Proposal]: ArgumentException.ThrowIfNullOrEmpty(string) #62628

As well as other proposals such as the ones for ArgumentOutOfRangeException here:

• [API Proposal]: ArgumentOutOfRangeException helper methods #69590

API Usage

public SomeClass(string reference, int? value)
{
    this.reference = ArgumentNullException.ThrowIfNull(reference);
    this.value = ArgumentNullException.ThrowIfNull(value);
}

Alternative Designs

If I was designing this from scratch, I'd rather have these methods named like this:

ArgumentNullException.EnsureNotNull(value)

As "Ensure{ConditionThatIDon'tWant}" makes more sense when a value is returned, and is also pretty common among existing guard classes and libraries.

It is also fairly well known that an "Ensure" method is expected to throw when the validation "fails".

Risks

I think my original proposal is fairly risk-free, but renaming the methods from ThrowIfX to EnsureNotX (or EnsureY when Y is the opposite of X) would obviously be a nasty breaking change.

[dotnet-issue-labeler]

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.

[huoyaoyuan]

This is binary breaking change.
One of the purpose of throw helpers is to reduce IL size of argument validation. If the value is returned, every call site that doesn't need it would require an additional pop instruction.

[julealgon]

One of the purpose of throw helpers is to reduce IL size of argument validation. If the value is returned, every call site that doesn't need it would require an additional pop instruction.

I had no idea unused return values had an overhead like this, very interesting to know. I'm kinda surprised the compiler is not able to optimize this away. Wouldn't it be able to tell that the value is not used and thus ignore it or something along those lines?

Would it potentially make sense to have a variation that returns the value then, on top of the original void ones?

[huoyaoyuan]

I had no idea unused return values had an overhead like this, very interesting to know. I'm kinda surprised the compiler is not able to optimize this away. Wouldn't it be able to tell that the value is not used and thus ignore it or something along those lines?

The pop instruction is to "ignore" it. IL execution model is stack-based machine. Every value put on the stack must be either consumed or thrown by pop.

Would it potentially make sense to have a variation that returns the value then, on top of the original void ones?

No. Although IL allows overload by return type, all of the high level languages disallow this.

[julealgon]

The pop instruction is to "ignore" it. IL execution model is stack-based machine. Every value put on the stack must be either consumed or thrown by pop.

Yeah I got that, but wouldn't it be possible to somehow avoid actually pushing the result on the stack if we knew upfront it was not being used? Wouldn't that be a possible optimization the compiler could do? Or is that not feasible because we are talking about the same function code? Maybe it could automatically generate a "void version" of the function when this situation is detected?

No. Although IL allows overload by return type, all of the high level languages disallow this.

I specifically avoided the word "overload" since I knew we couldn't overload just on return type. By variant I meant another method with a different name (for instance, the Ensure variant I mentioned earlier).

The problem would be that people wouldn't be aware of this detail you mentioned and end up using them interchangeably for the non-returning scenario...

How important is this IL optimization aspect? Was it a key component when coming up with the helpers initially? Is the restriction set in stone in such a way that we won't ever be able to return anything from the helpers because of it?

[huoyaoyuan]

Yeah I got that, but wouldn't it be possible to somehow avoid actually pushing the result on the stack if we knew upfront it was not being used? Wouldn't that be a possible optimization the compiler could do? Or is that not feasible because we are talking about the same function code?

Literally nothing can be changed around the IL semantics. The body of callee is agnostic about caller, and IL doesn't know anything about inlining.

Maybe it could automatically generate a "void version" of the function when this situation is detected?

.NET languages especially C# won't transform public API shape. The compiler would not emit anything public that's not declared by code (except record). Additionally, there's no situation for compiler to detect.

I specifically avoided the word "overload" since I knew we couldn't overload just on return type. By variant I meant another method with a different name (for instance, the Ensure variant I mentioned earlier).

This is possible. But I didn't find anyone interested in this from the original discussion #48573.
Anyway, the existing throw helper can't change in any aspect. The newly proposed one won't be tight with its characteristics.

[ghost]

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

I've been using the newly introduced ArgumentNullException.ThrowIfNull method quite a bit to simplify my code and reduce the reliance on nameof for passing the argument name.

However, I'm running into the following situation a lot:

public SomeClass(string value)
{
    ArgumentNullException.ThrowIfNull(value);
    this.value = value;    
}

Most often, this happens on constructors, where we want to validate the value and then assign it to a field in the class.

Prior to the helper method, I used to use throw expressions for this:

public SomeClass(string value)
{
    this.value = value ?? throw new ArgumentNullException(nameof(value));
}

I think code would be a lot simpler if we could combine both of these like this:

public SomeClass(string value)
{
    this.value = ArgumentNullException.ThrowIfNull(value);
}

By making it so that the exception validation helpers always returned the value on exit (instead of being void methods.

This is how most exception guard classes/libraries work, exactly because one of the main cases is to assign the "validated" value to some backing field after validation happens.

The "validate and return" pattern guarantees I'll only ever need to use one line to initialize any value and that there will be no need to repeat the value multiple times (once on validation, and another time on assignment).

This is also particularly important when dealing with base classes, where we need inline validation to happen but still pass the value forward:

public abstract Base
{
    public Base(string value)
    {
        ArgumentNullException.ThrowIfNull(value);
        this.value = value;
    }
}

public class Inheritor : Base
{
    public Inheritor(string value)
        // : base(ArgumentNullException.ThrowIfNull(value)) <- this won't work
        : base(value ?? throw new ArgumentNullException(nameof(value))) // <- fallback to throw expressions :(
    {
    }
}

API Proposal

Current:

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

New:

public class ArgumentNullException : ArgumentException
{
    public static T ThrowIfNull<T>([NotNull] T? argument, [CallerArgumentExpression("argument")] string? paramName = null)

    public static T ThrowIfNull<T>([NotNull] T? argument, [CallerArgumentExpression("argument")] string? paramName = null)
        where T : struct
}

This would apply to other such helpers, like ArgumentException.ThrowIfNullOrEmpty proposed here:

• [API Proposal]: ArgumentException.ThrowIfNullOrEmpty(string) #62628

As well as other proposals such as the ones for ArgumentOutOfRangeException here:

• [API Proposal]: ArgumentOutOfRangeException helper methods #69590

API Usage

public SomeClass(string reference, int? value)
{
    this.reference = ArgumentNullException.ThrowIfNull(reference);
    this.value = ArgumentNullException.ThrowIfNull(value);
}

Alternative Designs

If I was designing this from scratch, I'd rather have these methods named like this:

ArgumentNullException.EnsureNotNull(value)

As "Ensure{ConditionThatIDon'tWant}" makes more sense when a value is returned, and is also pretty common among existing guard classes and libraries.

It is also fairly well known that an "Ensure" method is expected to throw when the validation "fails".

Risks

I think my original proposal is fairly risk-free, but renaming the methods from ThrowIfX to EnsureNotX (or EnsureY when Y is the opposite of X) would obviously be a nasty breaking change.

Author:	julealgon
Assignees:	-
Labels:	api-suggestion, area-System.Runtime, untriaged
Milestone:	-

[sakno]

We can just different name to highlight a new semantics:

public class ArgumentNullException : ArgumentException
{
    public static T EnsureNotNull<T>([NotNull] T? argument, [CallerArgumentExpression("argument")] string? paramName = null);
}

@julealgon , it's not possible to overload the method only by different generic constraint. In reality, we need just one method with explicit support of nullable value type. JIT is smart enough to determine whether the particular type T is nullable and optimize null check accordingly.

[stephentoub]

My opinion is it's not worth adding a new method with a new name just to be able to write:

_value = ArgumentNullException.EnsureNotNull(arg);

instead of:

ArgumentNullException.ThrowIfNull(arg);
_value = arg;

It's still valid to write:

_value = arg ?? throw new ArgumentNullException(nameof(arg));

if that's your preference.

[julealgon]

@julealgon , it's not possible to overload the method only by different generic constraint. In reality, we need just one method with explicit support of nullable value type. JIT is smart enough to determine whether the particular type T is nullable and optimize null check accordingly.

@sakno , you'd be incorrect. This compiles and runs correctly:

public static class Guard
{
    public static T EnsureNotNull<T>([NotNull] T? argument, [CallerArgumentExpression("argument")] string? paramName = null)
    {
        _ = argument ?? throw new ArgumentNullException(paramName);

        return argument;
    }

    public static T EnsureNotNull<T>([NotNull] T? argument, [CallerArgumentExpression("argument")] string? paramName = null)
        where T : struct
    {
        _ = argument ?? throw new ArgumentNullException(paramName);

        return argument.Value;
    }
}

public class Something
{
    private readonly int number;
    private readonly string text;

    public Something(int? number, string? text)
    {
        this.number = Guard.EnsureNotNull(number);
        this.text = Guard.EnsureNotNull(text);
    }
}

If I remove the struct version, this line doesn't compile:

        this.number = Guard.EnsureNotNull(number);

[julealgon]

My opinion is it's not worth adding a new method with a new name just to be able to write:

_value = ArgumentNullException.EnsureNotNull(arg);

instead of:

ArgumentNullException.ThrowIfNull(arg);
_value = arg;

I mean..... I don't disagree, but that's only because the original method was poorly designed IMHO. By limiting the usages of the method you create something that is unnecessarily not orthogonal, which provides a pretty bad experience to people using the library: suddenly, it has all these exceptional cases where you have to drop it in favor of things like throw expressions and going back to having to specify the argument name... which is the whole point of the helper 😞

It's still valid to write:

_value = arg ?? throw new ArgumentNullException(nameof(arg));

if that's your preference.

I wouldn't say it is necessarily "preference": certainly using a construct where you don't need to pass the argument name is a lot more robust, so its objectively better to use it when possible. Honestly, this to me is a weak argument: for any given feature, "you can always do it the old way", but that doesn't mean you should have to. If the feature was conceived properly, it should be orthogonal: people expect it to work everywhere where the old pattern was used.

I just think that it should be possible to use it in all places where the old patterns were possible, and it would be incredibly simple to make it so by just allowing the method to return the value here.

Now, suddenly you just can't standardize all checks in code. Every situation needs to use a different method of argument validation. This increases confusion and makes the code brittle in places where you can't use the new helper. The lack of consistency alone could even be an argument against using ThrowIfNull in the first place.

My question is: is the IL optimization cited by @huoyaoyuan really worth that much that it supersedes usability/API orthogonality? From this thread, I'm assuming that's the only reason why the method doesn't return anything today (if there are other reasons please let me know).

[huoyaoyuan]

My question is: is the IL optimization cited by @huoyaoyuan really worth that much that it supersedes usability/API orthogonality?

This is totally unrelated to a new method now. The existing method cannot be changed, and will be used for ones requires it.

[tannergooding]

I'd agree with @stephentoub here and that this is likely not something worth adding.

We've already exposed APIs named ThrowIfNull and similar and you cannot overload by return type. Likewise, changing the signature is a breaking change so the "good" name is already taken.

This seems like, more generally, something you might want a language feature around instead, in which case I'd recommend opening a suggestion on dotnet/csharplang

[julealgon]

We've already exposed APIs named ThrowIfNull and similar and you cannot overload by return type. Likewise, changing the signature is a breaking change so the "good" name is already taken.

Yeah.... I understand the limitations for sure. It is too late now to change it, basically.

It is just unfortunate that such a common concern was tackled with a solution that is far from orthogonal and will create even more confusion over time around argument validation. The proposed design basically forces a mix of different approaches to be used simultaneously, instead of a unified, consistent approach.

I honestly feel like this was a huge missed opportunity and a classic case of premature optimization. Now that !! has been removed from C#11, I'm sure a proper contracts implementation will take several years to happen, and until then, we'll be left with this suboptimal (from a usability perspective in my opinion) API.

(BTW @tannergooding I know this is fairly minor, but you might want to close this as "won't fix" instead of "completed", since github allows that differentiation now. Might give more visibility in the future for people searching through the issues).

[stephentoub]

I think it's in the eye of the beholder. I think the approach of validating and then storing is perfectly reasonable and is no more or less unified than the proposal.

[julealgon]

@stephentoub note that "validating and then storing" is not possible when validating inline values to pass them to base classes, which is one of the cases I described.

I don't think the lack of orthogonality is subjective here. A method that returned the value would be usable in every single argument validation scenario the same way.

[stephentoub]

The inability to write arbitrary code in base/this delegation is an issue well beyond ThrowIfNull argument validation and deserves separate language consideration rather than contorting such an API to address the lack of such support.

The API would also need to be generic to return a strongly-typed reference usable in the same manner as the original, and that brings with it its own complexity and cost, plus there are a fair number of devs and analyzers that would consider such a return value (frequently ignored) to be a smell.

We'll need to agree to disagree.