Powershell: Nullable annotations

[iSazonov]

C# 8.0 introduces great feature - nullable reference types.
https://devblogs.microsoft.com/dotnet/try-out-nullable-reference-types/

Many developers already benefit from the feature in their projects and expect that PowerShell API will be nullable annotated too.

This is a lot of work. @iSazonov and @powercode agreed to start the project. But we need more contributors and code reviewers.
@vexx32 @SeeminglyScience @KirkMunro welcome and please ask your friends and followers.

To make this work efficiently, we need Rules and Plan.

Rules

Best start is .Net team experience https://github.com/dotnet/runtime/blob/master/docs/coding-guidelines/api-guidelines/nullability.md

Main rule is that _annotation PRs should not change code due to the high risk of making a bug_.

I believe we should strongly follow the rule too and make separate PRs if we see a need to refactor a code and especially if we see a bug.

We could save more time if we fixed most code style issues before starting the project.
I started the work in #11916 but again I need a help with code review. (#11916 fix ~5000 issues from ~10000, and I hope to fix rest in follow some PRs. You could pull such PRs too).

Please use one pattern:

• one PR per type with name Enable nullable: <namespace>.<type name>
• enable directive before first type line (after XML comments) and restore directive after last type line without extra empty lines:
  c# [Guid("AF86E2E0-B12D-4c6a-9C5A-D7AA65101E90")] [InterfaceType(ComInterfaceType.InterfaceIsIUnknown)] #nullable enable internal interface IInspectable { } #nullable restore
• add XML comments to non-nullable members like "This never returns null." - good documentation helps a lot

Plan

In the file
System.Management.Automation-20201109114545.xlsx

all SMA PowerShell types are sorted by dependency count.

We should annotate types by groups starting from group with dependency 0 (Group0), then 1 and so on.
Main rule here is - current annotating type should have all dependencies already annotated and merged.

[SeeminglyScience]

At the very least I'll definitely sign up for some reviews, I'd love to see this come together.

@SteveL-MSFT I'm not sure if y'all typically require a team member to sign off on a review before it can be merged, if so you may want to consider appointing a few folks from the community specifically for these annotation only PR's.

@JamesWTruher Once a lot of the this work is done it'd be amazing to see it in PowerShell Standard. Assuming your tooling doesn't already capture these (does it capture internal attribute annotations?), it would need to capture:

System.Diagnostics.CodeAnalysis.AllowNullAttribute
System.Diagnostics.CodeAnalysis.DisallowNullAttribute
System.Diagnostics.CodeAnalysis.MaybeNullAttribute
System.Diagnostics.CodeAnalysis.NotNullAttribute
System.Diagnostics.CodeAnalysis.MaybeNullWhenAttribute
System.Diagnostics.CodeAnalysis.NotNullWhenAttribute
System.Diagnostics.CodeAnalysis.NotNullIfNotNullAttribute
System.Diagnostics.CodeAnalysis.DoesNotReturnAttribute
System.Diagnostics.CodeAnalysis.DoesNotReturnIfAttribute
System.Diagnostics.CodeAnalysis.MemberNotNullAttribute
System.Diagnostics.CodeAnalysis.MemberNotNullWhenAttribute
System.Runtime.CompilerServices.NullableAttribute (embedded internal)
System.Runtime.CompilerServices.NullableContextAttribute (embedded internal)

Also if annotations were added to the 5.1 ref lib (wishful thinking I know, but 🤞) they would all need to be embedded internal.

[vexx32]

I'm sure I can do some work here, for sure. 🙂

I will _probably_ end up getting distracted with more style-related issues here and there as well, but I should be able to keep those and nullable annotation PRs completely separate.

[iSazonov]

@vexx32 As for style issues I foresaw this and started # 11916. I hope we do the work first.

I'm not sure if y'all typically require a team member to sign off on a review before it can be merged

It is good rule for functional changes because Engine sometimes is very complex.
I can fast merge PRs with annotations while they contains only annotations because they can break nothing in functionality. (We will fix annotations later based on feedbacks for a long time.)
But we must follow strong rule - move all code changes in other PRs. If we see that a code could be refactored as result of annotations we should do this after the annotations merged.

[vexx32]

I'll give that one a look over, bit of a big one, but does need doing. 👍

[powercode]

There are cases where the code wont build after annotations are added. In that case, we have to make changes, right?

But we should strive to keep changes to a minimum!

[powercode]

In SMA, is is also useful to start at the classes at the bottom of the dependency chain.
I started with LanguagePrimitives, PSObject and the PSObject collections.

[vexx32]

In terms of creating a list of types to start with as @iSazonov mentioned, I wrote this small function to determine the inheritance chain length of a given type:

function Get-InheritanceChainLength {
    [CmdletBinding()]
    param(
        [Parameter(Mandatory)]
        [Type]
        $Type
    )

    if ($Type.BaseType -eq $null) {
        return 1
    }

    (Get-InheritanceChainLength $Type.BaseType) + 1
}

Using this, we can get a listing of the types most deeply nested first:

[powershell].Assembly.GetTypes() |
    Sort-Object -Property @(
        @{Expression = { Get-InheritanceChainLength -Type $_ }; Descending = $true}
        @{ Expression = 'Name';Ascending = $true }
    )

That's only going to be a fraction of all the types since there are a good number of assemblies to work through, but it's a start.

[iSazonov]

Thanks @vexx32! It is great!

That's only going to be a fraction of all the types since there are a good number of assemblies to work through, but it's a start.

We could simply enumerate all out assemblies with ForEach-Object

[SteveL-MSFT]

I think adding @daxian-dbw and @rjmholt as reviewers (just need one of them to sign off, not both) would be sufficient. This would be great to have!

[powercode]

@vexx32 We don't really care about inheritance, but dependencies between types. There will be clusters of types at different levels, and it is easier do start at the bottom.

[xtqqczze]

@vexx32
Get-InheritanceTreeLength -> Get-InheritanceChainLength

[iSazonov]

After looking this script in depth I think we need a Roslyn analyzer because we take into account all type cross-references - if Class1 uses Class2 in field, property or local the Class2 should be annotated before Class1.
Has anybody an experience with Roslyn API to create such custom analyzer?

[iSazonov]

I implemented the helper tool.

Using MSBuild at 'C:\Users\1\AppData\Local\Microsoft\dotnet\sdk\5.0.100-rc.2.20479.15\' to load projects.
Loading project 'C:/Users/1/Documents/GitHub/iSazonov/PowerShell/src/System.Management.Automation/System.Management.Automation.csproj'
Evaluate        0:00.6018181    System.Management.Automation.csproj
Build           0:00.7572002    System.Management.Automation.csproj
Evaluate        0:00.0439293    Microsoft.PowerShell.CoreCLR.Eventing.csproj
Build           0:00.1137562    Microsoft.PowerShell.CoreCLR.Eventing.csproj
Resolve         0:00.0220096    Microsoft.PowerShell.CoreCLR.Eventing.csproj (net5.0)
Resolve         0:00.4936166    System.Management.Automation.csproj (net5.0)
Finished loading project 'C:/Users/1/Documents/GitHub/iSazonov/PowerShell/src/System.Management.Automation/System.Management.Automation.csproj'
Documents count = 733
Loading namespaces
Loading types
Type count = 2421
Evaluating type dependencies
Writing to file: System.Management.Automation-20201106180401.csv

It shows 2421 types only in SMA. I never thought that so many types are declared there.
There is even no way to publish this huge list in expanded form.

I have no idea how we can handle this. ✈️ I suspect that many types require refactoring and new xUnit tests.

[vexx32]

If we have the list I guess we can stash it in a spreadsheet and start with things that have no dependencies I guess? It would be some work to be sure, but it can be done piece by piece 🙂

[iSazonov]

Yes, I sorted the list so that to have types with no dependencies on top. (I want to add more info in the list before share.)

I believe we could be more productive if MSFT team reviewed the list first and indicated _design intentions_ which are not always obvious.

[vexx32]

Fully agreed on that point, but we may be waiting some time for that. It might be simpler to just do it as we're able and any design intentions may need to come from the PR reviews. Looking for design intent across the whole of S.M.A may prove a bit of a lengthy effort, even moreso than the code changes.

[iSazonov]

Here is a sorted list of SMA types.
System.Management.Automation-20201109114545.xlsx

I think we could start with interfaces in Group0 (DependencyCount = 0):

IBlockingEnumerator
ICmdletProviderSupportsHelp
IContentReader
IContentWriter
IDynamicParameters
IEtwActivityReverter
IInspectable
ILightCallSiteBinder
IMethodInvoker
IModuleAssemblyInitializer
IResourceSupplier
IRSPDriverInvoke
IScriptPosition
ISecurityDescriptorCmdletProvider
ISupportsTypeCaching
IValidateSetValuesGenerator

[xtqqczze]

Here is a sorted list of SMA types.
System.Management.Automation-20201109114545.xlsx

I think we could start with interfaces in Group0 (DependencyCount = 0):

IBlockingEnumerator
ICmdletProviderSupportsHelp
IContentReader
IContentWriter
IDynamicParameters
IEtwActivityReverter
IInspectable
ILightCallSiteBinder
IMethodInvoker
IModuleAssemblyInitializer
IResourceSupplier
IRSPDriverInvoke
IScriptPosition
ISecurityDescriptorCmdletProvider
ISupportsTypeCaching
IValidateSetValuesGenerator

It looks like adding the nullable annotations for these interfaces will be easy.

[iSazonov]

It looks like adding the nullable annotations for these interfaces will be easy.

Easy start. It won't always be that easy :-)

Please use one pattern:

• one PR per type with name Enable nullable: <namespace>.<type name>
• enable directive before first type line and restore directive after last type line:
  c# [Guid("AF86E2E0-B12D-4c6a-9C5A-D7AA65101E90")] [InterfaceType(ComInterfaceType.InterfaceIsIUnknown)] #nullable enable internal interface IInspectable { } #nullable restore

[xtqqczze]

I will start this work, starting at the bottom of the list with IValidateSetValuesGenerator and working up to the top. (in case anyone is already working top to bottom)

[xtqqczze]

@iSazonov Do we want to format the pre-processor directives to start at the beginning of the line? This would be standard indentation behaviour in Visual Studio. See https://github.com/PowerShell/PowerShell/pull/14018#discussion_r520015755.

[iSazonov]

If we can not to turn off the behavior in VS 2019 I agree to put this at the beginning of the line.

[powercode]

We will get a lot of CodeFactor errors since we touch code that is in violation of the rules.

Can we agree to ignore these errors and only focus on annotations?

[powercode]

Tick when starting work

• [x] IAssignableValue
• [x] IAstPostVisitHandler
• [x] IBoxableInstruction
• [x] IContainsErrorRecord
• [x] IDynamicPropertyCmdletProvider
• [x] IEtwEventCorrelator
• [x] IHasSessionStateEntryVisibility
• [x] IHostSupportsInteractiveSession
• [x] IHostUISupportsMultipleChoiceSelection
• [x] IInstructionProvider
• [x] IJobDebugger
• [x] IModuleAssemblyCleanup
• [x] IOutputProcessingState
• [x] IPropertyCmdletProvider
• [x] IRegistryWrapper
• [x] IScriptCommandInfo
• [x] IScriptExtent
• [x] ISubsystem
• [x] ISupportsAssignment
• [x] ITypeName
• [x] IArgumentCompleter
• [x] ICommandRuntime2
• [x] IContentCmdletProvider
• [x] IDispatch
• [x] IWSManNativeApiFacade
• [x] IConversionData
• [x] ICommandPredictor
• [x] ICommandRuntime
• [x] IParameterMetadataProvider
• [x] ICustomAstVisitor2
• [x] ICustomAstVisitor

[powercode]

To help with the workflow, I have this function

function NullableCommit {
    param(
        [Parameter(Mandatory)]
        [string] $Name,
        [ArgumentCompletions("System.Diagnostics", "System.Diagnostics.Eventing", "System.Diagnostics.Eventing.Reader", "System.Management.Automation", "System.Management.Automation.ComInterop", "System.Management.Automation.Configuration", 
        "System.Management.Automation.Help", "System.Management.Automation.Host", "System.Management.Automation.Internal", "System.Management.Automation.Internal.Host", "System.Management.Automation.InteropServices", "System.Management.Automation.Interpreter",
         "System.Management.Automation.Language", "System.Management.Automation.PerformanceData", "System.Management.Automation.Provider", "System.Management.Automation.PSTasks", "System.Management.Automation.Remoting", "System.Management.Automation.Remoting.Client", 
         "System.Management.Automation.Remoting.Internal", "System.Management.Automation.Remoting.Server", "System.Management.Automation.Remoting.WSMan", "System.Management.Automation.Runspaces", "System.Management.Automation.Runspaces.Internal", 
         "System.Management.Automation.Security", "System.Management.Automation.SecurityAccountsManager", "System.Management.Automation.SecurityAccountsManager.Extensions",
         "System.Management.Automation.SecurityAccountsManager.Native", "System.Management.Automation.SecurityAccountsManager.Native.NtSam", "System.Management.Automation.Subsystem", "System.Management.Automation.Tracing", "Microsoft.PowerShell.Commands")]
        [string] $Namespace = "System.Management.Automation"
    )

    git checkout -b nullable/$Name master
    Read-Host -Prompt "Press enter when done"
    git diff
    $choices = @(
        [System.Management.Automation.Host.ChoiceDescription]::new("&Commit and push")
        [System.Management.Automation.Host.ChoiceDescription]::new("&Fix")
        [System.Management.Automation.Host.ChoiceDescription]::new("&Diff")
    )

    while ($i -ne 0) {
        if ($i -eq 2) {
            git diff master
        }

        $i = $PSCmdlet.Host.UI.PromptForChoice("Commit?", "", $choices, 0)    
    }
    git commit -am "Enable nullable: $Namespace.$Name"
    git push

}

[powercode]

🎶It's been a hard day's night...

[powercode]

I believe that was all the interfaces of depth 0

[iSazonov]

We will get a lot of CodeFactor errors since we touch code that is in violation of the rules.

Can we agree to ignore these errors and only focus on annotations?

Yes, we should exclude unrelated changes. If a code requires reformatting or refactoring we should make this before in separate PRs with adding tests as needed.