Parameter null-checking analyzer and fixer

[RikkiGibson]

Related to #36024

[RikkiGibson]

The fix all in solution results for Roslyn Compilers.sln is an interesting read. 😊

[CyrusNajmabadi]

The way to fix this issue is to preprocess the full list of diagnostics you are fixing. So you'll group them by parameter and then fix the parameter once and the N associated statements at the same time.

[RikkiGibson]

It seems like the current approach of gathering and checking parameter locations in a set is adequate and doesn't require a separate initial pass over the diagnostics.

[Youssef1313]

Worth adding a test like this?

using System;

class C<T> where T : struct
{
   public void M(T? s!!) { }
}

[RikkiGibson]

See TestOnStructConstrainedTypeParameter

[RikkiGibson]

@CyrusNajmabadi I believe I have addressed all your comments. Please let me know if you have any others.

[CyrusNajmabadi]

Looking now :)

[CyrusNajmabadi]

Suggested change

	var argumentNullException = compilation.GetTypeByMetadataName(ArgumentNullExceptionName);
	var argumentNullException = compilation.GetBestTypeByMetadataName(ArgumentNullExceptionName);

[CyrusNajmabadi]

extract into helper plz (can be local function or method). mixing the && || and ! is too much for my brain :)

[CyrusNajmabadi]

i would extract the entire if-handling code to it's own helper.

[RikkiGibson]

I think I won't make this change due to lack of uniformity with the ExpressionStatement case. See #58182 (comment)

[CyrusNajmabadi]

similarly, can you extract this case out to a helper?

[RikkiGibson]

I found that since much of the complexity was in the pattern itself, the extracted logic didn't feel sufficiently complex to justify a single-use helper.

[CyrusNajmabadi]

consider reordering. it's a personal nit, but it seems more sensible to first see if you're doing new ArgumntNullException then check the argument.

[CyrusNajmabadi]

are there tests for records?

[RikkiGibson]

Are you referring to record primary constructors? Currently I don't think there's a way to get this particular code fix to activate on primary constructors. The "add null check" refactoring should work for it, but it doesn't. Opened #58779

[CyrusNajmabadi]

possibly comment that this is just because we don't support this scenario yet.

[CyrusNajmabadi]

add test for:

public C(
      string x // comment
    , string y // comment,
    , string z // comment
    )
{
}

[RikkiGibson]

Good test case! We actually put the !! on the next line here. It feels like the trailing trivia of the identifier should move over to the !!. Is there an existing helper that lets me "insert a token and move over another token's trivia" in this way?

edit: added some additional tests and just did it the way that seemed reasonable to me.

[CyrusNajmabadi]

nope. you'd have to write the logic yourself. note: i'm also ok with you leaving that out of this PR and waiting until tehre is user feedback :)

[CyrusNajmabadi]

Suggested change

	public const string UseParameterNullCheckingId = "IDE0190";
	public const string UseParameterNullCheckingId = "IDE0190";