Better way to implement rules in PS script

PSScriptAnalyzer today supports external implementation of rules in PowerShell script.

Alongside the proposal to allow custom binary rules (https://github.com/PowerShell/PSScriptAnalyzer/issues/1543), PSScriptAnalyzer should also look at how rules written in PowerShell are implemented to ensure that PS script rules:

• are easy to implement
• are consistent with each other and with binary rules, both in terms of having a uniform way to implement them, and a uniform way to reference them
• can be implemented without excessive boilerplate, and leverage PSScriptAnalyzer as much as possible

To address these goals, some proposals are:

Standard assumed arguments

PS script functions today have a very loose parameter contract that expects parameters that either end with the Ast suffix or the Token suffix, this means:

• We must spend time trawling through parameter names to find matching ones
• Functions must choose between using the AST and using tokens
• We can't call all functions the same way, meaning we must spend more time building the correct command call for each rule

Instead script rules should be required to accept a clear and concrete set of parameters, similar to the way TabExpansion2 does:

param(
    [ValidateNotNullOrEmpty()]
    [System.Management.Automation.Language.Ast]
    $Ast,

    [ValidateNotNullOrEmpty()]
    [System.Management.Automation.Language.Token[]]
    $Tokens,

    [string]
    $ScriptFilePath
)

Additional points:

• The API should only care about the parameters' names. Parameter attributes, [CmdletBinding()], positional and pipeline arguments don't matter to PSSA, as it will simply invoke rules with new PSCommand().AddCommand(<script rule>).AddParameter("Ast", ast).AddParameter("Tokens", tokens).AddParameter("ScriptFilePath", scriptFilePath)
• The $Tokens parameter should be immutable and ideally is a System.Collections.Generic.IReadOnlyList<Token>. But this may not work well with PowerShell, so the Token[] type may be preferable, with an understanding that mutating the token array is against the contract

Attribute based rule definition

Today, PS script rules are:

• defined by being in a module imported as a rule module
• named by function name
• namespaced by module name

To be consistent with https://github.com/PowerShell/PSScriptAnalyzer/issues/1543 and addressing https://github.com/PowerShell/PSScriptAnalyzer/issues/1545, rules instead should be declared with attributes:

[Microsoft.PowerShell.ScriptAnalyzer.Rule("AvoidAliases", "Avoid the use of aliases in PowerShell scripts")]

We could also provide a type accelerator for this attribute to reduce it to [Rule(...)].

This attribute would provide the possibility of namespace override: [Rule(..., Namespace = "MyRules")].

Write-Diagnostic instead of needing to construct diagnostic objects

Today script rules must construct and emit diagnostic objects themselves. This means:

• Script rules must use cumbersome constructor/New-Object calls
• Boilerplate is required to provide all arguments to a diagnostic constructor
• No special logic can be used to do things like:
  • Ensure the diagnostic is linked back to the rule that emitted it
  • Automatically infer properties about the diagnostic from its context
  • Suppress the diagnostic before it is created
  • Possibly perform context-dependent functionality

Instead, a PowerShell-native solution would be for PSSA to introduce a new cmdlet that hooks back into the hosting script analyzer:

Write-Diagnostic -Message <string> -Extent <IScriptExtent> [-Severity <DiagnosticSeverity>]

This command can:

• Simplify diagnostic emission
• Automatically suppress diagnostics internally
• Allow optional parameters and changes to the diagnostic constructor and write-diagnostic cmdlet without breaking APIs
• Even infer what rule it's being run from from the call stack (while this can be done, it's not clear it's a good idea)

Constrain and document the assumptions script rules can make about their executing context

Finally, today it's not clear what assumptions a script rule should make about the context in which it's run, in particular whether it shares context with the environment running Invoke-ScriptAnalyzer.

So that PSSA can be as efficient as possible, the only assumptions script rules should make about their running environment are:

• The rule-defining module and the current version of PSSA are imported into their executing runspace
• The script rule will be run on the same machine under the same user as the invoking PSSA process (which may not be using Invoke-ScriptAnalyzer, since it may be hosted)

In particular, some assumptions we don't want are:

• The script rule will be run in the same runspace or on the same thread as Invoke-ScriptAnalyzer
• Script rules will be run serially or in any defined order
• Script rules can access PSSA or other script rule module state
• Ideally, script rules should not assume they're being run in the same process as PSSA, but that may be an unhelpful and unneeded addition

Example

So an example of a rule defined with the new proposed API would be something like this:

function Test-UsesApprovedVerbs
{
    [Microsoft.PowerShell.ScriptAnalyzer.Rule("UseApprovedVerbs", "Ensure functions use approved PowerShell verbs", Namespace = "MyRules", Severity = "Information")]
    param(
        [System.Management.Automation.Language.Ast]
        $Ast,

        [System.Management.Automation.Language.Token[]]
        $Tokens,

        [string]
        $ScriptFilePath
    )

    $commandAsts = $Ast.FindAll({ <# logic to find non-compliant ASTs #> }, $true)
    foreach ($commandAst in $commandAsts)
    {
        # Extra parameter sets or transformations could be added here to make this nicer
        Write-Diagnostic -Extent $commandAst.Extent -Message "Does not comply" -Severity Information
    }
}

This rule can be referenced with MyRules/UseApprovedVerbs in settings.

Further considerations

• Should tokens be passed in an immutable IReadOnlyList<Token>?
• Should rules be required to process things in process blocks and have all inputs piped in with ValueFromPipelineByPropertyName? (This might improve performance)
• Is the assumption set too small?
• Is Write-Diagnostic the right implementation? In particular, how do we ensure every diagnostic has a RuleInfo object associated with it that links back to the providing rule (and ideally is referentially equal to all RuleInfo objects referring to the same rule)?
• Should Write-Diagnostic actually emit an object, or should it add objects to a list within PSSA?
• Should script rules be configurable? If so, how do they declare it? Should the configuration type be object, since PowerShell will handle any type there?

Inizia leggendo le proposte correlate nelle issue 1543 e 1545, quindi confrontale con il comportamento attuale della regola PowerShell esterna descritto qui. Il lavoro non è pronto per iniziare finché non saranno risolte le questioni di progettazione dell'API relative a parametri, attributi, Write-Diagnostic, contesto di esecuzione e configurazione; per considerarlo completato sarebbero necessari un design concordato e la sua implementazione.