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

The GitHub UI is misbehaving trying to leave feedback within the PR. But inClaimsIdentity.cs andClaimsPrincipal.cs: the 6.0 behavior is that for iterators, theArgumentNullException 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.

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

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?

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.

Thank you for the detailed answer.

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.

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

publicstaticvoidThrowIfNull([NotNull]object?argument1,[NotNull]object?argument2,[CallerArgumentExpression("argument1")]string?paramName1=null,[CallerArgumentExpression("argument2")]string?paramName2=null){if(argument1isnull){Throw(paramName1);}if(argument2isnull){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! ;) )

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

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.

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

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

Thanks,@GrabYourPitchforks and@jeffhandley, for reviewing.