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

PSSA should have a new rule to check for properly used Process blocks #1571

Closed
jbolton-corvalgroup opened this issue Aug 16, 2020 · 3 comments

Comments

@jbolton-corvalgroup
Copy link

For some cases the formatter is changing code where it cannot run. Rules that trigger this are:

  • PSUseConsistentWhitespace.CheckParameter when $True
  • PSAvoidUsingCmdletAliases when $True

The source script triggering this bug is not valid, however

  • the PSAvoidUsingCmdletAliases: pwsh will execute this with no errors (It runs normally, it never assigns $x)
  • the CheckParameter: This code is broken, pwsh will not execute this

Possible Cause of PSAvoidUsingCmdletAliases

From the docs: If your function defines a Begin, Process or End block, all of your code must reside inside those blocks. No code will be recognized outside the blocks if any of the blocks are defined.

I think the cause is the script reaches a statement that is not inside a begin/process/end block -- so it assumes the function is a non-pipeline function. In that context, process is not a keyword but an identifier. It replaces process with Get-Process (which would the correct behavior if it was a normal function)

Invoke-ScriptAnalyzer has the same behavior.

image

Potential fix for PSAvoidUsingCmdletAliases ?

  • PSSA should have a rule to check for functions that have code outside begin/process/end blocks.
  • Can the parser test if there are any begin/process/end statements, then treat it as a pipeline function?
  • Can the formatter abort if errors occur? (Right now there's no errors on mutation)

Steps to reproduce

$script = @'
function Do-Stuff {
$x = 2
Process { }
}
'@
$scriptFixed = @'
function Do-Stuff {
#$x = 2
Process { }
}
'@

$settings = @{
    IncludeRules = @('PSUseCorrectCasing', 'PSUseConsistentIndentation')
    Rules        = @{
        PSAvoidUsingCmdletAliases = @{
            Enable = $false
        }
        PSUseConsistentIndentation = @{
            Enable          = $true
            Kind            = 'space'
            PipelineIndentation = 'IncreaseIndentationAfterEveryPipeline'
            IndentationSize = 4
        }

    }
}

Invoke-Formatter -ScriptDefinition $script -Settings $settings
Invoke-Formatter -ScriptDefinition $scriptFixed -Settings $settings

Expected behavior

Indent and replace aliases.

function Do-Stuff {
    $x = 2
    Process { }
}

function Do-Stuff {
    #$x = 2
    Process { }
}

Actual behavior

Indent, and replaces keyword as if it was not in the keyword context.

function Do-Stuff {
    $x = 2
    Get-Process { }
}

function Do-Stuff {
    #$x = 2
    Process { }
}

Another example not using pipeline functions

Steps to reproduce

$Original = @'
Function BadCode {
    "stuff" | Format-CodeColor" 'ps1'

    $InputList | ForEach-Object {
    } | Select-Object -First 2
    | Join-String -sep ", " -OutputPrefix 'Results: '
}
'@

$settings = @{
    IncludeRules = @(
        "PSUseConsistentWhitespace"
    )
    Rules        = @{
        PSUseConsistentWhitespace = @{
            Enable         = $True
            CheckParameter = $false
        }
    }
}

$out1 = Invoke-Formatter -ScriptDefinition $Original -Settings $settings

$settings.rules.PSUseConsistentWhitespace.CheckParameter = $True
$out2 = Invoke-Formatter -ScriptDefinition $Original -Settings $settings

$out1
'+' * 30
$out2

if ($out1 -ne $out2) {
    Write-Error 'formatting does not match'
}

Expected behavior 2

Give an error, or don't mutate code

Actual behavior 2

Function BadCode {
    "`nPS> Top | Bot | Se" -OutputPrefix 'Results: '
}

Environment data

>> $PSVersionTable | ft

Name                           Value
----                           -----
PSVersion                      7.0.3
PSEdition                      Core
GitCommitId                    7.0.3
OS                             Microsoft Windows 10.0.19041
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

>> $psEditor | select EditorServicesVersion

EditorServicesVersion
---------------------
2.3.0.0
@ghost ghost added the Needs: Triage 🔍 label Aug 16, 2020
@ninmonkey
Copy link

Meta: Should I have broken this into two issues? (I submitted on the wrong account)

@rjmholt
Copy link
Contributor

rjmholt commented Aug 18, 2020

Indent, and replaces keyword as if it was not in the keyword context.

This is behaving as expected.

PSAvoidUsingCmdletAliases also warns about and replaces implicit Get-prefix aliasing.

In the example you give, process is not parsed as a keyword. If you invoke your function, it will run the Get-Process command. PSScriptAnalyzer is doing its best to tell you this.

Importantly, the PowerShell parser (the same one that parses your script on execution) is what makes this distinction before PSScriptAnalyzer does any processing. By the time the script gets to PSScriptAnalyzer, there's a large structural distinction between a process block and a process keyword:

PS> {
>> process { }
>> }.Ast

Attributes         : {}
UsingStatements    : {}
ParamBlock         :
BeginBlock         :
ProcessBlock       : process { }
EndBlock           :
DynamicParamBlock  :
ScriptRequirements :
Extent             : {
                     process { }
                     }
Parent             : {
                     process { }
                     }

PS> {
>> write-host 'Hi'
>> process { }
>> }.Ast

Attributes         : {}
UsingStatements    : {}
ParamBlock         :
BeginBlock         :
ProcessBlock       :
EndBlock           : write-host 'Hi'
                     process { }
DynamicParamBlock  :
ScriptRequirements :
Extent             : {
                     write-host 'Hi'
                     process { }
                     }
Parent             : {
                     write-host 'Hi'
                     process { }
                     }

In the first case, a ScriptBlockAst object is produced with a ProcessBlock parameter populated. In the second case, ProcessBlock is null and EndBlock is now populated. So the structure of the AST has removed any ambiguity by the time PSScriptAnalyzer sees your script.

You can see this in the absence of PSScriptAnalyzer by trying to define a function like this directly in PowerShell. When process is used after other statements, it's parsed as a command name within the implicit end block (in a function), so you see a runtime error message here from Get-Process:

PS> function Test
>> {
>>     Write-Host "Hi"
>>     process { "Hi" }
>> }
PS> test
Hi
Get-Process:
Line |
   4 |      process { "Hi" }
     |              ~~~~~~~~
     | Cannot evaluate parameter 'Name' because its argument is specified as a script block and there is no input. A script block cannot be evaluated without input.

When process comes first, it defines a process block, and the parser will error if naked statements also occur outside of blocks:

PS> function Test
>> {
>>     process { "Hi" }
>>     write-host "Hi"
>> }
ParserError:
Line |
   4 |      write-host "Hi"
     |      ~~~~~~~~~~
     | unexpected token 'write-host', expected 'begin', 'process', 'end', or 'dynamicparam'.

So the formatting here has not mutated your code to change its meaning; it still does the same thing as the original script you wrote.

A remaining question is whether it's worth investing in a rule to warn about using commands that shadow keywords like this.

@SydneyhSmith SydneyhSmith changed the title PSAvoidUsingCmdletAliases and CheckParameter damage code when code is outside process blocks PSSA should have a new rule to check for properly used Process blocks Aug 18, 2020
@ninmonkey
Copy link

@rjmholt This was resolved in #1638 . Do I need to do anything to close my ticket?

@SydneyhSmith SydneyhSmith moved this from Todo to Wishlist in American Pharoah Jul 13, 2022
Repository owner moved this from Wishlist to Done in American Pharoah Sep 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Status: Done
Development

No branches or pull requests

4 participants