Proposal: Standard ArgumentException messages

[justinvp]

Consider adding the ability to specify standard ArgumentException messages.

Rationale

When validating string, Array, and Collection parameters, in addition to checking for null and throwing ArgumentNullException, it is often necessary to check for empty and throw ArgumentException.

For example:

public void Foo(string name)
{
    if (name == null)
        throw new ArgumentNullException(nameof(name));
    if (name.Length == 0)
        throw new ArgumentException("Value cannot be empty.", nameof(name));

    ...
}

ArgumentNullException already has a generic localized message built-in, so typically all you have to do is specify the paramName:

throw new ArgumentNullException(nameof(name));

You get a localized "Value cannot be null." message from the framework for free.

However, ArgumentException requires specifying a message when specifying a paramName, so for the case of empty, you always have to specify your own "Value cannot be empty." message:

throw new ArgumentException("Value cannot be empty.", nameof(name));

It's on you to localize the message if you care about globalization/localization.

For .NET Core itself, this means that each assembly that has such validation checks for empty must have it's own associated localizable resource for the message embedded inside the assembly.

These kinds of empty checks exist throughout .NET Core. You can get a rough sense with this search: https://github.com/dotnet/corefx/search?q=%22new+ArgumentException%22+empty&type=Code

These existing checks have customized message resources, but many could be replaced with a generic "Value cannot be empty." message without losing any information about the error (given that the paramName is included with the message), allowing for the removal of many of the customized resources (if desired).

Proposed API

public class ArgumentException : SystemException, ISerializable
{
    // Proposed nested enum
    public enum StandardMessage
    {
        Empty,
        NullOrEmpty,
        NullOrWhiteSpace
    }

    // Proposed constructors
    public ArgumentException(StandardMessage message, Exception innerException);
    public ArgumentException(StandardMessage message, string paramName);
    public ArgumentException(StandardMessage message, string paramName, Exception innerException);

    // Existing constructors
    public ArgumentException();
    public ArgumentException(string message);
    public ArgumentException(string message, Exception innerException);
    public ArgumentException(string message, string paramName);
    public ArgumentException(string message, string paramName, Exception innerException);

    ...
}

Usage

public void Foo(string name)
{
    if (name == null)
        throw new ArgumentNullException(nameof(name));
    if (name.Length == 0)
        throw new ArgumentException(ArgumentException.StandardMessage.Empty, nameof(name));

    ...
}

Or simply:

public void Foo(string name)
{
    if (string.IsNullOrEmpty(name))
        throw new ArgumentException(ArgumentException.StandardMessage.NullOrEmpty, nameof(name));

    ...
}

Questions

• Since StandardMessage includes NullOrEmpty, should Null be included as well? What about OutOfRange?

  public enum StandardMessage
  {
     Null,
     OutOfRange,
     Empty,
     NullOrEmpty,
     NullOrWhiteSpace
  }

• Any others?

Alternative Proposal

Add ArgumentEmptyException along the lines of ArgumentNullException.

Usage

public void Foo(string name)
{
    if (name == null)
        throw new ArgumentNullException(nameof(name));
    if (name.Length == 0)
        throw new ArgumentEmptyException(nameof(name));

    ...
}

Downsides to ArgumentEmptyException:

• Adds a new developer error exception to the System namespace.
• Doesn't allow for other standard exception messages without adding more ArgumentException subclasses (e.g. ArgumentNullOrEmptyException or ArgumentNullOrWhiteSpaceException).

[KrzysztofCwalina]

I don't think we should be adding exception types for errors that are never to be handled programmatically. Maybe we should just add a static method to create ArgumentException instances for the "empty" case.

[justinvp]

Hmmm...

public void Foo(string name, int index)
{
    if (name == null)
        throw new ArgumentNullException("name");
    if (name.Length == 0)
        throw new ArgumentEmptyException("name");
    if (index < 0 || index >= name.Length)
        throw new ArgumentOutOfRangeException("name");

    ...
}

...versus something like:

public void Foo(string name, int index)
{
    if (name == null)
        throw new ArgumentNullException("name");
    if (name.Length == 0)
        throw ArgumentException.Empty("name");
    if (index < 0 || index >= name.Length)
        throw new ArgumentOutOfRangeException("name");

    ...
}

It is a little strange that creating an ArgumentException for the "Empty" case is different than "Null" and "OutOfRange".

If a static method for the "Empty" case was added, should static methods be added for "Null" and "OutOfRange" too, to provide a consistent way of creating these, or just live with the inconsistency?

public void Foo(string name, int index)
{
    if (name == null)
        throw ArgumentException.Null("name");
    if (name.Length == 0)
        throw ArgumentException.Empty("name");
    if (index < 0 || index >= name.Length)
        throw ArgumentException.OutOfRange("name");

    ...
}

Or perhaps defer and keep this in mind for the possible C# 7 language support for method contracts where there may be scenarios where it is desirable to still throw exceptions vs. failing fast.

[KrzysztofCwalina]

I agree that it's a bit strange, but adding a new type (presumably to the root namespace) so it is less strange does not seem to be a good cost/benefit, especially that parameters that cannot be empty are quite rare.
I would also not add other helpers (Null, OutOfRange, etc.) as it would amount to introducing a new idiom for creating exceptions, and again, I don't think it's worth it given how rare "empty" is.

[MgSam]

@KrzysztofCwalina I wouldn't say that this type of checking is rare at all. Any time you're passing a static string to a method you probably want to check that it is not empty (or whitespace). I've written similar code many, many times.

[akoeplinger]

I agree with @MgSam, the case doesn't sound rare to me too. ArgumentEmptyException feels a lot better than adding a static method to ArgumentException (as that would indeed be introducing an unwanted idiom).

[KrzysztofCwalina]

I honestly feel like the idiom should be:
if(String.IsNullOrEmpty(value)) {
throw new ArgumentException(nameof(value));
}
There is not reason to throw specific exception types for simple precondition violations. The calling code will not be handling these.

[stas-sultanov]

adding one more class does not looks like a good solution.
it will cause to add one more type in the catch clause to handle exception right.
let's look at another option:

1.Add enumeration that will describe actual reason of the argument inconsistency

public enum ArgumentInconsistencyType
{
Null,
Empty,
OutOfRange
}

2.Change ArgumentException to encapsulate argument name and inconsistency type.
3.Make ArgumentOutOfRangeException and ArgumentNullException obsolete.

[svick]

@stas-sultanov If you write just catch(ArgumentException ex), it will catch plain ArgumentException, but also ArgumentNullException, ArgumentOutOfRangeException and the proposed ArgumentEmptyException, since all of them inherit from ArgumentException. So I don't see why would it mean you have to expand your catch clauses.

(I also don't see how can you "handle exception right" when it comes to ArgumentException, but that's beside the point.)

[stas-sultanov]

@svick you forgot about atleast 7 more exceptions that also inherit from ArgumentException so in general case catch(ArgumentException ex) is not a good option.

[justinvp]

@stas-sultanov, as @KrzysztofCwalina mentions, you shouldn't be catching ArgumentException (or any of it's derived types). These are developer errors that should be fixed during development, not caught and handled in code. Hence the dilemma about adding a new ArgumentException type.

[justinvp]

@KrzysztofCwalina,

I honestly feel like the idiom should be:
if(String.IsNullOrEmpty(value)) {
throw new ArgumentException(nameof(value));
}

I'd like a simple idiom like this too, but what about the exception message? With that ArgumentException constructor, you're just passing the name of the parameter as the exception message.

I guess you could pass an empty message like this:

if (String.IsNullOrEmpty(value)) {
    throw new ArgumentException(string.Empty, nameof(value));
}

But then you're still not specifying what is wrong with parameter, leaving it up to the developer to guess or go to the documentation to read that it can't be null or empty.

[KrzysztofCwalina]

@justinvp, I thought about this more. Here is an idea I had:
public class ArgumentException : Exception {
public ArgumentException(StandardMessage message, string parameterName) { ... }

public enum StandardMessage {
     ArgumentEmpty,
     ArgumentNullOrEmpty,
     ....
}

}

then, calling code would look like:
if (String.IsNullOrEmpty(value)) {
throw new ArgumentException(StandardMessage.ArgumentNullOrEmpty, nameof(value));
}

[justinvp]

@KrzysztofCwalina, interesting idea. I like that it allows for adding more "standard messages" in the future without having to add more ArgumentException-derived types.

The enum would be a nested type within ArgumentException? If so, using StandardMessage would require prefixing ArgumentException. (e.g. ArgumentException.StandardMessage):

if (string.IsNullOrEmpty(value)) {
    throw new ArgumentException(ArgumentException.StandardMessage.ArgumentNullOrEmpty, nameof(value));
}

Somewhat verbose. I suppose the usage could be made slightly more succinct by shortening the enum value names:

if (string.IsNullOrEmpty(value)) {
    throw new ArgumentException(ArgumentException.StandardMessage.NullOrEmpty, nameof(value));
}

A non-nested enum would allow for an even more succinct usage:

if (string.IsNullOrEmpty(value)) {
    throw new ArgumentException(ArgumentExceptionMessage.NullOrEmpty, nameof(value));
}

I realize there is a desire to avoid adding types to the root System namespace. In this case, if the name of the non-nested enum is prefixed with ArgumentException it would be sorted/grouped with the other Argument- types, and wouldn't add much more "mental weight" to the namespace, IMO.

I wonder if there are any other "standard messages" worth considering for such an enum. Empty and NullOrEmpty are probably the most useful, but there may be others worth considering. A look through the uses of the Arg_ and Argument_ resources for mscorlib and similar resources copied and used throughout the corefx components may shed light on a few other widely used messages worth consolidating.

[KrzysztofCwalina]

I think it's reasonably verbose. It's close enough to retrieving a string from resources.

I think we could add NullOrWhitespace to the enum.

[KrzysztofCwalina]

@justinvp, so Justin, do you think this is something we should submit for formal API review?

[justinvp]

@KrzysztofCwalina, Yes. I'll update the speclet above with your suggested design before the API review.

[justinvp]

/cc @terrajobst

[terrajobst]

Looks reasonable to me.

@weshaggard what are your thoughts?

[OtherCrashOverride]

I think the proper solution, if this is intended to be implemented, is a class hierarchy:

class Exception;
class ArgumentException : Exception;
class EmptyArgumentException : ArgumentException;

OutOfRange is already handled by ArgumentOutOfRangeException
NullOrEmpty is already handled by ArgumentNullException and where its not (Empty), its ambiguous: is it null? or is it empty? If it cant be discerned, then just throw ArgumentException. The same applies to NullOrWhiteSpace

This implies we need a refactoring of the Exception classes where exceptions such ArgumentNullException becomes NullArgumentException and also inherits from ArgumentException. That ArgumentNullException and ArgumentOutOfRangeException are already accepted classes runs contrary to the argument that we should not be adding more exception classes 'that will never be handled'. If EmptyArgumentException is of no use, then so too is ArgumentNullException and ArgumentOutOfRangeException as they are both ArgumentException. With a class hierarchy, its up to the developer to decide whether trapping at fine grained or coarse level is needed and/or useful.

Alternatively, rather than breaking any classes or API's, instead of having enum ArgumentExceptionMessage, replace it with constant strings that may be used in the currently existing constructor:

public static class ArgumentExceptionMessage
{
    public static const string Empty = "The argument is empty."; // internationalized, etc
    public static const string OnFire = "The computer is on fire.";
}

[KrzysztofCwalina]

I don't think we should have hierarchies for exceptions that are usage errors and so should not be handled, but rather the calling code should be fixed. And argument exceptions are thrown in response to usage errors.

Also the strings cannot be const as they are localized depending on current culture.

[OtherCrashOverride]

I think it should be consistent. If we are not going to have different exceptions for usage errors, then we should remove ArgumentNullException, ArgumentOutOfRangeException, etc and replace them with ArgumentException. What is the reasoning (other than legacy) that differentiates them from the proposed ArgumentEmptyException and friends proposed here?

[KrzysztofCwalina]

We cannot remove these exceptions. Too massive of a breaking change.

[weshaggard]

I'm not entirely sold on the value of this yet but if we do go as far as modifying ArgumentException itself I think I prefer Justin's other sample:

public void Foo(string name, int index)
{
    if (name == null)
        throw ArgumentException.Null("name");
    if (name.Length == 0)
        throw ArgumentException.Empty("name");
    if (index < 0 || index >= name.Length)
        throw ArgumentException.OutOfRange("name");

    ...
}

I like those methods better then adding the enum, it is more concise and essentially just as extensible as the enum.

[OtherCrashOverride]

If you distil it down from the original proposal:

public enum StandardMessage
{
   Null,
   OutOfRange,
   Empty,
   NullOrEmpty,
   NullOrWhiteSpace
}

Null is already covered by ArgumentNullException.
OutOfRange is already covered by ArgumentOutOfRangeException.
NullOrEmpty is already covered by ArgumentNullException in the first case (ambiguous).
NullOrWhiteSpace is already covered by ArgumentNullException in the first case (ambiguous).

This means there are only two (2) new conditions being added:

public enum StandardMessage
{
   Empty,
   WhiteSpace
}

Empty only applies to indexable containers. Does it make sense for IEnumerable?
WhiteSpace only applies to System.String and System.Char.

Do these conditions provide valuable diagnostic information to developers and appear frequently enough to warrant first class exception support?

With regard to Empty, I am of the opinion that it is not "exceptional". Enumerating, for, or foreach over this type of parameter is effectively a noop and benign.

[Edit]
For clarity what I am stating is that for Empty and WhiteSpace, these are not actual ArgumentException. The arguments are perfectly valid. Instead, these are type of InvalidOperationException.

https://msdn.microsoft.com/en-us/library/system.invalidoperationexception%28v=vs.110%29.aspx
"The exception that is thrown when a method call is invalid for the object's current state."

[mikedn]

I'm not entirely sold on the value of this yet but if we do go as far as modifying ArgumentException itself I think I prefer Justin's other sample: ...

That would certainly be preferable to adding an enum because it solves another problem.

Throwing such an exception requires 3 or 4 calls today - one to allocate the exception, one to get the string, one to call the constructor and one to throw the exception. That's a lot of code considering that it is quite common for .NET methods to validate their arguments. If the methods are generic then you can end up with even more useless code if unshared generic instantiations are created due to value type arguments.

For this reason the framework itself already uses methods like ArgumentException.Null or even ArgumentException.ThrowNull in various places (mscorlib, LINQ). And other people have done this as well in their own code. And funnily enough everyone got fooled by the JIT compiler which currently inlines such methods.

[terrajobst]

The value seems to be too low to justify the work. @stephentoub estimated that it might at best save us 150 lines of code in CoreFX, which should be a good proxy for this feature (libraries). It's not that the idea is bad, but the cost-benefit ratio doesn't seem to be worth it.