Skip to content

Conversation

@samblackburn
Copy link
Owner

@samblackburn samblackburn commented Oct 10, 2025

Context

#11 added basic support for delegates by detecting calls to the special constructor they use, and hardcoding _ => default which works for any non-void signature with 1 parameter.

It works for what we need (currently there's only 1 delegate consumed across our code), but it would be nice to support all delegate signatures and to assert that the signature is still correct.

Approach

My approach to this PR was to run the DelegateWithOneArgument test, put a breakpoint in AssemblyAnalyzer and dump the opcodes for the method.

MyDelegate MakeDelegate() => Foo;
bool Foo(int i) => i > 3;
IL_0000: ldarg.0
IL_0001: ldftn System.Boolean Name.Space.ReferencingClass::Foo(System.Int32)
IL_0007: newobj System.Void MyDelegate::.ctor(System.Object,System.IntPtr)
IL_000c: ret

We can see that the instruction before the call to the constructor is loading a pointer to the function whose signature we care about.

Risks

This will break if the function pointer isn't loaded immediately before the constructor is called. To be honest I have no idea if this is likely to happen, but I've not found a way to make it happen yet.

Related reading

https://www.codeproject.com/articles/Dynamic-Runtime-Delegates-Conversion

Debugging is way easier when I can look for a method name
…hod we care about

So the IL from the `DelegateWithOneArgument` test looks like this:

IL_0000: ldarg.0
IL_0001: ldftn System.Boolean Name.Space.ReferencingClass::Foo(System.Int32)
IL_0007: newobj System.Void MyDelegate::.ctor(System.Object,System.IntPtr)
IL_000c: ret
@samblackburn samblackburn requested a review from Copilot October 10, 2025 15:59
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR enhances delegate support by detecting the actual method signatures of delegates instead of using a hardcoded fallback. The implementation analyzes IL opcodes to extract delegate signatures from the ldftn instruction that precedes delegate constructor calls.

Key changes:

  • Enhanced delegate signature detection by analyzing IL opcodes
  • Removed test ignores and added comprehensive delegate test coverage
  • Fixed test framework issue with multiple NuGet package versions

Reviewed Changes

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

File Description
Tests/TestFramework/ClrAssemblyCompiler.cs Fixed assertion to handle multiple package versions
Tests/DelegatesTests.cs Enabled previously ignored tests and added new test cases
NetDoc/Call.cs Added delegate signature detection logic using IL opcode analysis

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

m_Operand = (MemberReference) instruction.Operand;
if (instruction.OpCode == OpCodes.Ldfld || instruction.OpCode == OpCodes.Stfld)
if (instruction.OpCode == OpCodes.Newobj &&
MethodReference?.Parameters.Select(x => x.ParameterType.Name).SequenceEqual([nameof(Object), nameof(IntPtr)]) == true &&
Copy link

Copilot AI Oct 10, 2025

Choose a reason for hiding this comment

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

Potential null reference exception if instruction.Previous is null. This could happen if the Newobj instruction is the first instruction in a method.

Suggested change
MethodReference?.Parameters.Select(x => x.ParameterType.Name).SequenceEqual([nameof(Object), nameof(IntPtr)]) == true &&
MethodReference?.Parameters.Select(x => x.ParameterType.Name).SequenceEqual([nameof(Object), nameof(IntPtr)]) == true &&
instruction.Previous != null &&

Copilot uses AI. Check for mistakes.
@samblackburn samblackburn merged commit 5bfb99d into main Oct 10, 2025
1 check passed
@samblackburn samblackburn deleted the delegate-signatures branch October 10, 2025 16:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants