[API Proposal]: ArgumentException.ThrowIfNullOrEmpty(string)

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

[NotNull]

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

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

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.

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.

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.

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);
    }
}

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?

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?

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.

@danmoseley

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

@datvm

@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?

@datvm

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

Related: dotnet/roslyn#59094

Navigation Menu

Repository navigation

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

Background and motivation

API Proposal

API Usage

Alternative Designs

Risks

8 remaining items

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

	if (name == null)
	{
	throw new ArgumentNullException(nameof(name));
	}

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

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

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

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

Description

Background and motivation

API Proposal

API Usage

Alternative Designs

Risks

Activity

Clockwork-Muse commented on Dec 10, 2021

stephentoub commented on Dec 10, 2021

danmoseley commented on Dec 14, 2021

hrrrrustic commented on Dec 15, 2021

Rubenisme commented on Dec 23, 2021

terrajobst commented on Jan 4, 2022

8 remaining items

deeprobin commented on Jan 11, 2022

deeprobin commented on Jan 11, 2022

danmoseley commented on Jan 11, 2022

danmoseley commented on Jan 11, 2022

datvm commented on Jan 11, 2022

danmoseley commented on Jan 11, 2022

deeprobin commented on Jan 12, 2022

stephentoub commented on Jan 27, 2022

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions