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

[stephentoub]

In support of the now-defunct !! feature, @RikkiGibson built into a Roslyn an analyzer/fixer that found constructs like:

void M(string arg)
{
    if (arg is null)
        throw new ArgumentNullException(arg);
    ...
}

and replaced them with:

void M(string arg!!)
{
    ...
}

Now that !! is no more, we should salvage this work and transform it to instead use ArgumentNullException.ThrowIfNull if available:

void M(string arg)
{
    ArgumentNullException.ThrowIfNull(arg);
    ...
}

Tagging subscribers to this area: @mangod9
See info in area-owners.md if you want to be subscribed.

Issue Details

---

In support of the now-defunct !! feature, @RikkiGibson built into a Roslyn an analyzer/fixer that found constructs like:

void M(string arg)
{
    if (arg is null)
        throw new ArgumentNullException(arg);
    ...
}

and replaced them with:

void M(string arg!!)
{
    ...
}

Now that !! is no more, we should salvage this work and transform it to instead use ArgumentNullException.ThrowIfNull if available:

void M(string arg)
{
    ArgumentNullException.ThrowIfNull(arg);
    ...
}

Author:	stephentoub
Assignees:	-
Labels:	area-System.Threading, untriaged, code-analyzer, code-fixer, api-ready-for-review
Milestone:	7.0.0

[RikkiGibson]

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.

[CyrusNajmabadi]

Yup.

[RikkiGibson]

I'm going to do this before removing '!!' from the compiler, since it should let us leave a bunch of the existing IDE tests and plumbing in place.

[RikkiGibson]

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

[CyrusNajmabadi]

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.