PowerShell / PSScriptAnalyzer

Download ScriptAnalyzer from PowerShellGallery
https://www.powershellgallery.com/packages/PSScriptAnalyzer/
MIT License
1.86k stars 377 forks source link

UseConsistentWhitespace rule CheckOperator behavior is inconsistent across PowerShell versions & not well defined #1606

Open daviesj opened 4 years ago

daviesj commented 4 years ago

UseConsistentWhitespace rule CheckOperator behavior is inconsistent across PowerShell versions & not well defined

In trying to figure out why the CheckOperator option of the UseConsistentWhitespace rule handles unary operators inconsistently (see #1239), I also discovered that the rule also behaves differently from version 5.1 to version 7.0 with some binary operators (at least the bitwise ones). Once I found that, I searched the code and the documentation to see what operators it is actually intended to handle, and found that to be inconclusive if not contradictory (see here, here, and here). Therefore I am proposing the following:

Steps to reproduce

Invoke-Formatter {7-band3}

Expected behavior

7 -band 3

Actual behavior (PS 5.1)

7-band3

Actual behavior (PS 7.0)

7 -band 3

Environment data (PS 5.1)

> $PSVersionTable
Name                           Value
----                           -----
PSVersion                      5.1.18362.1110
PSEdition                      Desktop
PSCompatibleVersions           {1.0, 2.0, 3.0, 4.0...}
BuildVersion                   10.0.18362.1110
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.19.1

Environment data (PS 7.0)

> $PSVersionTable
Name                           Value
----                           -----
PSVersion                      7.0.3
PSEdition                      Core
GitCommitId                    7.0.3
OS                             Microsoft Windows 10.0.18363
Platform                       Win32NT
PSCompatibleVersions           {1.0, 2.0, 3.0, 4.0…}
PSRemotingProtocolVersion      2.3
SerializationVersion           1.1.0.1
WSManStackVersion              3.0

> (Get-Module -ListAvailable PSScriptAnalyzer).Version | ForEach-Object { $_.ToString() }
1.19.1

After looking at the code (see here, and here), my best guess is that the original intent was probably just to handle arithmetic (just +,-,/,*,%) and assignment operators. It looks like the intent of the IsOperator function was to check for:

So, still puzzled, I debugged it and found that TokenTraits.HasTrait does not behave as expected. (I opened https://github.com/PowerShell/PowerShell/issues/13820 to try and find out if there was a reason for this puzzling behaviour or if it would be worth changing.) It turns out that the function only does bitwise comparison and the BinaryPrecedence* values of TokenFlags are not actual bit flags, so code like TokenTraits.HasTrait(token.Kind, TokenFlags.BinaryPrecedenceAdd) returns true for more values than one would expect. It also behaves differently from PS 5.1 to PS 7.0 because the numeric values of the BinaryPrecedence* values were changed when Null Coalescing operator was implemented.

SydneyhSmith commented 4 years ago

Thanks @daviesj we really appreciate you digging so deep into this issue, the linked code and examples are super helpful for us-- we are happy to review the PR--thanks for linking