Powershell: Introduce "InvokedInCurrentScopeAttribute" for static analysis

[SeeminglyScience]

Summary of the new feature/enhancement

When a command has a ScriptBlock parameter, it can be difficult for static analysis tools like PSScriptAnalyzer to determine how the ScriptBlock will be invoked. This makes tracking some things significantly more difficult (like if an assigned variable is utilized). For example, although ForEach-Object is invoked in the current scope, it's not possible for PSSA to determine variable usage in this example:

$a = @()
0..10 | ForEach-Object { $a += $PSItem }
return $a

Proposed technical implementation details (optional)

Introduce an attribute InvokedInCurrentScopeAttribute that can be used to tell static analysis tools what scope the command plans to invoke the parameter.

using System;

namespace System.Management.Automation
{
    [AttributeUsage(AttributeTargets.Property | AttributeTargets.Parameter)]
    public sealed class InvokedInCurrentScopeAttribute : Attribute
    {
        public string WhenPresent { get; set; }
    }

    public partial sealed class ForEachObjectCommand
    {
        [InvokedInCurrentScope]
        public ScriptBlock Begin { get; set; }

        [InvokedInCurrentScope]
        public ScriptBlock[] Process { get; set; }

        [InvokedInCurrentScope]
        public ScriptBlock End { get; set; }

        [InvokedInCurrentScope]
        public ScriptBlock[] RemainingScripts { get; set; }
    }

    public partial sealed class WhereObjectCommand
    {
        [InvokedInCurrentScope]
        public ScriptBlock FilterScript { get; set; }
    }

    public partial class InvokeCommandCommand
    {
        [InvokedInCurrentScope(WhenPresent = nameof(NoNewScope))]
        public override ScriptBlock ScriptBlock { get; set; }
    }
}

Tooling support for this should be implemented similarly to how Roslyn handles nullability decorations. Mainly, looking for this decoration should be done by type name instead of identity. Binary modules targeting 5.1 should be able to define this attribute within their own assembly and still have it detected by analysis tools.

[iSazonov]

/cc @bergmeister Thoughts?

[iSazonov]

Perhaps we could add more for PSScriptAnalyzer in Engine/API.

[vexx32]

Perhaps we could create a supplementary module of static analysis attributes, so that we can ship that module in-box in future versions of us, but something like PSScriptAnalyzer could just take a dependency on that module and have them available downlevel to ps5.1 as well.

[bergmeister]

Such attribute would definitely help but since I assume this issue was created due to the known issue of the PSUseDeclaredVarsMoreThanAssignments PSSA rule that has false positives when variable usage crosses multiple scriptblocks? Therefore an alternative would also be to relax the rule and assume invocation in current scope. Personally, I was never too bother about its false positives and find the rule itself still useful. The problem of PSSA not being aware of Pester syntax like e.g. a variable declared in BeforeAll would still remain with such attributes I suppose.

[vexx32]

Would it? Both BeforeAll {} and It {} invoke their scripts in the same scope -- that of the parent Context {} or Describe {} block. As such, you could apply such an attribute to both It and the Before/After-type blocks and remove those false positives. There would still be _some_ if you're declaring values outside a context/describe, but iirc that's generally considered poor form anyway?

[SeeminglyScience]

Such attribute would definitely help but since I assume this issue was created due to the known issue of the PSUseDeclaredVarsMoreThanAssignments PSSA rule that has false positives when variable usage crosses multiple scriptblocks?

It's an easy example of something that already exists that could benefit, but not my motivation for creating the issue.

I was working on a sort of type inference overhaul. Part of that was making the variable inference more accurate which involves some flow analysis. I realized the same problem that plagues that rule would make any sort of flow analysis inaccurate whenever command with a scriptblock was used.

The problem of PSSA not being aware of Pester syntax like e.g. a variable declared in BeforeAll would still remain with such attributes I suppose.

I think you'd just put the attribute on BeforeAll no? I guess it depends how the rule is set up, but if you're actually trying to check execution flow, marking BeforeAll with InvokedInCurrentScope and leaving the It blocks undecorated should give enough information.

[bergmeister]

Hmm, you're right about Pester's BeforeAll. I initially thought PSSA would need to know the order of execution but it actually seems that PSUseDeclaredVarsMoreThanAssignments doesn't care about the order, i.e. foo $a; $a =42 makes it not trigger a warning, one could also argue this is actually a bug...
My main point is that if the problem we try to solve is reducing the false positives of this rule, we could relax it and just assume current scope, WDYT?

[SeeminglyScience]

My main point is that if the problem we try to solve is reducing the false positives of this rule, we could relax it an just assume current scope, WDYT?

Like I said that rule was just a convenient example of something existing that could benefit, but not why I opened the issue. I think a lot of potential tools could benefit from this sort of data.

That said, relaxing it to assume current scope would probably be sufficient for that rule specifically. Personally I'd like to see it catch more like the order problem you refer to, but that would probably be a pretty significant undertaking (e.g. if you're in a loop, that order is still valid for accessing the same var).

[iSazonov]

If we look C# nullable annotations there are many new attributes. For the issue I want to know - do we expect a set of new attributes for such annotations for static analysis? My concern is that it is public API and we need a good design before start.

[bergmeister]

Given that there is more than half a year of .Net 5 and therefore also PowerShell Previews, there should be enough time to test and iterate over the design. Now is actually the best time to experiment :)

[SeeminglyScience]

Yeah I think there's a lot of room for more annotations to assist with static analysis. Like @bergmeister mentioned, there's no need to wait until we figure out what all of them will be.

At a high level the most important things to me are:

1. They are used purely for static analysis. They have zero run time effect.
2. Tooling matches based on type name not identity.

To expand on #2, I expect to be able to target PowerShellStandard.Library 5.1 and still support a new static analysis attribute like this:

namespace System.Management.Automation
{
    [AttributeUsage(AttributeTargets.Property | AttributeTargets.Parameter)]
    internal sealed class InvokedInCurrentScopeAttribute : Attribute
    {
        public string WhenPresent { get; set; }
    }
}

namespace MyProject.Commands
{
    [Cmdlet(VerbsDiagnostic.Test, "MyCommand")]
    public InvokeMyCommandCommand : PSCmdlet
    {
        [Parameter]
        [InvokedInCurrentScope]
        public ScriptBlock Action { get; set; }
    }
}

Maybe PowerShell can be a little less strict than roslyn and ignore namespace as well.

[iSazonov]

there's no need to wait until we figure out what all of them will be.

The API is public and will be used in external repo - so we need to get approval from PowerShell Committee and security team. So it is better to start with good design so as not to waste their time.

I guess we need a base abstract class like "StaticAnalyzerAttribute".