System.Reflection: Replicate custom modifiers of method parameters in MemberRef signatures

[stakx]

This is in response to https://github.com/dotnet/corefx/issues/29254.

ModuleBuilder.GetMemberRefToken reproduces a method's signature for use in a MethodRef metadata entry, but currently ignores custom modifiers placed on the method's parameters... which leads to non-matching signatures and MissingMethodExceptions when using e.g. reflection.

This commit adds a new method overload (in order to avoid a larger refactoring for now) that allows the inclusion of custom modifiers on parameters.

This is currently work in progress:

• Needs further testing.
  I ran successful end-to-end tests using tests from Castle DynamicProxy, which has tests targeting this. I'm going to submit another PR with tests for dotnet/corefx.

• Are there any code paths into ModuleBuilder.GetMemberRefToken that are negatively affected by the change made?
  It appears unlikely and did not find any (but it's pretty hard to keep track here, given all the branching and similarly named methods strewn across at least two classes).

• Are there any other code paths into the method that could profit from the change made?
  Additional code paths have been included in the second commit of this PR.

• Shouldn't the same be done for the methods' return type (not just its parameters)?
  Yes, but as it turns out, doing this for return parameters poses some unique problems that are perhaps better dealt with in a separate, dedicated PR.

[ghost]

One quick note: this will propagate custom modifiers attached to the parameter type itself but not any types are used to construct the type. E.g. a parameter of type int[] can have a custom modifier attached to the int[] or the int - you'll only propagate the former.

This is a long standing defect in how Reflection thinks about custom modifiers and it's not likely you could address this here without majorly impacting the cost of the fix. Just noting it here so we know the limitation exists, that this fix still solves the problem you wanted to solve with that limitation and that we're proceeding with knowledge of that.

[stakx]

@atsushikan - I've suggested above that perhaps this PR shouldn't just focus on the ldtoken code path and on parameter cmods. Perhaps, for consistency, other code paths (e.g. call / calli) and return parameter cmods should also be taken into account. That would be a larger refactoring involving a variety of things, e.g.:

• Changing internal methods to accept additional parameters (Type[][] returnType[Required|Optional]CustomModifiers and parameterType[Required|Optional]CustomModifiers).
• Turning methods like MethodBuilder.GetMethodBaseReturnType into MethodBuilder.GetMethodBaseReturnParameter so that cmods can be retrieved.
• Some additional array allocations due to having to query ParameterInfo.Get[Required|Optional]CustomModifiers() in more places.
• Adding a new public API method overload ILGenerator.EmitCalli that also has parameters for cmods.

Is this the right time and place to do all of this (well, maybe not the last bullet point)? Or should I stick to the original, narrow scenario that led to this PR for now? (I'm fine with both.)

Update: I'll just go ahead and prepare all changes (except for additions to the public API). Given that the C# compiler also produces modreqs on return types (ref readonly returns) it seems sensible to include them, too. I can always drop some of the later commits if you feel I've taken this too far.

[ghost]

I would just turn this into a for (int i = …) loop.

[stakx]

@atsushikan - I started working on return parameters, but found that this poses some additional problems and puzzles to solve. I think it'd be better to deal with return parameters in a separate, dedicated PR. In my opinion, the present PR is ready for review / merging.

(Expand this to see an example of an additional problem unique to return parameters.)

In this PR, often a Type parameterType gets replaced with a ParameterInfo parameter from which custom modifiers can be accessed. If we follow the same procedure for return parameters, we need to e.g. deal with this existing method:

coreclr/src/System.Private.CoreLib/src/System/Reflection/RuntimeConstructorInfo.cs

Lines 231 to 232 in e353a22

	// This seems to always returns System.Void.
	internal override Type GetReturnType() { return Signature.ReturnType; }

Either a corresponding new method ParameterInfo RuntimeConstructorInfo.GetReturnParameter() is added (which would mean that a return ParameterInfo needs to be extracted from a signature, and cached), or the above existing method is removed (which might be preferable, as it doesn't appear to make too much sense to begin with).

It's these kinds of decisions I'd like to shift to a separate PR to keep the present one focused.

[ghost]

Every caller of this passes a non-null parameters. The ? isn't needed.

[stakx]

Thanks for the review. I've replaced all three null-conditional member accesses ?. with ..

[ghost]

The ? isn't needed. GetRequiredCustomModifiers never returns a null array.

[ghost]

Same comment regarding ?

[ghost]

Other than noted, LGTM.

[TAGC]

Thanks for the work @stakx. 👍

[stakx]

@TAGC - Thanks for the thumbs-up, I appreciate it! Not quite done yet, but we're getting there.