InvalidOperation Exception importing generic type with a type parameter from declaring type

[JohannesRudolph]

private class HasStringKeyedDictionary<TValue>
{
     public static readonly Dictionary<string, TValue> PARTIALLY_CLOSED = null;
}

Using module.Import( typeof( HasStringKeyedDictionary<> ).GetField( "PARTIALLY_CLOSED" ).FieldType ) throws an InvalidOperationException.

The error was introduced with one of the last change to ImportGenericContext and I used to get it to import properly before that (not sure if I have to blame f5a7557 or 4f76389).

The problem seems to be that the declaring type of the parameter TValue doesn't end up on the stack that ImportGenericContext wraps. I don't quite understand why that stack is necessary in the first place and what benefit it provides.

I agree with the notion that we have "scopes" when resolving generic type parameters (their scope is their declaring type) and these may be nested but System.Reflection gives you the right declaring type anyway so I don't see a reason we need the stack even though scopes may be nested and shadow equally named parameters.

[JohannesRudolph]

@jbevain can you comment on this? Would be happy to provide a fix, but would need to understand the rationale behind the new generic type parameter resolution system first.

[jbevain]

Currently at a conference, I'll have a look next week :)

[JohannesRudolph]

Hi Jb, did you get a chance to have a look?

On Fri, Mar 29, 2013 at 2:30 PM, Jb Evain notifications@github.com wrote:

Currently at a conference, I'll have a look next week :)

—
Reply to this email directly or view it on GitHubhttps://github.com//issues/124#issuecomment-15640627
.

[jbevain]

No not yet, I'm just back from intensive traveling :)

[rcollette]

I believe I may be running into this issue as well. I am using ILRepack which depends on Mono.Cecil. I use it to merge a build of google-api-dotnet-client.

I am getting the stack trace:
at Mono.Cecil.ImportGenericContext.TypeParameter(String type, Int32 position) in C:\Users\n1200632d\Documents\GitHub\il-repack\cecil\Mono.Cecil\Import.cs:line 96
at Mono.Cecil.MetadataImporter.ImportTypeSpecification(TypeReference type, ImportGenericContext context) in C:\Users\n1200632d\Documents\GitHub\il-repack\cecil\Mono.Cecil\Import.cs:line 590
at Mono.Cecil.MetadataImporter.ImportType(TypeReference type, ImportGenericContext context) in C:\Users\n1200632d\Documents\GitHub\il-repack\cecil\Mono.Cecil\Import.cs:line 445
at Mono.Cecil.ModuleDefinition.Import(TypeReference type, IGenericParameterProvider context) in C:\Users\n1200632d\Documents\GitHub\il-repack\cecil\Mono.Cecil\ModuleDefinition.cs:line 702
at ILRepacking.ILRepack.Import(TypeReference reference, IGenericParameterProvider context) in C:\Users\n1200632d\Documents\GitHub\il-repack\ILRepack\ILRepack.cs:line 1519
at ILRepacking.ILRepack.CloneTo(FieldDefinition field, TypeDefinition nt) in C:\Users\n1200632d\Documents\GitHub\il-repack\ILRepack\ILRepack.cs:line 1271
at ILRepacking.ILRepack.Import(TypeDefinition type, Collection`1 col, Boolean internalize) in C:\Users\n1200632d\Documents\GitHub\il-repack\ILRepack\ILRepack.cs:line 1873
at ILRepacking.ILRepack.RepackTypes() in C:\Users\n1200632d\Documents\GitHub\il-repack\ILRepack\ILRepack.cs:line 905
at ILRepacking.ILRepack.Repack() in C:\Users\n1200632d\Documents\GitHub\il-repack\ILRepack\ILRepack.cs:line 703
at ILRepacking.ILRepack.Merge() in C:\Users\n1200632d\Documents\GitHub\il-repack\ILRepack\ILRepack.cs:line 85
at ILRepack.MSBuild.Task.ILRepack.Execute() in C:\Users\n1200632d\Documents\GitHub\ILRepack.MSBuild.Task\src\ILRepack.cs:line 279

The method:
Mono.Cecil.MetadataImporter.ImportSpecification

has the following state:
type = "j__TPar"
type.etype = Var
type.DeclaringType.FullName = "<>f__AnonymousType02" context.stack[0] = {<c8ad25bc-1b21-47b3-9d17-d7a3eec10a92><>f__AnonymousType02}

The subsequent call to:
context.TypeParameter

fails with an InvalidOperationException because type.DeclaringType.FullName does not match the value in stack.

[gluck]

Errors (although same stack) are different:

I believe the first one is legit, as the field type argument TValue must be bound to the container class and has no meaning out of it. Hence things work well if you do this instead:

var typ = module.Import(typeof(HasStringKeyedDictionary<>));
module.Import(typeof(HasStringKeyedDictionary<>).GetField("PARTIALLY_CLOSED").FieldType, typ);

The second one is related to ILRepack renaming of duplicate types (hence the GUID prefix), which prevents the import code to match the type name, I'll try to work on it.

[JohannesRudolph]

@gluck: What do you mean with legit? The Exception is legit? I tend towards saying that an import of the field type should automatically import the generic type definition of it's generic class if hasn't already been imported, sticking to the principle of the least surprise.

[gluck]

If possible, yes I agree, but I'm not fully convinced it is.
I tried a bit to hack the below logic, but it didn't work so far:
"If I can't find the declaringType within the context, try to import it."

I believe you still need the context stack to prevent infinite recursion.

Note that before this change, Cecil was looking up the generic parameter in the directly enclosing type (ignoring the DeclaringType entirely), no matter if it made sense or not.

[JohannesRudolph]

I tried something along the lines of

var candidate = default( TypeReference );

// there should be only one generic context at a time
var context = AllGenericParameters(type).Select( x => x.DeclaringType ).Distinct().SingleOrDefault();
if (context != null)
    candidate = _module.Import( type, _module.Import( context ) );
else
    candidate = _module.Import( type );

....

IEnumerable<Type> AllGenericParameters( Type type )
{
    if (type.IsGenericParameter)
        yield return type;

    if (!type.ContainsGenericParameters)
        yield break;

    foreach (var targ in type.GetGenericArguments())
        foreach (var gp in AllGenericParameters(targ))
            yield return gp;
}

when importing and it seems to work for now.

[gluck]

I'll try to have a look some time.
To be clear, when you're saying 'it seems to work", did you run the Import unit tests with the change ?

[JohannesRudolph]

Just outlining an approach that works for me (and I have a very special use case for .Import as I only need it to generate stub TypeReferences for testing some code that uses cecil over real assemblies). The code posted above is used in my Module.Import wrapper, so no modifications to cecil itself.

[jbevain]

This is 6 years old, closing this. Thanks!