dotnet / dotnet/roslynator

RCS1165 seems misleading (perhaps should not be enabled by default)

Open
#384 10 comments 3 reactions 1 assignee Claimed by @josefpihrt View on GitHub
Area-Analyzers Feature Request
Dominant language
C#
Stars
3.5k
Forks
294
Avg merge
2h 30m
Merged PRs (30d)
4

Description

Often when I write a generic function which should work on a value of any type, I will want to check whether reference type values are null and do something else, like throw an `ArgumentNullException`. Here's my most common use case:

```C#
public void DoSomething(T value)
{
if (value == null)
throw new ArgumentNullException(nameof(value));

// Do some things.
}
```

Which gives me the warning "Unconstrained type parameter checked for null".

As far as I know, this code is correct and will compile into optimal code in all cases. When used with a value type, (I believe) the compiler will remove that code, which is desirable. The suggested code fix, which changes it to `EqualityComparer.Default.Equals(value, default(T))` is not semantically the same thing, and can't be elided by the compiler. I almost never want to check whether the value is default instead of null, because structs must behave reasonably in their default state. Furthermore using null perfectly describes my intent: if the value is null then throw, default has nothing to do with it.

When googling that error text, the top result is this [SO post](https://stackoverflow.com/questions/12396457/checking-instance-of-non-class-constrained-type-parameter-for-null-in-generic-me), which has a convoluted accepted answer, and which I think leads people further astray.

A common way people get around this error is to cast to object before checking for null, but as far as I'm concerned that's a terrible idea because you're now introducing a box for value types, which is the kind of performance hit generics help avoid.

Another way might be to create 2 different methods with different names, one with a constraint for reference types and another for value types. I don't believe this is a correct solution either if the code is otherwise identical, it complicates the API for no good reason.

For these reasons I believe this warning should be disabled by default. The code fix is fine to leave, even if it changes the semantics of the code which I find a little surprising. It seems to be warning me about something which at worst is not incorrect and, in at least one very common case, is actually the most correct option.

Thoughts? Am I missing some glaring issue this warning catches?

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.