PersistedAssemblyBuilder: Include custom modifiers when importing method reference signatures.

[teo-tsirpanis]

Fixes #123857.

Added test that passes locally after the fix.

[Copilot]

Pull request overview

Fixes PersistedAssemblyBuilder emitting incorrect method reference signatures by ensuring required/optional custom modifiers (e.g., modreq(InAttribute) for in parameters) are preserved, preventing TypeLoadException when loading saved assemblies.

Changes:

• Update signature blob generation to include custom modifiers from reflected parameter/return Types when explicit modifier arrays aren’t provided.
• Add a regression test covering an interface method override involving a required custom modifier (modreq).
• Update the test project to include TempDirectory test infrastructure and reference the System.Reflection.Emit project.

Reviewed changes

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

File	Description
src/libraries/System.Reflection.Emit/tests/System.Reflection.Emit.Tests.csproj	Adds TempDirectory support and a project reference needed for the new test scenario.
src/libraries/System.Reflection.Emit/tests/PersistedAssemblyBuilder/AssemblySaveTypeBuilderTests.cs	Adds regression test that saves/loads an assembly implementing an in-parameter interface method.
src/libraries/System.Reflection.Emit/src/System/Reflection/Emit/SignatureHelper.cs	Ensures method signature encoding includes required/optional custom modifiers from the Type when importing method references.

[Copilot]

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.

[teo-tsirpanis]

Hmm, Mono is failing. 🤔

[Copilot]

Pull request overview

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

[teo-tsirpanis]

I took a look at the new Mono failure. NestedType.TypeSignature.ParameterIndex contains the index of the method parameter, but here we treat it as the index of the generic parameter?

[jkotas]

Sounds like a bug to me.

[stephentoub]

🤖 Copilot Code Review — PR #123925

Holistic Assessment

Motivation: The PR addresses a clear bug: PersistedAssemblyBuilder failed to preserve custom modifiers (like modreq(InAttribute) for in parameters) when importing method signatures, causing TypeLoadException when loading saved assemblies. This is a genuine blocking issue for users trying to implement interfaces with in parameters via PersistedAssemblyBuilder.

Approach: The fix correctly falls back to extracting custom modifiers from the Type objects (via GetRequiredCustomModifiers() / GetOptionalCustomModifiers()) when explicit modifier arrays are not provided. This is the right approach since the GetModifiedParameterType() API was designed precisely for this use case. The PR also fixes a pre-existing Mono bug discovered during testing.

Net positive/negative: This is a clear net positive—it fixes a user-reported bug with proper regression test coverage and also addresses a related Mono issue.

---

Detailed Findings

✅ SignatureHelper.cs — Custom modifier fallback logic is correct

The changes to GetMethodSignature and WriteParametersSignature correctly add fallback logic to retrieve custom modifiers from the return type and parameter types when explicit modifier arrays are null:

// Return type: falls back to type's modifiers
WriteReturnTypeCustomModifiers(..., returnTypeRequiredModifiers ?? returnType?.GetRequiredCustomModifiers(), ...)

// Parameters: falls back to parameter's modifiers
Type[] modreqs = requiredModifiers?[i] ?? parameters[i].GetRequiredCustomModifiers();

This preserves the modifiers that are embedded in ModifiedType instances returned by GetModifiedParameterType().

✅ Mono ModifiedType fix — Correctly addresses ParameterIndex confusion

The Mono changes fix a bug where ParameterIndex was being confused between two different meanings:

1. The index of the method parameter (used in GetModifiedParameterType)
2. The index of a generic parameter (used in GetCustomModifiersFromModifiedType)

The fix:

• Changes TypeSignature to a readonly struct (good practice)
• Uses -1 to indicate "direct parameter type" (no generic argument drilling)
• Simplifies GetTypeParameter to properly handle function pointer parents

This aligns with the CoreCLR implementation which uses offset-based navigation rather than dual-meaning indices.

✅ Test coverage is adequate

The SaveInterfaceOverrideWithCustomModifier test:

• Creates an interface implementation with in parameters (triggers the bug)
• Uses GetModifiedParameterType() with null modifier arrays (the problematic pattern)
• Saves the assembly and loads it in a collectible ALC
• Verifies the type loads without TypeLoadException

The test includes a function pointer parameter with nested custom modifiers (delegate*<in long, void>), which exercises the more complex code paths.

💡 Test cleanup — Consider separating concerns

The test uses TempDirectory while most other tests in this file use TempFile. This is fine for the immediate need (the test needs the directory path to construct the file path), but consider whether using TempFile directly would be more consistent:

using (TempFile file = TempFile.Create())
{
    // ...
    assemblyBuilder.Save(file.Path);
}

This is a minor style observation, not blocking.

💡 AssemblySaveILGeneratorTests.cs — Unrelated changes

The BOM removal and nestedParam → sectorParam rename are unrelated to the PR's purpose. These are harmless cleanup but could be separated into a follow-up PR for cleaner git history.

---

Summary

LGTM. The PR correctly fixes the reported issue where PersistedAssemblyBuilder failed to preserve custom modifiers when importing method signatures. The fix is targeted, the test coverage is adequate, and the Mono fix addresses a real pre-existing bug discovered during testing. No blocking issues identified.

Note: This review was synthesized from multiple model perspectives (Claude Sonnet 4, Gemini 3 Pro). Both models agreed on the correctness of the fix.

[Copilot]

Pull request overview

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

[Copilot]

Pull request overview

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

[Copilot]

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated no new comments.

[Copilot]

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated no new comments.