runtime: [Analyzer Proposal]: Convert argument null checks to ArgumentNullException.ThrowIfNull

Can we also modify the existing “Add null check” quickfix in Visual Studio so that it has an option for generating an ArgumentNullException.ThrowIfNull call? 🙂

The “Add null check” quickfix currently looks like this:

Ideally, the quickfix would also have an option for generating a call to ArgumentNullException.ThrowIfNull, so that the resulting code looks like this:

public string Example(object name)
{
    ArgumentNullException.ThrowIfNull(name);

    return name.ToString();
}

(I originally posted this at https://github.com/dotnet/roslyn/issues/61181, and someone directed me here 🙂)

It might be good to check whether/when the boxing is elided by JIT in such scenarios.

Video

Make sense as proposed

We feel like this would also approve the other throw helpers:

• ObjectDisposedException.ThrowIf
• ArgumentException.ThrowIfNullOrEmpty

@CyrusNajmabadi and I chatted offline and we think that a pared-down version of the feature might be appropriate here.

We think that it might be most appropriate to handle this by:

1. removing the ‘UseParameterNullChecking’ analyzer/fixer that was added for ‘!!’ (possibly migrating the tests elsewhere).
2. adjust the implementation of the ‘UseThrowExpression’ analyzer to suggest the ‘ArgumentNullException.ThrowIfNull’ form if a null-checked parameter is not assigned to a field after.
3. ensure that we have separate editorconfig settings to control “prefer param ?? throw” independently from "prefer ThrowIfNull(param).

// before
public Widget(string param)
{
    if (param is null)
    {
        throw new ArgumentNullException(nameof(param));
    }
    this.param = param;
}

// after
public Widget(string param)
{
    this.param = param ?? throw new ArgumentNullException(nameof(param));
}

// before
public void M(string param)
{
    if (param is null)
    {
        throw new ArgumentNullException(nameof(param));
    }
}

// after
public void M(string param)
{
    ArgumentNullException.ThrowIfNull(param);
}

We also have concerns about the perf benefits that are suggested to be present when the user uses a helper method to throw for an uncommon case, versus using a throw directly in an uncommon code path in the same method. That’s specifically the fact that methods which directly include throw can’t be inlined, right?

It feels like it would be ideal to adjust the runtime so it is able to inline methods which contain throw, so that large amounts of existing code can benefit. To some extent it’s concerning to ask users to adjust their coding patterns to accommodate what seems like an internal behavior detail of the runtime.

I’m going to look at this one…

aspnetcore is interested in this analyzer. We have a lot of source code with traditional null checks. We’d use the analyzer+fixer to automate updating our repo source - https://github.com/dotnet/aspnetcore/issues/43482

It feels like it would be ideal to adjust the runtime so it is able to inline methods which contain throw, so that large amounts of existing code can benefit.

or, alternatively, the runtime should look specifically for if (x is null) throw ArgumentNullException(nameof(x))`` and treat that identically to whatever it generates for ArgumentNullException.ThrowIfNull(x)`. Basically, it’s not a pit of success for these two idiomatic expressions to have such different perf outcomes.

I think this is a good idea, and that this would be done by adjusting the existing analyzer/fixer in the Roslyn IDE code style layer. Tagging @CyrusNajmabadi for a second opinion.