Remove usage of !! from dotnet/runtime

[stephentoub]

React to:
https://devblogs.microsoft.com/dotnet/csharp-11-preview-updates/#remove-parameter-null-checking-from-c-11
https://github.com/dotnet/csharplang/blob/main/meetings/2022/LDM-2022-04-13.md#c-language-design-meeting-for-april-11th-2022

• Use ArgumentNullException.ThrowIfNull instead where possible. It's only usable for projects that only target .NET 6+, and it can't be used in places like this(...) or base(...).
• In other cases, if the project already has a ThrowHelper, augment it for null as needed and use that.
• For most of the extensions projects, add a ThrowHelper.ThrowIfNull that replicates ArgumentNullException.ThrowIfNull.
• For everything else, just use "throw new".

[GrabYourPitchforks]

Review based on acd12317c3d8d1b54ffd14a8a6624be8ab35059d

• System.Formats.Asn1 - OK
• System.Formats.Cbor - OK
• System.Memory - OK
• System.Memory.Data - OK
• System.Private.CoreLib/src/System/Security - OK
• System.Security.Claims - had question, see comment below
• System.Security.Cryptography - OK, but comments left

[GrabYourPitchforks]

The GitHub UI is misbehaving trying to leave feedback within the PR. But in ClaimsIdentity.cs and ClaimsPrincipal.cs: the 6.0 behavior is that for iterators, the ArgumentNullException is thrown lazily, where in 7.0 it's thrown eagerly. I believe your 7.0 behavior is correct but wanted to surface it as a potential breaking change and make sure we were ok with it.

[stephentoub]

But in ClaimsIdentity.cs and ClaimsPrincipal.cs: the 6.0 behavior is that for iterators, the ArgumentNullException is thrown lazily, where in 7.0 it's thrown eagerly. I believe your 7.0 behavior is correct but wanted to surface it as a potential breaking change and make sure we were ok with it.

Thanks, yes, I should have mentioned that. In going through and manually reviewing every iterator and async method that had !!, I fixed a few to do the "right thing", which is to continue throwing the ArgumentNullException out of the method call rather than having it delayed until MoveNext. There were these in Claims, a handful in Xml, and one or two others. I think they fall under the bug-for-bug compat category and don't need to be called out as breaks unless we start to see lots of folks actually getting bitten by them (which I doubt will happen).

[jeffhandley]

159 / 1412 files viewed

I will continue tomorrow.

[Tornhoof]

Just wondering, how did you undo the changes, it's a lot of changes and a large code base

1. Revert the changesets
2. Manual labor
3. Regex
4. Did you write a Roslyn codefix?

And if you did more than 1,2 how did you decide on that method?

[stephentoub]

how did you undo the changes, it's a lot of changes and a large code base

Revert wasn't ideal because:

• There were a lot of conflicts due to code merged since.
• A fair amount of new use of !! has been added since.
• Rolling out !! fixed a decent number of (minor) bugs, e.g. wrong parameter name used, missing parameter name, missing tests, argument exceptions propagating asynchronously instead of synchronously out of iterators and async methods, etc.
• We actually don't want what was there before but instead to use ThrowIfNull, as it's more concise and results in more efficient code, and !! is a good indicator of where it can/should be used (!! effectively generated ThrowIfNull calls).

My process:

1. Started by using VS to find expression-bodied members using !! and applying a refactoring to make them normal bodies. Most of these were in one LINQ-related file.
2. Searched for async in combination with !! to manually do the "right thing" in terms of exceptions propagating synchronously. This found some bugs where we weren't doing it correctly prior to !! . Did the same searching for IEnumerable to find iterators, also finding a few bugs !! had fixed.
3. Searched for use of !! on signatures with this/base delegation to manually handle cases where the check needed to be before delegation.
4. Wrote a Roslyn-based console app to find all !! on parameters, remove them, and add in an appropriate ThrowIfNull call. That handled 95% of the changes.
5. Searched for ones my tool missed (I knew it would miss some but it was cheaper to do the rest manually than to make the tool bullet-proof), e.g. use on indexers, and fixed those manually.
6. Built, fixed errors (primarily projects that couldn't use ThrowIfNull), rinsed and repeated until clean.
7. Manually code-reviewed ~1400 files. Most of the issues here were around trivia my tool didn't handle appropriately, e.g. pragmas at the beginning of a method that got messed up.
8. Mentally praised !! and all that it accomplished for us in its few months with us, and put up the PR.

[Tornhoof]

Thank you for the detailed answer.

[stephentoub]

I'll assume the downvotes are about C# 11 removing the !! feature rather than about the changes in this PR? We need to remove use of !! if we want dotnet/runtime to continue to compile, which is, you know, important 😄 If the downvotes are about the specifics of the changes here, I'm happy to hear specific feedback.

[SteveDunn]

I've taken a look through a couple of files. One thing that has struck me is how nice it'd be if we could shorten (yes, I recognise the irony!) the null checking code, e.g, instead of:

ArgumentNullException.ThrowIfNull(element);
ArgumentNullException.ThrowIfNull(attributeType);

we could have ...

ArgumentNullException.ThrowIfNull(element, attributeType);

I think this'd work if we added overloads, e.g.:

public static void ThrowIfNull(
    [NotNull] object? argument1, 
    [NotNull] object? argument2, 
    [CallerArgumentExpression("argument1")] string? paramName1 = null,
    [CallerArgumentExpression("argument2")] string? paramName2 = null)
{
    if (argument1 is null)
    {
        Throw(paramName1);
    }
    if (argument2 is null)
    {
        Throw(paramName2);
    }
}

We could have up to 5 parameters for normal software (and maybe a special 'Enterprise' version for Finance sector developers, with up to 30 parameters! ;) )

[SteveDunn]

Actually, forget my idea above; I implemented it, but method overload resolution means it likely will call the wrong overload. Examples in the PR.

[jeffhandley]

600 / 1416 files viewed. To be continued as soon as possible.

I found we're down to about 70 occurrences of an assignment with ?? throw new ArgumentNullException(nameof(.... If we want to eliminate that pattern, we could do so in a follow-up PR pretty quickly, but I don't know if that was the aim or not.

[stephentoub]

If we want to eliminate that pattern, we could do so in a follow-up PR pretty quickly, but I don't know if that was the aim or not.

My goal wasn't to remove them, but rather if a project already had a ThrowHelper, to use it consistently.

[jeffhandley]

Actually, forget my idea above; I implemented it, but method overload resolution means it likely will call the wrong overload. Examples in the PR.

Thanks for trying that out, @SteveDunn. The idea was intriguing.

[jeffhandley]

I left one comment that calls out a subtle change in logic that we might want to back out for caution. Several other comments around code comments. And a couple formatting fixes/suggestions.

@stephentoub - your devotion to this was admirable. This was a major undertaking in both directions! And even though !! got removed, your pair of PRs around null argument handling resulted in better consistency as well as mitigations to some lurking NullReferenceException possibilities.

Thank you for the effort that went into this and for teaching me a few things during the review.

[stephentoub]

Thanks, @GrabYourPitchforks and @jeffhandley, for reviewing.