Support for nullability annotations

[pchaurasia14]

[Plan finalized - 3/28]

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

List of assemblies and respective groups:

• Group 0
  • System.Xaml
  • System.Windows.Input.Manipulations
• Group 1
  • WindowsBase
• Group 2
  • System.Windows.Presentation
  • DirectWriteForwarder
  • UIAutomationTypes
• Group 3
  • UIAutomationProvider
  • PresentationCore
• Group 4
  • ReachFramework
  • UIAutomationClient
• Group 5
  • System.Printing
  • UIAutomationClientSideProviders
  • PresentationFramework
• Group 6
  • WindowsFormsIntegration
  • PresentationUI
• Group 7
  • Themes
    • PresentationFramework.Aero
    • PresentationFramework.Aero2
    • PresentationFramework.AeroLite
    • PresentationFramework.Classic
    • PresentationFramework.Luna
    • PresentationFramework.Royale
• Group 8
  • PresentationFramework extensions
    • SystemCore
    • SystemData
    • SystemDrawing
    • SystemXml
    • SystemXmlLinq
  • System.Windows.Controls.Ribbon

Thanks @dotMorten for the suggestion

[dotMorten]

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:

wpf/src/Microsoft.DotNet.Wpf/src/System.Xaml/System/Windows/Markup/ArrayExtension.cs

Lines 37 to 41 in c37ab55

	public ArrayExtension(
	Type arrayType)
	{
	_arrayType = arrayType ?? throw new ArgumentNullException(nameof(arrayType));
	}

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:

wpf/src/Microsoft.DotNet.Wpf/src/System.Xaml/System/Windows/Markup/ArrayExtension.cs

Lines 107 to 110 in c37ab55

	if (_arrayType == null)
	{
	throw new InvalidOperationException(SR.MarkupExtensionArrayType);
	}

However, in the property setter, no null check is made, so it could become null after all:

wpf/src/Microsoft.DotNet.Wpf/src/System.Xaml/System/Windows/Markup/ArrayExtension.cs

Lines 83 to 87 in c37ab55

	public Type Type
	{
	get { return _arrayType; }
	set { _arrayType = value; }
	}

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]

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]

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]

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]

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 Annotate remainder of .NET Core assemblies for nullable reference types runtime#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]

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]

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

[lindexi]

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

[SetTrend]

Is there a roadmap for this feature?