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.

[ghost]

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

Issue Details

---

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

Author:	MaceWindu
Assignees:	-
Labels:	bug, area-Infrastructure-libraries, blocking-release, regression-from-last-release
Milestone:	8.0.0

[jeffhandley]

We should fix this in .NET 8 since it is a build time regression. I've moved this over to infrastructure though since this and a few other libraries behave differently from expected regarding the src vs. ref assemblies.

[ViktorHofer]

I've moved this over to infrastructure though since this and a few other libraries behave differently from expected regarding the src vs. ref assemblies.

@jeffhandley all dotnet/runtime nuget packages ship implementation assemblies only. Reference assemblies have been removed from packages between .NET 5 and .NET 6. This bug tracks improving the nullable annotations in System.CodeDom and potentially in other assemblies (see list above). I don't see an infrastructure fault here (please let me know if you disagree).

EDIT: Just moved the label back and removed "regression-from-last-release" as we shipped CodeDom in this state with .NET 7.

[ghost]

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

Issue Details

---

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

Author:	MaceWindu
Assignees:	-
Labels:	bug, area-System.CodeDom, blocking-release, regression-from-last-release
Milestone:	8.0.0

[stephentoub]

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.

[jeffhandley]

Yeah -- I'd like to get infrastructure guidance here on the best way to resolve this. If the resolution is to fix up the annotations, so be it, and we'll bounce this back to the CodeDom area (after confirming other areas aren't also in a bad state). But if we think (through the infrastructure perspective) that we should reintroduce a reference assembly, then we might need more infrastructure help resolving it.

[ViktorHofer]

But if we think (through the infrastructure perspective) that we should reintroduce a reference assembly, then we might need more infrastructure help resolving it.

From an infrastructure perspective, we don't want to bring back reference assemblies in our nuget packages. IIRC some tools didn't work well with them which ultimately led to their removal. @ericstj can you please provide more context on why reference assemblies were removed?

[stephentoub]

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.

[ViktorHofer]

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.

To expand a little bit on this, we would also need to change the resolveContract build infrastructure in dotnet/runtime to make sure that projects which reference one of these ten projects, bind against the reference assembly, instead of the implementation assembly. But if we would do that, we would probably re-introduce #63467, in some form.

[ericstj]

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.

[elachlan]

Winforms has been blocked a few times by this but I think it won't be until .Net9 until we are fully annotated at best. Mostly because it has been a community initiative.

I imagine the people working on winforms annotations will be willing to put in a bit of effort to help annotate the libraries we make use of.

[jeffhandley]

I'm unsure we have sufficient justification to try to go correctly annotate these 10 libraries right now. My recommendation would be for us to proceed with @stephentoub's Option 3 ("Mark all of the src projects explicitly as disable and make the code compile with liberal use of #pragma warning disable/restore"). That looks like the quickest path to get us back to where we were when we shipped net7.0.

Then we can consider whether or not #41720 for annotating other remaining libraries should be in scope for .NET 9 (right now it's in Future).

[stephentoub]

I think (3) is trivial... just add:

  <!-- TODO https://github.com/dotnet/runtime/issues/78036: Until this project is annotated, disable NRT -->
  <PropertyGroup>
    <Nullable>disable</Nullable>
    <NoWarn>$(NoWarn);nullable</NoWarn>
  </PropertyGroup>

to each project (instead of whatever <Nullable/> it may already have.

[halgab]

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

[stephentoub]

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

[steveharter]

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 #90393 is not needed for v8 when #90401 is in.