[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);
    ...
}

[ghost]

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.

[Eli-Black-Work]

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 dotnet/roslyn#61181, and someone directed me here 🙂)

[terrajobst]

Video

Make sense as proposed

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

• ObjectDisposedException.ThrowIf
• ArgumentException.ThrowIfNullOrEmpty

[GrabYourPitchforks]

Implementation nit: make sure the analyzer doesn't result in inadvertent boxing.

// no boxing
public static void DoIt<T>(T value) {
    if (value is null) { throw new ArgumentNullException(/* ... */); }
}

// boxes in all cases
public static void DoIt<T>(T value) {
    ArgumentNullException.ThrowIfNull(value);
}

[Eli-Black-Work]

@GrabYourPitchforks I wonder if there should also be a performance analyzer that warns about user code that does that. I certainly wouldn't have thought of it 😅

[RikkiGibson]

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

[JamesNK]

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 - dotnet/aspnetcore#43482

[buyaa-n]

@RikkiGibson are you planning to implement this analyzer? If so, will remove the help-wanted label

[RikkiGibson]

Thanks for following up. I don't anticipate I will have time to work on it.

[buyaa-n]

Estimates:

Analyzer: Medium
Fixer: Medium

[danmoseley]

the UseParameterNullChecking analyzer/fixer, now removed, can be cribbed from https://github.com/dotnet/roslyn/pulls/58182

Would this go in the Roslyn repo, or the roslyn-analyzers repo? Not sure which is for what. I'm guessing !! was a language feature, hence it was in dotnet/roslyn.

I assume this analyzer would only handle ThrowIfNull, as others like ArgumentOutOfRangeException.ThrowIf, ObjectDisposedException.ThrowIf and the others proposed (#77749, #69590) are sufficiently different.

[stephentoub]

Would this go in the Roslyn repo, or the roslyn-analyzers repo?

roslyn-analyzers

I assume this analyzer would only handle ThrowIfNull

Each should have its own diagnostic ID, but whether they're implemented in the same analyzer is an implementation detail.

Are you asking because you're planning to work on it?

[stephentoub]

I'm going to look at this one...

[Eli-Black-Work]

@stephentoub Sorry to bug, but did @GrabYourPitchforks's nit at #68326 (comment) get addressed? (And if not, shall I file a new bug for that?)

[stephentoub]

The analyzer fires for generics as well. I went back and forth on whether to do so, as even though it results in boxing in the IL, the JIT will undo the boxing, at least in tier 1, e.g. SharpLab. But if we want to change the analyzer to not raise diagnostics for unconstrained or struct-constrained generic parameters, I wouldn't push back.

[Eli-Black-Work]

Got it, thanks 🙂