jbevain / cecil

Cecil is a library to inspect, modify and create .NET programs and libraries.
MIT License
2.74k stars 625 forks source link

Public GenericParameter constructor that takes context-free position #612

Closed jnm2 closed 5 years ago

jnm2 commented 5 years ago

In a project using Mono.Cecil to write IL, I need to specify member references such as System.Collections.Generic.Dictionary`2<string, string>::.ctor(class System.Collections.Generic.IEqualityComparer`1<!0>) Because the corresponding reference assemblies may not be available at the time, I'm not loading mscorlib.dll/System.Collections.dll/netstandard.dll etc. Just emitting the correct assembly name reference (if needed) and the correct IL. That requires me to construct the type specification for System.Collections.Generic.IEqualityComparer`1<!0> without having a GenericParameterCollection from the reference assembly to set the generic parameter position.

The only option so that the project is not blocked is to use reflection to get the Position property set to something other than -1 which of course serialization chokes on:

var genericParameter0 = (GenericParameter)typeof(GenericParameter)
    .GetConstructor(
        BindingFlags.NonPublic | BindingFlags.Instance,
        null,
        new[] { typeof(int), typeof(GenericParameterType), typeof(ModuleDefinition) },
        null)
    .Invoke(new object[] { 0, GenericParameterType.Type, module });

// Instantiate IEqualityComparer<> with Dictionary<,>'s TKey
var equalityComparer = new TypeReference(
    "System.Collections.Generic",
    "IEqualityComparer`1",
    module,
    scope: assemblyContainingSystemCollections);

var equalityComparerInstantiation = new GenericInstanceType(equalityComparer)
{
    GenericArguments = { genericParameter0 }
};

il.Emit(OpCodes.Newobj, new MethodReference(
    ".ctor",
    returnType: dictionaryField.Module.TypeSystem.Void,
    declaringType: dictionaryField.FieldType)
{
    HasThis = true,
    Parameters =
    {
        new ParameterDefinition(module.ImportReference(equalityComparerInstantiation))
    }
});

Ideally, this constructor would be public:

https://github.com/jbevain/cecil/blob/12da1bf7e0e10278e05eeb23adfc72760bbb6668/Mono.Cecil/GenericParameter.cs#L188-L198

namespace Mono.Cecil
{
    public sealed class GenericParameter : TypeReference, ICustomAttributeProvider
    {
        public GenericParameter(IGenericParameterProvider owner);

        public GenericParameter(string name, IGenericParameterProvider owner);

+       public GenericParameter(
+           int position,
+           GenericParameterType type,
+           ModuleDefinition module);
    }
}

Does this sound good to you?

jbevain commented 5 years ago

It's hard to say without looking more at what you're doing, but this should not be needed. Generic contexts are working with references and definitions.

Could you show more of what you're doing? You're not using genericParameter0 in the sample you posted. And the comment:

// Instantiate IEqualityComparer<> with Dictionary<,>'s TKey
var equalityComparerInstantiation = new TypeReference(

Feels wrong, an instantiation would be a GenericInstanceType.

jnm2 commented 5 years ago

Oops, I left out the GenericInstanceType part. I fixed it above. Here's that along with more of what I'm doing with it:

// Instantiate IEqualityComparer<> with Dictionary<,>'s TKey
var equalityComparerInstantiation = new GenericInstanceType(equalityComparer)
{
    GenericArguments = { genericParameter0 }
};

// Emit a call to the Dictionary`2 constructor with the signature
// void (System.Collections.Generic.IEqualityComparer`1<!0>)
il.Emit(OpCodes.Newobj, new MethodReference(
    ".ctor",
    returnType: dictionaryField.Module.TypeSystem.Void,
    declaringType: dictionaryField.FieldType)
{
    HasThis = true,
    Parameters =
    {
        new ParameterDefinition(module.ImportReference(equalityComparerInstantiation))
    }
});

All the type references and method references I'm constructing are not from loaded assemblies. I don't have the assemblies available to load, and if I did, that would be a lot of complexity.

All I'm interested in doing is emitting this IL:

newobj instance void class System.Collections.Generic.Dictionary`2<string, string>::.ctor(
    class System.Collections.Generic.IEqualityComparer`1<!0>)

and anything more than that is getting in the way.

jbevain commented 5 years ago

You want to emit:

newobj instance void class System.Collections.Generic.Dictionary`2<string, string>::.ctor(class System.Collections.Generic.IEqualityComparer`1<!0>)

Based on your code, the declaringType for the ctor is:

dictionaryField.FieldType

Which means that dictionaryField.FieldType is a GenericInstanceType that is instantiating Dictionary<TKey,TValue> with <string, string>.

So the ElementType of the GenericInstanceType is going to be a TypeReference, it's going to have a .GenericParameters collection.

Basically, you want to use the [0] of the GenericParameters of that instead of crafting your own.

jnm2 commented 5 years ago

@jbevain Aha! Silly question then. Thanks for helping me!

jnm2 commented 5 years ago

@jbevain Would it make sense to you to be able to implement this interface?

https://source.dot.net/#System.Reflection.Metadata/System/Reflection/Metadata/Decoding/ISignatureTypeProvider.cs,7c9ee489516b4e86

public interface ISignatureTypeProvider<TType, TGenericContext> : ISimpleTypeProvider<TType>, IConstructedTypeProvider<TType>
{
    /// <summary>
    /// Gets the a type symbol for the function pointer type of the given method signature.
    /// </summary>
    TType GetFunctionPointerType(MethodSignature<TType> signature);

    /// <summary>
    /// Gets the type symbol for the generic method parameter at the given zero-based index.
    /// </summary>
    TType GetGenericMethodParameter(TGenericContext genericContext, int index);

    /// <summary>
    /// Gets the type symbol for the generic type parameter at the given zero-based index.
    /// </summary>
    TType GetGenericTypeParameter(TGenericContext genericContext, int index);

I'm doing something very much like System.Reflection.Metadata but the type is being constructed by an ILAsm syntax parser. In the same way as SR.Metadata's decoder, the parser doesn't hand in the information about the owner of the type parameter, just its index.

jnm2 commented 5 years ago

Probably another silly question, because at the point when class System.Collections.Generic.IEqualityComparer`1<!0> is being parsed, the parser could somehow know to hand in the System.Collections.Generic.Dictionary`2<string, string> instance of TType which it had already constructed through the provider. Maybe I can figure this out when I get to it later.

Being able to use !0 free of context is so convenient though :) I'm not seeing the upside to plumbing everything through yet, except just to avoid reflection of course. Maybe something will break and then I'll get why.

jnm2 commented 5 years ago

Spent some time trying out passing the generic context around. What happens is that my ILAsm syntax parser has to begin buffering because the return type of a method reference like this can't be parsed until the declaring type syntax is parsed: !0 class Foo<string>::Bar()

Right now my parser works in a single forward pass. It figures out where the end of the return type is by parsing it, and parsing it requires calls to the type provider interface because the parser has no object model. But calling the type provider interface means creating GenericParameter instances for the return type before the declaring type has been parsed which is the needed context.

Making such a change is enough work to make me think reflection is probably my best option at the moment, since I currently gain no other benefit by associating !0 or !!0 with any particular type reference or method reference. Would this be enough reason to consider making the constructor public?

I listened to a .NET API review a while ago where they discussed the need to add an unbound generic parameter API to System.Reflection to enable GetMethod to resolve methods properly. I have no idea if that happened yet. My use case seems to be along the same conceptual lines.

jbevain commented 5 years ago

I understand how that could be not convenient for your particular use case but right now Cecil pretty much guarantees the coherence of the object model, which that change would break, so I'll be closing this.

Thanks!

jnm2 commented 5 years ago

@jbevain Would it break the coherence of the object model though? .NET Core 2.1 added Type.MakeGenericMethodParameter(int index) etc. (API review link and PR links: https://github.com/dotnet/corefx/issues/16567#issuecomment-325830190)

Is the System.Type object model now broken? It must be a matter of perspective somehow.

Obviously I find this disappointing since I have 20% more work to do on an already-large project as a result, versus a keyword change in Cecil. But that's something you have to be prepared for of course.

jbevain commented 5 years ago

Cecil has no ties to System / System.Reflection, and right now enforces a contract where you can compare definitions, including type parameters, by object identity. So yes, providing this API to create new instance of GenericParameter breaks that.

If Cecil doesn't work for your scenario it's perfectly fine, I suggest looking at the variety of managed reader and writers.

jnm2 commented 5 years ago

Thanks for explaining.

If Cecil doesn't work for your scenario it's perfectly fine, I suggest looking at the variety of managed reader and writers.

Do you know of anything that can load and resave an assembly to disk? Nothing in the BCL does this, including System.Reflection.Metadata, and the .NET team points people to Cecil who need anything like this.

jbevain commented 5 years ago

Maybe dnlib would work best for you?

https://github.com/0xd4d/dnlib

jnm2 commented 5 years ago

Maybe, I'll take a look at that. I appreciate all your help!