Why does ReSharper dislike ArgumentNullException.ThrowIfNullOrEmpty(string)?

Question

I am using ReSharper 2024.2 and still I get the suggestion that I should not be using the following code:

    public async Task<T> GetQueryAsync<T>(string query, bool isToBeValidated = true) where T : IEndPointResponse
{
    ArgumentNullException.ThrowIfNullOrEmpty(query);
    ArgumentNullException.ThrowIfNullOrWhiteSpace(query);
    ArgumentNullException.ThrowIfNull(Logger);

Instead it suggests I use

    public async Task<T> GetQueryAsync<T>(string query, bool isToBeValidated = true) where T : IEndPointResponse
{
    ArgumentException.ThrowIfNullOrEmpty(query);
    ArgumentException.ThrowIfNullOrWhiteSpace(query);
    ArgumentNullException.ThrowIfNull(Logger);

Or

    public async Task<T> GetQueryAsync<T>(string query, bool isToBeValidated = true) where T : IEndPointResponse
{
    if (string.IsNullOrEmpty(query) || string.IsNullOrWhiteSpace(query)) throw new ArgumentNullException(nameof(query));

The issue is that the calling the derived class method ArgumentException.ThrowIfNullOrEmpty() will return an ArgumentException!

Here's an example demonstrating my issue

[TestFixture]
public class Tests
{
    readonly MyClass _testClass = new();
#region Passes due to null being passed in and ArgumentNullException being thrown

[Test]
public void Test_MyMethod_Passes()
{
    Assert.Throws<ArgumentNullException>(() =>
    {
        _ = _testClass.Method(null); // passes with ArgumentNullException
    });
}

[Test]
public void Test_MyMethodAsync_Passes()
{
    Assert.ThrowsAsync<ArgumentNullException>(async () => await _testClass.MethodAsync(null).ToListAsync()); // passes with ArgumentNullException
}

#endregion

#region Fails with ArgumentException being returned instead of ArgumentNullException

[TestCase("")]
[TestCase("   ")]
public void Test_MyMethod(string someText)
{
    Assert.Throws<ArgumentNullException>(() => _testClass.Method(someText)); // fails with ArgumentException not ArgumentNullException
}

[TestCase("")]
[TestCase("   ")]
public void Test_MyMethodAsync_Fails(string someText)
{
    Assert.ThrowsAsync<ArgumentNullException>(async () => await _testClass.MethodAsync(someText).ToListAsync()); // fails with ArgumentException not ArgumentNullException
}

#endregion
}

public class MyClass
{
public List<string> Method(string? someText)
{
    ArgumentNullException.ThrowIfNull(someText);
    ArgumentException.ThrowIfNullOrEmpty(someText);
    ArgumentException.ThrowIfNullOrWhiteSpace(someText);

    var result = new List<string> { "Document1", "Document2" };
    return result;
}

public async IAsyncEnumerable<string> MethodAsync(string? someText)
{
    ArgumentNullException.ThrowIfNull(someText);
    ArgumentException.ThrowIfNullOrEmpty(someText);
    ArgumentException.ThrowIfNullOrWhiteSpace(someText);

    // Simulate async data fetching
    await Task.Delay(100);
    yield return "Document1";
    yield return "Document2";
}
}

TL;DR

If I want ArgumentNullException, I have to use ArgumentNulLException.ThrowIfNullOrEmpty(someText) otherwise I get ArgumentException.

"but this is terrible because all my unit tests expect ArgumentNullException" - what makes you think that calling ArgumentException.ThrowIfNullOrEmpty won't throw an ArgumentNullException? Just because it's declared in ArgumentException doesn't mean that's what it will throw. Did you try running your tests after applying the ReSharper suggestion? — Jon Skeet
– Jon Skeet, Commented Sep 30, 2024 at 17:52
Interesting - but I guess I was trying to understand why this is so? It seems to imply that it will return the static member type. How does it know to throw ArgumentNullException when calling a static type of the base class? I tried following the code but had trouble understanding what was going on — csharpforevermore
– csharpforevermore, Commented Sep 30, 2024 at 18:00
"It seems to imply that it will return the static member type" - where do you get that implication from? You're just calling a static method. It could be declared on any class - the fact that it's declared on ArgumentException is a coincidence. — Jon Skeet
– Jon Skeet, Commented Sep 30, 2024 at 18:01
Should I delete this question now? Can't find a suitable duplicate of it to match it to — csharpforevermore
– csharpforevermore, Commented Sep 30, 2024 at 18:04
No, I wouldn't delete it. It might be useful to someone else making the same mistake. — Jon Skeet
– Jon Skeet, Commented Sep 30, 2024 at 18:23

Jon Skeet · Accepted Answer · 2024-10-01 07:05:45Z

5

What is the rationale behind this? Is it a bug, or should I always throw ArgumentException?

Neither. You should understand that ArgumentException.ThrowIfNullOrEmpty throws ArgumentNullException if the argument is null. It's just a static method on ArgumentException. What type you call it "through" doesn't change the implementation.

Imagine the methods had actually been written like this:

public class UtilityClass
{
    public static void ThrowIfNullOrEmpty(string? argument,
        string? paramName = default)
    {
        if (argument is null)
        {
            throw new ArgumentNullException(paramName);
        }
    }
}

public class DerivedUtilityClass : UtilityClass
{
}

Now imagine your code was written like this:

public async Task<T> GetQueryAsync<T>(string query, bool isToBeValidated = true) where T : IEndPointResponse
{
    DerivedUtilityClass.ThrowIfNullOrEmpty(query);
    ...
}

ReSharper is effectively suggesting that your code should be written as:

```csharp
public async Task<T> GetQueryAsync<T>(string query, bool isToBeValidated = true) where T : IEndPointResponse
{
    // Note: UtilityClass not DerivedUtilityClass
    UtilityClass.ThrowIfNullOrEmpty(query);
    ...
}

The type you call the method on doesn't determine the exception thrown. The only case in which this refactoring wouldn't be appropriate would be if ArgumentNullException redeclared ThrowIfNullOrEmpty - which would be pretty ghastly, and which isn't the case. But in that situation, the "refactoring" would genuinely change which implementation is called. In reality, calling ArgumentNullException.ThrowIfNullOrEmpty is equivalent to calling ArgumentException.ThrowIfNullOrEmpty. It's also equivalent to calling IOException.ThrowIfNullOrEmpty - that wouldn't throw an IOException!

To address your revised code: your tests are wrong. Leaving out the irrelevant async and LINQ aspects, you're effectively asserting that ArgumentException.ThrowIfNullOrEmpty(someText) will throw ArgumentNullException when someText is "", i.e. not null.

That would:

Violate the intent of ArgumentNullException (which should be for null values only, not empty strings or whitespace)
Ignore the documented behavior of ArgumentException.ThrowIfNullOrEmpty (which explicitly states that it throws ArgumentException rather than ArgumentNullException for an empty string
Not change if you called it as ArgumentNullException.ThrowIfNullOrEmpty(someText). (That still throws ArgumentException.) In other words, the tests you've provided here still fail with your original code. (I've checked.)

edited Oct 1, 2024 at 7:05

answered Sep 30, 2024 at 17:57

Jon Skeet

1.5m893 gold badges9.3k silver badges9.3k bronze badges

Sign up to request clarification or add additional context in comments.

3 Comments

csharpforevermore Over a year ago

Please see my revised code example

Jon Skeet Over a year ago

@csharpforevermore: Yes, if you have tests which expect something other than the documented behavior, you should expect them to fail. See the new part of my answer from "To address your revised code" onwards.

csharpforevermore Over a year ago

So empty strings or whitespace should throw ArgumentExceptions. Nullable strings should throw ArgumentNullException. Got it.

Michał Turczyn · Accepted Answer · 2024-10-01 08:05:42Z

1

That's correct suggestion.

If you would read docs on ArgumentException.ThrowIfNullOrEmpty, it is written:

Exceptions
ArgumentNullException argument is null.
ArgumentException argument is empty.

So, when the argument is null it still throws ArgumentNullException.

Thus, suggestion is valid.

edited Oct 1, 2024 at 8:05

answered Sep 30, 2024 at 17:57

Michał Turczyn

41.2k18 gold badges58 silver badges87 bronze badges

Collectives™ on Stack Overflow

Why does ReSharper dislike ArgumentNullException.ThrowIfNullOrEmpty(string)?

2 Answers 2

3 Comments

Comments

Your Answer

Hot Network Questions

Collectives™ on Stack Overflow

2 Answers 2

3 Comments

Comments

Your Answer

Sign up or log in

Post as a guest

Related