Skip to content

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

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

Open
daviesj opened this issue Oct 26, 2020 · 1 comment

Comments

@daviesj
Copy link
Contributor

daviesj commented Oct 26, 2020

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:

  • a decision should be made on exactly which operators CheckOperators is going to apply to
  • the documentation, code, and tests should be updated to reflect this, and to be consistent across PS versions that the module supports (hopefully at least 5.1 and current Core version)

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:

  • Any assignment operator
  • Any operator with equal precedence to +
  • Any operator with equal precedence to *
  • Also && and ||. (I do not understand why these were specially included and other operators were not.)

So, still puzzled, I debugged it and found that TokenTraits.HasTrait does not behave as expected. (I opened PowerShell/PowerShell#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
Copy link
Collaborator

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

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

No branches or pull requests

2 participants