102

When working with C# 8 and the new non-nullable references, I realized that events are treated like fields. This means that they will cause a warning 90% of the time since they won't be initialized until someone subscribes to it.

Consider the following event:

public event EventHandler IdleTimeoutReached;

You get the following warning on the line of the constructor declaration.

CS8618 C# Non-nullable event is uninitialized. Consider declaring the event as nullable.

If you make it nullable, the warning disappears of course. However, this looks very weird to me.

public event EventHandler? IdleTimeoutReached;

An alternative would be assigning it a no-op, but this seems even worse.

public event EventHandler IdleTimeoutReached = new EventHandler((o, e) => { });

What's the correct way to handle this situation and get rid of the warning without just disabling it? Are there any official guidelines for this?

Improve this question
10
  • Look at nullable contexts at MSDN Commented Oct 14, 2019 at 12:54
  • 2
    Why is EventHandler? weird given that EventHandler is a delegate (reference type) that may acutally be uninitialized? Commented Oct 14, 2019 at 12:55
  • 7
    My analogy was like this: "If I don't come to your party, it won't be held right?" "No, of course it will still be held." Likewise my event still exists even if noone cares for it. This is of course not right because of the nature of c# events but from a very objective point of view, this seems like a considerable thought. I guess you just have to accept what events actually are and then it makes sense. Commented Oct 14, 2019 at 13:05
  • 3
    You must always take into account the possibility of the event being null, hence patterns like IdleTimeoutReached?.Invoke(). It would make no sense for the compiler to lie to you and say "no, it's fine" only to then have invoking the event actually fail because it's null. The fact that callers don't have to deal with this (because the += syntax takes care of it, and they're not allowed to get the value raw) is what's causing the confusion, but even to them the value really is null, and the delegate combination operator just transparently deals with that for them. Commented Oct 14, 2019 at 14:08
  • 2
    @MichaelPuckettII, there is a slightly more concise way to initialize event handler fields, described as "An alternate approach" by John Skeet back in 2015. In short, you can public event EventHandler SomeEvent = delegate {}. This initializer syntax works for any event type. Commented Oct 4, 2022 at 13:42

3 Answers 3

Reset to default
94

You should make the event nullable, because it is indeed nullable. It may look a bit strange but that's the correct way of expressing your intent.

It has always been necessary to check whether an event is null before raising it within the class. In this case nullable reference checking will warn you if you try to invoke it without checking for null.

    public class Publisher
    {
        public event EventHandler? IdleTimeoutReached;

        protected virtual void RaiseIdleTimeoutEvent()
        {
            // `IdleTimeoutReached` will be null if no subscribers; compiler gives warning about possible null reference
            IdleTimeoutReached.Invoke(this, EventArgs.Empty);

            // No compiler warning
            IdleTimeoutReached?.Invoke(this, EventArgs.Empty);
        }
    }

The nullable annotation really only adds value for the class invoking the event. It won't affect callers at all because the += / -= syntax takes care of correctly assigning/removing those delegates.

So you should use:

public event EventHandler? IdleTimeoutReached;

For events with EventArgs, use:

public event EventHandler<IdleTimeoutEventArgs>? IdleTimeoutReached;
Improve this answer
Sign up to request clarification or add additional context in comments.

7 Comments

It's weird to have to add ? to an event handler declaration. That would mean for over 25 years we've had these possible null reference exceptions lurking around uninitialized event handlers. Except...there is no concern for null event handlers, and no risk of a NullReferenceException because the += and -= take care of everything. Which means that the null code flow analysis doesn't understand that null isn't a problem in this case. This probably will get fixed with the API Annotations that they're writing for .NET Core first, and promise to add to .NET Framework in time.
@IanBoyd that's not true; you have always had to check events for null before raising them. The nullable event isn't for the subscribers (where += and -= does indeed take care of it); it's for the class you're invoking it from. See the OnRaiseCustomEvent method in Microsoft's code example; you must check if (raiseEvent != null) before invoking it. So yes, this possible NullReferenceException has been lurking the entire time.
Except that the warning isn't coming from where we "call" the event (which is already wrapped in a check for null). This error is happening on the class itself, just as the top of the class. It seems like it wants us to construct an EventHandler<T> object during object creation - but that's not needed, nor what any documentation says. The error is "Non-nullable event '%s' is uninitialized." Lets say i wanted to initialize it - there is no such thing. You can't initialize it even if you wanted to. So the warning is just weird.
@IanBoyd that's the same thing it does for all nullable reference types. When you see a warning "non-nullable x is uninitialized" there's the implied, "or make it nullable to leave it uninitialized". Agree their guidance and specific messaging here is lacking and that events should not be initialized with a default value. Same thing with EF Core DbSet properties on a DbContext... these are cases where things don't fit neatly into the strict checking. Your only options are to leave it non-nullable and suppress the warning, turn off nullable checks in the class, or make it nullable.
I like the definitive "It doesn't make much sense in this case, and so just add ? to shut up the compiler."
|
1

The whole point of non-nullable reference types in C# is to obviate the need for writing code that deals with nulls. Null checks make code more verbose and harder to read. By removing the need for always having to do null checking, code is made more concise and readable.

In that spirit, I would recommend that you require a reference to an event handler in the constructor for the type in which your event appears. This will make the compiler happy, and it means that every time your type containing an event is instantiated, there will be an event handler in place to handle your event. This ensures that your type is always used in the way that it is intended to be used.

Now, someone might say that it is better to make the event nullable so that it is optional. My view, however, is that if a type defines an event, it does so for an important reason, and if the user of that type wishes to ignore that important reason and not use the event, then that user can define an empty event handler so that it is made clear in the code that the event is intentionally being ignored.

In the end you get better communication between classes, and you also eliminate the need for null checks, so the benefits are twofold.

The code would look something like this:

class Program
{
    static void Main()
    {
        var myType1 = new MyType(IdleTimeout);
        
        // Or if you don't want a superfluous handler in your code,
        // you can use a Lambda.
        var myType2 = new MyType((sender, e) => {});
    }

    static void IdleTimeout(object? sender, EventArgs e)
    {
        // Event ignored.
    }
}

class MyType
{
    public MyType(EventHandler idleTimeoutReached)
    {
        IdleTimeoutReached += idleTimeoutReached;
    }

    public event EventHandler IdleTimeoutReached;

    public void TimeoutReached()
    {
        // Any required logic goes here.

        IdleTimeoutReached?.Invoke(this, EventArgs.Empty);
    }
}
Improve this answer

2 Comments

By that logic it would mean that in the case of for example controls with lots of events that currently can optionally be subscribed to, one would need to subscribe to every possible event the control exposes. I agree it's good practice in some cases, but it won't be feasible in others.
@Daap Good point. An event-laden UI control that requires acknowledgement of all events would be very annoying indeed. In any case, this is another approach that's useful for when events are mission critical.
1

I have the same confusion, and here is my answer after checked the source code of the .NET framework as @Tobias J said.

It seems that we need to append a ? to the type of the event handler as a nullable reference type. For example, the following code snippet of the INotifyPropertyChanged interface from the .NET 6.0 framework.

#region Assembly System.ObjectModel, Version=6.0.0.0, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a
// C:\Program Files\dotnet\packs\Microsoft.NETCore.App.Ref\6.0.19\ref\net6.0\System.ObjectModel.dll
#endregion

#nullable enable


namespace System.ComponentModel
{
    //
    // Summary:
    //     Notifies clients that a property value has changed.
    public interface INotifyPropertyChanged
    {
        //
        // Summary:
        //     Occurs when a property value changes.
        event PropertyChangedEventHandler? PropertyChanged;
    }
}
Improve this answer

Comments

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.