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?

まず、issue 1543 と 1545 の関連する提案を読み、ここで説明されている現在の外部 PowerShell ルールの動作と比較してください。パラメーター、属性、Write-Diagnostic、実行コンテキスト、構成に関する API 設計上の問題が解決されるまで、作業を開始できる状態にはありません。完了には、合意された設計とその実装が必要です。