@PoshChan please retry macos

@vexx32, successfully started retry of PowerShell-CI-macOS

In case someone looking at this PR wants to know, the reason for comparing against null using is null instead of == null is explained by Mads Torgersen here.

In case someone looking at this PR wants to know, the reason for comparing against null using is null instead of == null is explained by Mads Torgersen here.

Just to clarify, it's often considered a good practice in general but it doesn't actually make a difference when both sides are typed as object (SharpLab example). I like the change, just want to point that out because it can be confusing and I didn't hear that covered in the link.

This also changes IsNull() to use obj is null instead of obj == null.

As maintainer I don't like such sporadic changes. If this is a best practice we should keep our code base clean and fix all code base in one step. This should be well-documented (youtube is not document) and approved by maintainers. I suggest to open new issue for this.

I see what you mean. I can revert that part for now if we'd rather do these separately; this may be a wider-reaching change we need to account for across the entire codebase, and perhaps it's best to do that all at once.

That said... I also don't see the harm in updating the code practices here and there as we make other changes anyway to gradually reduce the instances of the less advisable practice as well.

Also, I know @KirkMunro was wanting that change to be in IsNull specifically, because that will affect a majority of user-facing PowerShell use cases, as far as I know. The rest of the codebase isn't immediately suffering for it, so in general I would consider it more akin to a style change except where public API is concerned, where it will directly (though likely subtly) impact users.

As maintainer I don't like such sporadic changes. If this is a best practice we should keep our code base clean and fix all code base in one step. This should be well-documented (youtube is not document) and approved by maintainers. I suggest to open new issue for this.

@iSazonov: This isn't sporadic. It's about updating a shared piece of code that is used by both the -is/-isnot operators and comparison operators in PowerShell, and there are two other open PRs that reference this code. Since multiple PRs are referencing this code that each have ongoing discussions, and since the update is so simple, it made sense to isolate it in its own PR so that it could go in more easily (the irony). It is a focused, and well-founded change about checking if PowerShell objects are null, that should be reviewed and merged.

Do I think that using is null instead of == null is a best practice in C# 7 or later?

Yes.

Do I think we should use is null instead of == null in new code that is written in the PowerShell code base?

Yes, because it's a best practice.

Do I think this is a best practice to follow to the point where we should update our entire code base?

No way. We haven't run into a problem with == null in our C# code yet that I am aware of, so why take on that effort and slow down everything else unnecessarily?

As for how well or not well documented you think that YouTube video is, well, Mads leads the C# design team, and what he said was pretty clear in that video. I can't find it in official docs though, because docs aren't always updated by the people who understand what should be said. The only Microsoft doc I found simply said you could use is null to check if something is null, with no mention at all of why they added this. That's not too surprising given that the doc update came from a community PR, and not the C# design team. We'll just have to deal with the imperfect world with open source community contributions and people having too much to do that we live in, I guess.

I am not against the best practices and new features that appeared just to help us. If I'll get understanding that this is great for PowerShell I'll first who ready to review all code base for this.
But as @vexx32 said:

that will affect a majority of user-facing PowerShell use cases, as far as I know. The rest of the codebase isn't immediately suffering for it, so in general I would consider it more akin to a style change

If it is important (do you insist on it?) here then why? Scripts have been malfunctioning for the past 15 (!) years and we should fix this? Such change of a public API is a breaking change in red area. It is unbelievable that such a change will be accepted.
Did you any investigation and can point scripts on GitHub where this is so critical that we must accept the breaking change?

On the other incredible side, what if there is code or scripts that are based on virtual Equals? And again we will break them.

As maintainer I'd want at first update our code style convention guideline and get MSFT docs so that we make reasonable requests to contributors at PR time review and not embarrass them and not waste time discussing this.

that will affect a majority of user-facing PowerShell use cases, as far as I know. The rest of the codebase isn't immediately suffering for it, so in general I would consider it more akin to a style change

If it is important (do you insist on it?) here then why? Scripts have been malfunctioning for the past 15 (!) years and we should fix this? Such change of a public API is a breaking change in red area. It is unbelievable that such a change will be accepted.
Did you any investigation and can point scripts on GitHub where this is so critical that we must accept the breaking change?

Just to be completely clear here, when the LHS is typed as object both styles generate the exact same IL. The only time it doesn't generate the same IL is when the LHS is typed as a class that defines the op_Equality operator. And by "typed as" I mean statically. The run-time type doesn't matter, only the design time declaration. In the vast majority of situations, it's just a style choice.

I have no strong personal opinions about whether PowerShell should or shouldn't prefer is null, just trying to clear up what it does.

The only time it doesn't generate the same IL is when the LHS is typed as a class that defines the op_Equality operator.

@SeeminglyScience Please share an example on Sharplab for my education.

@iSazonov

Here ya go. The first generates IL that specifically tests for a null reference, the second calls out to the custom equality operator. It's important to reiterate there is no virtual dispatch here. Because operators are static, it doesn't matter what the actual run-time type is, only the design time typing.

One question I've been asking myself today based on this discussion is whether or not it is worth distinguishing between the two. If by changing IsNull to actually use is null we are changing -eq $null to actually use is null as well, it makes sense to ask if that is what we actually want to do or not. As you point out, we lose the ability to decisively use the a custom equality operator if all equality comparisons to $null end up using is null internally.

There's no difference between is null and == null when the LHS is object. Check my other comments in this thread for examples/explanation.

Aye. And since we're talking about comparing against a parameter that is typed as object, this difference is purely stylistic here. There is no functional change in this instance.

Other usages within the PowerShell engine can be evaluated differently. But as with this one, anything that uses object as the compile-time type will be unaffected (this covers a majority of our public API).

@SeeminglyScience Thanks, apologies for missing that in your earlier comments.

Given that's the case, there is no reason not to merge this PR in as is, right, because it's just enhancing performance and making a very small style change. Either that or remove the style change and merge the perf change in, because it won't matter either way.

Yup, I'm good with either. 🙂

I also removed my dependency on this from #10704 because it isn't a requirement for that to be reviewed and merged in.

@SeeminglyScience Many thanks!

With @SeeminglyScience's samples my understanding is that the issue can be only with custom classes with wrong Equals implementation.
I found only 5 classes in PowerShell repo where we define Equals and equality operator. We could review the classes but it is old classes and I guess they is well-tested.

The samples also explicitly show that IL code is the same - no performance benefits.

Then I found that PowerShell repo contains 5599 of == null pattern. And, as @vexx32 mentioned, all is about object and standard Core classes. While I do not see where the use of this feature would be necessary.

All custom classes which could be passed to PowerShell and which have wrong design cannot be fixed in PowerShell engine with the feature. (We can’t even predict that there is a right design for these classes.)

Then I make quick search in CoreFX. I hoped to find useful examples and was very wonder that I find a few ones although this feature was mentioned two years ago (!). At first look they used it to fix very thin bugs (in unsafe code - that I saw). After that, I'm not surprised that this feature is undocumented.

So my current conclusion is that the PR doesn't add any value.

@vexx32 Blue labels is for issues. Yellow is for PRs (they is used to generate changelog automatically)

@iSazonov the is null portion of the PR can be removed if we don't want/need it. The primary purpose of the PR is to tidy up the execution order of IsNullLike() so that it checks for the most common values first, rather than wasting time checking DBNull.Value and NullString.Value first (which are, relatively, less common cases).

@vexx32 I suggest to open new PR for null ordering. Our discussion about is null will distract from reviewing the functional change.

No problem, will do. 🙂