Update ASP.NET Core to use C# 8's nullable reference types

[JamesNK]

Updating ASP.NET Core to use C# 8's nullable reference types would:

1. Help ASP.NET Core libraries avoid null reference exceptions internally. It will help us find and prevent our bugs and increase our developer productivity
2. Provide guidance to developers who are using ASP.NET Core about which APIs can accept and return a null reference and which APIs can't. This would improve the developer experience of using ASP.NET Core

@rynowak @davidfowl @DamianEdwards @Eilon

---

Projects in no particular order

• Microsoft.AspNetCore.DataProtection.EntityFrameworkCore
• Microsoft.AspNetCore.DataProtection.StackExchangeRedis
• Microsoft.AspNetCore.TestHost
• Microsoft.AspNetCore.Hosting.WindowsServices
• Microsoft.AspNetCore.ApiAuthorization.IdentityServer
• Microsoft.AspNetCore.Identity.EntityFrameworkCore
• Microsoft.AspNetCore.Identity.Specification.Tests
• Microsoft.AspNetCore.Identity.UI
• Microsoft.AspNetCore.Authentication.Certificate
• Microsoft.AspNetCore.Authentication.Facebook
• Microsoft.AspNetCore.Authentication.Google
• Microsoft.AspNetCore.Authentication.JwtBearer
• Microsoft.AspNetCore.Authentication.MicrosoftAccount
• Microsoft.AspNetCore.Authentication.Negotiate
• Microsoft.AspNetCore.Authentication.OpenIdConnect
• Microsoft.AspNetCore.Authentication.Twitter
• Microsoft.AspNetCore.Authentication.WsFederation
• Microsoft.Extensions.Logging.AzureAppServices
• Microsoft.AspNetCore.ConcurrencyLimiter
• Microsoft.AspNetCore.Diagnostics.EntityFrameworkCore
• Microsoft.AspNetCore.HeaderPropagation
• Microsoft.Extensions.Diagnostics.HealthChecks.EntityFrameworkCore
• Microsoft.AspNetCore.MiddlewareAnalysis
• Microsoft.AspNetCore.SpaServices.Extensions
• Microsoft.AspNetCore.Mvc.NewtonsoftJson
• Microsoft.AspNetCore.Mvc.Razor.RuntimeCompilation
• Microsoft.AspNetCore.Mvc.Testing
• Microsoft.AspNetCore.AzureAppServicesIntegration
• Microsoft.AspNetCore.SignalR.Client.Core
• Microsoft.AspNetCore.SignalR.Client
• Microsoft.AspNetCore.Http.Connections.Client
• Microsoft.AspNetCore.SignalR.Protocols.MessagePack
• Microsoft.AspNetCore.SignalR.Protocols.NewtonsoftJson
• Microsoft.AspNetCore.SignalR.Specification.Tests
• Microsoft.AspNetCore.SignalR.StackExchangeRedis
• Microsoft.Authentication.WebAssembly.Msal
• Microsoft.JSInterop.WebAssembly
• Microsoft.AspNetCore.Components.WebAssembly.Server
• Microsoft.AspNetCore.Components.WebAssembly.Authentication
• Microsoft.AspNetCore.Components.WebAssembly
• Microsoft.AspNetCore
• Microsoft.AspNetCore.DataProtection.Abstractions
• Microsoft.AspNetCore.Cryptography.Internal
• Microsoft.AspNetCore.Cryptography.KeyDerivation
• Microsoft.AspNetCore.DataProtection
• Microsoft.AspNetCore.DataProtection.Extensions
• Microsoft.AspNetCore.Antiforgery
• Microsoft.AspNetCore.Hosting.Abstractions
• Microsoft.AspNetCore.Hosting
• Microsoft.AspNetCore.Hosting.Server.Abstractions
• Microsoft.AspNetCore.Authentication.Abstractions
• Microsoft.AspNetCore.Authentication.Core
• Microsoft.Net.Http.Headers
• Microsoft.AspNetCore.Http.Abstractions
• Microsoft.AspNetCore.Http.Extensions
• Microsoft.AspNetCore.Http.Features
• Microsoft.AspNetCore.Http
• Microsoft.AspNetCore.Metadata
• Microsoft.AspNetCore.Routing.Abstractions
• Microsoft.AspNetCore.Routing
• Microsoft.AspNetCore.WebUtilities
• Microsoft.AspNetCore.Html.Abstractions
• Microsoft.AspNetCore.Identity
• Microsoft.Extensions.Identity.Core
• Microsoft.Extensions.Identity.Stores
• Microsoft.AspNetCore.Connections.Abstractions
• Microsoft.AspNetCore.Server.HttpSys
• Microsoft.AspNetCore.Server.IISIntegration
• Microsoft.AspNetCore.Server.IIS
• Microsoft.AspNetCore.Server.Kestrel.Core
• Microsoft.AspNetCore.Server.Kestrel
• Microsoft.AspNetCore.Server.Kestrel.Transport.Sockets
• Microsoft.AspNetCore.Authentication.Cookies
• Microsoft.AspNetCore.Authentication
• Microsoft.AspNetCore.Authentication.OAuth
• Microsoft.AspNetCore.Authorization
• Microsoft.AspNetCore.Authorization.Policy
• Microsoft.AspNetCore.CookiePolicy
• Microsoft.AspNetCore.Cors
• Microsoft.AspNetCore.Diagnostics.Abstractions
• Microsoft.AspNetCore.Diagnostics
• Microsoft.AspNetCore.Diagnostics.HealthChecks
• Microsoft.AspNetCore.HostFiltering
• Microsoft.AspNetCore.HttpOverrides
• Microsoft.AspNetCore.HttpsPolicy
• Microsoft.AspNetCore.Localization.Routing
• Microsoft.AspNetCore.Localization
• Microsoft.AspNetCore.ResponseCaching.Abstractions
• Microsoft.AspNetCore.ResponseCaching
• Microsoft.AspNetCore.ResponseCompression
• Microsoft.AspNetCore.Rewrite
• Microsoft.AspNetCore.Session
• Microsoft.AspNetCore.StaticFiles
• Microsoft.AspNetCore.WebSockets
• Microsoft.AspNetCore.Razor.Runtime
• Microsoft.AspNetCore.Razor
• Microsoft.AspNetCore.Mvc.Abstractions
• Microsoft.AspNetCore.Mvc.ApiExplorer
• Microsoft.AspNetCore.Mvc.Core
• Microsoft.AspNetCore.Mvc.Cors
• Microsoft.AspNetCore.Mvc.DataAnnotations
• Microsoft.AspNetCore.Mvc.Formatters.Json
• Microsoft.AspNetCore.Mvc.Formatters.Xml
• Microsoft.AspNetCore.Mvc.Localization
• Microsoft.AspNetCore.Mvc.RazorPages
• Microsoft.AspNetCore.Mvc.Razor
• Microsoft.AspNetCore.Mvc.TagHelpers
• Microsoft.AspNetCore.Mvc.ViewFeatures
• Microsoft.AspNetCore.Mvc
• Microsoft.AspNetCore.Http.Connections.Common
• Microsoft.AspNetCore.Http.Connections
• Microsoft.AspNetCore.SignalR.Protocols.Json
• Microsoft.AspNetCore.SignalR.Common
• Microsoft.AspNetCore.SignalR.Core
• Microsoft.AspNetCore.SignalR
• Microsoft.AspNetCore.Components.Authorization
• Microsoft.AspNetCore.Components
• Microsoft.AspNetCore.Components.Forms
• Microsoft.AspNetCore.Components.Server
• Microsoft.AspNetCore.Components.Web
• Microsoft.Extensions.FileProviders.Embedded
• Microsoft.Extensions.Caching.SqlServer
• Microsoft.Extensions.Caching.StackExchangeRedis
• Microsoft.Extensions.Configuration.KeyPerFile
• Microsoft.Extensions.Localization.Abstractions
• Microsoft.Extensions.Localization
• Microsoft.Extensions.ObjectPool
• Microsoft.JSInterop
• Microsoft.Extensions.WebEncoders
• Microsoft.Extensions.Diagnostics.HealthChecks.Abstractions
• Microsoft.Extensions.Diagnostics.HealthChecks

Skipped (potentially no longer commonly used, potential future removal, only used in tests, internal project, etc)

• Microsoft.AspNetCore.Owin
• Microsoft.AspNetCore.Testing
• Microsoft.AspNetCore.Server.Kestrel.Transport.Libuv
• Microsoft.AspNetCore.Authentication.AzureAD.UI
• Microsoft.AspNetCore.Authentication.AzureADB2C.UI
• Microsoft.AspNetCore.JsonPatch
• Microsoft.AspNetCore.NodeServices
• Microsoft.AspNetCore.SpaServices

Skipped (not for public consumption)

• Microsoft.AspNetCore.DeveloperCertificates.XPlat

[rynowak]

I hereby certify this idea as good.

[Eilon]

Assigning to @JamesNK to give this some more thought on what can concretely be done in ASP.NET Core and what value it would provide.

[JamesNK]

Over Christmas I'm going to play around with C# 8.0 and update Newtonsoft.Json to use nullable reference types. Will be able to give some more feedback then about how ready the feature is and much work it will be to use it. I suspect it will be large.

Some random thoughts:

1. It will probably require VS2019. VS2017 won't understand the syntax and will no longer compile
2. Lower level libraries should be updated first, e.g. Http should be updated before MVC so that MVC doesn't need to unnecessarily assert that types that come from Http are not null

[rynowak]

It will probably require VS2019. VS2017 won't understand the syntax and will no longer compile

If this is the case, then we'd probably want to wait until 2019 is RTM. We try to avoid requiring that contributors have previews of stuff. But given all of the other stuff going on in 3.0 it's possible we might need to require that for other reasons.

[rynowak]

@Eilon thanks for creating that label. As you know I am unable to create labels.

[JamesNK]

If you had certified it spicy then you would have this label to use

certified spicy 🌶️

🙃

[JamesNK]

Findings from converting Newtonsoft.Json over the break:

• a couple of days of work
• VS2019 Preview 1 null analysis is good but has some false positives. Should wait until it is closer to 3.0 before updating anything
• it will help prevent NREs
• the CI build should be updated to not fail on null reference related warnings. I think that as corefx and other dep libraries add support for nullable reference types then new warnings will automatically appear. Speaking of corefx, does anyone know if they have a plan for supporting nullable metadata in 3.0?

[Eilon]

@JamesNK - maybe start with @joshfree to see if his team is planning to take advantage of this.

[rynowak]

the CI build should be updated to not fail on null reference related warnings. I think that as corefx and other dep libraries add support for nullable reference types then new warnings will automatically appear.

This is concerning because we all really enjoy warnings as errors. I think it in this case it would probably be fine to support this guy into the libraries get stables.

Something interesting about this is that you get a new flavour of the classic warnings problem. Adding a warning is always a breaking change. Is nullability part of netstandard2.0? Can it ever change?

[JamesNK]

This is concerning because we all really enjoy warnings as errors.

Totes, warning as errors is the best. But there is a way to allow-list (@Eilon 😋) some warnings. See WarningsNotAsErrors - https://github.com/dotnet/project-system/pull/4404/files#diff-6483fde6c45c90fdfe98ca72eadfa651

[JamesNK]

I discussed this with Mads and Immo. The corefx approach is to have a sidecar file format containing metadata. The file format should be usable by all packages.

Some reasons for this approach:

1. It can be applied to older versions of the framework, including .NET Framework, without having to update code
2. It is faster and easier to add nullable metadata to only the public API compared to updating all source code to use nullable reference types.

I don't think (1) is much of a concern for us. We'd be happy including nullable metadata from the latest version going forward.

(2) would be much faster for us. Disadvantage is we'd miss out on catching nulls in our own code, and there would be ongoing upkeep of the sidecar file. When a new API is added then the nullable metadata would also need to be added to the file.

[mkArtakMSFT]

Moving this to backlog as we have no capacity to handle this in 3.0 release.

[magnusbakken]

Now that C#8 and VS16.3, etc. have been officially released, will you start looking into this again?

[fubar-coder]

Any plans for this feature? I currently have to work around this issue when I implement my own IConnectionUserFeature (and others).

[magnusbakken]

The main place this becomes an annoyance is when you're working in a project with Nullable=true and implementing an ASP.NET Core interface/abstract class with methods that ought to have question marks in them, but of course don't.

I think the only place I've actually run into myself is in IUserStore<T>. The methods FindByIdAsync and FindByNameAsync need to be able to return null if the user isn't found, but since the interface returns Task<T> the compiler won't allow it without a type assertion. In my implementation I basically just do if (user == null) return null!; The construct null! is pretty bizarre, but at least it's obvious when you look at it that it's a temporary workaround.

When you're calling an ASP.NET Core method that ought to return Foo? but actually returns Foo you just have to remember to check if it's null anyway, but with interfaces it's a little worse.

[fubar-coder]

My workaround is to temporarily disable NRT and restore it again, like this:

class YourImpl : IConnectionUserFeature {
#nullable disable
    public ClaimsPrincipal User { get; set; }
#nullable restore
}

It works, but the need to disable NRT checks is not what I expected.

[ghost]

Thanks for contacting us.

We're moving this issue to the Next sprint planning milestone for future evaluation / consideration. We would like to keep this around to collect more feedback, which can help us with prioritizing this work. We will re-evaluate this issue, during our next planning meeting(s).
If we later determine, that the issue has no community involvement, or it's very rare and low-impact issue, we will close it - so that the team can focus on more important and high impact issues.
To learn more about what to expect next and how this issue will be handled you can read more about our triage process here.

[mkArtakMSFT]

Moving to area-identity as that's where next set of changes need to happen

[JamesNK]

I split the remaining projects that are missing nullability into separate issues. Closing this issue.