Add nullability to HtmlAbstractions

[pranavkm]

Addresses #5680

[JamesNK]

Nice.

Is there a reason you picked this project? Adding annotations from the bottom up, i.e. projects with few or no dependencies, will create the least churn. This project doesn't seem to have any dependencies so you might already be doing that 😄

[pranavkm]

Yeah, I picked it because it was fairly low on the stack. I was planning on looking at Antiforgery, Auth, and CORS middleware next.

[JamesNK]

Ok!

If you're going to start tackling nullability across the stack then I have a WIP PR that never got merged for low-level HTTP bits: #18084

[ghost]

Thank you for submitting this for API review. This will be reviewed by @dotnet/aspnet-api-review at the next meeting of the ASP.NET Core API Review group. Please ensure you take a look at the API review process documentation and ensure that:

• The PR contains changes to the reference-assembly that describe the API change. Or, you have included a snippet of reference-assembly-style code that illustrates the API change.
• The PR describes the impact to users, both positive (useful new APIs) and negative (breaking changes).
• Someone is assigned to "champion" this change in the meeting, and they understand the impact and design of the change.

[JamesNK]

PR looks good.

One process thing I'd like to sort out before starting to merge is what would happen if we decide at some point that we don't want nullable annotations for 5.0. For example, perhaps we run out of time and don't want a mix of annotated and not annotated.

Do we need a rollback strategy? Is there a configuration setting that can be applied to a project that allows nullable annotations during compilation but doesn't include them with the output assembly. A project wide setting would be much preferable to striping annotations out.

[halter73]

Do we need a rollback strategy? Is there a configuration setting that can be applied to a project that allows nullable annotations during compilation but doesn't include them with the output assembly. A project wide setting would be much preferable to striping annotations out.

That's a good question. Removing nullable reference types from our projects at the last second does sound annoying. @jaredpar Do you know of an easy way to remove nullable reference annotations from the compiled output of an entire project?

[dougbu]

Do we need a rollback strategy?

Couldn't we "just" revert all of the commits for nullability additions? In anything but an extreme emergency, I expect this shouldn't be horribad because other changes are much more likely to be in the middle of methods.

[jaredpar]

The simplest approach would be to do the following:

1. Change to <Nullable>disabled</Nullable>
2. Suppress the warning about using ? inside a disabled nullable context.

That is a pretty targeted change you could make to disable nullable annotations.

That being said why not just ship in a partially annotated state? Sure the customer doesn't get full value from nullability but they will get more value than what they're getting without any annotations. The only issue would be communicating to customers that you're not finished and future releases would potentially add more annotations.

This is the approach we're likely to take with the Roslyn APIs.

[JamesNK]

It is unlikely we'd disable annotations, being halfway done was a hypothetical reason, I just want to understand our options before making wide ranging changes to source code.

The approach you've mentioned sounds good.

[ghost]

Hello @pranavkm!

Because this pull request has the auto-merge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

[rynowak]

booooooooooo......... My jank-mode code from the 70s should live on.