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

PSUseConsistentWhitespace: Handle redirect operators which are not in stream order #2001

Conversation

liamjpeters
Copy link
Contributor

PR Summary

When CheckParameter is $true for the rule PSUseConsistentWhitespace, it checks whitespace between parameters by inspecting the AST. It gets all direct children of each CommandAst and checks, in order, that each's extent is separated by exactly 1 character.

This relies on the implicit assumption that the AST returns the children in something resembling token order.

In the case of redirect operators, this is not always the case (they anecdotally appear to be returned in the order of the stream they're redirecting). See the issue here for more detailed breakdown.

This PR adds sorting of the children before subsequently performing the same checks on the tokens extents.

Running the below across PS5.1 and PS7.4 with and without the PR change applied shows the performance impact of the change to be minimal.

Measure-Command {
    foreach ($i in 1..10000) {
        Invoke-Formatter -ScriptDefinition "Invoke-Foo $i 3>&1 2>&1 1>`$null" -Settings @{
            Rules = @{
                PSUseConsistentWhitespace = @{
                    Enable         = $true
                    CheckParameter = $true
                }
            }
        }
    }
}
PS5.1 Pre-PR PS5.1 Post-PR PS7.4.2 Pre-PR PS7.4.2 Post-PR
Invoke-Formatter 231.3535187 156.0559906 16.8896873 13.4352551
Invoke-ScriptAnalyzer 253.1291937 250.9393157 21.8151956 19.8125177

The 'speedup' is presumably that we're no longer emitting a diagnostic record

Fixes #2000

PR Checklist

@bergmeister
Copy link
Collaborator

Awesome @liamjpeters . I will try to take some time soon to review this and all your other good work that I've glanced over. Thanks for your patience in the meantime, definitely excited to get your changes in.

Copy link
Member

@andyleejordan andyleejordan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks great, thanks!

@andyleejordan andyleejordan merged commit d1a1bcb into PowerShell:master Jul 16, 2024
1 check passed
@liamjpeters liamjpeters deleted the #2000WhitespaceBetweenParamsRemovingRedirectors branch August 7, 2024 08:02
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

Successfully merging this pull request may close these issues.

whitespaceBetweenParameters Removing Redirectors
4 participants