runtime: Some incorrect NRT annotations in System.CodeDom

we’ll bounce this back to the CodeDom area (after confirming other areas aren’t also in a bad state)

There’s a 0% chance that the others are all fine.

All of these src projects use <Nullable>annotations</Nullable>:

d:\repos\runtime\src\libraries\Microsoft.Extensions.Hosting.Systemd\src\Microsoft.Extensions.Hosting.Systemd.csproj(9):<Nullable>annotations</Nullable>
d:\repos\runtime\src\libraries\System.CodeDom\src\System.CodeDom.csproj(6):<Nullable>annotations</Nullable>
d:\repos\runtime\src\libraries\System.Diagnostics.EventLog\src\System.Diagnostics.EventLog.csproj(5):<Nullable>annotations</Nullable>
d:\repos\runtime\src\libraries\System.Diagnostics.PerformanceCounter\src\System.Diagnostics.PerformanceCounter.csproj(5):<Nullable>annotations</Nullable>
d:\repos\runtime\src\libraries\System.DirectoryServices.AccountManagement\src\System.DirectoryServices.AccountManagement.csproj(8):<Nullable>annotations</Nullable>
d:\repos\runtime\src\libraries\System.DirectoryServices.Protocols\src\System.DirectoryServices.Protocols.csproj(7):<Nullable>annotations</Nullable>
d:\repos\runtime\src\libraries\System.IO.Ports\src\System.IO.Ports.csproj(7):<Nullable>annotations</Nullable>
d:\repos\runtime\src\libraries\System.Management\src\System.Management.csproj(7):<Nullable>annotations</Nullable>
d:\repos\runtime\src\libraries\System.Runtime.Caching\src\System.Runtime.Caching.csproj(5):<Nullable>Annotations</Nullable>
d:\repos\runtime\src\libraries\System.Speech\src\System.Speech.csproj(9):<Nullable>annotations</Nullable>

<Nullable>annotations</Nullable> is dangerous… it says “my annotations are all correct; use them but don’t validate them”. That’s ok as a stopgap if the library won’t be directly referenced, but if it is, it means every reference type showing up as a return type is a potential false negative (because reference types become non-nullable and the compiler won’t warn you if you’re returning null) and every reference type showing up as a parameter is a potential false positive (because reference types become non-nullable so a consumer passing null will get warnings even if null is actually permitted by the API). It’s trivial to find cases where that happens, e.g. MemoryCache’s indexer returns null if the key isn’t in the cache, yet it’s annotated as returning the non-nullable object, so there’s no warning when trying to dereference it even though it’ll null ref at run-time:

using System.Reflection;
using System.Runtime.Caching;

var nc = new NullabilityInfoContext();
NullabilityInfo ni = nc.Create(typeof(MemoryCache).GetProperty("Item")!);
Console.WriteLine(ni.ReadState); // prints "NotNull"

var cache = new MemoryCache("something");
object result = cache["item"];
Console.WriteLine(result.GetHashCode()); // NRE

I see four reasonable fixes here for .NET 8:

1. Correctly annotate all of these libraries. This is the right fix, but it would also take a small army to get it done correctly for .NET 8. It’d be the same process we previously employed for annotating libraries, doing so for each of these 10.
2. Ship the reference assemblies for these. Most of the refs here also have an incorrect context set, either implicitly defaulting to enable or explicitly having annotations, and all of those would need to switch to disable as part of this.
3. Mark all of the src projects explicitly as <Nullable>disable</Nullable> and make the code compile with liberal use of #pragma warning disable/restore.
4. Some combination of (1)/(3), doing (1) where it’s easy to get it done and (3) for the rest.

I’ve just started the work to fully annotate the System.CodeDom library

For v8, I don’t think there is enough time to fully annotate and review System.CodeDom - RC1 snap is in 3 days. However, a fully annotated CodeDom would be welcome in v9.

I assume that https://github.com/dotnet/runtime/pull/90393 is not needed for v8 when https://github.com/dotnet/runtime/pull/90401 is in.

That’d be very welcome, @halgab, thanks.

I’ve just started the work to fully annotate the System.CodeDom library. Would the team be interested in such a contribution?

Things like the Workflow compiler, SGEN, and other design time tools in VS load references with reflection - which loads assemblies for execution and fails with reference assemblies. Numerous reports about broken tools here are what led us to eventually just remove reference assemblies from packages. We also previously discussed with Roslyn/Nuget if it ever makes sense to make a public way to include reference assemblies in packages and the conclusion was no. IMO it doesn’t make sense to add back shipping reference assemblies here - they’ve been gone for multiple releases and the regressions we’d bring to design time experiences are worse than whatever nullable problems it might fix.

I agree with @stephentoub’s suggestion to go with option 4. That seems like the best way to get us back to what we shipped in 7.0 while still adding some incremental value.

I don’t see an infrastructure fault here

It depends what we want the fix to be, even if a temporary fix for 8. A valid option is adding back a reference assembly. It’s not the only option, obviously.