Fix RemoveImpl nullability#129661
Conversation
MichalPetryka
requested review from
MichalStrehovsky,
steveisok and
vitek-karas
as code owners
dotnet-policy-service
Bot
added
the
community-contribution
|
Tagging subscribers to this area: @steveisok, @dotnet/area-system-reflection |
| 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; |
There was a problem hiding this comment.
This doesn't change behaviour in practice since all runtimes override the method.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
They're a part of the API contract in ref assembly so we can't remove them.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
Add null-forgiving operator to value in Equals check.
3 participants
RemoveImpl accepts null just like CombineImpl does.