Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

$null should be on the left side of equality comparison - ignore Arrays? #1227

Closed
PrzemyslawKlys opened this issue Apr 24, 2019 · 5 comments
Closed

Comments

@PrzemyslawKlys
Copy link
Contributor

Before submitting a bug report:

  • Make sure you are able to repro it on the latest released version
  • Perform a quick search for existing issues to check if this bug has already been reported

Steps to reproduce

Wrote a short blog post about it: https://evotec.xyz/the-curious-case-of-null-should-be-on-the-left-side-of-equality-comparisons-psscriptanalyzer/

But the code is as below:

$Object1 = @{
    Name   = 'Test1'
    Status = 1
}
$Object2 = @{
    Name   = 'Tests2'
    Status = $null
}

$Array = @($Object1, $Object2)

$Test1 = While ($Array.Status -ne $null) {
    $true
    foreach ($A in $Array) {
        $A.Status = $null
    }
}

$Test1

$Test2 = While ($null -ne $Array.Status) {
    $true
    foreach ($A in $Array) {
        $A.Status = $null
    }
}

$Test2

It will never end for $Test2.

Expected behavior

No tip for the Array.

Actual behavior

Tip that tells the user to fix the code.

Environment data

> $PSVersionTable
Name                           Value
----                           -----
PSVersion                      5.1.17763.316
PSEdition                      Desktop
PSCompatibleVersions           {1.0, 2.0, 3.0, 4.0...}
BuildVersion                   10.0.17763.316
CLRVersion                     4.0.30319.42000
WSManStackVersion              3.0
PSRemotingProtocolVersion      2.3
SerializationVersion           1.1.0.1
> (Get-Module -ListAvailable PSScriptAnalyzer).Version | ForEach-Object { $_.ToString() }
1.17.1
1.18.0
@jhoneill
Copy link

jhoneill commented May 1, 2019

It is needed FOR arrays ... Change your line status = 1 to status = 0 and see what happens. Is that what you expect ?

It is arguable wether the problem this rule is trying to fix applies to -NE $null but with -EQ it also applies to zero, $false or "" The -eq (or -gt , or -Match etc) opeator applied to an array gives the members for which it is true - it is easier to see with zeros than nulls : try

 @(0,1,2,3) -eq 0 
 @(0,1,2,3,0) -eq 0 

The the first gives 1 zero and the second give 2 zeros. An array containing one element which converts boolean false is treated as false, but an array with multiple items is boolean true even if every item is false. try

[boolean]( @(0,1,2,3,0) -eq 0 )
[boolean]( @(0,1,2,3) -eq 0 )
[boolean]@($null,$null)

Tests for -eq 1 or -eq true etc don't need multiple instances to give a result of true but tests for 0, false , "" or null do.

$null -ne $x is true when $x is an array where all the members are null. It is an array, not null. Which isn't the test you want in your example.
$x -Ne $null is true if $x is a single value, or is an array with either multiple "false" values or at least one "true" value - which works your example. But these two give different results.
[boolean]( @(0,$null) -ne $Null )
[boolean]( @(1,$null) -ne $Null )

So do these two

[boolean]( @($true, $false ,$true) -eq $false )
[boolean]( @($true, $false ,$false) -eq $false )

A test which checks for "something is/is-not X" but gives different results depending on what is not X or how many are X is exactly the sort of thing which should give a warning.

The same problem even applies to

[boolean]( @(0,'A','B') -match '\d' )
[boolean]( @(1,'A','B') -match '\d' )

@PrzemyslawKlys
Copy link
Contributor Author

@jhoneill Thanks for the explanation. You're probably right. I will accept if this is something that can't be addressed.

Thank you again!

@rjmholt
Copy link
Contributor

rjmholt commented May 29, 2019

@PrzemyslawKlys

I think the heart of the issue is that this code:

$Test1 = While ($Array.Status -ne $null) {
    $true
    foreach ($A in $Array) {
        $A.Status = $null
    }
}

is doing a $null comparison, when a more explicit status would be more appropriate.

Compare with:

$Test1 = While ($Array.Status -ne 'Done') {
    $true
    foreach ($element in $Array) {
        $element.Status = 'Done'
    }
}

PSScriptAnalyzer relies on certain heuristics -- assumptions about your intent and how code is being used. And the assumption in this particular rule is that when people compare with $null they intend to do a direct referential comparison with the other value they provide.

Most of the time that's correct, and this rule has probably saved plenty of people trouble. But in your case it doesn't apply -- in fact what you're doing is exactly the behaviour it's trying to warn about (which is perfectly valid behaviour and your usage makes sense).

But I think the answer here is to just switch out the $null comparison for something more explicit, so that PSSA and readers of your script (including you in a year's time) will be able to tell you intend for array mapping to happen.

@PrzemyslawKlys
Copy link
Contributor Author

I've fixed my code since I've reported it initially. That is the "code" I was using (and the reason why I've reported it - and had a blog post about it)

Original code:

function Stop-Runspace {
    [cmdletbinding()]
    param(
        [Array] $Runspaces,
        [string] $FunctionName,
        [System.Management.Automation.Runspaces.RunspacePool] $RunspacePool,
        [switch] $ExtendedOutput
    )
    [Array] $List = while ($Runspaces.Status -ne $null) {
    #[Array] $List = While (@($Runspaces | Where-Object -FilterScript {$null -ne $_.Status}).count -gt 0) {
        foreach ($Runspace in $Runspaces | Where-Object { $_.Status.IsCompleted -eq $true }) {
            $Errors = foreach ($e in $($Runspace.Pipe.Streams.Error)) {
                Write-Error -ErrorRecord $e
                $e
            }
            foreach ($w in $($Runspace.Pipe.Streams.Warning)) {
                Write-Warning -Message $w
            }
            foreach ($v in $($Runspace.Pipe.Streams.Verbose)) {
                Write-Verbose -Message $v
            }
            if ($ExtendedOutput) {
                @{
                    Output = $Runspace.Pipe.EndInvoke($Runspace.Status)
                    Errors = $Errors
                }
            } else {
                $Runspace.Pipe.EndInvoke($Runspace.Status)
            }
            $Runspace.Status = $null
        }
    }
    $RunspacePool.Close()
    $RunspacePool.Dispose()
    if ($List.Count -eq 1) {
        return , $List
    } else {
        return $List
    }
}

Fixed code (that someone suggested):

function Stop-Runspace {
    [cmdletbinding()]
    param(
        [Array] $Runspaces,
        [string] $FunctionName,
        [System.Management.Automation.Runspaces.RunspacePool] $RunspacePool,
        [switch] $ExtendedOutput
    )
      [Array] $List = While (@($Runspaces | Where-Object -FilterScript {$null -ne $_.Status}).count -gt 0) {
        foreach ($Runspace in $Runspaces | Where-Object { $_.Status.IsCompleted -eq $true }) {
            $Errors = foreach ($e in $($Runspace.Pipe.Streams.Error)) {
                Write-Error -ErrorRecord $e
                $e
            }
            foreach ($w in $($Runspace.Pipe.Streams.Warning)) {
                Write-Warning -Message $w
            }
            foreach ($v in $($Runspace.Pipe.Streams.Verbose)) {
                Write-Verbose -Message $v
            }
            if ($ExtendedOutput) {
                @{
                    Output = $Runspace.Pipe.EndInvoke($Runspace.Status)
                    Errors = $Errors
                }
            } else {
                $Runspace.Pipe.EndInvoke($Runspace.Status)
            }
            $Runspace.Status = $null
        }
    }
    $RunspacePool.Close()
    $RunspacePool.Dispose()
    if ($List.Count -eq 1) {
        return , $List
    } else {
        return $List
    }
}

I'm learning a lot from you guys. Thank you.

And to add, the warning is a warning that I was applying without "thinking". I've even encouraged @bergmeister to add "auto" fix by default for finding this in your code during formatting. And it seems that it isn't that straightforward and without manual testing may bring unexpected results.

@PrzemyslawKlys
Copy link
Contributor Author

I'm happy to close this issue if you don't need it?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants