Skip to content

Commit

Permalink
Improve Nullabillity on .NET 5
Browse files Browse the repository at this point in the history
  • Loading branch information
@TheCodeTraveler
TheCodeTraveler committed Nov 2, 2020
1 parent b69bb7e commit a5c4816
Show file tree
Hide file tree
Showing 5 changed files with 23 additions and 21 deletions.
3 changes: 2 additions & 1 deletion Src/AsyncAwaitBestPractices.MVVM/AsyncAwaitBestPractices.MVVM.csproj
View file
Original file line number Diff line number Diff line change
Expand Up @@ -33,8 +33,9 @@
- Add `AsyncCommand<TExecute, TCanExecute>`
- Add `AsyncValueCommand<TExecute, TCanExecute>`
- Implement SourceLink
- Support .NET 5
</PackageReleaseNotes>
<Version>5.0.0</Version>
<Version>5.0.1</Version>
<RepositoryUrl>https://github.com/brminnick/AsyncAwaitBestPractices</RepositoryUrl>
<Product>$(AssemblyName) ($(TargetFramework))</Product>
<AssemblyVersion>1.0.0.0</AssemblyVersion>
Expand Down
22 changes: 11 additions & 11 deletions ...syncAwaitBestPractices.UnitTests/WeakEventManagerTests/Tests_WeakEventManager_Delegate.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ class Tests_WeakEventManager_Delegate : BaseTest, INotifyPropertyChanged
{
readonly WeakEventManager _propertyChangedWeakEventManager = new WeakEventManager();

public event PropertyChangedEventHandler PropertyChanged
public event PropertyChangedEventHandler? PropertyChanged
{
add => _propertyChangedWeakEventManager.AddEventHandler(value);
remove => _propertyChangedWeakEventManager.RemoveEventHandler(value);
Expand All @@ -21,10 +21,10 @@ public void WeakEventManagerDelegate_HandleEvent_ValidImplementation()
PropertyChanged += HandleDelegateTest;
bool didEventFire = false;

void HandleDelegateTest(object sender, PropertyChangedEventArgs e)
void HandleDelegateTest(object? sender, PropertyChangedEventArgs e)
{
Assert.IsNotNull(sender);
Assert.AreEqual(this.GetType(), sender.GetType());
Assert.AreEqual(this.GetType(), sender?.GetType());

Assert.IsNotNull(e);

Expand All @@ -46,7 +46,7 @@ public void WeakEventManagerDelegate_HandleEvent_NullSender()
PropertyChanged += HandleDelegateTest;
bool didEventFire = false;

void HandleDelegateTest(object sender, PropertyChangedEventArgs e)
void HandleDelegateTest(object? sender, PropertyChangedEventArgs e)
{
Assert.IsNull(sender);
Assert.IsNotNull(e);
Expand All @@ -69,7 +69,7 @@ public void WeakEventManagerDelegate_HandleEvent_InvalidEventArgs()
PropertyChanged += HandleDelegateTest;
bool didEventFire = false;

void HandleDelegateTest(object sender, PropertyChangedEventArgs e) => didEventFire = true;
void HandleDelegateTest(object? sender, PropertyChangedEventArgs e) => didEventFire = true;

//Act

Expand All @@ -86,10 +86,10 @@ public void WeakEventManagerDelegate_HandleEvent_NullEventArgs()
PropertyChanged += HandleDelegateTest;
bool didEventFire = false;

void HandleDelegateTest(object sender, PropertyChangedEventArgs e)
void HandleDelegateTest(object? sender, PropertyChangedEventArgs e)
{
Assert.IsNotNull(sender);
Assert.AreEqual(this.GetType(), sender.GetType());
Assert.AreEqual(this.GetType(), sender?.GetType());

Assert.IsNull(e);

Expand All @@ -113,7 +113,7 @@ public void WeakEventManagerDelegate_HandleEvent_InvalidHandleEventEventName()
PropertyChanged += HandleDelegateTest;
bool didEventFire = false;

void HandleDelegateTest(object sender, PropertyChangedEventArgs e) => didEventFire = true;
void HandleDelegateTest(object? sender, PropertyChangedEventArgs e) => didEventFire = true;

//Act
_propertyChangedWeakEventManager.RaiseEvent(this, new PropertyChangedEventArgs("Test"), nameof(TestStringEvent));
Expand Down Expand Up @@ -149,7 +149,7 @@ public void WeakEventManagerDelegate_UnassignedEvent()

PropertyChanged += HandleDelegateTest;
PropertyChanged -= HandleDelegateTest;
void HandleDelegateTest(object sender, PropertyChangedEventArgs e) => didEventFire = true;
void HandleDelegateTest(object? sender, PropertyChangedEventArgs e) => didEventFire = true;

//Act
#pragma warning disable CS8625 //Cannot convert null literal to non-nullable reference type
Expand All @@ -168,7 +168,7 @@ public void WeakEventManagerDelegate_UnassignedEventManager()
bool didEventFire = false;

PropertyChanged += HandleDelegateTest;
void HandleDelegateTest(object sender, PropertyChangedEventArgs e) => didEventFire = true;
void HandleDelegateTest(object? sender, PropertyChangedEventArgs e) => didEventFire = true;

//Act
unassignedEventManager.RaiseEvent(null, null, nameof(PropertyChanged));
Expand All @@ -185,7 +185,7 @@ public void WeakEventManagerDelegate_HandleEvent_InvalidHandleEvent()
PropertyChanged += HandleDelegateTest;
bool didEventFire = false;

void HandleDelegateTest(object sender, PropertyChangedEventArgs e) => didEventFire = true;
void HandleDelegateTest(object? sender, PropertyChangedEventArgs e) => didEventFire = true;

//Act

Expand Down
3 changes: 2 additions & 1 deletion Src/AsyncAwaitBestPractices/AsyncAwaitBestPractices.csproj
View file
Original file line number Diff line number Diff line change
Expand Up @@ -32,8 +32,9 @@
New In This Release:
- `WeakEventManager` performance improvements by implementing `ValueTuple`
- Implement SourceLink
- Support .NET 5
</PackageReleaseNotes>
<Version>5.0.0</Version>
<Version>5.0.1</Version>
<RepositoryUrl>https://github.com/brminnick/AsyncAwaitBestPractices</RepositoryUrl>
<Product>$(AssemblyName) ($(TargetFramework))</Product>
<AssemblyVersion>1.0.0.0</AssemblyVersion>
Expand Down
4 changes: 2 additions & 2 deletions Src/AsyncAwaitBestPractices/WeakEventManager/WeakEventManager.extensions.shared.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -59,14 +59,14 @@ public partial class WeakEventManager
/// </summary>
/// <param name="handler">Handler</param>
/// <param name="eventName">Event name</param>
public void AddEventHandler(Delegate handler, [CallerMemberName] string eventName = "") => AddEventHandler(in handler, in eventName);
public void AddEventHandler(Delegate? handler, [CallerMemberName] string eventName = "") => AddEventHandler(in handler, in eventName);

/// <summary>
/// Removes the event handler.
/// </summary>
/// <param name="handler">Handler</param>
/// <param name="eventName">Event name</param>
public void RemoveEventHandler(Delegate handler, [CallerMemberName] string eventName = "") => RemoveEventHandler(in handler, in eventName);
public void RemoveEventHandler(Delegate? handler, [CallerMemberName] string eventName = "") => RemoveEventHandler(in handler, in eventName);

/// <summary>
/// Invokes the event EventHandler
Expand Down
12 changes: 6 additions & 6 deletions Src/AsyncAwaitBestPractices/WeakEventManager/WeakEventManager.shared.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ public partial class WeakEventManager<TEventArgs>
/// </summary>
/// <param name="handler">Handler</param>
/// <param name="eventName">Event name</param>
public void AddEventHandler(in EventHandler<TEventArgs> handler, [CallerMemberName] in string eventName = "")
public void AddEventHandler(in EventHandler<TEventArgs>? handler, [CallerMemberName] in string eventName = "")

This comment has been minimized.

Sign in to view

Copy link
@SIDOVSKY

SIDOVSKY Apr 1, 2021

@brminnick Is it a mistype? There is a nullability check for this parameter in the method body.
(the same for other overloads below)

This comment has been minimized.

Sign in to view

Copy link
@TheCodeTraveler

TheCodeTraveler Apr 1, 2021 •
edited
Loading

Author Owner

I did it on purpose because .NET (specifically .NET 5.0) has marked events on certain interfaces as Nullable.

One example is INotifyPropertyChanged; as of .NET 5.0, it requires event PropertyChangedEventHandler? PropertyChanged, and so we implement it with WeakEventManager like this:

class Example : INotifyPropertyChanged
{
    readonly WeakEventManager _propertyChangedWeakEventManager = new();

    public event PropertyChangedEventHandler? PropertyChanged
    {
        add => _propertyChangedWeakEventManager.AddEventHandler(value);
        remove => _propertyChangedWeakEventManager.RemoveEventHandler(value);
    }
}

If I remove nullability on the parameters for AddEventHandler and RemoveEventHandler, it makes the implementation of WeakEventManager slightly more clunky:

class Example : INotifyPropertyChanged
{
    readonly WeakEventManager _propertyChangedWeakEventManager = new();

    public event PropertyChangedEventHandler? PropertyChanged
    {
        add
        {
            if (value is not null)
                _propertyChangedWeakEventManager.AddEventHandler(value);
        }
        remove
        {
            if (value is not null)
                _propertyChangedWeakEventManager.RemoveEventHandler(value);
        }
    }
}

This comment has been minimized.

Sign in to view

Copy link
@TheCodeTraveler

TheCodeTraveler Apr 1, 2021

Author Owner

Have you hit a bug because due to this implementation?

This comment has been minimized.

Sign in to view

Copy link
@TheCodeTraveler

TheCodeTraveler Apr 1, 2021

Author Owner

Follow up question - do you know if it's possible for add and remove to pass in null? E.g. What scenario would cause value to be null in add or remove?

This comment has been minimized.

Sign in to view

Copy link
@SIDOVSKY

SIDOVSKY Apr 2, 2021

Have you hit a bug because due to this implementation?

Fortunately no, just found it strange.

Follow up question - do you know if it's possible for add and remove to pass in null? E.g. What scenario would cause value to be null in add or remove?

Actually this is a valid syntax:

PropertyChanged += null;
PropertyChanged -= null;

One possible scenario where a null handler may be passed in is when such Dispose is called twice in a row:

public void Dispose()
{
    PropertyChanged -= _handler;
    _handler = null;
}

as of .NET 5.0, it requires event PropertyChangedEventHandler? PropertyChanged

Looks like it's a language limitation that the nullability cannot be specified separately for the event type and its accessors. There is a discussion for that.
It might be solved with a nullability attribute but none of the existing ones are suitable.
I have no idea how to overcome that.

What I can suggest is to replace ArgumentNullException with return to start ignoring null handlers internally.
Null values are ignored in the example you provided:

add
{
    if (value is not null)
        _propertyChangedWeakEventManager.AddEventHandler(value);
}

and also in the default event implementation where Delegate.Combine and Delegate.Remove ignore nulls.

I'm not sure that throwing an ArgumentNullException for the handler is important here. Instead, it may confuse users about the method behavior.
{
if (IsNullOrWhiteSpace(eventName))
throw new ArgumentNullException(nameof(eventName));
Expand All @@ -36,7 +36,7 @@ public void AddEventHandler(in EventHandler<TEventArgs> handler, [CallerMemberNa
/// </summary>
/// <param name="action">Handler</param>
/// <param name="eventName">Event name</param>
public void AddEventHandler(in Action<TEventArgs> action, [CallerMemberName] in string eventName = "")
public void AddEventHandler(in Action<TEventArgs>? action, [CallerMemberName] in string eventName = "")
{
if (IsNullOrWhiteSpace(eventName))
throw new ArgumentNullException(nameof(eventName));
Expand All @@ -52,7 +52,7 @@ public void AddEventHandler(in Action<TEventArgs> action, [CallerMemberName] in
/// </summary>
/// <param name="handler">Handler</param>
/// <param name="eventName">Event name</param>
public void RemoveEventHandler(in EventHandler<TEventArgs> handler, [CallerMemberName] in string eventName = "")
public void RemoveEventHandler(in EventHandler<TEventArgs>? handler, [CallerMemberName] in string eventName = "")
{
if (IsNullOrWhiteSpace(eventName))
throw new ArgumentNullException(nameof(eventName));
Expand All @@ -68,7 +68,7 @@ public void RemoveEventHandler(in EventHandler<TEventArgs> handler, [CallerMembe
/// </summary>
/// <param name="action">Handler</param>
/// <param name="eventName">Event name</param>
public void RemoveEventHandler(in Action<TEventArgs> action, [CallerMemberName] in string eventName = "")
public void RemoveEventHandler(in Action<TEventArgs>? action, [CallerMemberName] in string eventName = "")
{
if (IsNullOrWhiteSpace(eventName))
throw new ArgumentNullException(nameof(eventName));
Expand Down Expand Up @@ -109,7 +109,7 @@ public partial class WeakEventManager
/// </summary>
/// <param name="handler">Handler</param>
/// <param name="eventName">Event name</param>
public void AddEventHandler(in Delegate handler, [CallerMemberName] in string eventName = "")
public void AddEventHandler(in Delegate? handler, [CallerMemberName] in string eventName = "")
{
if (IsNullOrWhiteSpace(eventName))
throw new ArgumentNullException(nameof(eventName));
Expand All @@ -125,7 +125,7 @@ public void AddEventHandler(in Delegate handler, [CallerMemberName] in string ev
/// </summary>
/// <param name="handler">Handler</param>
/// <param name="eventName">Event name</param>
public void RemoveEventHandler(in Delegate handler, [CallerMemberName] in string eventName = "")
public void RemoveEventHandler(in Delegate? handler, [CallerMemberName] in string eventName = "")
{
if (IsNullOrWhiteSpace(eventName))
throw new ArgumentNullException(nameof(eventName));
Expand Down

0 comments on commit a5c4816

Please sign in to comment.