Skip to content

Fix RemoveImpl nullability #129661

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
MichalPetryka wants to merge 6 commits into
base: main
Choose a base branch
from
Open

Fix RemoveImpl nullability #129661

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
291e410
Fix RemoveImpl nullability
MichalPetryka Jun 20, 2026
7c3769b
Fix RemoveImpl method to handle null delegate
MichalPetryka Jun 20, 2026
75bc5a3
Update RemoveImpl method signature to be nullable
MichalPetryka Jun 20, 2026
b6178dc
Use null-forgiving operator in delegate comparison
MichalPetryka Jun 20, 2026
d2e634f
Rename variable 'value' to 'v' for clarity
MichalPetryka Jun 21, 2026
c0fe733
Merge branch 'main' into removeimpl-null
MichalPetryka Jun 22, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions src/coreclr/System.Private.CoreLib/src/System/MulticastDelegate.CoreCLR.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -290,29 +290,29 @@ private static bool EqualInvocationLists(object[] a, object[] b, int start, int
// look at the invocation list.) If this is found we remove it from
// this list and return a new delegate. If its not found a copy of the
// current list is returned.
protected sealed override Delegate? RemoveImpl(Delegate value)
protected sealed override Delegate? RemoveImpl(Delegate? value)
{
// There is a special case were we are removing using a delegate as
// the value we need to check for this case
//
MulticastDelegate? v = value as MulticastDelegate;

if (v == null)
return this;

if (v._invocationList is not object[])
{
if (_invocationList is not object[] invocationList)
{
// they are both not real Multicast
if (Equals(value))
if (Equals(v))
return null;
}
else
{
int invocationCount = (int)_invocationCount;
for (int i = invocationCount; --i >= 0;)
{
if (value.Equals(invocationList[i]))
if (v.Equals(invocationList[i]))
{
if (invocationCount == 2)
{
Expand Down
2 changes: 1 addition & 1 deletion src/coreclr/nativeaot/System.Private.CoreLib/src/System/Delegate.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -602,7 +602,7 @@ private static bool EqualInvocationLists(Wrapper[] a, Wrapper[] b, int start, in
// look at the invocation list.) If this is found we remove it from
// this list and return a new delegate. If its not found a copy of the
// current list is returned.
protected virtual Delegate? RemoveImpl(Delegate d)
protected virtual Delegate? RemoveImpl(Delegate? d)
{
// There is a special case were we are removing using a delegate as
// the value we need to check for this case
Expand Down
2 changes: 1 addition & 1 deletion src/libraries/System.Private.CoreLib/src/System/Delegate.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,7 @@ public abstract partial class Delegate : ICloneable, ISerializable
#if !NATIVEAOT
protected virtual Delegate CombineImpl(Delegate? d) => throw new MulticastNotSupportedException(SR.Multicast_Combine);

protected virtual Delegate? RemoveImpl(Delegate d) => d.Equals(this) ? null : this;
protected virtual Delegate? RemoveImpl(Delegate? d) => Equals(d) ? null : this;

@MichalPetryka MichalPetryka Jun 20, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This doesn't change behaviour in practice since all runtimes override the method.

@jkotas jkotas Jun 21, 2026 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

These 3 methods cannot be overridden by user code either. Would it be possible to make them non-virtual instead of this? Follow the same pattern as HasSingleTarget for now, ideally we would move the code from Multicast to Delegate to avoid the forwarder, but that should be its own PR.

Making these 3 methods non-virtual would save 3 virtual slots in every delegate type.

@MichalPetryka MichalPetryka Jun 21, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

They're a part of the API contract in ref assembly so we can't remove them.

@jkotas jkotas Jun 21, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

It is fine to change the ref assemblies as long as the change is non-breaking. Can you think about a scenario that would be broken by removing virtual on these methods?

@MichalPetryka MichalPetryka Jun 21, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

It is fine to change the ref assemblies as long as the change is non-breaking. Can you think about a scenario that would be broken by removing virtual on these methods?

One I can think of is that creating a closed delegate to it would now be different between Delegate and MulticastDelegate.

@jkotas jkotas Jun 22, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

You can do that only via private reflection or invalid IL. We are not concerned about that.

https://github.com/dotnet/runtime/blob/main/docs/coding-guidelines/breaking-change-rules.md has the list of allows vs. disallowed changes. Removing virtual on these methods is equivalent to combination of the following allowed changes:

  • Moving a method onto a class higher in the hierarchy tree of the type from which it was removed
  • Decreasing the visibility of a protected member when there are no accessible (public or protected) constructors or the type is sealed

@MichalPetryka MichalPetryka Jun 22, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

You can do that only via private reflection or invalid IL.

Since the method is protected and in ref assembly, afair the reflection is not private here?

@MichalStrehovsky MichalStrehovsky Jun 22, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

You can do that only via private reflection or invalid IL.

Since the method is protected and in ref assembly, afair the reflection is not private here?

The existing "Decreasing the visibility of a protected member when there are no accessible (public or protected) constructors or the type is sealed" would be reflection-breaking too if this was considered supported reflection. The only thing that could legitimately reflect on the protected members would be delegate descendants, but they cannot effectively have any code in it.


public virtual Delegate[] GetInvocationList() => [this];

Expand Down
4 changes: 2 additions & 2 deletions src/libraries/System.Runtime/ref/System.Runtime.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2370,7 +2370,7 @@ public virtual void GetObjectData(System.Runtime.Serialization.SerializationInfo
public static bool operator !=(System.Delegate? d1, System.Delegate? d2) { throw null; }
public static System.Delegate? Remove(System.Delegate? source, System.Delegate? value) { throw null; }
public static System.Delegate? RemoveAll(System.Delegate? source, System.Delegate? value) { throw null; }
protected virtual System.Delegate? RemoveImpl(System.Delegate d) { throw null; }
protected virtual System.Delegate? RemoveImpl(System.Delegate? d) { throw null; }
public partial struct InvocationListEnumerator<TDelegate> where TDelegate : System.Delegate
{
public TDelegate Current { get { throw null; } }
Expand Down Expand Up @@ -4803,7 +4803,7 @@ public abstract partial class MulticastDelegate : System.Delegate
public override void GetObjectData(System.Runtime.Serialization.SerializationInfo info, System.Runtime.Serialization.StreamingContext context) { }
public static bool operator ==(System.MulticastDelegate? d1, System.MulticastDelegate? d2) { throw null; }
public static bool operator !=(System.MulticastDelegate? d1, System.MulticastDelegate? d2) { throw null; }
protected sealed override System.Delegate? RemoveImpl(System.Delegate value) { throw null; }
protected sealed override System.Delegate? RemoveImpl(System.Delegate? value) { throw null; }
}
public sealed partial class MulticastNotSupportedException : System.SystemException
{
Expand Down
2 changes: 1 addition & 1 deletion src/mono/System.Private.CoreLib/src/System/MulticastDelegate.Mono.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -197,7 +197,7 @@ private static int LastIndexOf(Delegate[] haystack, Delegate[] needle)
return -1;
}

protected sealed override Delegate? RemoveImpl(Delegate value)
protected sealed override Delegate? RemoveImpl(Delegate? value)
{
if (value == null)
return this;
Expand Down
Loading