Skip to content
This repository has been archived by the owner on Jan 23, 2023. It is now read-only.

Make all event types nullable #25752

Merged
merged 1 commit into from Jul 17, 2019
Merged

Make all event types nullable #25752

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
6 changes: 3 additions & 3 deletions src/System.Private.CoreLib/shared/System/AppContext.cs
View file
Open in desktop
Expand Up @@ -63,12 +63,12 @@ public static void SetData(string name, object? data)
}

#pragma warning disable CS0067 // events raised by the VM
public static event UnhandledExceptionEventHandler UnhandledException; // TODO-NULLABLE: Should all events use nullable delegate types?
public static event UnhandledExceptionEventHandler? UnhandledException;

public static event System.EventHandler<FirstChanceExceptionEventArgs> FirstChanceException; // TODO-NULLABLE: Should all events use nullable delegate types?
public static event System.EventHandler<FirstChanceExceptionEventArgs>? FirstChanceException;
#pragma warning restore CS0067

public static event System.EventHandler ProcessExit; // TODO-NULLABLE: Should all events use nullable delegate types?
public static event System.EventHandler? ProcessExit;

internal static void OnProcessExit()
{
Expand Down
20 changes: 10 additions & 10 deletions src/System.Private.CoreLib/shared/System/AppDomain.cs
View file
Open in desktop
Expand Up @@ -2,7 +2,7 @@
// The .NET Foundation licenses this file to you under the MIT license.
// See the LICENSE file in the project root for more information.

#pragma warning disable CS0414 // events are assigned but not used
#pragma warning disable CS0067 // events are declared but not used

using System.Diagnostics;
using System.IO;
Expand Down Expand Up @@ -41,7 +41,7 @@ public sealed partial class AppDomain : MarshalByRefObject

public PermissionSet PermissionSet => new PermissionSet(PermissionState.Unrestricted);

public event UnhandledExceptionEventHandler UnhandledException // TODO-NULLABLE: Should all events use nullable delegate types?
public event UnhandledExceptionEventHandler? UnhandledException
{
add { AppContext.UnhandledException += value; }
remove { AppContext.UnhandledException -= value; }
Expand All @@ -67,15 +67,15 @@ public string FriendlyName

public bool IsHomogenous => true;

public event EventHandler DomainUnload = null!; // TODO-NULLABLE: Should all events use nullable delegate types?
public event EventHandler? DomainUnload;

public event EventHandler<FirstChanceExceptionEventArgs> FirstChanceException // TODO-NULLABLE: Should all events use nullable delegate types?
public event EventHandler<FirstChanceExceptionEventArgs>? FirstChanceException
{
add { AppContext.FirstChanceException += value; }
remove { AppContext.FirstChanceException -= value; }
}

public event EventHandler ProcessExit // TODO-NULLABLE: Should all events use nullable delegate types?
public event EventHandler? ProcessExit
{
add { AppContext.ProcessExit += value; }
remove { AppContext.ProcessExit -= value; }
Expand Down Expand Up @@ -232,27 +232,27 @@ public static long MonitoringSurvivedProcessMemorySize

public Assembly[] GetAssemblies() => AssemblyLoadContext.GetLoadedAssemblies();

public event AssemblyLoadEventHandler AssemblyLoad // TODO-NULLABLE: Should all events use nullable delegate types?
public event AssemblyLoadEventHandler? AssemblyLoad
{
add { AssemblyLoadContext.AssemblyLoad += value; }
remove { AssemblyLoadContext.AssemblyLoad -= value; }
}

public event ResolveEventHandler AssemblyResolve // TODO-NULLABLE: Should all events use nullable delegate types?
public event ResolveEventHandler? AssemblyResolve
{
add { AssemblyLoadContext.AssemblyResolve += value; }
remove { AssemblyLoadContext.AssemblyResolve -= value; }
}

public event ResolveEventHandler ReflectionOnlyAssemblyResolve = null!; // TODO-NULLABLE: Should all events use nullable delegate types?
public event ResolveEventHandler? ReflectionOnlyAssemblyResolve;

public event ResolveEventHandler TypeResolve // TODO-NULLABLE: Should all events use nullable delegate types?
public event ResolveEventHandler? TypeResolve
{
add { AssemblyLoadContext.TypeResolve += value; }
remove { AssemblyLoadContext.TypeResolve -= value; }
}

public event ResolveEventHandler ResourceResolve // TODO-NULLABLE: Should all events use nullable delegate types?
public event ResolveEventHandler? ResourceResolve
{
add { AssemblyLoadContext.ResourceResolve += value; }
remove { AssemblyLoadContext.ResourceResolve -= value; }
Expand Down
2 changes: 1 addition & 1 deletion src/System.Private.CoreLib/shared/System/Diagnostics/Contracts/Contracts.cs
View file
Open in desktop
Expand Up @@ -703,7 +703,7 @@ private static void ReportFailure(ContractFailureKind failureKind, string? userM
/// full trust, because it will inform you of bugs in the appdomain and because the event handler
/// could allow you to continue execution.
/// </summary>
public static event EventHandler<ContractFailedEventArgs> ContractFailed // TODO-NULLABLE: Should all events use nullable delegate types?
public static event EventHandler<ContractFailedEventArgs>? ContractFailed
{
add
{
Expand Down
63 changes: 35 additions & 28 deletions src/System.Private.CoreLib/shared/System/Diagnostics/Tracing/EventSource.cs
View file
Open in desktop
Expand Up @@ -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
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@stephentoub, the EventSource changes seem reasonable to me. Thanks.

In reply to: 304586154 [](ancestors = 304586154)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM as well

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks

{
add
{
if (value == null)
return;

m_eventCommandExecuted += value;

// If we have an EventHandler<EventCommandEventArgs> attached to the EventSource before the first command arrives
Expand Down Expand Up @@ -4042,7 +4045,7 @@ public class EventListener : IDisposable
/// In a multi-threaded environment, it is possible that 'EventSourceEventWrittenCallback'
/// events for a particular eventSource to occur BEFORE the EventSourceCreatedCallback is issued.
/// </summary>
public event EventHandler<EventSourceCreatedEventArgs> EventSourceCreated // TODO-NULLABLE: Should all events use nullable delegate types?
public event EventHandler<EventSourceCreatedEventArgs>? EventSourceCreated
{
add
{
Expand All @@ -4060,7 +4063,7 @@ public class EventListener : IDisposable
/// This event is raised whenever an event has been written by a EventSource for which
/// the EventListener has enabled events.
/// </summary>
public event EventHandler<EventWrittenEventArgs> EventWritten = null!; // TODO-NULLABLE: Should all events use nullable delegate types?
public event EventHandler<EventWrittenEventArgs>? EventWritten;

static EventListener()
{
Expand Down Expand Up @@ -4251,7 +4254,7 @@ internal protected virtual void OnEventSourceCreated(EventSource eventSource)
/// <param name="eventData"></param>
internal protected virtual void OnEventWritten(EventWrittenEventArgs eventData)
{
EventHandler<EventWrittenEventArgs> callBack = this.EventWritten;
EventHandler<EventWrittenEventArgs>? callBack = this.EventWritten;
if (callBack != null)
{
callBack(this, eventData);
Expand Down Expand Up @@ -4483,7 +4486,7 @@ internal static object EventListenersLock
}
}

private void CallBackForExistingEventSources(bool addToListenersList, EventHandler<EventSourceCreatedEventArgs> callback)
private void CallBackForExistingEventSources(bool addToListenersList, EventHandler<EventSourceCreatedEventArgs>? callback)
{
lock (EventListenersLock)
{
Expand All @@ -4507,36 +4510,40 @@ private void CallBackForExistingEventSources(bool addToListenersList, EventHandl
s_Listeners = this;
}

// Find all existing eventSources call OnEventSourceCreated to 'catchup'
// Note that we DO have reentrancy here because 'AddListener' calls out to user code (via OnEventSourceCreated callback)
// We tolerate this by iterating over a copy of the list here. New event sources will take care of adding listeners themselves
// EventSources are not guaranteed to be added at the end of the s_EventSource list -- We re-use slots when a new source
// is created.
WeakReference[] eventSourcesSnapshot = s_EventSources.ToArray();
if (callback != null)
{
// Find all existing eventSources call OnEventSourceCreated to 'catchup'
// Note that we DO have reentrancy here because 'AddListener' calls out to user code (via OnEventSourceCreated callback)
// We tolerate this by iterating over a copy of the list here. New event sources will take care of adding listeners themselves
// EventSources are not guaranteed to be added at the end of the s_EventSource list -- We re-use slots when a new source
// is created.
WeakReference[] eventSourcesSnapshot = s_EventSources.ToArray();

#if DEBUG
bool previousValue = s_ConnectingEventSourcesAndListener;
s_ConnectingEventSourcesAndListener = true;
try
{
#endif
for (int i = 0; i < eventSourcesSnapshot.Length; i++)
bool previousValue = s_ConnectingEventSourcesAndListener;
s_ConnectingEventSourcesAndListener = true;
try
{
WeakReference eventSourceRef = eventSourcesSnapshot[i];
if (eventSourceRef.Target is EventSource eventSource)
#endif
for (int i = 0; i < eventSourcesSnapshot.Length; i++)
{
EventSourceCreatedEventArgs args = new EventSourceCreatedEventArgs();
args.EventSource = eventSource;
callback(this, args);
WeakReference eventSourceRef = eventSourcesSnapshot[i];
if (eventSourceRef.Target is EventSource eventSource)
{
EventSourceCreatedEventArgs args = new EventSourceCreatedEventArgs();
args.EventSource = eventSource;
callback(this, args);
}
}
}
#if DEBUG
}
finally
{
s_ConnectingEventSourcesAndListener = previousValue;
}
}
finally
{
s_ConnectingEventSourcesAndListener = previousValue;
}
#endif
}

Validate();
}
finally
Expand Down
2 changes: 1 addition & 1 deletion src/System.Private.CoreLib/shared/System/Exception.cs
View file
Open in desktop
Expand Up @@ -166,7 +166,7 @@ public override string ToString()
return s;
}

protected event EventHandler<SafeSerializationEventArgs> SerializeObjectState // TODO-NULLABLE: Should all events use nullable delegate types?
protected event EventHandler<SafeSerializationEventArgs>? SerializeObjectState
{
add { throw new PlatformNotSupportedException(SR.PlatformNotSupported_SecureBinarySerialization); }
remove { throw new PlatformNotSupportedException(SR.PlatformNotSupported_SecureBinarySerialization); }
Expand Down
6 changes: 3 additions & 3 deletions src/System.Private.CoreLib/shared/System/Progress.cs
View file
Open in desktop
Expand Up @@ -56,7 +56,7 @@ public Progress(Action<T> handler) : this()
/// Handlers registered with this event will be invoked on the
/// <see cref="System.Threading.SynchronizationContext"/> captured when the instance was constructed.
/// </remarks>
public event EventHandler<T> ProgressChanged = null!; // TODO-NULLABLE: Should all events use nullable delegate types?
public event EventHandler<T>? ProgressChanged;

/// <summary>Reports a progress change.</summary>
/// <param name="value">The value of the updated progress.</param>
Expand All @@ -66,7 +66,7 @@ protected virtual void OnReport(T value)
// Inside the callback, we'll need to check again, in case
// an event handler is removed between now and then.
Action<T>? handler = _handler;
EventHandler<T> changedEvent = ProgressChanged;
EventHandler<T>? changedEvent = ProgressChanged;
if (handler != null || changedEvent != null)
{
// Post the processing to the sync context.
Expand All @@ -86,7 +86,7 @@ private void InvokeHandlers(object? state)
T value = (T)state!;

Action<T>? handler = _handler;
EventHandler<T> changedEvent = ProgressChanged;
EventHandler<T>? changedEvent = ProgressChanged;

if (handler != null) handler(value);
if (changedEvent != null) changedEvent(this, value);
Expand Down
2 changes: 1 addition & 1 deletion src/System.Private.CoreLib/shared/System/Reflection/Assembly.cs
View file
Open in desktop
Expand Up @@ -117,7 +117,7 @@ public virtual Type[] GetTypes()
return Activator.CreateInstance(t, bindingAttr, binder, args, culture, activationAttributes);
}

public virtual event ModuleResolveEventHandler ModuleResolve { add { throw NotImplemented.ByDesign; } remove { throw NotImplemented.ByDesign; } } // TODO-NULLABLE: Should all events use nullable delegate types?
public virtual event ModuleResolveEventHandler? ModuleResolve { add { throw NotImplemented.ByDesign; } remove { throw NotImplemented.ByDesign; } }

public virtual Module ManifestModule { get { throw NotImplemented.ByDesign; } }
public virtual Module? GetModule(string name) { throw NotImplemented.ByDesign; }
Expand Down
4 changes: 2 additions & 2 deletions src/System.Private.CoreLib/shared/System/Runtime/CompilerServices/ContractHelper.cs
View file
Open in desktop
Expand Up @@ -17,7 +17,7 @@ public static class ContractHelper
/// event handlers sets the Cancel flag in the ContractFailedEventArgs, then the Contract class will
/// not pop up an assert dialog box or trigger escalation policy.
/// </summary>
internal static event EventHandler<ContractFailedEventArgs> InternalContractFailed; // TODO-NULLABLE: Should all events use nullable delegate types?
internal static event EventHandler<ContractFailedEventArgs>? InternalContractFailed;
stephentoub marked this conversation as resolved.
Show resolved Hide resolved

/// <summary>
/// Rewriter will call this method on a contract failure to allow listeners to be notified.
Expand All @@ -42,7 +42,7 @@ public static class ContractHelper
try
{
displayMessage = GetDisplayMessage(failureKind, userMessage, conditionText);
EventHandler<ContractFailedEventArgs> contractFailedEventLocal = InternalContractFailed;
EventHandler<ContractFailedEventArgs>? contractFailedEventLocal = InternalContractFailed;
if (contractFailedEventLocal != null)
{
eventArgs = new ContractFailedEventArgs(failureKind, displayMessage, conditionText, innerException);
Expand Down
20 changes: 10 additions & 10 deletions src/System.Private.CoreLib/shared/System/Runtime/Loader/AssemblyLoadContext.cs
View file
Open in desktop
Expand Up @@ -38,11 +38,11 @@ private enum InternalState
// synchronization primitive to protect against usage of this instance while unloading
private readonly object _unloadLock;

private event Func<Assembly, string, IntPtr> _resolvingUnmanagedDll = null!;
private event Func<Assembly, string, IntPtr>? _resolvingUnmanagedDll;

private event Func<AssemblyLoadContext, AssemblyName, Assembly> _resolving = null!;
private event Func<AssemblyLoadContext, AssemblyName, Assembly>? _resolving;

private event Action<AssemblyLoadContext> _unloading = null!;
private event Action<AssemblyLoadContext>? _unloading;

private readonly string? _name;

Expand Down Expand Up @@ -168,7 +168,7 @@ public IEnumerable<Assembly> Assemblies
//
// Inputs: Invoking assembly, and library name to resolve
// Returns: A handle to the loaded native library
public event Func<Assembly, string, IntPtr> ResolvingUnmanagedDll // TODO-NULLABLE: Should all events use nullable delegate types?
public event Func<Assembly, string, IntPtr>? ResolvingUnmanagedDll
{
add
{
Expand All @@ -186,7 +186,7 @@ public IEnumerable<Assembly> Assemblies
//
// Inputs: The AssemblyLoadContext and AssemblyName to be loaded
// Returns: The Loaded assembly object.
public event Func<AssemblyLoadContext, AssemblyName, Assembly?> Resolving // TODO-NULLABLE: Should all events use nullable delegate types?
public event Func<AssemblyLoadContext, AssemblyName, Assembly?>? Resolving
{
add
{
Expand All @@ -198,7 +198,7 @@ public IEnumerable<Assembly> Assemblies
}
}

public event Action<AssemblyLoadContext> Unloading // TODO-NULLABLE: Should all events use nullable delegate types?
public event Action<AssemblyLoadContext>? Unloading
{
add
{
Expand All @@ -212,17 +212,17 @@ public IEnumerable<Assembly> Assemblies

#region AppDomainEvents
// Occurs when an Assembly is loaded
internal static event AssemblyLoadEventHandler AssemblyLoad; // TODO-NULLABLE: Should all events use nullable delegate types?
internal static event AssemblyLoadEventHandler? AssemblyLoad;

// Occurs when resolution of type fails
internal static event ResolveEventHandler TypeResolve; // TODO-NULLABLE: Should all events use nullable delegate types?
internal static event ResolveEventHandler? TypeResolve;

// Occurs when resolution of resource fails
internal static event ResolveEventHandler ResourceResolve; // TODO-NULLABLE: Should all events use nullable delegate types?
internal static event ResolveEventHandler? ResourceResolve;

// Occurs when resolution of assembly fails
// This event is fired after resolve events of AssemblyLoadContext fails
internal static event ResolveEventHandler AssemblyResolve; // TODO-NULLABLE: Should all events use nullable delegate types?
internal static event ResolveEventHandler? AssemblyResolve;
#endregion

public static AssemblyLoadContext Default => DefaultAssemblyLoadContext.s_loadContext;
Expand Down
2 changes: 1 addition & 1 deletion src/System.Private.CoreLib/shared/System/Runtime/Serialization/ISafeSerializationData.cs
View file
Open in desktop
Expand Up @@ -139,7 +139,7 @@ namespace System.Runtime.Serialization
//
// 2. Add a protected SerializeObjectState event, which passes through to the SafeSerializationManager:
//
// protected event EventHandler<SafeSerializationEventArgs> SerializeObjectState
// protected event EventHandler<SafeSerializationEventArgs>? SerializeObjectState
// {
// add { _safeSerializationManager.SerializeObjectState += value; }
// remove { _safeSerializationManager.SerializeObjectState -= value; }
Expand Down
2 changes: 1 addition & 1 deletion src/System.Private.CoreLib/shared/System/Threading/Tasks/TaskScheduler.cs
View file
Open in desktop
Expand Up @@ -445,7 +445,7 @@ protected bool TryExecuteTask(Task task)
/// Each handler is passed a <see cref="T:System.Threading.Tasks.UnobservedTaskExceptionEventArgs"/>
/// instance, which may be used to examine the exception and to mark it as observed.
/// </remarks>
public static event EventHandler<UnobservedTaskExceptionEventArgs> UnobservedTaskException; // TODO-NULLABLE: Should all events use nullable delegate types?
public static event EventHandler<UnobservedTaskExceptionEventArgs>? UnobservedTaskException;

////////////////////////////////////////////////////////////
//
Expand Down