[API Proposal]: ArgumentException.ThrowIfNullOrEmpty(string)

[datvm]

Background and motivation

Inspired by ArgumentNullException.ThrowIfNull(object). string.IsNullOrEmpty(text) is a frequent check especially in web/Console project. At first I thought InvalidDataException would be the best name but it is in System.IO namespace, so I think the closest should be ArgumentException so user doesn't need to import that namespace.

Now:

if (string.IsNullOrEmpty(input)) { throw new ArgumentException($"{nameof(input)} must contain value."); }

If the API is approved:

ArgumentException.ThrowIfNullOrEmpty(input);

While we are at it, personally I rarely use it but maybe ThrowIfNullOrWhiteSpace may be useful for others as well?

API Proposal

namespace System
{
    public class ArgumentException : SystemException
    {
        public static void ThrowIfNullOrEmpty([NotNull] string? argument, [CallerArgumentExpression("argument")] string? paramName = null)
    }
}

API Usage

ArgumentException.ThrowIfNullOrEmpty(input);

Alternative Designs

No response

Risks

No response

[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.

[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

Inspired by ArgumentNullException.ThrowIfNull(object). string.IsNullOrEmpty(text) is a frequent check especially in web/Console project. At first I thought InvalidDataException would be the best name but it is in System.IO namespace, so I think the closest should be ArgumentException so user doesn't need to import that namespace.

Now:

if (string.IsNullOrEmpty(input)) { throw new ArgumentException($"{nameof(input)} must contain value."); }

If the API is approved:

ArgumentException.ThrowIfNullOrEmpty(input);

While we are at it, personally I rarely use it but maybe ThrowIfNullOrWhiteSpace may be useful for others as well?

API Proposal

namespace System
{
    public class ArgumentException : SystemException
    {
        public static void ThrowIfNullOrEmpty([NotNull] string argument, [CallerArgumentExpression("argument")] string? paramName = null)
    }
}

API Usage

ArgumentException.ThrowIfNullOrEmpty(input);

Alternative Designs

No response

Risks

No response

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

[Clockwork-Muse]

[NotNull]

... but the parameter can be null, since that's the point of the exception?

[stephentoub]

The argument to the existing ThrowIfNull is declared as:

[NotNull] object? argument

which uses ? to avoid warnings when being used with something that may be null, but then [NotNull] to say "upon successful return from this method, it's guaranteed argument isn't null."

[danmoseley]

Makes sense to me. It would collapse stuff like

runtime/src/libraries/System.Private.CoreLib/src/System/Threading/Mutex.Windows.cs

Lines 44 to 52 in dfbae37

	if (name == null)
	{
	throw new ArgumentNullException(nameof(name));
	}
	if (name.Length == 0)
	{
	throw new ArgumentException(SR.Argument_EmptyName, nameof(name));
	}

from 10 lines to 1.

In that example, it has a custom message, ie "Empty name is not legal.", and using this proposed API would change that to something generic. I think that's totally fine. There are cases where perhaps the message adds slightly more value eg "Empty file name is not legal." and perhaps those would not change to use this API, which is fine.

[hrrrrustic]

Maybe it would also make sense to add overload with IReadOnlyCollection<T> or simply ICollection<T>?

public static void ThrowIfNullOrEmpty<T>([NotNull] I/*ReadOnly*/Collection<T> argument, [CallerArgumentExpression("argument")] string? paramName = null);

Usages:

runtime/src/libraries/System.Diagnostics.EventLog/src/System/Diagnostics/EventLog.cs

Lines 264 to 265 in 895c99c

	if (source == null || source.Length == 0)
	throw new ArgumentException(SR.Format(SR.MissingParameter, nameof(source)));

runtime/src/libraries/System.Security.Cryptography.X509Certificates/src/System/Security/Cryptography/X509Certificates/X509Certificate.cs

Lines 84 to 85 in 895c99c

	if (rawData == null || rawData.Length == 0)
	throw new ArgumentException(SR.Arg_EmptyOrNullArray, nameof(rawData));

Didn't try to find splitted if-checks

Makes sense to me. It would collapse stuff like

runtime/src/libraries/System.Private.CoreLib/src/System/Threading/Mutex.Windows.cs

Lines 44 to 52 in dfbae37

	if (name == null)
	{
	throw new ArgumentNullException(nameof(name));
	}
	if (name.Length == 0)
	{
	throw new ArgumentException(SR.Argument_EmptyName, nameof(name));
	}

from 10 lines to 1.

In that example, it has a custom message, ie "Empty name is not legal.", and using this proposed API would change that to something generic. I think that's totally fine. There are cases where perhaps the message adds slightly more value eg "Empty file name is not legal." and perhaps those would not change to use this API, which is fine.

[Rubenisme]

If this is done, then after that I'm sure someone will ask: "What about an ArgumentException.ThrowIfNullOrWhiteSpace?".
I think the string.IsNullOrWhiteSpace method gets used a lot less than the string.IsNullOrEmpty method, so might not be as useful.

[terrajobst]

Video

• Looks good as proposed
  • Might be good to do a pass what ArgumentOutOfRangeException cases we use internally. We could base it off the unique resource strings we use in corlib.
• The API should
  • Throw ArgumentNullException if argument is null, otherwise ArgumentException
• We don't believe we need an overload of ThrowIfNullOrEmpty for collections as this feels rare.

namespace System
{
    public partial class ArgumentException
    {
        public static void ThrowIfNullOrEmpty([NotNull] string? argument,
                                              [CallerArgumentExpression("argument")] string? paramName = null);
    }
}

[hrrrrustic]

Guys check suggestion about ICollection above please :)

[terrajobst]

@hrrrrustic I edited my comment.

[danmoseley]

@datvm do you want to add the API now?

[datvm]

@danmoseley awesome, please go ahead

[deeprobin]

To conform to the string overloads, it would also make sense to create an overload for IsNullOrWhiteSpace.

namespace System
{
    public partial class ArgumentException
    {
        public static void ThrowIfNullOrEmpty([NotNull] string? argument,
                                              [CallerArgumentExpression("argument")] string? paramName = null);
        public static void ThrowIfNullOrWhiteSpace([NotNull] string? argument,
                                              [CallerArgumentExpression("argument")] string? paramName = null);
    }
}

[terrajobst]

Do you find yourself ever checking against whitespace only?

[deeprobin]

Only against whitespace actually not. But checking against null or whitespace is a common practice.
This includes partial argument parsing of commands (if you don't want a command to be trimmed, for example).

Even if there are not many use cases for this case with me, I think imo it would be good to add this case anyway for completeness.

[datvm]

add this case anyway for completeness.

I wonder what's the API designer team philosophy on this. From the video I watched, it seems they prefer to have only very needed APIs added instead of "completeness". I understand and agree with this approach since .NET now can be used in Web environment through WebAssembly as well so they probably want the standard library to be as small as possible. You should check the above video, they discussed about the possibilities of adding other throw helpers as well.

[deeprobin]

Makes sense to me. It would collapse stuff like

runtime/src/libraries/System.Private.CoreLib/src/System/Threading/Mutex.Windows.cs

Lines 44 to 52 in dfbae37

	if (name == null)
	{
	throw new ArgumentNullException(nameof(name));
	}
	if (name.Length == 0)
	{
	throw new ArgumentException(SR.Argument_EmptyName, nameof(name));
	}

from 10 lines to 1.

In that example, it has a custom message, ie "Empty name is not legal.", and using this proposed API would change that to something generic. I think that's totally fine. There are cases where perhaps the message adds slightly more value eg "Empty file name is not legal." and perhaps those would not change to use this API, which is fine.

But that would change the exception behavior, since an ArgumentException would be thrown instead of an ArgumentNullException?

Or should the proposal throw an ArgumentNullException? In that case it would make sense to move it to the ArgumentNullException class?

[deeprobin]

add this case anyway for completeness.

I wonder what's the API designer team philosophy on this. From the video I watched, it seems they prefer to have only very needed APIs added instead of "completeness". I understand and agree with this approach since .NET now can be used in Web environment through WebAssembly as well so they probably want the standard library to be as small as possible. You should check the above video, they discussed about the possibilities of adding other throw helpers as well.

You're right about that.
For NativeAOT we could consider that isn't it? Or are unused methods not optimized away?

[danmoseley]

Or should the proposal throw an ArgumentNullException? In that case it would make sense to move it to the ArgumentNullException class?

As noted above, it should throw ANE (which is-a AE) if the argument is null.

[danmoseley]

@danmoseley awesome, please go ahead

@datvm I meant, are you interested in making the PR? No obligation, it's just that you opened the issue so asking you first. If anyone else is interested, they're welcome too.

We have full instructions for how to contribute and can help as much as you need.

[datvm]

@danmoseley ah thanks for clarifying. I will leave this one for someone else. I will try next time when I have studied an example pull request.

[danmoseley]

@datvm sounds good, if you are interested there are issues with the 'easy' label and perhaps there's one you might be interested to start with.

Anyone else interested in offering a PR here?

[deeprobin]

@datvm sounds good, if you are interested there are issues with the 'easy' label and perhaps there's one you might be interested to start with.

Anyone else interested in offering a PR here?

Doesn't seem like a big deal. If it's nothing urgent, I'm happy to do it.

[stephentoub]

Related: dotnet/roslyn#59094