New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Nullable annotations #12631
Comments
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:
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. |
@vexx32 As for style issues I foresaw this and started # 11916. I hope we do the work first.
It is good rule for functional changes because Engine sometimes is very complex. |
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. |
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. |
Thanks @vexx32! It is great!
We could simply enumerate all out assemblies with ForEach-Object |
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! |
@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. |
This comment has been minimized.
This comment has been minimized.
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. |
I implemented the helper tool.
It shows 2421 types only in SMA. I never thought that so many types are declared there. I have no idea how we can handle this. |
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 🙂 |
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. |
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. |
Here is a sorted list of SMA types. I think we could start with interfaces in Group0 (DependencyCount = 0): IBlockingEnumerator |
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:
[Guid("AF86E2E0-B12D-4c6a-9C5A-D7AA65101E90")]
[InterfaceType(ComInterfaceType.InterfaceIsIUnknown)]
#nullable enable
internal interface IInspectable { }
#nullable restore |
I will start this work, starting at the bottom of the list with |
@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? |
🎶It's been a hard day's night... |
I believe that was all the interfaces of depth 0 |
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. Only 3 interface types is not annotated:
|
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. |
@iSazonov Why is this closed? Will we not accept annotation PR anymore? |
@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. |
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:
Enable nullable: <namespace>.<type name>
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
The text was updated successfully, but these errors were encountered: