Skip to content

Move bulk of the CoreCLR delegate implementation to Delegate.CoreCLR.cs - #130738

Merged
jkotas merged 2 commits into
dotnet:mainfrom
jkotas:issue-130612
Jul 15, 2026
Merged

Move bulk of the CoreCLR delegate implementation to Delegate.CoreCLR.cs#130738
jkotas merged 2 commits into
dotnet:mainfrom
jkotas:issue-130612

Conversation

@jkotas

@jkotasjkotas commented Jul 14, 2026

Copy link
Copy Markdown
Member

@jkotas
Move bulk of the CoreCLR delegate implementation to Delegate.CoreCLR.cs
12287ac
- Matches structure of NativeAOT delegate impletation
- Avoids Unsafe.As casts
- Avoids C# "new slots" that are breaking code produced by managed C++. Fixes app compat bug dotnet#130612.
CopilotAI review requested due to automatic review settings July 14, 2026 22:49
@azure-pipelines

azure-pipelinesBot commented Jul 14, 2026

Copy link
Copy Markdown
Azure Pipelines:
Successfully started running 3 pipeline(s).
12 pipeline(s) were filtered out due to trigger conditions.
There may be pipelines that require an authorized user to comment /azp run to run.

@dotnet-policy-service

dotnet-policy-serviceBot commented Jul 14, 2026

Copy link
Copy Markdown
Contributor

Tagging subscribers to this area: @steveisok, @dotnet/area-system-reflection
See info in area-owners.md if you want to be subscribed.

jkotas
jkotas commented Jul 14, 2026
View reviewed changes
Comment threadsrc/mono/System.Private.CoreLib/src/System/MulticastDelegate.Mono.cs
jkotas
jkotas commented Jul 14, 2026
View reviewed changes
Comment threadsrc/libraries/System.Private.CoreLib/src/System/Delegate.cs
jkotas
jkotas commented Jul 14, 2026
View reviewed changes
Comment threadsrc/coreclr/System.Private.CoreLib/src/System/Delegate.CoreCLR.cs
@jkotasjkotas mentioned this pull request Jul 14, 2026
Closed
CopilotAI reviewed Jul 14, 2026
View reviewed changes

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Refactors delegate implementation across CoreCLR / Mono / shared CoreLib sources by moving multicast-combine/remove and delegate-constructor logic into Delegate.CoreCLR.cs, aligning structure with other runtimes and addressing an app-compat regression involving delegate member accessibility/resolution.

Changes:

  • CoreCLR: move multicast combine/remove + delegate ctor helpers from MulticastDelegate.CoreCLR.cs into Delegate.CoreCLR.cs, and update VM binders/callers to reference System.Delegate instead of System.MulticastDelegate.
  • Mono: avoid hidden new members on MulticastDelegate by renaming internal helpers and routing through Delegate (and switch Target to use GetTarget()).
  • Shared: move COM visibility attributes onto the shared Delegate/MulticastDelegate definitions and unify delegate serialization failure to SerializationException.

Reviewed changes

Copilot reviewed 11 out of 11 changed files in this pull request and generated 2 comments.

Show a summary per file
FileDescription
src/mono/System.Private.CoreLib/src/System/MulticastDelegate.Mono.csRemoves hidden new members (serialization + target) and renames internal helpers to avoid name collisions.
src/mono/System.Private.CoreLib/src/System/Delegate.Mono.csRoutes Target/combine/remove/invocation-list through non-colliding helpers on MulticastDelegate.
src/libraries/System.Private.CoreLib/src/System/MulticastDelegate.csAdds COM visibility attributes in shared source and declares ISerializable on the shared type.
src/libraries/System.Private.CoreLib/src/System/Delegate.csAdds COM visibility attributes and changes GetObjectData to throw SerializationException.
src/coreclr/vm/corelib.hMoves delegate ctor method bindings from MULTICAST_DELEGATE to DELEGATE.
src/coreclr/vm/comdelegate.cppUpdates delegate construction to bind to METHOD__DELEGATE__CTOR_* methods.
src/coreclr/vm/appdomain.cppRemoves CLASS__MULTICAST_DELEGATE from the reflection-invocation skip list.
src/coreclr/System.Private.CoreLib/System.Private.CoreLib.csprojDrops compilation of MulticastDelegate.CoreCLR.cs (logic moved elsewhere).
src/coreclr/System.Private.CoreLib/src/System/MulticastDelegate.CoreCLR.csDeleted; prior CoreCLR multicast/ctor implementation relocated.
src/coreclr/System.Private.CoreLib/src/System/Delegate.CoreCLR.csBecomes the primary home for multicast combine/remove and delegate ctor helpers in CoreCLR.
src/coreclr/nativeaot/System.Private.CoreLib/src/System/Delegate.csRemoves an unused internal helper and makes HasSingleTargetpartial for consistency.

Comment threadsrc/mono/System.Private.CoreLib/src/System/MulticastDelegate.Mono.cs
Comment threadsrc/coreclr/System.Private.CoreLib/src/System/Delegate.CoreCLR.cs
MichalPetryka
MichalPetryka reviewed Jul 15, 2026
View reviewed changes

@MichalPetrykaMichalPetryka left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Some other usings may be not needed now.

Comment threadsrc/coreclr/System.Private.CoreLib/src/System/Delegate.CoreCLR.cs
Comment threadsrc/coreclr/vm/corelib.h Outdated
Comment threadsrc/libraries/System.Private.CoreLib/src/System/Delegate.cs
CopilotAI review requested due to automatic review settings July 15, 2026 06:38
CopilotAI reviewed Jul 15, 2026
View reviewed changes

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 12 out of 12 changed files in this pull request and generated 1 comment.

Comment threadsrc/coreclr/System.Private.CoreLib/src/System/Delegate.CoreCLR.cs
This was referenced Jul 15, 2026
Open
Open
tannergooding
tannergooding reviewed Jul 15, 2026
View reviewed changes
Comment threadsrc/coreclr/System.Private.CoreLib/src/System/Delegate.CoreCLR.cs
tannergooding
tannergooding reviewed Jul 15, 2026
View reviewed changes
Comment threadsrc/coreclr/System.Private.CoreLib/src/System/Delegate.CoreCLR.cs
tannergooding
tannergooding approved these changes Jul 15, 2026
View reviewed changes

@tannergoodingtannergooding left a comment

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.

Changes LGTM. Just a couple small questions

@jkotas
jkotas merged commit 327e247 into dotnet:mainJul 15, 2026
149 of 151 checks passed
@jkotas

jkotas commented Jul 15, 2026

Copy link
Copy Markdown
MemberAuthor

@MichalPetryka You can start working on the follow up optimizations now

@dotnet-maestrodotnet-maestroBot mentioned this pull request Jul 16, 2026
Merged
@dotnet-milestone-botdotnet-milestone-botBot added this to the 11.0-preview7 milestone Jul 16, 2026
@github-actionsgithub-actionsBot locked and limited conversation to collaborators Aug 16, 2026
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@tannergoodingtannergoodingtannergooding approved these changes
@AaronRobinsonMSFTAaronRobinsonMSFTAaronRobinsonMSFT left review comments
Copilot code reviewCopilot
Copilot review effort
Applies to this pull request for everyone.Learn more about Copilot code review.
Copilot left review comments
@steveisoksteveisokAwaiting requested review from steveisoksteveisok is a code owner
@vitek-karasvitek-karasAwaiting requested review from vitek-karasvitek-karas is a code owner
@MichalStrehovskyMichalStrehovskyAwaiting requested review from MichalStrehovskyMichalStrehovsky is a code owner
+1 more reviewer
@MichalPetrykaMichalPetrykaMichalPetryka approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

@jkotasjkotas

Labels

area-System.Reflection

Projects

None yet

Milestone

11.0-preview7

Development

Successfully merging this pull request may close these issues.

5 participants

@jkotas@tannergooding@AaronRobinsonMSFT@MichalPetryka