Add NullableAttribute

[terrajobst]

Right now, the C# compiler code spits the attribute. We should add expose them to avoid having to spread them everywhere.

namespace System.Runtime.CompilerServices
{
    [AttributeUsage(
        AttributeTargets.Class |
        AttributeTargets.GenericParameter |
        AttributeTargets.Event |
        AttributeTargets.Field |
        AttributeTargets.Property |
        AttributeTargets.Parameter |
        AttributeTargets.ReturnValue)]
    public sealed class NullableAttribute : Attribute
    {
        public NullableAttribute(byte flag);
        public NullableAttribute(byte[] flags);
        public ReadOnlySpan<byte> Flags { get; }
    }
}

/cc @dotnet/ldm @dotnet/fxdc

[jcouv]

There will likely be other attributes for annotating APIs:

• [NotNullWhenTrue] (e.g. TryGetValue) and [NotNullWhenFalse] (e.g. string.IsNullOrEmpty)
• [EnsuresNotNull] (e.g. ThrowIfNull)
• [AssertsTrue] (e.g. Debug.Assert) and [AssertsFalse]

We may add a couple more (we're discussing [MaybeNull] and a couple more)

[terrajobst]

@jcouv @jaredpar any reason why we shouldn't add the NullableAttribute to CoreFX? If so, would you mind submitting a PR or at least provide the API surface?

Could you double check that the above API shape works for you?

[jcouv]

The API shapes looks good to me. Tagging @AlekseyTs @cston FYI

The only thing I would add is the attribute targets. Here's what our compiler doc captures:

namespace System.Runtime.CompilerServices
{
    [AttributeUsage(
        AttributeTargets.Class |
        AttributeTargets.GenericParameter |
        AttributeTargets.Event | AttributeTargets.Field | AttributeTargets.Property |
        AttributeTargets.Parameter | AttributeTargets.ReturnValue,
        AllowMultiple = false)]
    public sealed class NullableAttribute : Attribute
    {
        public readonly byte[] NullableFlags;
    
        public NullableAttribute(byte b) 
        {
            NullableFlags = new byte[] { b };
        }
        
        public NullableAttribute(byte[] b)
        {
            NullableFlags = b;
        }
    }
}

https://github.com/dotnet/roslyn/blob/master/docs/features/nullable-reference-types.md#annotations

[terrajobst]

I assume you don't care whether we expose the flags array as a field or property because you don't ever read it, right? You only care about the metadata passed to the .ctor, correct?

[jcouv]

@terrajobst Correct. The compiler only cares about the signature of the .ctors.
The purpose of storing the information in a field or property is to make it available for reflection. I think we do that for other compiler-related attributes and so probably should do the same here (in some shape or another).

[jcouv]

FYI the metadata format produced by the compiler is going to change in the next week or so. We'll be adding more attributes (NullableContextAttribute and NullableMembersAttribute).
More details forthcoming in dotnet/roslyn#36152

[terrajobst]

Video

We decided to keep doing the code spit because

1. Any code reasoning about the attribute has to assume that attribute is emitted, so you can't optimize the path.
2. The size is small

[jcouv]

Note: the compiler is going to ship with a very slightly different API than last discussed (see dotnet/roslyn#36604). The field is an array instead of a ReadOnlySpan and it is named NullableFlags.

[roji]

@jcouv just to confirm - are you guys still planning to do any more changes in the nullability metadata? Your comment above about the changes is more recent than the closed issue it linked to.

[agocke]

Nope, no more planned changes to metadata, only diagnostic adjustment.

[roji]

@agocke thanks for the confirmation!

[cston]

@roji, one correction to the spec: The compiler only emits NullableContextAttribute on method and type definitions, the compiler does not support a module-level NullableContextAttribute. The spec will be updated to reflect that in dotnet/roslyn#37610.

[roji]

Thanks @cston!