Cleanup dead delegate code by MichalPetryka · Pull Request #129662 · dotnet/runtime

MichalPetryka · 2026-06-21T00:31:25Z

Split off from #99200

MichalPetryka · 2026-06-21T00:32:10Z


        internal void StoreDynamicMethod(MethodInfo dynamicMethod)
        {
-            Debug.Assert(_invocationCount == 0);


This assert was incorrect for oven virtual delegates.

This method is only ever used to create dynamic method delegate. I do not think there is a way to have open virtual delegate that points to dynamic method.

We may want to delete this method instead (see my other feedback).

dotnet-policy-service · 2026-06-21T00:32:37Z

Tagging subscribers to this area: @agocke
See info in area-owners.md if you want to be subscribed.

jkotas · 2026-06-21T00:59:12Z

-        // in the other cases we return the method desc for the invoke
-        innerDel = (DELEGATEREF) thisDel->GetInvocationList();
-        bool fOpenVirtualDelegate = false;
+        //                    or _invocationList points to a LoaderAllocator/DynamicResolver


I do not think _invocationList ever points to a LoaderAllocator/DynamicResolver in the current implementation. Do you agree? I think all comments and code (e.g. InvocationListLogicallyNull ) that deal with it can be deleted.

Ah right I've only moved it there in my other PR. Since that makes it the case, I'd not touch this however.

Before this delta, the comment was referring to wrapper Delegate so one can guess it is obsolete. After this delta, the comment is simply incorrect that is not an improvement.

I understand that some of the changes will conflict. It is the cost of splitting large changes into multiple PRs. It is important that each of the split PRs is coherent on its own as much as possible.

jkotas · 2026-06-21T01:03:56Z

@@ -379,8 +379,7 @@

            // Initialize the method...


Suggested change

// Initialize the method...

Non-sensical comment

jkotas · 2026-06-21T01:05:25Z

            Delegate d = InternalAlloc(rtType);
-            // This is a new internal API added in Whidbey. Currently it's only
-            // used by the dynamic method code to generate a wrapper delegate.
+            // This is a new internal API added in Whidbey.


Suggested change

// This is a new internal API added in Whidbey.

Whibey shipped in 2005. This is not new anymore.

jkotas · 2026-06-21T01:10:37Z

@@ -3268,7 +3268,6 @@ DacDbiInterfaceImpl::DelegateType DacDbiInterfaceImpl::GetDelegateType(VMPTR_Obj
    {
        // If this delegate points to a static function or this is a open virtual delegate, this should be non-null


Is this comment correct? Open virtual delegate is going to have _invocationCount != 0.

runtime/src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Contracts/Object_1.cs

Lines 166 to 170 in e556d5a

// Classify by invocation count first:

// anything other than 0 indicates a multicast/wrapper/special-sig delegate

// that this API does not interpret further. Only when invocationCount==0

// do MethodPtr/MethodPtrAux unambiguously identify a closed/open delegate.

DelegateType delegateType = DelegateType.Unknown;

runtime/docs/design/datacontracts/Object.md

Line 214 in e556d5a

0 => del.MethodPtrAux == TargetCodePointer.Null

should be update to match too

I've fixed that in the other PR so I'd rather not touch this here.

I thought that the primary goal of this PR is to delete all remnants of the wrapper stubs that I have missed.

jkotas · 2026-06-21T01:14:04Z

        // internal implementation details (FCALLS and utilities)
        //

        // V2 internal API.


Suggested change

jkotas · 2026-06-21T01:16:05Z

        //

        // V2 internal API.
        internal static Delegate CreateDelegateNoSecurityCheck(Type type, object? target, RuntimeMethodHandle method)


Suggested change

internal static Delegate CreateDelegateForDynamicMethod(Type type, object? target, RuntimeMethodHandle method)

We may want to give this method more self-describing name.

Also, we may want to pass in MethodInfo as an argument and delete StoreDynamicMethod.
.

jkotas · 2026-06-21T01:28:49Z


        internal void StoreDynamicMethod(MethodInfo dynamicMethod)
        {
-            Debug.Assert(_invocationCount == 0);


This method is only ever used to create dynamic method delegate. I do not think there is a way to have open virtual delegate that points to dynamic method.

We may want to delete this method instead (see my other feedback).

jkotas · 2026-06-21T01:46:18Z

-                fOpenVirtualDelegate = true;
-            }
-        }
+        // we return the method desc for the invoke


This looks like unfinished sentence. Should this say "... for the first two cases" or something?

Cleanup dead delegate code

cfa940d

github-actions Bot added the area-VM-coreclr label Jun 21, 2026

dotnet-policy-service Bot added the community-contribution Indicates that the PR has been added by a community member label Jun 21, 2026

MichalPetryka commented Jun 21, 2026

View reviewed changes

jkotas reviewed Jun 21, 2026

View reviewed changes

		@@ -3268,7 +3268,6 @@ DacDbiInterfaceImpl::DelegateType DacDbiInterfaceImpl::GetDelegateType(VMPTR_Obj
		{
		// If this delegate points to a static function or this is a open virtual delegate, this should be non-null

	// Classify by invocation count first:
	// anything other than 0 indicates a multicast/wrapper/special-sig delegate
	// that this API does not interpret further. Only when invocationCount==0
	// do MethodPtr/MethodPtrAux unambiguously identify a closed/open delegate.
	DelegateType delegateType = DelegateType.Unknown;


	internal static Delegate CreateDelegateForDynamicMethod(Type type, object? target, RuntimeMethodHandle method)

Conversation

MichalPetryka commented Jun 21, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dotnet-policy-service Bot commented Jun 21, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants