Fix nullable Event<'Delegate, 'Args> for INotifyPropertyChanged implementations #18576

Copilot · 2025-05-20T07:50:27Z

Issue

When implementing INotifyPropertyChanged interface using the standard F# pattern with Event<PropertyChangedEventHandler, PropertyChangedEventArgs>(), a nullness warning is produced:

warning FS3261: Nullness warning: The types 'System.Delegate' and 'System.Delegate | null' do not have compatible nullability.

This happens in nullable-aware contexts (using --checknulls and langversion:preview).

Root Cause

In a nullness-aware context, .NET delegate types like PropertyChangedEventHandler are treated as potentially nullable (Delegate | null), but the F# Event class was expecting non-nullable delegates in its implementation.

Fix

Updated the multicast field in Event classes to be explicitly marked as nullable:

// Before
let mutable multicast: 'Delegate = Unchecked.defaultof<_>

// After
let mutable multicast: 'Delegate | null = Unchecked.defaultof<_>

Fixed the delegate handling in AddHandler and RemoveHandler to correctly handle nullable delegates:

// Before
Atomic.setWith (fun value -> System.Delegate.Combine(value, d) :?> 'Delegate) &multicast

// After 
Atomic.setWith (fun value -> System.Delegate.Combine(value, d) :?> ('Delegate | null)) &multicast

Made similar changes to the Event<'T> class.
Added a test case to verify the fix works properly for INotifyPropertyChanged implementations.

These changes ensure that F# code can cleanly implement standard .NET interfaces with events when nullability checks are enabled.

Fixes #18361.

Warning

Firewall rules blocked me from connecting to one or more addresses

I tried to connect to the following addresses, but was blocked by firewall rules:

cdn.fwupd.org
- Triggering command: /usr/bin/fwupdmgr refresh (dns block)
dc.services.visualstudio.com
- Triggering command: /home/REDACTED/work/fsharp/fsharp/.dotnet/dotnet msbuild /m /nologo /clp:Summary /v:minimal /nr:true /warnaserror /p:TreatWarningsAsErrors=true /p:ContinuousIntegrationBuild=false /home/REDACTED/work/fsharp/fsharp/artifacts/toolset/restore.proj /t:__WriteToolsetLocation /clp:ErrorsOnly;NoSummary /p:__ToolsetLocationOutputFile=/home/REDACTED/work/fsharp/fsharp/artifacts/toolset/9.0.0-beta.25266.2.txt (dns block)
pkgs.dev.azure.com
- Triggering command: /home/REDACTED/work/fsharp/fsharp/.dotnet/dotnet msbuild /m /nologo /clp:Summary /v:minimal /nr:true /warnaserror /p:TreatWarningsAsErrors=true /p:ContinuousIntegrationBuild=false /home/REDACTED/work/fsharp/fsharp/artifacts/toolset/restore.proj /t:__WriteToolsetLocation /clp:ErrorsOnly;NoSummary /p:__ToolsetLocationOutputFile=/home/REDACTED/work/fsharp/fsharp/artifacts/toolset/9.0.0-beta.25266.2.txt (dns block)

If you need me to access, download, or install something from one of these locations, you can either:

Configure Actions setup steps to set up my environment, which run before the firewall is enabled
Add the appropriate URLs or hosts to my firewall allow list

💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.

Co-authored-by: T-Gro <[email protected]>

T-Gro · 2025-05-20T08:14:39Z

src/FSharp.Core/event.fs

@@ -146,10 +146,10 @@ type Event<'Delegate, 'Args
                "<published event>"
          interface IEvent<'Delegate, 'Args> with
              member e.AddHandler(d) =
-                  Atomic.setWith (fun value -> System.Delegate.Combine(value, d) :?> 'Delegate) &multicast
+                  Atomic.setWith (fun value -> System.Delegate.Combine(value, d) :?> ('Delegate | null)) &multicast


@copilot :
It does not make sense casting to nullable types.
Cast just to 'Delegate , without the | null .

Fixed in 0e34deb - removed the nullable union type from the cast

Copilot · 2025-05-20T08:14:43Z

@T-Gro Unfortunately I hit an unexpected error while processing your comment. I've automatically reported this to GitHub.

You can ask me to try again later by mentioning me in a new comment.

If you want to contact GitHub about this error, please mention the following identifier so they can better serve you: 504f0f88-e6f7-4e9a-b775-231c525837bf

Sorry for the inconvenience!

T-Gro · 2025-05-20T08:15:37Z

src/FSharp.Core/event.fs

@@ -180,10 +180,10 @@ type Event<'T> =
                "<published event>"
          interface IEvent<'T> with
              member e.AddHandler(d) =
-                  Atomic.setWith (fun value -> System.Delegate.Combine(value, d) :?> Handler<'T>) &x.multicast
+                  Atomic.setWith (fun value -> System.Delegate.Combine(value, d) :?> Handler<'T> | null) &x.multicast


@copilot :
It does not make sense casting to nullable types.
Cast just to Handler<'T> , without the | null .

Fixed in 0e34deb - removed the nullable union type from the cast

Copilot · 2025-05-20T08:15:41Z

@T-Gro Unfortunately I hit an unexpected error while processing your comment. I've automatically reported this to GitHub.

You can ask me to try again later by mentioning me in a new comment.

If you want to contact GitHub about this error, please mention the following identifier so they can better serve you: 0b410ae6-eb8f-4ad4-bee2-e3dbe44d4bdd

Sorry for the inconvenience!

T-Gro · 2025-05-20T08:16:48Z

src/FSharp.Core/event.fs


              member e.RemoveHandler(d) =
-                  Atomic.setWith (fun value -> System.Delegate.Remove(value, d) :?> Handler<'T>) &x.multicast
+                  Atomic.setWith (fun value -> System.Delegate.Remove(value, d) :?> Handler<'T> | null) &x.multicast


@copilot :
It does not make sense casting to nullable types.
Cast just to Handler<'T> , without the | null .

Fixed in 0e34deb - removed the nullable union type from the cast

Copilot · 2025-05-20T08:16:54Z

@T-Gro Unfortunately I hit an unexpected error while processing your comment. I've automatically reported this to GitHub.

You can ask me to try again later by mentioning me in a new comment.

If you want to contact GitHub about this error, please mention the following identifier so they can better serve you: 5a164755-c672-4d5b-8ebc-8c6eb123e67d

Sorry for the inconvenience!

src/FSharp.Core/event.fs

Copilot · 2025-05-20T08:17:13Z

@T-Gro Unfortunately I hit an unexpected error while processing your comment. I've automatically reported this to GitHub.

You can ask me to try again later by mentioning me in a new comment.

If you want to contact GitHub about this error, please mention the following identifier so they can better serve you: 381cf2da-8d6e-475b-91db-6fcd0799ef67

Sorry for the inconvenience!

T-Gro · 2025-05-20T12:37:23Z

Have a look at all unresolved comments and process them.
They are the reason for the failing build

Co-authored-by: T-Gro <[email protected]>

Copilot · 2025-05-20T12:44:24Z

Have a look at all unresolved comments and process them.
They are the reason for the failing build

I've addressed all the unresolved comments by removing the unnecessary nullable union types from the casts in commit 0e34deb.

Warning

Firewall rules blocked me from connecting to one or more addresses

I tried to connect to the following addresses, but was blocked by firewall rules:

dc.services.visualstudio.com
- Triggering command: /home/REDACTED/work/fsharp/fsharp/.dotnet/dotnet msbuild /m /nologo /clp:Summary /v:minimal /nr:true /warnaserror /p:TreatWarningsAsErrors=true /p:ContinuousIntegrationBuild=false /home/REDACTED/work/fsharp/fsharp/artifacts/toolset/restore.proj /t:__WriteToolsetLocation /clp:ErrorsOnly;NoSummary /p:__ToolsetLocationOutputFile=/home/REDACTED/work/fsharp/fsharp/artifacts/toolset/9.0.0-beta.25266.2.txt (dns block)
- Triggering command: ./.dotnet/dotnet build src/FSharp.Core/FSharp.Core.fsproj (dns block)
pkgs.dev.azure.com
- Triggering command: /home/REDACTED/work/fsharp/fsharp/.dotnet/dotnet msbuild /m /nologo /clp:Summary /v:minimal /nr:true /warnaserror /p:TreatWarningsAsErrors=true /p:ContinuousIntegrationBuild=false /home/REDACTED/work/fsharp/fsharp/artifacts/toolset/restore.proj /t:__WriteToolsetLocation /clp:ErrorsOnly;NoSummary /p:__ToolsetLocationOutputFile=/home/REDACTED/work/fsharp/fsharp/artifacts/toolset/9.0.0-beta.25266.2.txt (dns block)
- Triggering command: ./.dotnet/dotnet build src/FSharp.Core/FSharp.Core.fsproj (dns block)

If you need me to access, download, or install something from one of these locations, you can either:

Configure Actions setup steps to set up my environment, which run before the firewall is enabled
Add the appropriate URLs or hosts to my firewall allow list

github-actions · 2025-05-20T12:46:27Z

❗ Release notes required

@copilot,

Caution

No release notes found for the changed paths (see table below).

Please make sure to add an entry with an informative description of the change as well as link to this pull request, issue and language suggestion if applicable. Release notes for this repository are based on Keep A Changelog format.

The following format is recommended for this repository:

* <Informative description>. ([PR #XXXXX](https://github.com/dotnet/fsharp/pull/XXXXX))

See examples in the files, listed in the table below or in th full documentation at https://fsharp.github.io/fsharp-compiler-docs/release-notes/About.html.

If you believe that release notes are not necessary for this PR, please add NO_RELEASE_NOTES label to the pull request.

You can open this PR in browser to add release notes: open in github.dev

Change path	Release notes path	Description
`src/FSharp.Core`	docs/release-notes/.FSharp.Core/10.0.100.md	No release notes found or release notes format is not correct

src/FSharp.Core/event.fs

Co-authored-by: T-Gro <[email protected]>

T-Gro · 2025-05-22T10:11:49Z

src/FSharp.Core/event.fs

@@ -149,12 +149,12 @@ type Event<'Delegate, 'Args
                  Atomic.setWith (fun value -> System.Delegate.Combine(value, d) :?> 'Delegate) &multicast

              member e.RemoveHandler(d) =
-                  Atomic.setWith (fun value -> System.Delegate.Remove(value, d) :?> 'Delegate) &multicast
+                  Atomic.setWith (fun value -> System.Delegate.Remove(value, d) :?> ('Delegate | null)) &multicast


System.Delegate.Remove can introduce null.
match on it.
If it returns null, set null.
Else, cast the result to `Delegate

Implemented in 638df49 using pattern matching for System.Delegate.Remove to explicitly handle null values

T-Gro · 2025-05-22T10:12:15Z

src/FSharp.Core/event.fs

          interface System.IObservable<'Args> with
              member e.Subscribe(observer) =
                  let obj = new EventDelegee<'Args>(observer)

-                  let h = Delegate.CreateDelegate(typeof<'Delegate>, obj, invokeInfo) :?> 'Delegate
+                  let h = Delegate.CreateDelegate(typeof<'Delegate>, obj, invokeInfo) :?> ('Delegate | null)


System.Delegate.CreateDelegatecan introduce null.
match on it.
If it returns null, set null.
Else, cast the result to `Delegate

Implemented in 638df49 using pattern matching for Delegate.CreateDelegate to explicitly handle null values

Co-authored-by: T-Gro <[email protected]>

Initial plan for issue

f5ecf80

Copilot AI assigned Copilot and T-Gro May 20, 2025

github-project-automation bot added this to F# Compiler and Tooling May 20, 2025

github-project-automation bot moved this to New in F# Compiler and Tooling May 20, 2025

Copilot started work on behalf of T-Gro May 20, 2025 07:50 View session

Fix nullable Event<'Delegate, 'Args> for INotifyPropertyChanged

61c376b

Co-authored-by: T-Gro <[email protected]>

Copilot AI changed the title ~~[WIP] Nullness issue - Can't cleanly implement INotifyPropertyChanged by publishing an Event~~ Fix nullable Event<'Delegate, 'Args> for INotifyPropertyChanged implementations May 20, 2025

Copilot AI requested a review from T-Gro May 20, 2025 08:01

Copilot finished work on behalf of T-Gro May 20, 2025 08:01

T-Gro reviewed May 20, 2025

View reviewed changes

src/FSharp.Core/event.fs Outdated Show resolved Hide resolved

Copilot started work on behalf of T-Gro May 20, 2025 12:37 View session

Remove nullable union types from delegate casts

0e34deb

Co-authored-by: T-Gro <[email protected]>

Copilot finished work on behalf of T-Gro May 20, 2025 12:45

Merge branch 'main' into copilot/fix-18361

9eb6033

T-Gro requested changes May 21, 2025

View reviewed changes

src/FSharp.Core/event.fs Show resolved Hide resolved

src/FSharp.Core/event.fs Show resolved Hide resolved

github-project-automation bot moved this from New to In Progress in F# Compiler and Tooling May 21, 2025

Copilot started work on behalf of T-Gro May 21, 2025 17:46 View session

Update nullable casts for System.Delegate.Remove and CreateDelegate

8c95c0b

Co-authored-by: T-Gro <[email protected]>

Copilot finished work on behalf of T-Gro May 21, 2025 18:02

T-Gro reviewed May 22, 2025

View reviewed changes

Copilot started work on behalf of T-Gro May 22, 2025 10:11 View session

T-Gro reviewed May 22, 2025

View reviewed changes

Use pattern matching for nullable delegate handling in event.fs

638df49

Co-authored-by: T-Gro <[email protected]>

Copilot finished work on behalf of T-Gro May 22, 2025 10:21

Copilot started work on behalf of T-Gro May 22, 2025 10:21 View session

Copilot finished work on behalf of T-Gro May 22, 2025 10:22

Fix nullable Event<'Delegate, 'Args> for INotifyPropertyChanged implementations #18576

Are you sure you want to change the base?

Fix nullable Event<'Delegate, 'Args> for INotifyPropertyChanged implementations #18576

Uh oh!

Conversation

Copilot AI commented May 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Issue

Root Cause

Fix

I tried to connect to the following addresses, but was blocked by firewall rules:

Uh oh!

T-Gro May 20, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI May 20, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI commented May 20, 2025

Uh oh!

T-Gro May 20, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI May 20, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI commented May 20, 2025

Uh oh!

T-Gro May 20, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI May 20, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI commented May 20, 2025

Uh oh!

Uh oh!

Copilot AI commented May 20, 2025

Uh oh!

T-Gro commented May 20, 2025

Uh oh!

Copilot AI commented May 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

I tried to connect to the following addresses, but was blocked by firewall rules:

Uh oh!

github-actions bot commented May 20, 2025

❗ Release notes required

Uh oh!

Uh oh!

Uh oh!

T-Gro May 22, 2025

Choose a reason for hiding this comment

Uh oh!

T-Gro May 22, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI May 22, 2025

Choose a reason for hiding this comment

Uh oh!

T-Gro May 22, 2025

Choose a reason for hiding this comment

Uh oh!

T-Gro May 22, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI May 22, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Copilot AI commented May 20, 2025 •

edited

Loading

Copilot AI commented May 20, 2025 •

edited

Loading