@iSazonov Can you review?

@xtqqczze The PR contains very many files. Please re-pack to 20-30-40 commits by 20-30 files. This will very seed up the code review. Thanks!

@iSazonov Could you use the Reviewable link instead?

@xtqqczze Please resolve merge conflicts

@daxian-dbw Could you please help to resolve the merge conflict?

@iSazonov I will resolve conflicts now

@iSazonov Merge conflicts are resolved.

@xtqqczze Please rebase to pass CIs

Since there is high risk to catch merge conflicts I will merge.

This one too. I don't understand why we do this. == null is conventional and clear, there is simply no readability issue at all.
We won't prevent future code changes to use == null and it will just be inconsistent in the end.

I think we should revert this PR as soon as possible. /cc @TravisEz13 @adityapatwardhan @anmenaga @rjmholt

@iSazonov and @xtqqczze First, I want to thank both of you for continuously driving the code clean-up efforts and it's really appreciated!

As for this PR, the rest of maintainers discussed about it offline, and we all agree that we shouldn't force is null over == null. We think they clearly are appropriate in different scenarios. Changes in this PR will not only result in conflicts in other existing PRs, but also raise disputes in future contributions because people will continue to use == null and argue for it. So we decided to revert this PR.

To make it work better in future, @TravisEz13 submitted #13321 to add an action to label the size of a PR. Large size PRs would require a vote of the maintainers to accept.

Also, @iSazonov it would be great if you can join our maintainer meetings, which happens every two weeks. @adityapatwardhan will chat with you about it and decide the time that works for all of us.

We think they clearly are appropriate in different scenarios.

Different? It is clearly documented for constants.
We can see that IL code is the same for both patterns in .Net 5.0 (until Equals() is not overloaded)

After Mads Torgersen announced this on .Net Conf 2019 it has become a favorite feature for many developers. I'd expect they will be amazed if we do not modernized PowerShell code.

Changes in this PR will not only result in conflicts in other existing PRs, but also raise disputes in future contributions because people will continue to use == null and argue for it.

We already have great experience in the area. We had many such PRs. After we fixed most of CodeFactor issues and all code base is in the same style we no longer have style discussions with contributors (and no new CodeFactor issues) - (1) they follow code style, (2) they follow standard and expected style rules, (3) they see CodeFactor warnings.
The only thing that we haven’t done yet is we haven’t forced on Roslyn analyzers for base rules.

I understand that MSFT team follows the traditional conservatism that is well characteristic of closed source projects, but open source projects work differently, which has already been clearly demonstrated many times in this project - they require polishing and modernizing OSS code.
I hope MSFT team will find many benefits from this for their day-to-day work on this and other projects.

I'm still for keeping this PR personally. Yes, it's resulted in merge conflicts that I've fixed (twice now, guess I should have been slower on the uptake hehe) but the merge conflicts it tends to create are relatively small and simple to resolve, so I don't see it as being a big deal.

However, if we're concerned about merge conflicts, it's the order of merging PRs that matters, so we'd be better off focusing on improvements to the triage process than the exact nature of a given PR and how that affects it being accepted or not.

This one too. I don't understand why we do this. == null is conventional and clear

Tbh it's really not that clear that when you're doing a simple null check you need to worry about a type having a wonky op_Equality implementation. Or that obj == null ? new() : obj and obj ?? new() would result in different code if a op_Equality is defined. (SharpLab demo)

there is simply no readability issue at all.

I don't really disagree here, but keep in mind that readability is subjective. Less definitive language would probably be advisable.

We think they clearly are appropriate in different scenarios.

Yeah I agree with that. == null is still useful if you want to explicitly opt into an op_Equality implementation. Ideally with a comment explaining that.

Only after C# 8.0 the different scenarios is possible. And I am very curious why anyone could implement such an unusual op_Equality implementation that looks like a tricky bug.

Only after C# 8.0 the different scenarios is possible. And I am very curious why anyone could implement such an unusual op_Equality implementation that looks like a tricky bug.

I've absolutely seen op_Equality implementations that didn't handle null correctly. Note that I'm arguing for is null syntax. Using the existing == null syntax instead opens you up to these types of bugs without much benefit. There's almost never a reason to opt into op_Equality when you're null checking.

This is exactly the reason why this PR should be adopted as quickly as possible. (re-reverted :-) )

This is exactly the discussion that I tried to avoid getting into 😄

We already have great experience in the area. We had many such PRs. After we fixed most of CodeFactor issues and all code base is in the same style we no longer have style discussions with contributors (and no new CodeFactor issues)

I don't think it's the same for this case. A contributor shouldn't be forced to choose the pattern matching over the conventional !=/== when comparing to null. I don't think this is a style that we should forcefully enforce in this code base. Has dotnet/runtime repo changed all !=/== null to the is/is not null pattern matching style?

As for the op_Equality implementations that didn't handle null correctly, I would personally argue !=/== is better in terms of revealing those bugs :) So, you can see that this is pretty subjective, and I don't think we should enforce a rule on a case like this.

Has dotnet/runtime repo changed all !=/== null to the is/is not null pattern matching style?

~25% files. They usually force after creating a Roslyn analyzer.

@daxian-dbw I'd be surprised that PowerShell is expected to find bugs in someone else's utilization of an op_Equals overload?

Surely it's preferable to use the more robust implementation that avoids treading on the proverbial broken glass here? 😕

How do you know for sure that's a bug then? The type could be purposefully considered as a null, something like the AutomationNull :) Besides, we have been using ==/!= null for more than a decade, so I doubt the op_Equality implementation concern is practically a problem to PowerShell.

It depends on the purpose, perhaps, but in general just because the author of a type decided that their type is equivalent to null doesn't mean that for all cases in the internals of another library / PowerShell it should be considered equivalent.

Additionally, while I don't really know a great way to phrase this (but I do think it needs to be raised nonetheless), I don't think the argument of "we've always done it this way" holds literally any water, frankly. If we wait for a bug to show up even when there's a clear way to avoid it ever becoming a concern, I think that's cause for concern. Reactive solutions are often rushed; proactive solutions give us the opportunity to consider more robust solutions and avoid problems in the first place, fix coding practices and set in stone less error-prone practices.

Are there places you might want to use == null and permit odd behaviour? Potentially, sure. But everywhere in a code base? I really hope not.

Just because a tool is new and it would be some amount of work to move to it doesn't mean it's not worth using.

Additional food for thought:

• is null provides compile-time type safety against value type comparisons
• Blogpost by Jaxx - it's frankly annoying to debug such issues, and is null never needs to call potentially expensive methods to do the comparison.
• Some direct comparisons

How do you know for sure that's a bug then?

I'd say it is always bug if we see comparison with null but this works differently.
If an engine calls a virtual op_Equality and then dereference it will throw if the op_Equality pass null "by design" because the engine know nothing about the "design". is null was introduced to explicitly protect a base code from such things.

All nullability support in Roslyn follow this too.

I understand if somebody has some branches in backlog and have to rebase and resolve merge conflicts occasionally it is annoying. Also I understand MSFT team has to spend a time on minor commit reviews.
But MSFT team has already opened this door for several years, concluding that we want to use new features. This spawned style discussions that ended as soon as we fixed over 60,000 style issues.
Now the situation is similar - we can postpone this change, but all this time questions will arise until this change occurs.
.Net team also does a similar job from time to time.

What I tried to say is that your argument goes both way.

With the current use of ==/!= null being in the code base for over a decade, we can say the current behavior of PowerShell is that it respects the op_Equality implementation of a custom type. Blindly changing all ==/!= null to is/is not null is essentially a breaking change -- in order to tolerate a hypothetical custom type whose op_Equality handles null incorrectly, which by the way doesn't work correctly with any existing version of PowerShell so far, you choose to break another hypothetical custom type whose op_Equality intentionally handles null in the way that works with existing versions of PowerShell.

If a custom type doesn't handle ==/!= null correctly, then it's a bug of that type/library and it should be fixed in that library. I don't think we should instead ask a tool to tolerate the bug of that library. And if you want that custom type to really work with PowerShell properly, then it has to fix the null handling anyway because otherwise it won't work in PS 7.0 and PS 5.1.

With the current use of ==/!= null being in the code base for over a decade, we can say the current behavior of PowerShell is that it respects the op_Equality implementation of a custom type.

If I understand correctly a root of your concern is that PowerShell may call third-party op_Equality (not from .Net Runtime).
Can you please point such place in Engine? Sorry for the ask to spend your time.
I can think only about custom providers (completers is not so critical). Is the Engine provider code really sensitive to custom op_Equality in the null context?

My comment is completely based on the behavior difference between is/is not and ==/!= around the op_Equality implementation, which is the main argument you guys have. If you can point to me one place where changing to is/is not null would allow a buggy custom type that handles null incorrectly to work with PowerShell, then that's a place where you are potentially breaking a currently working custom type.

All I want to say is that I don't think the op_Equality implementation thing is practically a concern to PowerShell. Especially, if you choose to favor a buggy custom type, you will potentially break another custom type that is working fine with PowerShell today.

Let's be aware of the subtle differences between those 2 syntax, and choose what to use based on the problem you are solving, instead of just preferring one over another universally.

Sure, absolutely. But I think the vast majority of the cases where we currently use == null in the codebase we're better off with is null.

My comment is completely based on the behavior difference between is/is not and ==/!= around the op_Equality implementation, which is the main argument you guys have.

The best case scenario for that is less efficient code gen (even if inlined probably). I have a really hard time thinking of a situation where it would be genuinely useful to return true when the LHS is a concrete object. We don't even do that with AutomationNull because you still need to tell the difference between null and autonull, hell PSObject doesn't even implement op_Equality.

Is there a big risk of a bad op_Equality implementation in a type this repo directly references? Nah, not really. Risk is pretty small all things considered. That said it is still a pretty clear benefit (no matter how small) over == null. If your argument was "I understand the benefits but they're too small to outweigh the comfort level I have with == null" then I probably wouldn't have popped into this thread. I wouldn't agree but 🤷 it's low enough impact that that's not an outrageous statement.

@iSazonov @daxian-dbw Too many merge conflicts to re-revert this PR now. I think this should be revisited once dotnet/runtime has made started widespread '== null' -> 'is null' changes.

Too many merge conflicts to re-revert this PR now.

It already was reverted.

I think this should be revisited once dotnet/runtime has made started widespread '== null' -> 'is null' changes.

I'd expect they will do this after fork to .Net 6.0.

@iSazonov @daxian-dbw Too many merge conflicts to re-revert this PR now. I think this should be revisited once dotnet/runtime has made started widespread '== null' -> 'is null' changes.

These changes are now being made: dotnet/runtime#42692