ArgumentException raising ArgumentNullException on ThrowIfNullOrEmpty

[StevePy]

Description

This seems to be a break in inheritance where a base class should never be aware of its subclasses. The ThrowNullOrEmptyException method in ArgumentException is handing off to a static method in the ArgumentNullException subclass which results in the ArgumentNullException being raised. When I am using the ArgumentException.ThrowIfNullOrEmpty() guard and I would expect an ArgumentException if that guard trips.

I can see why it was written that way, but if I had an Animal base class and a Bear, an Elk, and an Eagle subclass extending Animal, it would be very weird to have a static method in Animal calling something in just Bear. You wouldn't do it in within the scope of an "as Animal" instance, and it does not seem right to do it in the static scope either.

Reproduction Steps

As a very base example:

[TestCase("")]
[TestCase(null)]
public void AssertValueWasRequired(string? value)
{
    Assert.Throws<ArgumentException>(() => var model = new Model(value!));
}

public class Model
{
    public Model(string value)
    {
        ArgumentException.ThrowIfNullOrEmpty(value, nameof(value));
    }
}

Expected behavior

ArgumentException raised in both cases.

Actual behavior

The first test case passes, the second fails receiving ArgumentNullException instead of ArgumentException.

Regression?

I don't believe a regression, ThrowIfNullOrEmpty guards were added in .Net Core. The methods to hand off to the subclass ArgumentNullException was added 2-3 years ago.

Known Workarounds

It is an exception guard so not a breaking issue, the unit tests that might be asserting the guards need to handle the fact that two different exceptions could be returned for the same guard.

Configuration

.Net Core 9, Windows 10, x64. Not specific to this configuration.

Other information

No response

[RenderMichael]

ArgumentException raised in both cases.

This is already the case. ArgumentNullException is an ArgumentException.

This seems to be a break in inheritance where a base class should never be aware of its subclasses

This is a loose coding guideline that’s violated all the time. Any time you see factory Create methods, chances are a derived type is returned from the base type.

Personally I was hesitant to use ThrowIfNullOrEmpty at first, for the inverse reason. I want to throw ArgumentNullException when the argument is null, and the throw helper only became usable for me when I realized it did just that.

[elgonzo]

How ArgumentException.ThrowIfNullOrEmpty()'s code is structured is a non-public implementation detail. There can be a discussion had about it, but the style in which it is implemented in itself is not a bug.
ArgumentNullException is an ArgumentException (as @RenderMichael pointed out) that is being (or should be) thrown when an argument/parameter value is null -- it always was and still is the same convention. ArgumentException.ThrowIfNullOrEmpty() not throwing an ArgumentNullException when the argument value is null would be a violation of that longstanding convention and would basically become in conflict with the documentation for ArgumentException itself. Please take a look at the Remarks section: https://learn.microsoft.com/en-us/dotnet/api/system.argumentexception?view=net-9.0#remarks:

The primary derived classes of ArgumentException are ArgumentNullException and ArgumentOutOfRangeException. These derived classes should be used instead of ArgumentException, except in situations where neither of the derived classes is acceptable. For example, exceptions should be thrown by:

• ArgumentNullException whenever null is passed to a method that does not accept it as a valid argument.

If you are going to test something, you can't just make tests based off your assumptions and intuitions. You have to test against the defined/documented behavior. This is what the documentation for ArgumentException.ThrowIfNullOrEmpty() has to say about the kind of exceptions it can throw (https://learn.microsoft.com/en-us/dotnet/api/system.argumentexception.throwifnullorempty?view=net-9.0#system-argumentexception-throwifnullorempty(system-string-system-string)):

Exceptions

ArgumentNullException
argument is null.

ArgumentException
argument is empty.

There are no workarounds necessary, just stick to the documented behavior when writing your tests. Therefore, the correct expectation to begin with is that your 2nd test case has to fail as it is supposed to.

[StevePy]

The issue isn't the fact that an ArgumentNullException is raised for a #null vs. an ArgumentException being raised for an empty string. When the guard methods were introduced on ArgumentNullException.ThrowIfNull() I was originally wishing that it would have covered "OrEmpty" strings as well, but was happy with it being covered under ArgumentException where it belongs.

Though I do get it, ArgumentNullException "is-a" ArgumentException, so returning one itself isn't a bug. Part of the issue is NUnit's behaviour as it is a bit disappointing that Assert.Throws<ArgumentException>() is not satisfied when an ArgumentNullException is raised, though I suspect there are good reasons to be pessimistic. However a work-around where the type inspection is more forgiving is Assert.Catch<ArgumentException>() which can pretty much be substituted and perform the same check.

My test doesn't care whether the scenario raises a specific exception in each case, more-so that an exception is bubbled. This is a rare check in a unit test as most public behaviour I am testing handles exceptions and returns a particular result. This comes up more when I have written more low-level functionality that I want specific tests covering that behaviour rather than relying on just "touches" from higher level tests.

This would have be a completely non-issue if the guards were in fact treated using a factory-like class rather than factory methods. For instance if I have ArgumentGuard.ThrowIfNull() resulting in a thrown ArgumentNullException vs ArgumentGuard.ThrowIfNullOrEmpty() throwing an ArgumentNullException or ArgumentException as suitable. A class like ArgumentGuard is serving as an abstract factory (not-factory, "thrower?") class. The issue/smell I was raising is having factory-like methods as part of a specific type that are handing off and returning different types. In object instantiation land I might create an AnimalFactory class to create Bears, Elk, and Eagles etc. but I don't believe it would be good practice to have factory methods creating sub-classes in the Animal base class.

Edit: Fine to have this closed as a non-issue / as-designed given the test assertion workaround /w Assert.Catch works just fine.

[RenderMichael]

Part of the issue is NUnit's behaviour as it is a bit disappointing that Assert.Throws<ArgumentException>() is not satisfied when an ArgumentNullException is raised

Modern NUnit syntax encourages you to use the constraint syntax, with the Assert.That method. In your case, it would look like Assert.That(() => var model = new Model(value!), Throws.TypeOf<ArgumentException>()); for type equality assertions (and would fail in your case) and Assert.That(() => var model = new Model(value!), Throws.InstanceOf<ArgumentException>()); for assignability assertion, which looks like the right thing for your scenario.

[dotnet-policy-service]

Tagging subscribers to this area: @dotnet/area-system-runtime
See info in area-owners.md if you want to be subscribed.

[tannergooding]

This would have be a completely non-issue if the guards were in fact treated using a factory-like class rather than factory methods. For instance if I have ArgumentGuard.ThrowIfNull() resulting in a thrown ArgumentNullException vs ArgumentGuard.ThrowIfNullOrEmpty() throwing an ArgumentNullException or ArgumentException as suitable. A class like ArgumentGuard is serving as an abstract factory (not-factory, "thrower?") class. The issue/smell I was raising is having factory-like methods as part of a specific type that are handing off and returning different types. In object instantiation land I might create an AnimalFactory class to create Bears, Elk, and Eagles etc. but I don't believe it would be good practice to have factory methods creating sub-classes in the Animal base class.

If I remember correctly, such concepts were discussed as part of exposing the ThrowIf*() helpers as well, and it was decided against due to the additional bloat, potential versioning issues, discoverability issues, etc.

In general the BCL tends to not have such separate factory classes and in places that they have been defined/exposed in the past, we've often found them to be too limiting for long term versioning. -- This is notably also why System.Math is no longer being versioned since .NET 7 and instead the APIs are mirrored directly on the primitive types now (which was done as part of generic math and also allows the built-in primitive types to implement the various INumber<T> and related interfaces).

There are dozens to hundreds of design patterns, and for every design pattern that says "x is a code smell" there is an alternative pattern that says its the right thing and that y is the code smell instead. The BCL typically uses the Framework Design Guidelines (https://www.amazon.com/Framework-Design-Guidelines-Conventions-Addison-Wesley/dp/0135896460 -- of which a limited and slightly outdated high level overview is https://learn.microsoft.com/en-us/dotnet/standard/design-guidelines/). This book comes from decades of combined design experience in .NET and other languages from the API review team and other key .NET team members (past and present). The general adherence to it and decisions that are based on it are part of what has allowed .NET to stand the test of time and have so few breaking changes and almost no redesigns for its core. It's what has allowed code written and types designed 20 years ago to still be just as applicable today in most scenarios.