Nullable annotations

Ilya (@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. Ilya (@iSazonov) and Staffan Gustafsson (@powercode) agreed to start the project. But we need more contributors and code reviewers.
Rain Sallow (/u/ta11ow) (@vexx32) Patrick Meinecke (@SeeminglyScience) Kirk Munro (@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:

    [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

( Previous 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.

Status

Working on Group0

Done - most of interface types was annotated.

In process - now we can annotate structs with dependency 0

Steve Lee (@SteveL-MSFT)

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

Steve Lee (@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.

James Truher (@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.

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.

Rain Sallow (/u/ta11ow) (@vexx32)

Rain Sallow (/u/ta11ow) (@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.

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

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!

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.

Ilya (@iSazonov)

In terms of creating a list of types to start with as Ilya (@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.

Rain Sallow (/u/ta11ow) (@vexx32)

Thanks Rain Sallow (/u/ta11ow) (@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

Dongbo Wang (@daxian-dbw)

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

Rain Sallow (/u/ta11ow) (@vexx32)

Rain Sallow (/u/ta11ow) (@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.

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?

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.

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)

Ilya (@iSazonov)

Ilya (@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 #14018 (comment).

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

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?

Tick when starting work

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

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

}

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

I believe that was all the interfaces of depth 0

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.

Updated file System.Management.Automation-20201109114545.xlsx in OP.
~~About 50% interface types was annotated.~~

Only 3 interface types is not annotated:

IDispatch
ICommandRuntime
IParameterMetadataProvider

Most of interface types was annotated.

Now we can annotate structs with dependency 0. It is ~71 struct but most of them is for p/invokes and annotations are formal. Welcome to grab!

Since MSFT team is not very active I will do fast merge since it is not critical changes.

Ilya (@iSazonov)

Ilya (@iSazonov) Why is this closed? Will we not accept annotation PR anymore?

xtqqczze

xtqqczze We need to process about 20 classes daily to get the job done in a reasonable amount of time. But we see near-zero activity and complete indifference from MSFT team.
So I have no desire to keep track of dead issues.

To move forward, we should:

Enable nullable warning context globally.
Add #nullable disable to each file.

This way, all new files will automatically have nullables enabled, which will be progress at least.

Navigation Menu

Repository navigation

Nullable annotations #12631

Rules

Plan

Status

7 remaining items

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Nullable annotations #12631

Description

Rules

Plan

Status

Activity

SeeminglyScience commented on May 12, 2020

vexx32 commented on May 12, 2020

iSazonov commented on May 12, 2020

vexx32 commented on May 12, 2020

powercode commented on May 13, 2020

powercode commented on May 13, 2020

vexx32 commented on May 13, 2020

iSazonov commented on May 14, 2020

SteveL-MSFT commented on May 20, 2020

powercode commented on May 20, 2020

xtqqczze commented on Jul 6, 2020

iSazonov commented on Sep 1, 2020

iSazonov commented on Nov 6, 2020

7 remaining items

xtqqczze commented on Nov 9, 2020

xtqqczze commented on Nov 9, 2020

iSazonov commented on Nov 9, 2020

powercode commented on Nov 19, 2020

powercode commented on Nov 19, 2020

powercode commented on Nov 19, 2020

powercode commented on Nov 20, 2020

powercode commented on Nov 20, 2020

iSazonov commented on Nov 20, 2020

iSazonov commented on Dec 11, 2020

iSazonov commented on May 20, 2021

xtqqczze commented on Aug 25, 2021

iSazonov commented on Aug 26, 2021

xtqqczze commented on Aug 23, 2025

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions