Skip to content
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

Null Reference Types for System.Windows.Forms #2678

Closed
wants to merge 9 commits into from
Closed

Conversation

@Logerfo
Copy link
Contributor

Logerfo commented Jan 9, 2020

Fixes #1107.
I'm not sure if master is the right target branch here...

Proposed changes

As suggested by @drewnoakes, I added the <Nullable>enable</Nullable> attribute to the System.Windows.Forms.csproj file and the #nullable disable directive to each and every .cs file, after their headers. I'm not sure if (or when) other projects in this solution should also be considered. (See #2676).
The goal is to remove the directive file by file, enabling the nullable reference types and fixing the resultant warnings gradually.
In case we need it, another suggestion from @drewnoakes is the usage of the #nullable enable annotations directive in cases where full compliance would be complex to achieve, but still being able to annotate the API surface.
I chose to start with a very common file: Control.cs, which is already refactored in order to comply with NRT without warnings in this branch. This doesn't mean the file is not subject anymore: as more files get refactored, already refactored files might need to be revisited, since non-obvious nullable references might become obvious.
Some (but not all) "dammit" operators (!) and suppressions hides possible null reference exceptions, which I didn't care to touch in order to maintain the current behavior. If it's of the maintainers interested to fix those, the work will be greater.
It comes to my mind that auto-generation codes also have to be refactored in order to avoid already warned generated codes. I don't know in which project the code generation codes are.
I also think it's very important to keep this branch always up to date, periodically rebasing it and fixing merge conflicts as they are acknowledged.
I encourage everyone to review this starter code in order to avoid replicating possible mistakes.
I'd be glad to give write permission to my fork to anyone who want to help and doesn't have write permission to dotnet/winforms.

Customer Impact

  • Considering the idea of annotating the API surface, this will allow Windows Forms projects targeting .NET 5.0 to enjoy a complete NRT complied environment.
  • Considering the idea of annotating the whole codebase, this will allow contributors and collaborators to work with better quality codes.

Regression?

  • No

Risk

  • There is a risk of deploying a nullable member which is not supposed to be one, since reverting it to notnull would be a breaking change.

Tests

  • Should we also refactor existing tests? Should we add more tests?

cc @RussKie @hughbe @sharwell

Microsoft Reviewers: Open in CodeFlow
Logerfo added 6 commits Jan 9, 2020
@sharwell

This comment has been minimized.

Copy link
Member

sharwell commented Jan 9, 2020

and the #nullable disable directive to each and every .cs file, after their headers.

I would prefer to only add the directive to files that meet one of the following conditions:

  1. One or more nullability warnings is reported in the file
  2. The file declares one or more public APIs which exposes a reference type in some way (these require additional scrutiny for the public API surface)

For #2676 I accounted for the first, but not the second (because I specifically reviewed the limited public API surface in the same pull request).

@codecov

This comment has been minimized.

Copy link

codecov bot commented Jan 9, 2020

Codecov Report

Merging #2678 into master will increase coverage by 0.00717%.
The diff coverage is 79.78142%.

@@                Coverage Diff                 @@
##              master      #2678         +/-   ##
==================================================
+ Coverage   57.59523%   57.6024%   +0.00717%     
==================================================
  Files           1235       1235                 
  Lines         418368     418368                 
  Branches       38816      38816                 
==================================================
+ Hits          240960     240990         +30     
+ Misses        172035     172005         -30     
  Partials        5373       5373
Flag Coverage Δ
#Debug 57.6024% <79.78142%> (+0.00717%) ⬆️
#production 31.33112% <79.78142%> (+0.01173%) ⬆️
#test 98.93347% <ø> (ø) ⬆️
@Logerfo

This comment has been minimized.

Copy link
Contributor Author

Logerfo commented Jan 9, 2020

@sharwell I didn't noticed your recent PR, that's awesome!

I think we can go ahead and remove the directive from the files that doesn't meet the conditions you mentioned as a first step before annotating other files, what do you think about that approach?

@Logerfo Logerfo changed the title Nrt Null Reference Types for System.Windows.Forms Jan 9, 2020
@Logerfo

This comment has been minimized.

Copy link
Contributor Author

Logerfo commented Jan 9, 2020

I just noticed #2646. Maybe we could merge (or split even more) these requests?

Logerfo added 2 commits Jan 9, 2020
These files have no warnings and no public reference members. They are mostly enum or simple event args files.
@RussKie

This comment has been minimized.

Copy link
Member

RussKie commented Jan 9, 2020

Thank you everyone involved, your work is much appreciated.

Given this change has a significant surface I will feel significantly more comfortable accepting changes if those were structured in the following way:

  1. (Semi)-automated update where files are marked as non-nullable (as per @drewnoakes's suggestion). No changes to API signatures of any kind.
  2. Nullability-related API changes.

I expect (1) to be spanning pretty much all files in the project. This change will be impossible to review in any meaningful way, and for the most part will be taken at the face value. That's why it is important that the change doesn't contain any manual changes to API.
Whilst I agree with @sharwell on his comments above (re: #2676), I am not sure it is practical or feasible to do in the SWF project. The SWFP doesn't have a great deal of reference types, and, thus, it is possible to disable nullability checks in specific files only.

The (2) change(s) will be scoped and targeted - few API at a time. Possible not even complete types, because some of our types definitions span thousands and tens of thousands of lines.
An instead of starting with Control or Form perhaps it is worth starting with "small" types (i.e. those with small definitions) and "leaf" types (e.g. EventArgs-derived).

@Logerfo

This comment has been minimized.

Copy link
Contributor Author

Logerfo commented Jan 10, 2020

@RussKie if I understood correctly, you mean you want a single PR containing only 092f1a5 and 5814610 (I think 86c3bdf could be considered as well, maybe we could run an API-diff to be sure), and then multiple PRs containing API changes. For instance: a PR for 8533483, another one for bacabc6... It doesn't mean the whole type or file will get annotated, but also doesn't mean only a single file or type is allowed per PR: it depends on the context, specially considering the review effort.
If that's it, I can start right away.

@drewnoakes

This comment has been minimized.

Copy link
Member

drewnoakes commented Jan 10, 2020

@sharwell's idea is sound. Consider however that it frontloads the manual work, which in this case is a considerable enough to lead to fatigue and probable mistakes. It also means reviewers don't see untouched files which may actually contain public API.

@RussKie

This comment has been minimized.

Copy link
Member

RussKie commented Jan 13, 2020

@Logerfo you understood it correctly 👍

@sharwell @drewnoakes any objections to this plan?

@sharwell

This comment has been minimized.

Copy link
Member

sharwell commented Jan 13, 2020

No objection from me

@drewnoakes

This comment has been minimized.

Copy link
Member

drewnoakes commented Jan 13, 2020

Looks good.

I believe an update to the Public API analyzer was merged recently that adds support for NRT. It would be worth considering using that for any follow up PRs that start enabling NRT per-file.

@RussKie

This comment has been minimized.

Copy link
Member

RussKie commented Jan 14, 2020

I believe an update to the Public API analyzer was merged recently that adds support for NRT.

For the general benefit, Drew was referring to Microsoft.CodeAnalysis.PublicApiAnalyzers, which we don't currently use.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.