whitespaceBetweenParameters Removing Redirectors #2000

Activity

1. Stephen (s-nix)

   added

   Issue-Bug

   on Apr 30, 2024
2. Sydney Smith (SydneyhSmith)

   transferred this issue fromPowerShell/vscode-powershellon May 7, 2024
3. SydneyhSmith commented on May 7, 2024

   

   Sydney Smith (SydneyhSmith)

   on May 7, 2024

   Collaborator

   More actions

   Thanks @nixuno we were able to reproduce this
4. Sydney Smith (SydneyhSmith)

   added

   Area - Rules

   Up for Grabs

   on May 7, 2024
5. liamjpeters commented on May 13, 2024

   

   Liam Peters (liamjpeters)

   on May 13, 2024

   Contributor

   More actions

   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.

   PSScriptAnalyzer/Rules/UseConsistentWhitespace.cs

   Lines 399 to 400 in e1dc126

   	List<Ast> commandParameterAstElements = commandAst.FindAll(
   	testAst => testAst.Parent == commandAst, searchNestedScriptBlocks: false).ToList();

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

   PSScriptAnalyzer/Rules/UseConsistentWhitespace.cs

   Lines 410 to 411 in e1dc126

   	var expectedStartColumnNumberOfRightExtent = leftExtent.EndColumnNumber + 1;
   	if (rightExtent.StartColumnNumber > expectedStartColumnNumberOfRightExtent)

   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.
6. Andy Jordan (andyleejordan)

   closed this as completedin #2001on Jul 16, 2024