Use is null/is not null for null checking

[marek-safar]

This avoids calling user operators where they exist and enables more
C# compiler comparison checks

Analyzers will be enabled in follow up PR with more tricky updates to follow so I don't need to keep resolving conflicts in 2964 changed files

@stephentoub

[stephentoub]

Thanks, @marek-safar. I spot checked and it looks fine. How did you make these changes? Via an analyzer/code fixer, or via some search/replace?

[stephentoub]

Also, do you know what accounts for the two-line difference in:

?

[marek-safar]

Via an analyzer/code fixer, or via some search/replace?

Simple grep. There are more cases which will need changing because the use uncommon pattern.

Also, do you know what accounts for the two-line difference in:

There are few cases which compiler issues now warnings for and could be removed. For example https://github.com/dotnet/runtime/pull/43360/files#diff-ff940bf6ef3e4720479720d29abed80108b5a7ce256b073da88ee1581102b811L199

[marek-safar]

@stephentoub any though on why could this cause error in one of facades generator?

Microsoft.DotNet.GenPartialFacadeSource.targets(30,5): error : Unable to cast object of type 'Microsoft.CodeAnalysis.CSharp.Syntax.CompilationUnitSyntax' to type 'Microsoft.CodeAnalysis.CSharp.Syntax.BaseTypeDeclarationSyntax'. [/__w/1/s/src/libraries/System.Collections/src/System.Collections.csproj]

[GrabYourPitchforks]

I haven't reviewed the code yet (3700 changed files), but are we sure that none of these call sites intended to call user-defined overloads if they exist, and were we careful to exclude things like tools and unit tests from the search-and-replace? Those tools or unit tests could be intentionally exercising user defined operators.

[marek-safar]

are we sure that none of these call sites intended to call user-defined overloads if they exist,

What would be the scenario? All I can think of is to trigger some side-effect but that sounds like something we'd like to avoid anyway.

[GrabYourPitchforks]

What would be the scenario?

Unit-testing user-defined operators, primarily. We risk lowering unit test coverage if we're changing unit test code.

[stephentoub]

I didn't notice we were changing test code. I think we should start with just src. We don't run analyzers on tests, either.

[marek-safar]

I think we should start with just src.

OK, I'll revert test code changes

[GrabYourPitchforks]

I started looking at little more deeply at the IL changes introduced by this PR, and it turns out it uncovered a latent Roslyn bug. (Hooray!)

We should revert any code that changed this construct:

void* somePtr = ...;
if (somePtr == null) { /* ... */ }

To:

void* somePtr = ...;
if (somePtr is null) { /* ... */ }

The latter case produces invalid IL.

[stephentoub]

The latter case produces invalid IL.

Ouch. I assume you're opening an issue?
cc: @jaredpar

[jaredpar]

Yeah Levi pinged us offline, we're looking into it.

[GrabYourPitchforks]

Roslyn issue: dotnet/roslyn#48563

[marek-safar]

We should revert any code that changed this construct:

@GrabYourPitchforks any tip how to find them?

[GrabYourPitchforks]

Maybe an analyzer could find them? I don't think it's possible via a pure regex.

[GrabYourPitchforks]

This really needs a comment, an issue tracking removal of this line, or something similar.

[marek-safar]

Comment added

[GrabYourPitchforks]

Latest PR still includes non-shipping code or projects which aren't guaranteed to be built using the latest toolset. See runtime/src/coreclr/src/System.Private.CoreLib/Tools/GenUnicodeProp/ for one example.

[GrabYourPitchforks]

Thinking on this a bit more, I wonder if the best course of action would instead be to start with an analyzer / fixer, then run that fixer over our code base. I get the intent of this PR, but it seems the majority of call sites don't actually result in more efficient codegen. It's also on occasion resulting in more verbose code, as "is not null" is longer than "!= null ".

If we had a fixer that targeted types that has user defined equality operators, it could potentially result in a much smaller PR. It also avoids the problem where we make a bunch of changes now, only for our devs to go and reintroduce "== null" syntax in a future PR.

[stephentoub]

@GrabYourPitchforks, I agree with you: dotnet/roslyn#38506.

[marek-safar]

If we had a fixer that targeted types that has user defined equality operators, it could potentially result in a much smaller PR. It also avoids the problem where we make a bunch of changes now, only for our devs to go and reintroduce "== null" syntax in a future PR.

Is this what we'd like to do? Have a mix of all possible combinations for null checking?

I'm trying to figure out what to do with this PR and if it's worth salvaging anything from it

[jaredpar]

If we had a fixer that targeted types that has user defined equality operators, it could potentially result in a much smaller PR

Why would you want the new syntax for only types that have custom equality operators? That seems to put a lot of undue burden on the end developer. Lots of tedious searching to do a null check.

The idea of using is object, is null, and the variants has the exact opposite intent. It allows developers to remove the burden of understanding equality operators entirely. The developer wants to unambiguously check for null or non-null and does not want to do a tedious search for such operators.

I understand the inclination to stick with existing checks but only doing these checks on types that have equality operators misses the intent of embracing this style of checking (IMHO at least).

[stephentoub]

My ideal is two diagnostic IDs:

• one that fires when there's no possible semantic change, either because the type has no operator== or because of special-casing for e.g. string. We could run such an analyzer and be confident with little to no review of the auto fix results.
• one that fires on the rest. The auto fixes here would need auditing.

[GrabYourPitchforks]

@jaredpar I was under the impression that the C# team was still promoting == null and != null as the preferred equality checking mechanism and that is null and is not null were more of a secondary thing. Hence my desire to keep the PR minimal. If my understanding was incorrect and you see the language evolving differently (e.g., preferring is [not] null), then we should keep this PR as currently scoped.

@stephentoub's recommendation seems viable. If we had those fixers in place, it would solve both problems that I mentioned with this PR: it would prevent stray == null and != null syntax from being introduced in the future, and it would ensure that anything that might change runtime behavior is audited by a human.

[jaredpar]

@GrabYourPitchforks

I was under the impression that the C# team was still promoting == null and != null as the preferred equality checking mechanism and that is null and is not null were more of a secondary thing

Think I'm not quite getting my point across. The C# team doesn't have any type of official position here (at least not that I'm aware of). Hence this isn't about conforming to some guidelines. Different teams make different choices about this. I personally love is null & is object but that's just me 😄

My comments are mostly in response to this

If we had a fixer that targeted types that has user defined equality operators, it could potentially result in a much smaller PR

My read here is that you preferred the use of is null and variants applied only to types that have equality operators. If that interpretation is wrong I apologize.

I feel like that approach though misses the point of embracing is null. That style is all about developers writing code that unambiguously operates as they intend around checking for null or the absence of it. The intent is to make it as simple as straight forward as possible. There is no need to go searching around for equality operators because is null has only one interpretation. If the only use is reserved for cases where equality operators exist then it really defeats the purpose of the change.

@stephentoub's recommendation seems viable.

Indeed. If the runtime team wants to embrace is null as a style then that is a very reasonable way to approach it.

[ViktorHofer]

// Auto-generated message

69e114c which was merged 12/7 removed the intermediate src/coreclr/src/ folder. This PR needs to be updated as it touches files in that directory which causes conflicts.

To update your commits you can use this bash script: https://gist.github.com/ViktorHofer/6d24f62abdcddb518b4966ead5ef3783. Feel free to use the comment section of the gist to improve the script for others.

[marek-safar]

Sadly there seems to be disagreement on the prefered null check on libraries team.