PowerShell / PSScriptAnalyzer

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

whitespaceBetweenParameters Removing Redirectors #2000

Closed nixuno closed 4 months ago

nixuno commented 7 months ago

Prerequisites

Summary

When formatting the following PowerShell while powershell.codeFormatting.whitespaceBetweenParameters = true, it removes two of the redirectors:

Pre-Format

Import-Module -Name Module 3>&1 2>&1 1>$null

Post-Format

Import-Module -Name Module 1>$null

I would expect the redirectors to be left alone.

PowerShell Version

Name                           Value
----                           -----
PSVersion                      7.4.2
PSEdition                      Core
GitCommitId                    7.4.2
OS                             Microsoft Windows 10.0.22631
Platform                       Win32NT
PSCompatibleVersions           {1.0, 2.0, 3.0, 4.0…}
PSRemotingProtocolVersion      2.3
SerializationVersion           1.1.0.1
WSManStackVersion              3.0

Name             : Visual Studio Code Host
Version          : 2024.2.1
InstanceId       : 71fd0027-ddfe-4add-82a3-1e0b2dbe844e
UI               : System.Management.Automation.Internal.Host.InternalHostUserInterface
CurrentCulture   : en-US
CurrentUICulture : en-US
PrivateData      : Microsoft.PowerShell.ConsoleHost+ConsoleColorProxy
DebuggerEnabled  : True
IsRunspacePushed : False
Runspace         : System.Management.Automation.Runspaces.LocalRunspace

Visual Studio Code Version

1.88.1
e170252f762678dec6ca2cc69aba1570769a5d39
x64

Extension Version

ms-vscode.powershell@2024.2.1

Steps to Reproduce

  1. Save the following to test.ps1:
Import-Module -Name Module 3>&1 2>&1 1>$null
  1. Press Shift + Alt + F to format the document.
  2. Review improper formatting

Visuals

No response

Logs

No response

SydneyhSmith commented 6 months ago

Thanks @nixuno we were able to reproduce this

liamjpeters commented 6 months ago

This is quite an interesting one 😀. A slightly simplified repro in PSScriptAnalyzer is:

Invoke-Formatter -ScriptDefinition 'Invoke-Foo 3>&1 1>&1 2>&1' -Settings @{
    Rules = @{
        PSUseConsistentWhitespace = @{
            Enable         = $true
            CheckParameter = $true
        }
    }
}

Results in:

Invoke-Foo 1>&1

The PSUseConsistentWhitespace rule, when CheckParameter is true, is looking over all the CommandAst nodes.

For each CommandAst, it's finding all the direct children of the current CommandAst node.

https://github.com/PowerShell/PSScriptAnalyzer/blob/e1dc126c361398d6ead6556c24c544702b1918cb/Rules/UseConsistentWhitespace.cs#L399-L400

It's checking then that each one sequentially is separated by a 1 character gap.

https://github.com/PowerShell/PSScriptAnalyzer/blob/e1dc126c361398d6ead6556c24c544702b1918cb/Rules/UseConsistentWhitespace.cs#L410-L411

It seems to be making the implicit assumption that the direct children of the CommandAst are read from the AST in token order. This is, apparently, not the case.

Looking at the AST of our example Invoke-Foo 3>&1 1>&1 2>&1:

ScriptBlockAst [0,25)
└ NamedBlockAst [0,25)
  └ PipelineAst [0,25)
    └ CommandAst [0,25)
      ├ StringConstantExpression [0,10)
      ├ MergingRedirectionAst [16,20)
      ├ MergingRedirectionAst [21,25)
      └ MergingRedirectionAst [11,15)

We see that evidentially, the MergingRedirectionAst nodes are read from the tree in stream order (very interesting!).

Note the start of each MergingRedirectionAst nodes extent - the number after [ - is not in order down the list. The first node read from the tree is the second token, 1>&1, followed by the last token, 2>&1, and finally the first of the redirects in our input command, 3>&1.

I'm not smart enough to confirm this within the main Powershell Repo's parser - but I've tried with various orders of Invoke-Foo 3>&1 2>&1 1>&1 5>&1 4>&1 6>&1 and it appears to be consistent.

My unrefined naive solution would be to sort the command parameters (the direct children of the CommandAst), first by their Extents StartLineNumber, then by the StartColumnNumber. Then proceed as planned.

This works as expected and our repro code correctly produces:

Invoke-Foo 3>&1 1>&1 2>&1

I would need to look at how badly this sorting impacts performance and for any edge cases.