Repository navigation
Some projects aren't enabled for nullable #41696
Description
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
Activity
terrajobst commentedon Sep 1, 2020
@jeffhandley please assign a dev to this so we can take this for approval.
safern commentedon Sep 1, 2020
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 commentedon Sep 1, 2020
System.Runtime.CompilerServices.Unsafe's implementation is IL, not C#.
ericstj commentedon Sep 1, 2020
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 commentedon Sep 1, 2020
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 commentedon Sep 1, 2020
I don't think so.
stephentoub commentedon Sep 1, 2020
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 commentedon Sep 1, 2020
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 commentedon Sep 1, 2020
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 commentedon Sep 1, 2020
These are the ones that are really missing nullable enable:
System.IO.Packaging: both annotated correctly, we missed to add enable on the ref: 518df4f
System.Resources.Extensions: Missing nullable enable on both src and ref: https://github.com/dotnet/runtime/pull/37597/files
terrajobst commentedon Sep 2, 2020
@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 commentedon Sep 2, 2020
Ok
eerhardt commentedon Sep 2, 2020
System.CodeDom shows up for one API:
This class is shared between
System.Private.DataContractSerializationandSystem.CodeDom. In the former it is internal, and public in the latter. When I annotatedSystem.Private.DataContractSerialization, I needed to makeArrayElementTypenullable, 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:
stephentoub commentedon Sep 2, 2020
It's not.