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

[API Proposal]: Additional ArgumentNullException.ThrowIfNull overloads #58047

Closed
DaZombieKiller opened this issue Aug 24, 2021 · 12 comments · Fixed by #61633
Closed

[API Proposal]: Additional ArgumentNullException.ThrowIfNull overloads #58047

DaZombieKiller opened this issue Aug 24, 2021 · 12 comments · Fixed by #61633
Labels
api-approved API was approved in API review, it can be implemented area-System.Runtime
Milestone

Comments

@DaZombieKiller
Copy link
Contributor

DaZombieKiller commented Aug 24, 2021

Background and motivation

While migrating code to start using ArgumentNullException.ThrowIfNull, I'm finding it cumbersome that there are no overloads for passing in nullable value types or pointers. This leads to code looking inconsistent because it uses a mix of ThrowIfNull and either custom throw helpers or conditionals, it feels like I'm still dealing with all the problems that ThrowIfNull was added to solve, just for pointers and value types instead of classes.

API Proposal

namespace System
{
    public partial class ArgumentNullException : ArgumentException
    {
        /// <summary>Throws an <see cref="ArgumentNullException"/> if <paramref name="argument"/> is null.</summary>
        /// <param name="argument">The pointer argument to validate as non-null.</param>
        /// <param name="paramName">The name of the parameter with which <paramref name="argument"/> corresponds.</param>
        public static unsafe void ThrowIfNull(void* argument, [CallerArgumentExpression("argument")] string? paramName = null);
        
        /// <summary>Throws an <see cref="ArgumentNullException"/> if <paramref name="argument"/> is null.</summary>
        /// <param name="argument">The argument to validate as non-null.</param>
        /// <param name="paramName">The name of the parameter with which <paramref name="argument"/> corresponds.</param>
        public static void ThrowIfNull<T>([NotNull] T? argument, [CallerArgumentExpression("argument")] string? paramName = null)
            where T : struct;
    }
}

API Usage

public static int DereferenceSomePointer(int* pointer)
{
    ArgumentNullException.ThrowIfNull(pointer);
    return *pointer;
}

public static int DereferenceAnotherPointer(IntPtr pointer)
{
    ArgumentNullException.ThrowIfNull((void*)pointer);
    return *(int*)pointer;
}

public static int GetValueOfNullableInt(int? nullable)
{
    ArgumentNullException.ThrowIfNull(nullable);
    return nullable.Value;
}

[Flags]
public enum OperationFlags
{
    None       = 0,
    RequireInt = 1 << 0
}

public static void PerformOperation(OperationFlags flags, int? integer = null)
{
    if (flags.HasFlag(OperationFlags.RequireInt))
        ArgumentNullException.ThrowIfNull(integer);

    // perform operation
}

Risks

  • The Nullable<T> overload could potentially result in a lot of generic instantiations.
  • The pointer overload is an unsafe method being exposed on an extremely common safe type. I'm not sure how the .NET team feels about this.
@DaZombieKiller DaZombieKiller added the api-suggestion Early API idea and discussion, it is NOT ready for implementation label Aug 24, 2021
@dotnet-issue-labeler
Copy link

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.

@dotnet-issue-labeler dotnet-issue-labeler bot added the untriaged New issue has not been triaged by the area owner label Aug 24, 2021
@teo-tsirpanis
Copy link
Contributor

teo-tsirpanis commented Aug 24, 2021

I don't think an overload of ThrowIfNull accepting a pointer needs to be in the BCL. Pointers should be an implementation detail where internal code will ensure that they are never null by not accepting them from public APIs. And in the very case that they are actually exposed, they should either use spans if applicable, or perform the null checks at the edge by themselves.


The Nullable<T> overload seems to me that it does not have a valid use-case. Why even throw when a value whose type by design permits null. is null? If people don't want a struct to be null, they wouldn't use Nullable<T>; a principle of Domain-Driven Design is that illegal states should be unrepresentable.

@tannergooding
Copy link
Member

tannergooding commented Aug 25, 2021

Pointers should be an implementation detail where internal code will ensure that they are never null by not accepting them from public APIs. And in the very case that they are actually exposed, they should either use spans if applicable, or perform the null checks at the edge by themselves.

That is the exact purpose of the proposed API overload, to allow such code to throw if the input is invalid (null) in a simplified manner and which can help with inlining (which is often important when you are using pointers, as that tends to be "perf critical" code). We have several cases of public APIs in the BCL taking pointers or providing pointer overloads. For example, the Span, ReadOnlySpan, and String types all have constructors to support scenarios where you start with a pointer. While they are often implementation details, that doesn't necessarily mean the BCL should hinder your ability to work with them when you do have them.

@tannergooding tannergooding added area-System.Runtime and removed untriaged New issue has not been triaged by the area owner labels Aug 25, 2021
@ghost
Copy link

ghost commented Aug 25, 2021

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

While migrating code to start using ArgumentNullException.ThrowIfNull, I'm finding it cumbersome that there are no overloads for passing in nullable value types or pointers. This leads to code looking inconsistent because it uses a mix of ThrowIfNull and either custom throw helpers or conditionals, it feels like I'm still dealing with all the problems that ThrowIfNull was added to solve, just for pointers and value types instead of classes.

API Proposal

namespace System
{
    public partial class ArgumentNullException : ArgumentException
    {
        /// <summary>Throws an <see cref="ArgumentNullException"/> if <paramref name="argument"/> is null.</summary>
        /// <param name="argument">The pointer argument to validate as non-null.</param>
        /// <param name="paramName">The name of the parameter with which <paramref name="argument"/> corresponds.</param>
        public static unsafe void ThrowIfNull(void* argument, [CallerArgumentExpression("argument")] string? paramName = null);
        
        /// <summary>Throws an <see cref="ArgumentNullException"/> if <paramref name="argument"/> is null.</summary>
        /// <param name="argument">The argument to validate as non-null.</param>
        /// <param name="paramName">The name of the parameter with which <paramref name="argument"/> corresponds.</param>
        public static void ThrowIfNull<T>([NotNull] T? argument, [CallerArgumentExpression("argument")] string? paramName = null)
            where T : struct;
    }
}

API Usage

public static int DereferenceSomePointer(int* pointer)
{
    ArgumentNullException.ThrowIfNull(pointer);
    return *pointer;
}

public static int DereferenceAnotherPointer(IntPtr pointer)
{
    ArgumentNullException.ThrowIfNull((void*)pointer);
    return *(int*)pointer;
}

public static int GetValueOfNullableInt(int? nullable)
{
    ArgumentNullException.ThrowIfNull(nullable);
    return nullable.Value;
}

Risks

  • The Nullable<T> overload could potentially result in a lot of generic instantiations.
  • The pointer overload is an unsafe method being exposed on an extremely common safe type. I'm not sure how the .NET team feels about this.
Author: DaZombieKiller
Assignees: -
Labels:

api-suggestion, area-System.Runtime

Milestone: -

@tannergooding
Copy link
Member

I don't see an issue with the pointer overload as I think it fits for the same scenarios that the existing API was exposed.

Nullable<T> on the other hand does seem questionable. As mentioned by others above, I would expect you to simply take T in this scenario and not T? (the same goes for nullable annotated reference types, why take T? if you are going to throw). Additionally, the only difference between it and Nullable<T>.Value is the exception thrown (ArgumentNull vs InvalidOperation).

@DaZombieKiller
Copy link
Contributor Author

The comments regarding the Nullable<T> overload are completely reasonable. My own use cases require the pointer overload, with the Nullable<T> overload essentially being suggested for completeness' sake. If it's ultimately decided that the Nullable<T> overload is not necessary, I have no complaints.

@DaZombieKiller
Copy link
Contributor Author

I've updated the OP to provide a motivating use case for the Nullable<T> overload: a situation where you have a method taking a nullable value type parameter that is only conditionally allowed to be null.

@teo-tsirpanis
Copy link
Contributor

I got your point @tannergooding, since ThrowIfNull is already approved and you have no objection in having an unsafe API in such place, neither have I.

@acaly
Copy link

acaly commented Aug 27, 2021

Many part of the C# language and runtime is keeping a symmetry between reference-typed nulls and Nullable<T> nulls (boxing, null equality, ?? and ?. operators, etc.). I do think it makes sense to keep that symmetry here.

@AraHaan
Copy link
Member

AraHaan commented Sep 4, 2021

I think I would like an overload of ThrowIfNull like so:

public static unsafe void ThrowIfNull<T>(params T? argument);

Then one can use it like so:

internal static void Something(string something, string something2)
{
    ArgumentNullException.ThrowIfNull(something, something2);
    // snip.
}

Limitations for this: all params passed in the single call would have to be the same type, this overload would be uncaring about the parameter names (for those who does not care to pass them into the exception type themselves).

@DaZombieKiller
Copy link
Contributor Author

@AraHaan I think it would be best to wait for params Span<T> to be implemented before an overload taking multiple arguments is considered, but I otherwise agree that it would be useful.

@tannergooding tannergooding added api-ready-for-review API is ready for review, it is NOT ready for implementation and removed api-suggestion Early API idea and discussion, it is NOT ready for implementation labels Sep 7, 2021
@jeffhandley jeffhandley added this to the Future milestone Sep 15, 2021
@bartonjs
Copy link
Member

bartonjs commented Sep 21, 2021

Video

We think that the void* version makes sense, but the Nullable<T> version doesn't (any message associated with that error should be more complicated than the default).

namespace System
{
    public partial class ArgumentNullException : ArgumentException
    {
        /// <summary>Throws an <see cref="ArgumentNullException"/> if <paramref name="argument"/> is null.</summary>
        /// <param name="argument">The pointer argument to validate as non-null.</param>
        /// <param name="paramName">The name of the parameter with which <paramref name="argument"/> corresponds.</param>
        public static unsafe void ThrowIfNull(void* argument, [CallerArgumentExpression("argument")] string? paramName = null);
   }
}

@bartonjs bartonjs added api-approved API was approved in API review, it can be implemented and removed api-ready-for-review API is ready for review, it is NOT ready for implementation labels Sep 21, 2021
@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Nov 15, 2021
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Nov 18, 2021
@ghost ghost locked as resolved and limited conversation to collaborators Dec 19, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-approved API was approved in API review, it can be implemented area-System.Runtime
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants