[Mono-dev] Fwd: [PATCH] Generic Type Definition / Open Instantiation mismatch patch.

[Rodrigo Kumpera]

Hey guys,

The patch if finally done and it works perfectly well against head. This
patch turned to be very different from what it started to look like and
fortunately this meant a reduction of more than an order of magnitude in its
size, from 3.000 lines to a bit more than 230.

To have the patch make sense I need to first define what it fixes. Right now
mono treat the generic type definition and the open instantiation over its
own parameters (or just open instantiation from now on) as two different
classes. For example, given the type "class Example<T,M>", the generic type
definition is "Example`2" and the open instantiation is "Example`2<T,M>"
where T and M are the parameters of Example`2. If that doesn't make much
sense, the following code might help:

Type gtd = typeof (Example<,>);
Type openInst = gtd.MakeGenericType (gtd.GetGenericArguments ()); //openInst
and gtd should be the same instance

This property is very important for all parts of mono that play with raw
(non inflated) generic code. Analyzing the IL of generic code ilustrate this
issue well better:

.class public A`1<T> extends [mscorlib]System.Object
{
    .field  private  !0 fld

    .method public instance void Example ()
    {
        .maxstack 2
        .locals init (
            !T      V_0)
        ldarg.0
        ldfld !0 class A`1<!0>::fld
        stloc.0
        ret
    }
}

The current situation is that the non-inflated method Example will be bound
to a different MonoClass instance than the token of ldfld. Which will lead
the runtime to view it as an error. Right now only the verifier is affected
by this situation but I think the generics code sharing work might profit
from the fix too.

The original idea of how to fix it was to change the this_arg and byref_arg
of the MonoClass struct to have type MONO_TYPE_GENERICINST. With that set,
when a generic type was initialized, the open instantation MonoGenericClass
would be bound to it. This works quite well except that it introduced a lot
of issues. The main one that the container_type of generic instantiations
would no longer be a MONO_TYPE_CLASS.

First all code than handled MONO_TYPE_GENERICINST need to be changed to
couple with this fact, this required changes across most of metadata and
even in mini.c, some places expect a MONO_TYPE_CLASS and don't even guard
the retrieval from the data union of MonoType. This led to many hard to spot
bugs.

The last issue was with TypeBuilders, the way they are currently set, the
associated MonoClass cannot be a MONO_TYPE_GENERICINST until CreateType ()
is called. Changing (this|byval)_arg.type during
mono_reflection_create_runtime_class turned to be a bad idea, as it breaks
the invariant that once the MonoClass is first initialized it won't change
(this|byval)_arg.type in such drastic way. I had to patch too many places
trying to get around this issue.

The bottom line is that this approach won't work and the resulting patch
would introduce a big chunk of new bugs. In the other hand, the runtime
seens to be fine with breaking another premise, that all MonoClass instances
bould to MonoGenericClass have type MONO_TYPE_GENERICINST.

This is how it works: when the generic type definition is initialized at the
end of either mono_reflection_create_runtime_class or
mono_class_setup_mono_type, we lookup the open instantiation and bind it -
IOW we set MonoGenericClass::cached_class to the
MonoClass instance of the generic type definition.

There is one finall twist on this patch, the way System.Reflection.Emit in
mono is incompatible with .net. On mono, TypeBuilder::MakeGenericType
returns a Type that is functional (one you can call GetConstructors). This
means that you cannot have the same behavior that happens with types loaded
from an assembly. To get arround this fact, either I could either "fix" gmcs
to work with the crippled .net SRE API, or special case for TypeBuilder. I
have choosen the later one as it makes more sense.

This mean that in mono_metadata_lookup_generic_class we check if we are
looking up the open instantiation of a TypeBuilder and return a
MonoGenericClass than won't be the same for the created type.

What could receive some love in the patch is the way the check for the
TypeBuilder is done could be optimized by doing pre-calculating it in
mono_metadata_get_generic_inst. This could be done if you guys find that's a
good idea, I'm just not sure if it's work the trouble. Right now the tests
are using NUnit, maybe some should be moved to the regression suite.


Rodrigo
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://lists.ximian.com/pipermail/mono-devel-list/attachments/20070921/780500ec/attachment.html 
-------------- next part --------------
A non-text attachment was scrubbed...
Name: generic_type_definition.diff
Type: text/x-patch
Size: 6706 bytes
Desc: not available
Url : http://lists.ximian.com/pipermail/mono-devel-list/attachments/20070921/780500ec/attachment.bin 
-------------- next part --------------
A non-text attachment was scrubbed...
Name: GenericsTest.cs
Type: text/x-csharp
Size: 4016 bytes
Desc: not available
Url : http://lists.ximian.com/pipermail/mono-devel-list/attachments/20070921/780500ec/attachment-0001.bin