Run darc update for arcade

[safern]

A build is finally out with the new compiler.

@stephentoub if you haven't done it I can remove the ! in this PR for DoesNotReturn attributes.

@agocke are the non-nullable uninitialized events warning that I needed to workaround with null! expected?

[stephentoub]

@agocke are the non-nullable uninitialized events warning that I needed to workaround with null! expected?

I hope that's a bug.

[agocke]

By design. Field like events are like any other fields. They were being skipped by mistake.

[stephentoub]

Seriously? So literally every event needs to have = null!; added to it? Or every event needs to be declared as nullable?

[agocke]

What's wrong with adding a ?? People often forget that events can be null and null ref when invoking. This is why we recommend ?.Invoke

[stephentoub]

I haven't forgotten; it's why I opened dotnet/roslyn#34982 months ago.

What's wrong with adding a ??

It's the backing field that needs the ?. There's no reason the consumer adding to or removing from the event needs to see the type as nullable. Yet now we're either requiring every event to have a public type with ?, or we're requiring the implementer to use = null! and then not get appropriate warnings. Either way, not a good outcome.

[stephentoub]

So, @MadsTorgersen, is our recommendation that every event anyone declares ever use a nullable delegate type?

[agocke]

@stephentoub I think I still don't understand the design problem. My recommendation would be that this is declared

public event EventHandler<T>? ProgressChanged;

This seems to match the expectation, because below in OnReport you read the event and then check to see if it's null. It looks like the event is nullable by design.

[stephentoub]

This seems to match the expectation, because below in OnReport you read the event and then check to see if it's null. It looks like the event is nullable by design.

There are two faces of an event: what's seen outside of the class, and what's seen inside. The language is generating a backing field for the event, and that's what you're accessing when you read from it; then it's treated like a field. But the public face of the event only lets you += and -= it via its add/remove accessors, and this recommendation is saying that every event needs to support having null added and removed.

My recommendation would be that this is declared

I understand. For every event anyone ever declares.

If that's really what we want, fine, but I don't remember it being discussed at LDM, or maybe I just couldn't attend that meeting. We have ~25 events "incorrectly" annotated then that need to be fixed (I don't know why @safern only needed to annotate ~7 with = null!).

It's really late in the game for changes like this to be showing up. Are there more we should expect?

[stephentoub]

Let me try to express me concerns more fully. The above syntax:

public event EventHandler<T> ProgressChanged;

is the simplified code the language lets you write. I could also have written it in the expanded form as something like:

private EventHandler<T>? _progressChanged;

public event EventHandler<T> ProgressChanged
{
    add { _progressChanged += value; }
    remove { _progressChanged -= value; }
}

There are actually two things here that can be annotated: the public event, whose annotations dictate whether value can be null, and the private backing field, whose annotations reflect that fact that it will always start life as null, as is the case for pretty much every event ever written. By suggesting that developers write:

public event EventHandler<T>? ProgressChanged;

it's conflating the two, making both nullable even when in general I'd expect we only want the backing field to be. Now, it's true that most events will do just fine with nulls, since they'll rely on Delegate.Combine (+=) and Delegate.Remove (-=) for implementation, and those support null. But from a philosophical standpoint, is that the API we want broadcast, encouraging nulls to be used with += and -=? I thought one of our goals in this whole effort was to actually minimize how much nullness was encouraged.

[agocke]

I see. This was implemented as a bug fix (there was a TODO in the analyzer). It was not intended to implement an event-specific analysis