Analyzer proposal: Discourage == null and != null syntax

[AArnott]

The == null and != null syntax in C# is vulnerable to allowing dead checks against struct types and even misleading callers when the types being tested have overridden these operators.

C# has is null, is not null and is object syntax that is immune to all these deficiencies, but most C# developers are not familiar with the preferred syntax and most code is written using the discouraged syntax.

Analyzers could call these out and offer bulk code fixes to switch to the newer syntax.

In fact I've already done it here: https://github.com/AArnott/CSharpIsNull

Is there interest in taking this into the SDK?

[dotnet-issue-labeler]

I couldn't figure out the best area label to add to this issue. If you have write-permissions please help me learn by adding exactly one area label.

[alexrp]

The == null and != null syntax in C# is vulnerable to allowing dead checks against struct types

It should be noted that this is only the case if the type overloads the equality operators. If it doesn't, you'll get CS0019 as expected.

(I was about to respond that I couldn't reproduce that problem, then it occurred to me that overloaded operators might change the result.)

It's honestly pretty bizarre that this even compiles... I think it has something to do with nullable value types.

I would definitely like a built-in analyzer that tries to catch this. It's super misleading and almost certainly a bug. I think the right way to do this, though, is to catch this specific pattern, rather than advocate for a specific style of null check.

[AArnott]

is to catch this specific pattern

Which pattern is this that you're advocating catching?

As for CS0019, yes I'm seeing that (now) to. Is that new for some particular C# language or compiler version that would explain why I thought as of a year ago or so, CS0019 wouldn't be produced?

[AArnott]

@jaredpar weren't you the one advocating for this syntax?
https://twitter.com/jaredpar/status/1229850618749050880

[alexrp]

Which pattern is this that you're advocating catching?

x == null where x is a value type containing an overloaded == operator. (And the same for !=.)

[carlossanlop]

Should we consider expanding either rule IDE0041 or rule IDE0150 with the proposed analyzer/fixer behavior? Both existing SDK rules were mentioned in the Twitter thread.

Note: only IDE0041 is documented in MS Docs. IDE0150 is not.

Also, dotnet/roslyn-analyzers#5337 seems related. Since dotnet/runtime is the correct repo for new analyzer discussions, I think we can close that issue as duplicate of this. <- Statement corrected by Stephen below.

[stap123]

+1 for this rule, would seem logical for including this given the is null rule is already available.

[ghost]

This is a great idea. Can we get this added?

[stephentoub]

Since dotnet/runtime is the correct repo for new analyzer discussions, I think we can close that issue as duplicate of this.

I don't think dotnet/runtime is the correct repo for all new analyzers.

Analyzer discussions in dotnet/runtime are for analysis related to APIs and runtime functionality, e.g. you're using XYZ API incorrectly, or you're using something in such a way that's going to run afoul of how it'll execute with the runtime.

This issue is about warning about use of particular language syntax, regardless of APIs or runtime, and pushing you to use different language syntax. This is what all of the IDEXXXX rules are about. I want the folks responsible for the language/compiler to also be responsible for making recommendations about which parts of the language should or should not be used, and how, and for it to be appropriately tied in with all of the UI in VS around style preferences.

As such I think this should be moved to dotnet/roslyn or dotnet/roslyn-analyzers, and we probably need to be crisper about where and how we review analyzer proposals. (cc: @jeffhandley, @mavasani)

(All that said, I think such an analyzer is a fine thing to consider as part of VS and/or Microsoft.CodeAnalysis.CSharp.CodeStyle.)

[carlossanlop]

Moving to dotnet/roslyn for further triage.

[CyrusNajmabadi]

C# has is null, is not null and is object syntax that is immune to all these deficiencies, but most C# developers are not familiar with the preferred syntax and most code is written using the discouraged syntax.

I don't believe this is discouraged syntax. Indeed, for some types it is the way to properly do a test. For example, ImmutableArray overrides these operators so you can do:

void Foo(ImmutableArray<string> array)
{
    if (array == null) ...

in particular is null won't work here, and is not object will give the wrong answer.

[AArnott]

array == null works with ImmutableArray<T>? Wow. I wouldn't have guessed it (even though I wrote it originally). Comparing a struct to null is usually a bug. I would have used IsDefault on it or whatever that property is called.

[CyrusNajmabadi]

even though I wrote it originally

:D

Comparing a struct to null is usually a bug.

I think teh intent here was to make ImmutableArray a near drop-in replacement for an actual array. With an actual array you can have null checks, so we wanted to maintain that to prevent churn when using this. Yes, it's a struct. But it's a struct to get a no-overhead wrapper over a reference type. So in that regard it really is intending to feel as close to a ref-type as possible.

[jmarolf]

Design Review Notes

Conclusion

We will add a new diagnostic id for this case that the user can configure. Naming TBD but something like "Prefer is null checks" would be exposes to the user.

When this rule is on for the following patterns on the left the developer will be offered to translate to the code on the right:

• a == null -> a is null
• a != null -> a is not null
• a is object -> a is not null
• a is {} -> a is not null
• T? a is T -> a is not null

iff the type of a does not overload the equality operator. The only exception to the does not overload the equality operator rule is the string type as that is sealed and we know what its implementation does.

Discussion

• Sam: is null is not semantically equivalent to Equals or == so we should not offer this for those cases
  • Cyrus: I think we should do this
• Cyrus: feels like a rule I would enable more for code clarity than code style since it makes it clear what the intent is: i.e. I intended to use an overloaded operator here instead of just doing a default null check
• Cyrus: should we have a new rule for this
  • Yes
• Sam: A similar rule to this would be: "do not use default if you could use null instead"
• Jason: we could color operators differently if they are overloaded, we should ensure this works well in projects such as unity
• Cyrus: lets also allow is null on strings as we know the behavior there
  • DavidW: what about string? is string, should that be offered as well?
  • Jason: should we also allow this for records?
    • Only if they are sealed or the compiler has some way of marking a record as using the default equality rules
• Sam: What about explicit cast to object and ReferenceEquals?
  • Jason: we already have support for that.
  • Cyrus: if you have ReferenceEquals(null) we suggest is null but not the more general case
• Sam: what about is object or is {}? Should this be offered for these cases?
  • Everyone: yes please
  • Cyrus: note that we should not offer this for is {} identifier as that is not the same thing
• Sam: What about multi-targeted project?
  • Jason: that is already an issue for all features today.

[ghost]

I think teh intent here was to make ImmutableArray a near drop-in replacement for an actual array. With an actual array you can have null checks, so we wanted to maintain that to prevent churn when using this. Yes, it's a struct. But it's a struct to get a no-overhead wrapper over a reference type. So in that regard it really is intending to feel as close to a ref-type as possible.

I would not try to accommodate all corner case or unconventional implementations. Folks can always suppress if needed. This rule will do a lot of good for folks over time.

[CyrusNajmabadi]

I would not try to accommodate all corner case

ImmutableArray is not a corner case. :)

[jmarolf]

I would say that types found in the BCL are not corner cases and whatever rules we ship must, at minimum, work with the types that ship by default with C#.

[myokeeh]

• T? a is T -> a is not null

• Sam: what about is object or is {}? Should this be offered for these cases?

  • Everyone: yes please
  • Cyrus: note that we should not offer this for is {} identifier as that is not the same thing

I just switched over to C#10 on a project and noticed a bunch of IDE0150 Null check can be clarified which points to a missing page: https://docs.microsoft.com/en-us/dotnet/fundamentals/code-analysis/style-rules/ide0150

Why is is not null preferred over is SomeT? Is it more efficient?

[CyrusNajmabadi]

Why is is not null preferred over is SomeT

No. It's preferred as it's clearer what the purpose of the check is. For example:

string s = ...
if (s is string) ...

This code is less clear. Given that s is a string, testing to see if it's a string looks superfluous. In this case, the only thing that is actually being tested is not the type, but just that the variable is non-null.

Since that's all that is being validated, it's clearer and more idiomatic to just actually simplify the check to just be: if (s is not null). Now the code actually reflects exactly what is being tested and is entirely clear about that.

[elachlan]

This rule would be super useful. Winforms has a long running issue to manually fix occurrences and this analyzer would allow resolving the issue in minutes instead of multiple PRs and manual/regex changes.

[AArnott]

@elachlan Can you use the analyzer and code fix that I wrote? (Linked in the description)?

[Youssef1313]

iff the type of a does not overload the equality operator. The only exception to the does not overload the equality operator rule is the string type as that is sealed and we know what its implementation does.

@jmarolf There are other types in the BCL, e.g, System.Xml.Linq.XName. Should we consider anything coming from corlib instead of System.String only?

[Rasmus715]

I'm rooting for this one as well!

[santo998]

Any update on this?