Some incorrect NRT annotations in System.CodeDom

[MaceWindu]

Description

Looks like System.CodeDom 7.0.0 enabled or updated NRT annotations and now we get build errors:

• CodePrimitiveExpression.ctor(object) should be CodePrimitiveExpression.ctor(object?), test
• object CodePrimitiveExpression.Value should be object? CodePrimitiveExpression.Value, test
• set of Executor. ExecWaitWithCapture methods with ref string parameters which should be ref string?, test

Could be more, but that's all we encountered during build of our code. I see tests already check for null values so it is possible to use NRT warnings from test code to detect more issues in this area.

Reproduction Steps

see referenced existing tests that test affected APIs with null

Expected behavior

No NRT errors

Actual behavior

CS8601, CS8604 errors on build

Regression?

No response

Known Workarounds

No response

Configuration

No response

Other information

No response

[stephentoub]

I'm surprised you're getting warnings... the ref assembly for System.CodeDom explicitly disables nullable reference types:

runtime/src/libraries/System.CodeDom/ref/System.CodeDom.csproj

Line 4 in 2a65e07

<Nullable>disable</Nullable>

and does not contain any annotations:
https://github.com/dotnet/runtime/blob/release/7.0/src/libraries/System.CodeDom/ref/System.CodeDom.cs

@ViktorHofer, do we have a package authoring problem here? It looks like the nuget package doesn't contain a ref assembly, only the library, which hasn't been fully annotated yet and isn't ready for it to have its annotations consumed.

[ViktorHofer]

@ViktorHofer, do we have a package authoring problem here? It looks like the nuget package doesn't contain a ref assembly, only the library, which hasn't been fully annotated yet and isn't ready for it to have its annotations consumed.

@stephentoub dotnet/runtime libraries packages don't contain reference assemblies anymore since .NET 6 RC1. I wasn't involved in that change directly, but there were good reasons for it and @ericstj can for sure enumerate them.

I called this specific issue out in March 2022, #66090:

Since .NET 6, reference assemblies aren't part of our out-of-band packages anymore. That means that our customers when consuming our packages, bind/compile against the package's implementation assembly and not its reference assembly. Therefore, the API surface area that we carefully maintain via the reference source files isn't the actual API surface area that our customers target.

Also related: #58163

[stephentoub]

dotnet/runtime libraries packages don't contain reference assemblies anymore since .NET 6 RC1.

Then we have a real problem here that may extend beyond codedom. The annotations (or lack thereof) currently in the codedom src are haphazard and not intended for consumption.

[ViktorHofer]

Agreed that this should be fixed. These are the libraries that have a different nullable reference type compiler setting between the non-shipping contract and the implementation:

• System.CodeDom
• System.Diagnostics.EventLog
• System.Diagnostics.PerformanceCounter
• System.DirectoryServices.AccountManagement
• System.Management

[stephentoub]

I'd expect we'd have a possible issue with any library src project using <Nullable>annotations</Nullable>, since whether or not its ref assembly agrees, it hasn't been validated fully enough to switch it over to <Nullable>enabled</Nullable>. That would also cover:

• Microsoft.Extensions.Hosting.Systemd
• System.DirectoryServices.Protocols
• System.IO.Ports
• System.Runtime.Caching
• System.Speech

[ericstj]

@eerhardt seems to be a result of #68102, was there a reason why we left NRT enabled for these src assemblies?

[eerhardt]

seems to be a result of #68102, was there a reason why we left NRT enabled for these src assemblies?

This wasn't the result of #68102. Before that change, the ref\System.CodeDom.csproj and the src\System.CodeDom.csproj projects differed in their nullable settings. Using the parent commit of that change to view the settings:

(note before that change, not having Nullable in the .csproj meant disable):

runtime/src/libraries/System.CodeDom/ref/System.CodeDom.csproj

Lines 1 to 6 in 8b6830a

	<Project Sdk="Microsoft.NET.Sdk">
	<PropertyGroup>
	<TargetFrameworks>$(NetCoreAppCurrent);$(NetCoreAppMinimum);netstandard2.0;$(NetFrameworkMinimum)</TargetFrameworks>
	</PropertyGroup>
	<ItemGroup>

runtime/src/libraries/System.CodeDom/src/System.CodeDom.csproj

Lines 2 to 6 in 8b6830a

	<PropertyGroup>
	<AllowUnsafeBlocks>true</AllowUnsafeBlocks>
	<DefineConstants>$(DefineConstants);CODEDOM</DefineConstants>
	<TargetFrameworks>$(NetCoreAppCurrent);$(NetCoreAppMinimum);netstandard2.0;$(NetFrameworkMinimum)</TargetFrameworks>
	<Nullable>annotations</Nullable>

The intention of #68102 wasn't to change any project's nullable setting. It was only to flip the default value and remove <Nullable>enable</Nullable> from the majority of the projects.

[eerhardt]

Comparing the 6.0.0 and the 7.0.0 version, I see this difference in IL:

I'm trying to figure out exactly why that got added.

[stephentoub]

I assume it was my 896eb15 and my not realizing at the time we didn't ship the ref.

[eerhardt]

I assume it was my 896eb15 and my not realizing at the time we didn't ship the ref.

Yes, I think you're right there. I saw that change in the blame - but since it was August 2021, I assumed that was already in 6.0. Looking again, that commit didn't make 6.0.