Runtime: Some projects aren't enabled for nullable

[terrajobst]

I did a pass to compare syntax of APIs as they are in reference assemblies and how they are in implementation assemblies. The full spreadsheet can be downloaded from here.

The spreadsheet only shows API differences if the reference assembly and the implementation assembly differ with their <Nullable>enable</Nullable> setting (i.e one defines NullableAttribute and one does not). This affects these assemblies:

• Microsoft.VisualBasic.Core
• System.CodeDom
• System.IO.Ports
• System.Runtime.CompilerServices.Unsafe
• System.Security.Permissions

The first one is expected because we decided to only annotate the reference assembly; the others aren't.

/cc @dotnet/nullablefc

[msftbot[bot]]

Tagging subscribers to this area: @safern, @viktorhofer
See info in area-owners.md if you want to be subscribed.

[terrajobst]

@jeffhandley please assign a dev to this so we can take this for approval.

[safern]

System.IO.Packaging has <Nullable>annotations</Nullable> on the source and the ref doesn't have <Nullable>enable</Nullable> we should update both as well to enable nullability on both.

[stephentoub]

System.Runtime.CompilerServices.Unsafe's implementation is IL, not C#.

[ericstj]

System.Runtime.CompilerServices.Unsafe's implementation is IL, not C#.

Good point, we could copy the annotations to the .il file from the ref. Is there value in such an excercise? It could help anything in the repo that consumes the implementation directly, though I'm not sure we have such a case.

[stephentoub]

System.IO.Ports's src is annotations because it has some shared files that use the syntax. The ref doesn't need that. That difference is fine.

[stephentoub]

Is there value in such an excercise?

I don't think so.

[stephentoub]

I don't see System.CodeDom's src or ref csproj containing Nullable, so I'm not sure why it's in the list. Same for System.Security.Permissions.

[terrajobst]

I don't see System.CodeDom's src or ref csproj containing Nullable, so I'm not sure why it's in the list. Same for System.Security.Permissions.

I don't read the project file, I only check whether only one of the assemblies (ref or impl) defines NullableAttribute. Looks like CodeTypeReference.ArrayElementType is annotated in impl only.

[stephentoub]

I don't read the project file, I only check whether only one of the assemblies (ref or impl) defines NullableAttribute.

That's not sufficient. Any assembly that includes a shared file that uses nullable syntax will have that emitted by the compiler (though it'll then exclude it if it's not public). It's only relevant if we want the annotations on all public API to show up, which we don't there, as the assembly wasn't fully annotated.

If you're saying we should put annotations in both just you satisfy your tool, that's one thing. But it doesn't mean it's necessary for correctness.

[safern]

These are the ones that are really missing nullable enable:

System.IO.Packaging: both annotated correctly, we missed to add enable on the ref: https://github.com/dotnet/runtime/commit/518df4f20d20f233727cba2fcc42603a31f750df
System.Resources.Extensions: Missing nullable enable on both src and ref: https://github.com/dotnet/runtime/pull/37597/files

[terrajobst]

@stephentoub

I’m not trying to make statements about correctness. The goal of this tool was merely to find discrepancies between ref and impl. My assumption was that we use a manual process to ensure these are intentional. This started because we found discrepancies that didn’t seem intentional. The heuristics like looking at differences in the presence of NullableAttribute were only meant to reduce the noise because we have a lot of discrepancies. We discussed this plan in the nullable teams channel and I ran & tweaked the tool based on @safern’s feedback.

The right long term plan seems to be in improving GenAPI and ApiCompat. And in a perfect world reference assemblies wouldn’t need as much manual tweaking as they do today b/c it’s prone to errors. A Roslyn based GenAPI might help there because it would allow reading the manual files and only generate the remainders.

[stephentoub]

The heuristics like looking at differences in the presence of NullableAttribute were only meant to reduce the noise

Ok

[eerhardt]

I don't see System.CodeDom's src or ref csproj containing Nullable, so I'm not sure why it's in the list.

System.CodeDom shows up for one API:

This class is shared between System.Private.DataContractSerialization and System.CodeDom. In the former it is internal, and public in the latter. When I annotated System.Private.DataContractSerialization, I needed to make ArrayElementType nullable, since it can be null. But System.CodeDom is not in netcoreapp, but it is a netstandard2.0 library, so it hasn't been annotated.

I believe we can ignore the System.CodeDom difference - it is expected. If we think this is an issue, we should be able to fix it by using #if:

```C#
public

if FEATURE_SERIALIZATION

    CodeTypeReference?

else

    CodeTypeReference

endif

    ArrayElementType { get; set; }

```

[stephentoub]

If we think this is an issue

It's not.