search

dotnet / wpf

WPF is a .NET Core UI framework for building Windows desktop applications.
MIT License
7.08k stars 1.17k forks source link

Support for nullability annotations #7563

Open pchaurasia14 opened 1 year ago

pchaurasia14 commented 1 year ago

[Plan finalized - 3/28]

Based on community feedback, here is the plan for supporting Nullability annotations.

List of assemblies and respective groups:

Thanks @dotMorten for the suggestion

dotMorten commented 1 year ago

Took a brief look, and there definitely needs to be made some calls now and then on behavior around null. Just the first class I hit looking a bit at this, and you see something like this: https://github.com/dotnet/wpf/blob/c37ab558d3aebf3bfe0038c86e8a4be78d09e03c/src/Microsoft.DotNet.Wpf/src/System.Xaml/System/Windows/Markup/ArrayExtension.cs#L37-L41 It's clear that _arrayType is not nullable, since we don't allow null in the constructor. It is furthermore confirmed when looking in the ProvideValue method: https://github.com/dotnet/wpf/blob/c37ab558d3aebf3bfe0038c86e8a4be78d09e03c/src/Microsoft.DotNet.Wpf/src/System.Xaml/System/Windows/Markup/ArrayExtension.cs#L107-L110 However, in the property setter, no null check is made, so it could become null after all: https://github.com/dotnet/wpf/blob/c37ab558d3aebf3bfe0038c86e8a4be78d09e03c/src/Microsoft.DotNet.Wpf/src/System.Xaml/System/Windows/Markup/ArrayExtension.cs#L83-L87 So that begs the question: Should we make the Type argument nullable. And if we do that, should we remove the null check in the constructor? Or if we decide to not make the type property nullable, then should we start throwing in the property setter if the value provided is null? Both cases are slight changes in behavior. Or do we just mark it not-nullable (since it seems that is the intent), yet keep the current behavior where we do allow a null to sneak in there?

miloush commented 1 year ago

In general I think there is much higher chance of the whole annotation project make it if it is metadata changes only, avoiding any changes in behaviour.

maxkoshevoi commented 1 year ago

In general I think there is much higher chance of the whole annotation project make it if it is metadata changes only, avoiding any changes in behaviour.

That's how it was done in the runtime. And separate issues were created to fix !s

danmoseley commented 1 year ago

Cc @stephentoub in case he has suggestions or thoughts about organizing this process in WPF based on what we learned.

There’s also guidance here

https://github.com/dotnet/runtime/blob/57bfe474518ab5b7cfe6bf7424a79ce3af9d6657/docs/coding-guidelines/api-guidelines/nullability.md

Certainly the more the community can do here, and it’s super susceptible to parallelization across interested folks, the more time the WPF maintainers can put on features.

stephentoub commented 1 year ago

in case he has suggestions or thoughts about organizing this process in WPF based on what we learned

I was pleased with the process we followed in dotnet/runtime. It was painstaking work, but the net result was very good.

The process was effectively this:

  1. We ordered our assemblies by dependencies, starting with corelib, and then working our way up. That way, when we went to annotate an assembly, everything it depended on was already annotated. You can see the bucketing created for this in https://github.com/dotnet/runtime/issues/2339.
  2. Annotation was done according to the guidelines at https://github.com/dotnet/runtime/blob/main/docs/coding-guidelines/api-guidelines/nullability.md.
  3. For each assembly, we did two high-level passes as part of the work to annotate an assembly, contributing to a single PR for that assembly. First, we went through every member exposed from the assembly (e.g. public members on public types) and made the annotations to be correct based on our understanding of what the API's requirements/semantics were, regardless of the underlying implementation and without paying attention to resulting compiler warnings. Second, without changing those annotations, we made the assembly compile, which necessitated a long slog of fixing nullable warnings-as-errors until they were all gone. For larger assemblies, we'd typically do this per file, e.g. choose the next file with public members to attack, mark that file as #nullable enable, annotate all its public members, get it to compile, move on to the next file. Then once we'd successfully marked all files and everything was compiling, flip the assembly to be <Nullable>enable</Nullable> and remove all the #nullable enables from the individual files.
  4. While doing that, we encountered primarily three types of issues: a) Places we incorrectly annotated the public surface area in the first pass, e.g. because we discovered that nulls weren't allowed somewhere we thought they were; we would then go back and fix these public annotations. b) Places where there were inconsistencies in how nulls were handled, e.g. a constructor didn't allow null but a setter on a corresponding property did; we'd open an issue for deciding what to do about that later, typically also adding a // TODO-NULLABLE https://link/to/issue: Figure out what to do about blah kind of comment in the code. c) Places where there was an obvious bug due to improperly handling nulls. If these were truly bugs, we'd fix it and include a test, but in general we'd prefer to instead open an issue for subsequent analysis and follow-up. One of our goals to keep things simple was that, in general, the annotation PRs shouldn't affect the IL at all, other than metadata, preferring to leave changes that could impact the resulting IL to subsequent dedicated PRs.
pchaurasia14 commented 1 year ago

Finalized the plan for nullability annotations support - top post is updated now.

7665 disables few warnings for ref projects.

Will be updating it with more warnings that need to be suppressed.

halgab commented 1 year ago

Using Microsoft.CodeAnalysis.PublicApiAnalyzers was a prerequisite for the winforms repo (see dotnet/winforms#2705). Is it also the case here?

lindexi commented 1 year ago

@halgab This project has its own baseline component. So I don't think Microsoft.CodeAnalysis.PublicApiAnalyzers is necessary