MetadataAsSource isn't aware of nullability

[jcouv]

The output from MetadataAsSource is lacking nullability annotations (?).
The [Nullable] attribute is displayed, instead of the ? in the proper location(s).

Note: we need to display #nullable enable/disable which can be tricky

FYI @dpoeschl

[jasonmalinowski]

@jcouv @333fred So adding the ?s back is easy enough, but what's the plan for the attribute? I had thought that generally the compiler took a tact that attributes that were processed by the compiler and reflected in the symbol API were dropped so things round tripped better, but it looks like we also spit the [Dynamic] attribute so maybe that's not the case. Any other recollections of how we represented things like this in the symbol API?

(I might be confusing this with the "special" attributes that don't become attributes but rather special bits in metadata, I don't recall.)

[RikkiGibson]

@jasonmalinowski Certain special attributes get "filtered out" by the compiler when you call e.g. PEMethodSymbol.GetAttributes().

See this helper method used to implement PEMethodSymbol.GetAttributes():

roslyn/src/Compilers/CSharp/Portable/Symbols/Metadata/PE/PEModuleSymbol.cs

Line 408 in f0c9a94

internal ImmutableArray<CSharpAttributeData> GetCustomAttributesFilterCompilerAttributes(EntityHandle token, out bool foundExtension, out bool foundReadOnly)

It seems like there are some other compiler-aware special attributes that don't get filtered out in MetadataAsSource. I think NullableAttribute should get filtered out for method, property, field, event, and parameter symbols in order to be a "best effort" at replicating the original source member signatures.

[jasonmalinowski]

Yeah, I see we don't filter out dynamic either; I wasn't sure what the intended compiler design is here. It's not hard for us to filter on the IDE side of things and I don't mind doing it....I just didn't recall if it was the compiler's job to do it in the first place and we wanted to keep it consistent.

[jcouv]

My take is that we shouldn't show [Nullable(...)] in metadata-as-source, only annotations because we have actual language syntax to represent them (although oblivious vs. not-annotated is a bit tricky/verbose).
In my mind [Dynamic] should also be filtered out. Not sure why it wasn't.
We can discuss whether such filtering should be done in the compiler or the IDE layer.

[jasonmalinowski]

And yes, it absolutely should be filtered out; I was surprised to see the [Dynamic] issue and wasn't sure if that was an oversight or something got broken.

[jasonmalinowski]

@jcouv Did some filtering ultimately get done in the compiler layer? I'm seeing some bugs fixed I didn't expect to see fixed.

[jasonmalinowski]

Ah I think I see what happened: I had some scenarios where NullableAttributes weren't be applied because NullableContextAttributes were, and we were incorrectly dropping those.

[jasonmalinowski]

(well incorrectly in the sense of "we seem to be dropping attributes of all kinds, including NullableContextAttributes")

[jasonmalinowski]

Where this is now:

We correctly spit ? in the various places, but we don't spit anything to give you a good hint of whether the original source was a #nullable enable or #nullable disable context. We need to handle things like that for other scenarios as well, so this will still need updating accordingly. We also still spit the attributes as well which is annoying but something we'll be cleaning up in 16.4.

[RikkiGibson]

It looks like we still spit a NullableAttribute for some nested nullability scenarios.

I agree that providing synthetic #nullable enable directives instead of NullableContextAttribute would go a long way toward providing clarity.

Dictionary MetadataAsSource snippet

    //
    // Summary:
    //     Represents a collection of keys and values.
    //
    // Type parameters:
    //   TKey:
    //     The type of the keys in the dictionary.
    //
    //   TValue:
    //     The type of the values in the dictionary.
    [DefaultMember("Item")]
    [NullableAttribute(0)]
    [NullableContextAttribute(1)]
    public class Dictionary<TKey, [NullableAttribute(2)]
    TValue> : ICollection<KeyValuePair<TKey, TValue>>, IEnumerable<KeyValuePair<TKey, TValue>>, IEnumerable, IDictionary<TKey, TValue>, IReadOnlyCollection<KeyValuePair<TKey, TValue>>, IReadOnlyDictionary<TKey, TValue>, ICollection, IDictionary, IDeserializationCallback, ISerializable where TKey : notnull
    {
        //
        // Summary:
        //     Initializes a new instance of the System.Collections.Generic.Dictionary`2 class
        //     that is empty, has the default initial capacity, and uses the default equality
        //     comparer for the key type.
        public Dictionary();
        //
        // Summary:
        //     Initializes a new instance of the System.Collections.Generic.Dictionary`2 class
        //     that contains elements copied from the specified System.Collections.Generic.IDictionary`2
        //     and uses the default equality comparer for the key type.
        //
        // Parameters:
        //   dictionary:
        //     The System.Collections.Generic.IDictionary`2 whose elements are copied to the
        //     new System.Collections.Generic.Dictionary`2.
        //
        // Exceptions:
        //   T:System.ArgumentNullException:
        //     dictionary is null.
        //
        //   T:System.ArgumentException:
        //     dictionary contains one or more duplicate keys.
        public Dictionary(IDictionary<TKey, TValue> dictionary);
        //
        // Parameters:
        //   collection:
        public Dictionary([NullableAttribute(new[] { 1, 0, 1, 1 })] IEnumerable<KeyValuePair<TKey, TValue>> collection);
        //
        // Summary:
        //     Initializes a new instance of the System.Collections.Generic.Dictionary`2 class
        //     that is empty, has the default initial capacity, and uses the specified System.Collections.Generic.IEqualityComparer`1.
        //
        // Parameters:
        //   comparer:
        //     The System.Collections.Generic.IEqualityComparer`1 implementation to use when
        //     comparing keys, or null to use the default System.Collections.Generic.EqualityComparer`1
        //     for the type of the key.
        public Dictionary([NullableAttribute(new[] { 2, 1 })] IEqualityComparer<TKey>? comparer);
        //
        // Summary:
        //     Initializes a new instance of the System.Collections.Generic.Dictionary`2 class
        //     that is empty, has the specified initial capacity, and uses the default equality
        //     comparer for the key type.
        //
        // Parameters:
        //   capacity:
        //     The initial number of elements that the System.Collections.Generic.Dictionary`2
        //     can contain.
        //
        // Exceptions:
        //   T:System.ArgumentOutOfRangeException:
        //     capacity is less than 0.
        public Dictionary(int capacity);
        //
        // Summary:
        //     Initializes a new instance of the System.Collections.Generic.Dictionary`2 class
        //     that contains elements copied from the specified System.Collections.Generic.IDictionary`2
        //     and uses the specified System.Collections.Generic.IEqualityComparer`1.
        //
        // Parameters:
        //   dictionary:
        //     The System.Collections.Generic.IDictionary`2 whose elements are copied to the
        //     new System.Collections.Generic.Dictionary`2.
        //
        //   comparer:
        //     The System.Collections.Generic.IEqualityComparer`1 implementation to use when
        //     comparing keys, or null to use the default System.Collections.Generic.EqualityComparer`1
        //     for the type of the key.
        //
        // Exceptions:
        //   T:System.ArgumentNullException:
        //     dictionary is null.
        //
        //   T:System.ArgumentException:
        //     dictionary contains one or more duplicate keys.
        public Dictionary(IDictionary<TKey, TValue> dictionary, [NullableAttribute(new[] { 2, 1 })] IEqualityComparer<TKey>? comparer);
        //
        // Parameters:
        //   collection:
        //
        //   comparer:
        public Dictionary([NullableAttribute(new[] { 1, 0, 1, 1 })] IEnumerable<KeyValuePair<TKey, TValue>> collection, [NullableAttribute(new[] { 2, 1 })] IEqualityComparer<TKey>? comparer);
        //
        // Summary:
        //     Initializes a new instance of the System.Collections.Generic.Dictionary`2 class
        //     that is empty, has the specified initial capacity, and uses the specified System.Collections.Generic.IEqualityComparer`1.
        //
        // Parameters:
        //   capacity:
        //     The initial number of elements that the System.Collections.Generic.Dictionary`2
        //     can contain.
        //
        //   comparer:
        //     The System.Collections.Generic.IEqualityComparer`1 implementation to use when
        //     comparing keys, or null to use the default System.Collections.Generic.EqualityComparer`1
        //     for the type of the key.
        //
        // Exceptions:
        //   T:System.ArgumentOutOfRangeException:
        //     capacity is less than 0.
        public Dictionary(int capacity, [NullableAttribute(new[] { 2, 1 })] IEqualityComparer<TKey>? comparer);
        //
        // Summary:
        //     Initializes a new instance of the System.Collections.Generic.Dictionary`2 class
        //     with serialized data.
        //
        // Parameters:
        //   info:
        //     A System.Runtime.Serialization.SerializationInfo object containing the information
        //     required to serialize the System.Collections.Generic.Dictionary`2.
        //
        //   context:
        //     A System.Runtime.Serialization.StreamingContext structure containing the source
        //     and destination of the serialized stream associated with the System.Collections.Generic.Dictionary`2.
        protected Dictionary(SerializationInfo info, StreamingContext context);

[CyrusNajmabadi]

Where did we land on this @RikkiGibson @jcouv @jasonmalinowski ? Is compiler supposed to strip this from teh API (that would be my preference), or will the attributes be there, and the consumers need to ignore them because they're redundant?

[jasonmalinowski]

@CyrusNajmabadi I don't think we decided; until somebody wanted to actually do the much more complicated bit of sticking the #nullable enable things everywhere this is just on the backlog.

[RikkiGibson]

I'm honestly not sure. If we strip the NullableAttribute and just ensure that metadata type symbols have the correct nullable annotations, we get a lot of the way there for showing when types are nullable. But if we also strip the NullableContextAttribute for consistency, I don't see how MetadataAsSource will be able to insert #nullable enable directives in the appropriate spots to show whentypes are non-nullable.

Is there some kind of tag that could be placed on the syntax that would indicate the equivalent to "this node is in a #nullable enable context`? Then MetadataAsSource could keep a stack of nullable contexts as it looks through the declaration tree.

[CyrusNajmabadi]

Ok, talked to @RikkiGibson offline about this. There should be enough information in the type-symbols for what we're generating to be able to figure this out. Similar to how we have to look at the types to know if we have to add the unsafe modifier, since htat's not actually in a symbol.