Runtime: Annotate remainder of .NET Core assemblies for nullable reference types

Created on 27 Aug 2019 · 17Comments · Source: dotnet/runtime

In .NET Core 3.0, we annotated System.Private.CoreLib for nullable reference types, along with the reference assemblies that surface those types, and the implementations for all of the partial facades behind those reference assemblies. That represents something around 20% of the surface area. Post-.NET Core 3.0, we plan to annotate the rest.

Our desired approach to annotating an assembly is to only do so once all of its dependencies are annotated. That way, the assembly can be annotated as a unit, and barring any bugs that need to be fixed in a dependency, we won't need to spend time fixing up new warnings in higher-level assemblies as lower-level assemblies are annotated. This leads to a semi-ordering of how assemblies should be annotated, from the "bottom up". Here is a rough grouping. We can annotate everything in group 1 (in parallel if desired), then everything in group 2 (in parallel if desired), etc. There is of course more parallelism than this available, as not every assembly in group N has a dependency on every assembly in group N - 1, but the graph is complicated and this is easier to visualize and reason about.

We will:

Submit individual PRs, one for each assembly. Each PR should include changes to both the src and the ref. Each PR should contain only changes related to the nullable annotations/attributes, no other changes.
PRs can be merged once the annotations have been appropriately reviewed in PR.
API reviews will happen subsequently, after PRs have been merged, in batch based on everything in master.

cc: @dotnet/nullablefc, @cartermp

0 (Completed in .NET Core 3.0)

[x] System.Buffers
[x] System.Collections
[x] System.Collections.Concurrent
[x] System.Diagnostics.Contracts
[x] System.Diagnostics.Debug
[x] System.Diagnostics.StackTrace
[x] System.Diagnostics.Tools
[x] System.Diagnostics.Tracing
[x] System.Memory
[x] System.Numerics.Vectors
[x] System.Reflection.Emit
[x] System.Reflection.Emit.ILGenerator
[x] System.Reflection.Emit.Lightweight
[x] System.Reflection.Primitives
[x] System.Resources.ResourceManager
[x] System.Runtime
[x] System.Runtime.Extensions
[x] System.Runtime.InteropServices
[x] System.Runtime.Intrinsics
[x] System.Runtime.Loader
[x] System.Security.Principal
[x] System.Text.Encoding.Extensions
[x] System.Threading
[x] System.Threading.Overlapped
[x] System.Threading.Tasks
[x] System.Threading.Thread
[x] System.Threading.ThreadPool
[x] System.Threading.Timer

1

[x] Microsoft.Win32.Primitives
[x] System.Collections.NonGeneric
[x] System.Collections.Specialized
[x] System.ComponentModel
[x] System.ComponentModel.Primitives
[x] System.Console
[x] System.Diagnostics.DiagnosticSource
[x] System.Drawing.Primitives
[x] System.IO.Compression
[x] System.IO.Pipelines
[x] System.Linq
[x] System.Linq.Parallel
[x] System.Net.WebHeaderCollection
[x] System.ObjectModel
[x] System.Reflection.DispatchProxy
[x] System.Reflection.TypeExtensions
[x] System.Resources.Writer
[x] System.Runtime.CompilerServices.Unsafe
[x] System.Runtime.CompilerServices.VisualC
[x] System.Runtime.InteropServices.RuntimeInformation
[x] System.Runtime.Numerics
[x] System.Runtime.Serialization.Formatters
[x] System.Runtime.Serialization.Primitives
[x] System.Security.Cryptography.Primitives
[x] System.Security.Principal
[x] System.Text.Encoding.CodePages
[x] System.Text.Encodings.Web
[x] System.Text.Json
[x] System.Text.RegularExpressions
[x] System.Threading.Channels
[x] System.Threading.Tasks.Parallel
[x] System.Transactions.Local
[x] System.Web.HttpUtility

2

[x] System.Collections.Immutable
[x] System.ComponentModel.EventBasedAsync
[x] System.IO.Compression.Brotli
[x] System.IO.FileSystem
[x] System.IO.FileSystem.DriveInfo
[x] System.IO.FileSystem.Watcher
[x] System.IO.MemoryMappedFiles
[x] System.Linq.Expressions
[x] System.Reflection.Metadata
[x] System.Security.Claims
[x] System.Security.Cryptography.Encoding

3

[x] Microsoft.Win32.SystemEvents
[x] System.ComponentModel.Composition
[x] System.Diagnostics.FileVersionInfo
[x] System.Dynamic.Runtime
[x] System.IO.Compression.ZipFile
[x] System.Linq.Queryable
[x] System.Reflection.MetadataLoadContext
[x] System.Security.AccessControl
[x] System.Security.Cryptography.Algorithms
[x] System.Security.Principal.Windows

4

[x] Microsoft.Win32.Registry
[x] System.Diagnostics.Process
[x] System.Diagnostics.TraceSource
[x] System.Drawing.Common
[x] System.IO.FileSystem.AccessControl
[x] System.IO.IsolatedStorage
[x] System.IO.Pipes
[x] System.Net.Primitives
[x] System.Net.WebSockets
[x] System.Security.Cryptography.Cng
[x] System.Security.Cryptography.Csp
[x] System.Security.Cryptography.OpenSsl
[x] System.Security.Cryptography.X509Certificates

5

[x] Microsoft.VisualBasic.Core
[x] System.IO.Pipes.AccessControl
[x] System.Net.NameResolution
[x] System.Net.Ping
[x] System.Net.Security
[x] System.Net.ServicePoint
[x] System.Net.Sockets
[x] System.Security.Cryptography.Pkcs

6

[x] System.Net.NetworkInformation
[x] System.Net.WebProxy

7

[x] System.Net.Http

8

[x] System.Net.Requests
[x] System.Net.WebSockets.Client
[x] System.Utf8String.Experimental
[x] System.Runtime.WindowsRuntime
[x] System.Runtime.WindowsRuntime.UI.Xaml
[x] Microsoft.CSharp
[x] System.Private.Xml (this and the below System.Xml.* contracts should all be done together)
[x] System.Xml.ReaderWriter
[x] System.Xml.XPath
[x] System.Xml.XPath.XDocument
[x] System.Xml.XDocument
[x] System.Xml.XmlDocument
[x] System.Xml.XmlSerializer

9

[ ] System.ComponentModel.TypeConverter (will be annotated in 6.0; in progress by @safern)
[x] System.Data.Common
[x] System.Data.DataSetExtensions
[x] System.Diagnostics.TextWriterTraceListener
[x] System.Net.HttpListener
[x] System.Net.Mail
[x] System.Net.WebClient
[x] System.Private.DataContractSerialization
[x] System.Private.Xml.Linq
[x] System.Runtime.Serialization.Json
[x] System.Runtime.Serialization.Xml

10 (part of netcoreapp but reference netstandard)

With netstandard not annotated, we will need to be cognizant of the fact that all dependencies will be viewed as oblivious.

[x] System.ComponentModel.Annotations
[x] System.Threading.Tasks.Dataflow

11 (not part of netcoreapp but reference netcoreapp)

[ ] System.DirectoryServices
[x] System.Data.Odbc
[ ] System.Diagnostics.EventLog
[ ] System.DirectoryServices.Protocols
[ ] System.Security.Permissions
[x] System.ServiceProcess.ServiceController
[ ] System.Windows.Extensions

12 (built from dotnet/runtime but not in netcoreapp and reference netstandard)

With netstandard not annotated, we will need to be cognizant of the fact that all dependencies will be viewed as oblivious:

[x] Microsoft.Win32.Registry.AccessControl => netstandard
[ ] System.CodeDom => netstandard
[ ] System.ComponentModel.Composition.Registration => netstandard, System.Reflection.Context
[ ] System.Composition.AttributedModel => netstandard
[ ] System.Composition.Convention => netstandard, System.Composition.AttributedModel
[ ] System.Composition.Hosting => netstandard, System.Composition.Runtime
[ ] System.Composition.Runtime => netstandard
[ ] System.Composition.TypedParts => netstandard, System.Composition.Runtime, System.Composition.AttributedModel, System.Composition.Hosting
[ ] System.Configuration.ConfigurationManager => netstandard, System.Security.Cryptography.ProtectedData
[x] System.Data.OleDb => netstandard, System.Diagnostics.PerformanceCounter, System.Configuration.ConfigurationManager
[ ] System.Diagnostics.PerformanceCounter => System.Configuration.ConfigurationManager
[ ] System.DirectoryServices.AccountManagement => System.Configuration.ConfigurationManager
[x] System.IO.Packaging => netstandard
[ ] System.IO.Ports => netstandard
[ ] System.Management => System.CodeDom
[x] System.Net.Http.WinHttpHandler => netstandard
[x] System.Numerics.Tensors => netstandard
[x] System.Reflection.Context => netstandard
[x] System.Resources.Extensions => netstandard
[ ] System.Runtime.Caching => netstandard, System.Configuration.ConfigurationManager
[x] System.Security.Cryptography.ProtectedData => netstandard
[ ] System.Security.Cryptography.Xml => netstandard
[ ] System.ServiceModel.Syndication => netstandard
[x] System.Threading.AccessControl => netstandard

13

[ ] Remove #nullable enable from individual files after all dependent projects annotated

area-Meta tracking

Source

stephentoub

🚀13 👀1

Most helpful comment

Only looking at Microsoft.NetCoreApp (because that's all we we were shooting for):

| Framework | #AnnotatableAPIs | #AnnotatedAPIs | % |
|---------------|------------------|----------------|-----|
| netcoreapp2.1 | 15,487 | 0 | 0% |
| netcoreapp3.0 | 16,582 | 5,142 | 31% |
| netcoreapp3.1 | 16,582 | 5,142 | 31% |
| net5.0 | 17,043 | 13,680 | 80% |

Currently missing in .NET 5:

System.ComponentModel.TypeConverter
System.Net.HttpListener
System.Runtime.Serialization.Json
System.Runtime.Serialization.Xml
System.Xml.ReaderWriter
System.Xml.XDocument
System.Xml.XmlSerializer
System.Xml.XPath.XDocument

terrajobst on 20 Aug 2020

👍17 ❤5

All 17 comments

Do you think we should take the effort to annotate our test projects as well? It seems that it might be a good way to catch annotations bugs, particularly in the public API surface.

eiriktsarpalis on 20 Dec 2019

Do you think we should take the effort to annotate our test projects as well? It seems that it might be a good way to catch annotations bugs, particularly in the public API surface.

No.

1) Even if we did, we wouldn't want to do it until all of our surface area was annotated, as tests use a lot more than just their associated library.

2) They also use unannotated 3rd-party libs, too, like xunit itself.

3) A lot of our tests are about validating behavior upon misuse, including null behaviors. There will be tons upon tons of suppressions.

4) The vast majority of tests do not compose calls the way apps do. It is extremely unlikely in my opinion that they would find meaningful annotation mistakes, even more so because we'll be in the habit with (3) of adding suppressions everywhere.

5) Lots of tests implicitly use reflection, e.g. inputs passed to theories, where the nullability of the inputs won't be transferred to the callee, making it useless.

I don't see the ROI.

stephentoub on 20 Dec 2019

👍3

Is there a plan for annotating Microsoft.Extensions.* packages that now live in dotnet/runtime?

JamesNK on 5 Jun 2020

@JamesNK for .NET 5 we will annotate all of the shared framework. That doesn't include separate packages like this. We'd like everything to be annotated, and would take PR's for it (presumably trying to work up library dependency order)

danmosemsft on 5 Jun 2020

Ok. The ASP.NET team is starting to annotate aspnetcore, and Microsoft.Extensions.DependencyInjection and Microsoft.Extensions.Configuration are low level dependencies in our stack.

We might submit PRs as part of that work.

JamesNK on 5 Jun 2020

It might suffice to annotate the abstractions for these extension libraries to start with. However, one thing to note is that these packages currently only target ns2.0 and do not cross-compile. @danmosemsft, what's your recommendation here to turn on nullability for these assemblies? Should we consider cross-compiling to net5?

pranavkm on 5 Jun 2020

Ah - I overlooked that. Hmm, I don't think this would be a good enough reason to cross-compile. cc @stephentoub in case he has another idea.

danmosemsft on 5 Jun 2020

Cross-compilation isn't needed.

Our general approach has been to annotate the implementation and use that to validate the surface area annotations. But that's less meaningful when the targeted surface area (ns2.0) isn't annotated.

I'd suggest just annotating the library's surface area / ref assembly, at least to start. You can then annotate the implementation as well if desired, keeping in mind that a) all of the depended-on targeted surface area will be oblivious, and b) some mechanisms for warning suppression won't work without some hacks (e.g. Debug.Assert). You could temporarily retarget locally to net5 to further help validate, even if you don't check in the build changes.

stephentoub on 5 Jun 2020

👍1

Actually, I just discovered we actually suppress all nullable warnings in dotnet/runtime when building for netstandard2.0. That means that, at present, there's little-to-no value in trying to annotate the src, other than for the public API signatures to match what's put in the ref, because there's going to be zero validation of anything.

stephentoub on 5 Jun 2020

I looked System.DirectoryServices and found many inconsistencies there. Which option does MSFT team prefer - ref-only annotation or full src review with code fixes?
The second option may require a lot of effort since there are a lot of files and a lot of comments from owners of the code since the design is not always clear (it seems there is a lot of confidence in ADDS).

iSazonov on 25 Jun 2020

I looked System.DirectoryServices and found many inconsistencies there

Inconsistencies?

Which option does MSFT team prefer - ref-only annotation or full src review with code fixes?

In general we strongly prefer to do a full source annotation (details at https://github.com/dotnet/runtime/blob/master/docs/coding-guidelines/api-guidelines/nullability.md). This isn't always possible or fruitful, though. For example, we have some libraries that build against netstandard2.0, and the benefit of doing full source annotation there is significantly diminished because the targeted surface area isn't annotated; as such, we actually disable all nullable warnings for such builds. As another example, Microsoft.VisualBasic.Core is ref only because we can't annotate the VB source, and Microsoft.CSharp is ref only because the code is mostly a direct port of an old C++ codebase that's also generally only consumed directly by compiler-generated code, and there'd be little value in exerting the effort to null annotate the implementation.

System.DirectoryServices, though, references netcoreapp, so we would want to fully annotate its implementation.

stephentoub on 25 Jun 2020

Inconsistencies

return new Guid(_schemaGuidBinaryForm) can throw ArgumentNullException that is weird for reading operation:

https://github.com/dotnet/runtime/blob/0141587160b8391272f7261ad3926f62326ea1bb/src/libraries/System.DirectoryServices/src/System/DirectoryServices/ActiveDirectory/ActiveDirectorySchemaClass.cs#L995-L1024

It seems it should be return _schemaGuidBinaryForm is null ? Guid.Empty : new Guid(_schemaGuidBinaryForm)

I found some points with such inconsistencies.

System.DirectoryServices, though, references netcoreapp, so we would want to fully annotate its implementation.

There are over 120 files. Makes sense to split the work to some PRs?

iSazonov on 25 Jun 2020

There are over 120 files. Makes sense to split the work to some PRs?

Personally I think it'd be better to do it all in one. I expect you'll find that updates to annotations in one file cause new warnings in other files that have already been annotated.

stephentoub on 8 Jul 2020