Create DoNotCompareSpanToNullAnalyzer

[CollinAlpert]

This PR adds an analyzer which warns when a Span<T> or ReadOnlySpan<T> is compared to null. Different than described in the issue it does not detect is null/is not null patterns, since that does not compile. For the same reason it does not analyze Memory<T> and ReadOnlyMemory<T>.

Fixes dotnet/runtime#84265.

[codecov]

Codecov Report

Attention: Patch coverage is 96.64179% with 9 lines in your changes are missing coverage. Please review.

Project coverage is 96.46%. Comparing base (59b2d9d) to head (1f07efe).

Additional details and impacted files

@@           Coverage Diff            @@
##             main    #6838    +/-   ##
========================================
  Coverage   96.46%   96.46%            
========================================
  Files        1427     1432     +5     
  Lines      341928   342196   +268     
  Branches    11268    11280    +12     
========================================
+ Hits       329848   330113   +265     
  Misses       9231     9231            
- Partials     2849     2852     +3     

[CollinAlpert]

@buyaa-n @carlossanlop Can I get a review on this?

[buyaa-n]

Thank you @CollinAlpert, overall the PR looks good.

Testing with runtime found 20 hits, all looks valid. Seems the fixer is not working for Debug.Assert(conditional) statements, please check.

[gewarren]

I left a couple suggestions for the message/description.

[buyaa-n]

I left a couple suggestions for the message/description.

Thank you @gewarren looks much better!

[buyaa-n]

@CollinAlpert thank you for addressing feedback, looks good, but there were different suggestion on the PR that fixing findings in runtime that changes the requirement:

Comparing Span with default is equally problematic as comparing with null. The analyzer should flag comparing with default as well.

Please apply this suggestion by updating the suggestion in the message and removing the code-fix that offering to use default. We could suggest remove the condition instead, but that might be cause other issues, so maybe just suggest only the IsEmpty. And please update to flag comparing with default too. Tag @jkotas and @stephentoub in case they have other/better suggestions.

[buyaa-n]

Thank you for addressing the requested changes @CollinAlpert, comparing with 'default' flagged a few more cases in runtime. Using same message for both case looks confusing even comparing null and default literally same. Anyway, lets hold this analyzer PR for a little more. Based on the expert's feedback on the runtime PR, we might want to do a few more changes in this PR.

[CollinAlpert]

Sure, sounds reasonable!

[MartyIX]

Testing with runtime found 20 hits, all looks valid.

How did you actually do it please? I know one can start Visual Studio and press ALT+F11 to check a complete solution. However, I would really like to know if one can run a single analyzer on a whole repo in an easy way.

[buyaa-n]

How did you actually do it please? I know one can start Visual Studio and press ALT+F11 to check a complete solution. However, I would really like to know if one can run a single analyzer on a whole repo in an easy way.

Here is the doc Testing against the Runtime and Roslyn Analyzers repo,

[buyaa-n]

Sure, sounds reasonable!

Thank you @CollinAlpert, we want to only offer IsEmpty for both case, but seems better to have different messaging, I think title and description can be same. From findings in runtime repo comparison with null is mostly redundant and comparison with default better to be changed to IsEmpty, so messages could be something like this:

• Comparing a span to 'null' might be redundant, the 'null' literal will be implicitly converted to a 'Span<T>.Empty'
• Comparing a span to 'default' might not do what you intended, make the code more explicit by checking 'IsEmpty'

Title can be: Do not compare Span<T> to 'null' or 'default'
Description can be: Comparing a span to 'null' or 'default' might not do what you intended, the 'null' literal will be implicitly converted to a 'Span<T>.Empty', same for 'default'. Remove redundant comparison or make the code more explicit by using 'IsEmpty'.

CC @gewarren for message improvement ideas.

[gewarren]

I'd tweak the description a bit to:

Comparing a span to 'null' or 'default' might not do what you intended. 'default' and the 'null' literal are implicitly converted to 'Span<T>.Empty'. Remove the redundant comparison or make the code more explicit by using 'IsEmpty'.

[buyaa-n]

LGTM, thank you @CollinAlpert!