Pulling up an event to an interface should not add/reproduce the nullable annotation

[Tragetaschen]

Version Used:
16.4.1

Steps to Reproduce:

#nullable enable
using System;

public interface ITest
{
}

public class Test : ITest
{
    public event Action? E; // run "Pull 'E' up to 'ITest'" here
}

Expected Behavior:

 public interface ITest
 {
+    event Action E;
 }

Actual Behavior:

 public interface ITest
 {
+    event Action? E;
 }

[stephentoub]

I don't know why the annotation would be dropped here. All events are expected to support adding/removing null delegates, and upon implementing the interface, having the type be non-nullable will lead to warnings (and then if they're suppressed, likely false negatives upon invocation of the event).

[CyrusNajmabadi]

This is a bit of an interesting case in that i don't think a user externally could ever observe the nullability of these events. i.e. when accessing through the interface, you can't ever refer to the 'Action' value itself (in order to observe it's null/non-null-ness). All you can do is ever do += and -=, and tehse will always be safe.

So i would ask:

1. should we disallow/warn when using ? on event members in interfaces?
2. should we make the change the OP asks for.

All events are expected to support adding/removing null delegates, and upon implementing the interface, having the type be non-nullable will lead to warnings

Is this the case? I don't get any warnings with this @stephentoub 👍

    class Program
    {
        public void DoWork(ITest i)
        {
            Action? e = null;
            i.E += e;
        }
    }

    public interface ITest
    {
        event Action E;
    }

@jcouv for thoughts. I'm not sure how much of NRT+events is intentional, or just where we've landed today unintentionally.

[stephentoub]

Is this the case?

It's supposed to be.
#29839 (comment)
https://github.com/dotnet/csharplang/blob/master/meetings/2019/LDM-2019-07-17.md

[CyrusNajmabadi]

Hrm.. we may want to reconsider that. Esp. since we've now shipped with thsi behavior and likely could not change it. i.e. it seems like we do respect this:

but the += and -= are also nullable

We can't now null check these and introduce new warnigns (esp. on code which seems like it should be totally correct and would not throw).

I guess we should take this to LDM to see waht to do here. @jcouv do you want to drive?

[stephentoub]

Esp. since we've now shipped with thsi behavior and likely could not change it.

Shipped when? We've made plenty of changes for .NET 5 that introduce new nullable warnings. That was part of the deal for rolling out nullable reference types.

[stephentoub]

esp. on code which seems like it should be totally correct and would not throw

If it's declared to be non-nullable, throwing is perfectly valid behavior if null is passed in. Such non-nullability is simply considered bad form for events.

[jcouv]

We currently do very little nullability analysis of events (known gap). It's still possible to fix some of those for .NET 5.
Tracked by #29839

[CyrusNajmabadi]

Closing out due to lack of feedback here.