Add DoNotPassStructToArgumentNullExceptionThrowIfNullAnalyzer

[CollinAlpert]

Fixes dotnet/runtime#85154

[codecov]

Codecov Report

Attention: 16 lines in your changes are missing coverage. Please review.

Comparison is base (ab13ac7) 96.45% compared to head (b3eebbb) 96.46%.

Additional details and impacted files

@@           Coverage Diff            @@
##             main    #6815    +/-   ##
========================================
  Coverage   96.45%   96.46%            
========================================
  Files        1422     1427     +5     
  Lines      340585   341582   +997     
  Branches    11230    11246    +16     
========================================
+ Hits       328511   329503   +992     
+ Misses       9236     9234     -2     
- Partials     2838     2845     +7     

[stephentoub]

Thanks, @CollinAlpert.

I suggest we broaden this slightly. The reason structs don't make sense here is because they'll be boxed to objects and then will never be null, making the whole operation an expensive nop. That same reasoning applies to other misuse, as well, for example accidentally passing a nameof(...) to ThrowIfNull (dotnet/runtime#89751 (comment)), or passing new Class(). Basically, make this analyzer about catching non-sensical uses of ThrowIfNull where the argument is provably never null by the compiler. (It should not rely on nullable annotations.) So, boxing structs, nameof, new Class(...), and anything else we can think of that's provably never null.

[Youssef1313]

Suggested change

	.FirstOrDefault(m => m is IMethodSymbol method && method.Parameters[0].Type.SpecialType == SpecialType.System_Object);
	.FirstOrDefault(m => m is IMethodSymbol method && method.Parameters.Length > 0 && method.Parameters[0].Type.SpecialType == SpecialType.System_Object);

[sharwell]

You could go further if you want:

m is IMethodSymbol { Parameters: [{ Type.SpecialType: SpecialType.System_Object }, ..] }

[CollinAlpert]

@stephentoub @Youssef1313 Thank you for your feedback, I have addressed it and added appropriate tests.

[mpidash]

Seems like CA1869 (and CA1870) got used so this analyzer has to move to CA1871.

[sharwell]

❓ Do we really need this? SharpLab suggests that release builds will not box a non-null value in this case.

[CollinAlpert]

Can you share an example? I can see the box instruction here.

[buyaa-n]

Sorry for delayed review @CollinAlpert, left some comments/questions, mostly NITs. Overall the PR looks great, thank you!

Testing with runtime got 1 valid hit, the analyzer/fixer works well in functional testing.

[buyaa-n]

LGTM, thank you @CollinAlpert!