PSPossibleIncorrectComparisonWithNull rule is questionable

[KirkMunro]

I disagree with the use of the PSPossibleIncorrectComparisonWithNull rule. Script authors need to understand how comparisons against $null work when it comes to collections, however there are scenarios where you may want to use a comparison operator with a collection against $null. For example, if you have some null values in a collection, you can remove them like this:

$d = @('a',$null,'b',$null,'c') -ne $null
$d.Count # returns 3

In that scenario, you don't want to $null on the left hand side of the -ne operator, because that is a totally different function.

Further, unlike other languages, it is not possible to unintentionally cause assignment when using the PowerShell comparison operators. For example, in C++, I compare values using ==. But if I mistakenly type a single '=', then I'm performing an assignment. In PowerShell, you can't do anything to -eq/-ne to make them do assignment.

The only potential risk with $null on the right hand side is when the item on the left hand side is an array. If it is not, there is no need to make the change. Why then would PSPossibleIncorrectComparisonWithNull raise what is essentially a compiler warning for values that are not arrays?

If this rule truly must stick around, then I would like it changed such that it only generates a warning when it either does not know the type of the object on the left hand side of the equality comparison operator or when the type of the object on the left hand side is a collection.

[imfrancisd]

This rule can also be limited to Boolean expressions so that this rule has a higher chance of being correct.

For example, with this line:

    $a = $someArray -ne $null

we can't know for sure if "$someArray -ne $null" is expected to be an array or a Boolean.

However, with this line:

    $a = ($someArray -ne $null) -and $b

or this line:

    if ($someArray -ne $null)

we know for sure that "$someArray -ne $null" is supposed to be a Boolean (because PowerShell will turn it into a Boolean if it isn't), so the rule has a higher chance of being correct.

[KirkMunro]

I like that idea. That sounds like a better alternative than the current implementation.

[yutingc]

Thanks @KirkMunro and @imfrancisd , we will modify the rule accordingly.

[quoctruong]

Fixed with #250. The rule is now raised only if the object on the LHS has an unspecified type or is an array.

[shawnwat]

Can someone suggest reading to help me understand better why $null needs to be on the left. I am pretty sure if ($null -ne $args) { } is the correct way I just not sure why.

[nightroman]

@shawnwat , see Comparison-operators-with-collections and the example with null looks-like-object-is-null.ps1.

[dagwieers]

Whoever thought that checking equality is not commutative clearly had no clue what he/she was doing. If one thing equals another, the other equals that one thing, but not in Powershell...

It's absurd that ($a -eq $null) is not the same as ($null -eq $a), no matter what rationale you could think of.

[bergmeister]

@dagwieers I believe there was some rationale but just to add one more case: You can even have the situation where -eq and ne yield the same result due to not returning anything:

if (@() -eq $null) { 'true' }else { 'false' }
if (@() -ne $null) { 'true' }else { 'false' }

[dagwieers]

Back on topic now, this test does not take into account type-casted variables, like:

    [string]$value = Get-AnsibleParam ...

however it does seem to work if this is being done:

    $value = (Get-AnsibleParam ...) -as [string]

In this second example the issue is not raised. I wonder why that makes a difference. I opened #943 for this.

Now, in our situation we know that Get-AnsibleParam -type "str" will only return a value of type [string] or type $null, but there does not seem a way to tell the StaticAnalyzer this. Personally I think there should be an easier operator to compare if objects are equal (e.g. by address).

Python does:

    if value is None:

Jinja2 has:

    {% if value is sameas none %}

Powershell has:

    if [Object]::ReferenceEquals($value, $null) {

It would be nice if this worked in Powershell:

    if ($value -is $null) {

[stetan]

This rule is confusing and sounds awkward when you read the code if implemented.

With the first code line below, you're stating that $Null is the most important part of the code, whereas with the second (and what I personally think is correct way to code this type of comparison), you are stating that $Value is the more important item - which it is. Also, the first line is confusing to read and interpret what is happening quickly.

If ($Null -ne $Value){...}

This code should be allowed as valid:
If ($Value -ne $Null){...}

As someone also said earlier, you can't accidentally cast $Value to BE $Null as you can with C by missing the second equals sign in the comparison statement.

Does anyone know what the rationale behind this check was in the first place?

[SeeminglyScience]

@stetan The reason for the rule is because PowerShell comparison operators enumerate collections on the left hand side. Here's an example from PowerShellTraps posted by @nightroman earlier in the thread.

$object = @(1, $null, 2, $null)

# "not safe" comparison with $null, perhaps a mistake
if ($object -eq $null) {
    # -eq gets @($null, $null) which is evaluated to $true by if
    'This is called.'
}

# safe comparison with $null
if ($null -eq $object) {
    'This is not called.'
}

[DavidBerg-MSFT]

While I appreciate (and frequently use) PowerShell's ability to automatically expand out arrays (e.g a.b where a is an array and b is not an array property, like Length, equates to building an array of a[*].b), it never occurred to me that a -eq b would be treated the same way, presumably returning the elements of a that are equal to b, further it's hard to see any valid use for this feature (maybe for -match or other partial comparisons, not for -eq or -ne). Regardless I fixed a bunch of code I had since it's now obvious it was wrong (I'm surprised I didn't see more failures), but I feel bad because I know in the past I've seen people write $null -eq foo and I've told them that was bad style, that foo should come first since it's more important (and I will continue to push that in other languages, but not PowerShell...)