Make all event types nullable #25752
Conversation
Late-breaking design change: all events should be declared with nullable delegate types.
|
I support taking this into 3.0 so that we are realistically exercising the new compiler feature. |
Thanks, @danmosemsft. Though keep in mind it's more than that: this affects public surface area, so it's not just about exercising the compiler, but also about making sure we have the correct public surface area annotations. |
src/System.Private.CoreLib/shared/System/Runtime/CompilerServices/ContractHelper.cs
Show resolved
Hide resolved
| @@ -479,10 +479,13 @@ public override string ToString() | |||
| /// <summary> | |||
| /// Fires when a Command (e.g. Enable) comes from a an EventListener. | |||
| /// </summary> | |||
| public event EventHandler<EventCommandEventArgs> EventCommandExecuted // TODO-NULLABLE: Should all events use nullable delegate types? | |||
| public event EventHandler<EventCommandEventArgs>? EventCommandExecuted | |||
There was a problem hiding this comment.
@billwert, @brianrob, @noahfalk, could I get a code review of these EventSource tweaks, in particular the changes here and in CallBackForExistingEventSources, which skip code if a null callback is passed. In addition to adding the appropriate nullable annotations, this prevents code like this from null ref'ing:
using System.Diagnostics.Tracing;
using System.Threading.Tasks;
class Program
{
static void Main()
{
Task.Run(() => { });
var listener = new MyEventListener();
listener.EventSourceCreated += null;
}
}
internal class MyEventListener : EventListener
{
protected override void OnEventSourceCreated(EventSource eventSource) =>
EnableEvents(eventSource, EventLevel.LogAlways);
}where today that'll null ref:
Unhandled exception. System.NullReferenceException: Object reference not set to an instance of an object.
at System.Diagnostics.Tracing.EventListener.CallBackForExistingEventSources(Boolean addToListenersList, EventHandler`1 callback) in D:\repos\coreclr\src\System.Private.CoreLib\shared\System\Diagnostics\Tracing\EventSource.cs:line 4530
at System.Diagnostics.Tracing.EventListener.add_EventSourceCreated(EventHandler`1 value) in D:\repos\coreclr\src\System.Private.CoreLib\shared\System\Diagnostics\Tracing\EventSource.cs:line 4049
at Program.Main() in C:\Users\stoub\source\repos\ConsoleApp13\ConsoleApp13\Program.cs:line 10
There was a problem hiding this comment.
@stephentoub, the EventSource changes seem reasonable to me. Thanks.
In reply to: 304586154 [](ancestors = 304586154)
Right, but same rationale. This gives our customers are more realistic view of what nullable in the FX will look like. |
We'll all in violent agreement. I was just saying that "realistically exercising the new compiler feature" made it sound to me like the approval was based on testing the compiler and making sure its features were working correctly, and as you agree, it's more than that. I could have just misunderstood Dan's comments. |
|
Fair. @danmosemsft, please clarify whether this information changes your sign-off. |
Yes and no. It's only a bug if nulls are allowed, and previously the type was saying they weren't :) But I'm happy you're happy. :) |
|
Well, before null reference types the type couldn't differentiate, so I'll take what I can get 😄 |
Late-breaking design change: all events should be declared with nullable delegate types. Signed-off-by: dotnet-bot <dotnet-bot@microsoft.com>
Late-breaking design change: all events should be declared with nullable delegate types. Signed-off-by: dotnet-bot <dotnet-bot@microsoft.com>
Late-breaking design change: all events should be declared with nullable delegate types.
Late-breaking design change: all events should be declared with nullable delegate types.
Late-breaking design change: all events should be declared with nullable delegate types. Signed-off-by: dotnet-bot <dotnet-bot@microsoft.com>
Late-breaking design change: all events should be declared with nullable delegate types. Signed-off-by: dotnet-bot <dotnet-bot@microsoft.com>
Late-breaking design change: all events should be declared with nullable delegate types. Signed-off-by: dotnet-bot <dotnet-bot@microsoft.com>
Late-breaking design change: all events should be declared with nullable delegate types. Signed-off-by: dotnet-bot <dotnet-bot@microsoft.com>
Late-breaking design change: all events should be declared with nullable delegate types. Signed-off-by: dotnet-bot <dotnet-bot@microsoft.com>
Late-breaking design change: all events should be declared with nullable delegate types. Signed-off-by: dotnet-bot <dotnet-bot@microsoft.com>
Late-breaking design change: all events should be declared with nullable delegate types. Commit migrated from dotnet/coreclr@8ba2e15
Late-breaking design change: all events should be declared with nullable delegate types.
@danmosemsft, this will need to be ported to release/3.0 (#25749 should as well to help minimize conflicts, now and in the future), and also reference assemblies updated. We also need to do the same exercise for corefx, though I expect there are few events there in the relevant code (we need to check).
cc: @safern, @dotnet/nullablefc