Remove all use of <Nullable>annotations</Nullable>

[stephentoub]

We've inadvertently shipped multiple libraries with incorrect nullable reference type annotations because: a) Their ref assemblies aren't actually being shipped, and/or b) Their projects are using <Nullable>annotations</Nullable>

<Nullable>annotations</Nullable> means "I'm nullable annotated but don't validate them", which means consumers of these libraries see annotations that we haven't thoughtfully added or reviewed.

This removes all use of it and gets us back to a place where we're only shipping nullable annotations for libraries where we've done the work to ensure they're correct. We can subsequently finish annotating these stragglers, for which I opened #90400.

Fixes #78036

[ghost]

Tagging subscribers to this area: @dotnet/area-infrastructure-libraries
See info in area-owners.md if you want to be subscribed.

Issue Details

---

We've inadvertently shipped multiple libraries with incorrect nullable reference type annotations because: a) Their ref assemblies aren't actually being shipped, and/or b) Their src projects using <Nullable>annotations</Nullable>

<Nullable>annotations</Nullable> means "I'm nullable annotated but don't validate them", which means consumers of these libraries see annotations that we haven't thoughtfully added or reviewed.

This removes all use of it and gets us back to a place where we're only shipping nullable annotations for libraries where we've done the work to ensure they're correct. We can subsequently finish annotating these stragglers.

Fixes #78036

Author:	stephentoub
Assignees:	-
Labels:	area-Infrastructure-libraries
Milestone:	8.0.0

[ericstj]

The build failures on allconfigurations are concerning. Interop source generator isn't running. Having a look at binlog.

[ericstj]

Interesting -- here's the cause:

runtime/eng/generators.targets

Lines 6 to 9 in 0dfe81a

	<!-- Disable the library import generator when the project requires polyfill source files but doesn't have nullable reference types enabled. -->
	<EnableLibraryImportGenerator Condition="'$(EnableLibraryImportGenerator)' == '' and
	!$([MSBuild]::IsTargetFrameworkCompatible('$(TargetFramework)', 'net7.0')) and
	('$(Nullable)' == 'disable' or '$(Nullable)' == '')">false</EnableLibraryImportGenerator>

@ViktorHofer could that permit the generator if nullable was disabled but the project explicitly included those polyfill files (via property)?

[stephentoub]

Thanks for tracking it down. Why are we disabling the generator in that case? Is it because the files in question have nullable annotations that would result in warnings when nullable is disabled? If that's the case, we should instead be suppressing the warnings with <NoWarn>nullable</NoWarn> when nullability is explicitly disabled rather than disabling the generator.

[ericstj]

Thanks for tracking it down. Why are we disabling the generator in that case? Is it because the files in question have nullable annotations that would result in warnings when nullable is disabled? If that's the case, we should instead be suppressing the warnings with <NoWarn>nullable</NoWarn> when nullability is explicitly disabled rather than disabling the generator.

Agreed. To add to that - I would expect the user gesture for using the source generator to actually address it with attribute usage - so even if the generator was included it ought to be a noop unless someone added those attributes.

[ViktorHofer]

@ViktorHofer could that permit the generator if nullable was disabled but the project explicitly included those polyfill files (via property)?

Why are we disabling the generator in that case? Is it because the files in question have nullable annotations that would result in warnings when nullable is disabled?

That's infrastructure owned by @jkoritzinsky. Jeremy, please take a look.

[jkoritzinsky]

The polyfill sources have been annotated with nullable reference types and we were getting errors on compiling them when nullable reference types were disabled. Does Roslyn allow using #nullable enable when nullable reference types are explicitly disabled and when they're explicitly enabled? If so, we can just annotate those files with #nullable enable and remove that condition.

[jkoritzinsky]

#nullable enable works here, so I've added it to the file. That should fix the build.

[stephentoub]

Build failure is dotnet/dnceng#450

[am11]

This was annotated in #67513. Removing <Nullable> and <NoWarn> from M.E.H.Systemd's ref and src csproj seems to build.

[stephentoub]

seems to build

That doesn't guarantee they're correct, though.

Someone should re-review the annotations to ensure they're correct, submitting a separate PR to flip it to enabled.

I'm surprised, for example, that there's no validation for the loggerFactory argument here:

runtime/src/libraries/Microsoft.Extensions.Hosting.Systemd/src/SystemdLifetime.cs

Line 32 in 71a3d36

public SystemdLifetime(IHostEnvironment environment, IHostApplicationLifetime applicationLifetime, ISystemdNotifier systemdNotifier, ILoggerFactory loggerFactory)

It'll null ref if someone passes null, something we try to avoid in general even for non-nullable parameters (and the other args are validated). Logging is generally also optional. Is that supposed to be a nullable factory and the ctor uses a default nop logger if it's null?

[stephentoub]

cc: @maxkoshevoi, @eerhardt

[eerhardt]

It looks like this library was nullable enabled in the middle of the !! changes.

!! was added on Feb 8, 2022:
3ae8739

Added nullable=enabled on April 18, 2022:
9849af2

!! was removed on April 21, 2022 (and also changed to nullable=annotations):
215b39a

I'll put up a PR to fix Systemd's nullability.