Merge pull request #2841 from lambdageek/dev/bug-40060

[corlib] Type.GetType parser and resolver improvements

We have two type parsers the one invoked by `Type.GetType(string)` (and some others) which is implemented in C in `reflection.c`, and the one invoked by `Type.GetType(string, Func<...>, Func<...>)` implemented in managed code as `System.TypeSpec`.

In both cases `string→Type` conversion goes in two stages: parsing and resolution.  Parsing just changes the string into an AST.  And resolution crawls the AST and constructs the appropriate types.

This fixes [Bugzilla #40060](https://bugzilla.xamarin.com/show_bug.cgi?id=40060)

1. Fix the resolver on the native side for types like `"System.Generic.Collections.Dictionary[System.Int32,MyType]"` where the generic type is from corlib and a type argument is from the current assembly.
2. Fix `get_caller_no_reflection` to work correctly when the call stack has a System.Reflection method as the immediate caller of the icall.
3. Fix the resolver on the native side to work when it's called from corlib's System namespace, not just System.Reflection.
4. Fix the managed parser to work with types like `"MyGeneric[MyType]"` where the generic argument is not an assembly qualified name or there's a mixture of aqn and non-aqn type arguments.  Also rationalize how the code handles assembly qualification in general.
5. Test cases

diff --git a/mcs/class/corlib/System/TypeSpec.cs b/mcs/class/corlib/System/TypeSpec.cs
index 9ad485e07cd52b254e1265c18d95f3e0a2fc9c4d..3cf08ae352089a434b6c8ac006bf6ab5e9c6e1a3 100644 (file)
--- a/mcs/class/corlib/System/TypeSpec.cs
+++ b/mcs/class/corlib/System/TypeSpec.cs
@@ -218,7 +218,7 @@ namespace System {
                        if (typeName == null)
                                throw new ArgumentNullException ("typeName");
 
-                       TypeSpec res = Parse (typeName, ref pos, false, false);
+                       TypeSpec res = Parse (typeName, ref pos, false, true);
                        if (pos < typeName.Length)
                                throw new ArgumentException ("Count not parse the whole type name", "typeName");
                        return res;
@@ -287,7 +287,7 @@ namespace System {
                {
                        Assembly asm = null;
                        if (assemblyResolver == null && typeResolver == null)
-                               return Type.GetType (name.DisplayName, throwOnError, ignoreCase);
+                               return Type.GetType (DisplayFullName, throwOnError, ignoreCase);
 
                        if (assembly_name != null) {
                                if (assemblyResolver != null)
@@ -376,6 +376,12 @@ namespace System {
                        pos = p;
                }
 
+               static void BoundCheck (int idx, string s)
+               {
+                       if (idx >= s.Length)
+                               throw new ArgumentException ("Invalid generic arguments spec", "typeName");
+               }
+
                static TypeIdentifier ParsedTypeIdentifier (string displayName)
                {
                        return TypeIdentifiers.FromDisplay(displayName);
@@ -383,6 +389,17 @@ namespace System {
 
                static TypeSpec Parse (string name, ref int p, bool is_recurse, bool allow_aqn)
                {
+                       // Invariants:
+                       //  - On exit p, is updated to pos the current unconsumed character.
+                       //
+                       //  - The callee peeks at but does not consume delimiters following
+                       //    recurisve parse (so for a recursive call like the args of "Foo[P,Q]"
+                       //    we'll return with p either on ',' or on ']'.  If the name was aqn'd
+                       //    "Foo[[P,assmblystuff],Q]" on return p with be on the ']' just
+                       //    after the "assmblystuff")
+                       //
+                       //  - If allow_aqn is True, assembly qualification is optional.
+                       //    If allow_aqn is False, assembly qualification is prohibited.
                        int pos = p;
                        int name_start;
                        bool in_modifiers = false;
@@ -450,18 +467,24 @@ namespace System {
                                                data.AddModifier (new PointerSpec(pointer_level));
                                                break;
                                        case ',':
-                                               if (is_recurse) {
+                                               if (is_recurse && allow_aqn) {
                                                        int end = pos;
                                                        while (end < name.Length && name [end] != ']')
                                                                ++end;
                                                        if (end >= name.Length)
                                                                throw new ArgumentException ("Unmatched ']' while parsing generic argument assembly name");
                                                        data.assembly_name = name.Substring (pos + 1, end - pos - 1).Trim ();
-                                                       p = end + 1;
+                                                       p = end;
                                                        return data;                                            
                                                }
-                                               data.assembly_name = name.Substring (pos + 1).Trim ();
-                                               pos = name.Length;
+                                               if (is_recurse) {
+                                                       p = pos;
+                                                       return data;
+                                               }
+                                               if (allow_aqn) {
+                                                       data.assembly_name = name.Substring (pos + 1).Trim ();
+                                                       pos = name.Length;
+                                               }
                                                break;
                                        case '[':
                                                if (data.is_byref)
@@ -482,11 +505,17 @@ namespace System {
                                                                if (aqn)
                                                                        ++pos; //skip '[' to the start of the type
                                                                args.Add (Parse (name, ref pos, true, aqn));
-                                                               if (pos >= name.Length)
-                                                                       throw new ArgumentException ("Invalid generic arguments spec", "typeName");
+                                                               BoundCheck (pos, name);
+                                                               if (aqn) {
+                                                                       if (name [pos] == ']')
+                                                                               ++pos;
+                                                                       else
+                                                                               throw new ArgumentException ("Unclosed assembly-qualified type name at " + name[pos], "typeName");
+                                                                       BoundCheck (pos, name);
+}
 
                                                                if (name [pos] == ']')
-                                                                               break;
+                                                                       break;
                                                                if (name [pos] == ',')
                                                                        ++pos; // skip ',' to the start of the next arg
                                                                else
@@ -523,7 +552,7 @@ namespace System {
                                                break;
                                        case ']':
                                                if (is_recurse) {
-                                                       p = pos + 1;
+                                                       p = pos;
                                                        return data;
                                                }
                                                throw new ArgumentException ("Unmatched ']'", "typeName");

diff --git a/mcs/class/corlib/Test/System/TypeTest.cs b/mcs/class/corlib/Test/System/TypeTest.cs
index 4279cee8ec9d835c8c29d2e4ec86feccb179020b..cbf44c17f358df1b94a0c1977772271af40ac3e4 100644 (file)
--- a/mcs/class/corlib/Test/System/TypeTest.cs
+++ b/mcs/class/corlib/Test/System/TypeTest.cs
@@ -4059,17 +4059,27 @@ namespace MonoTests.System
                        } catch (ArgumentNullException) {}
                }
 
-               void MustAE (string tname) {
+               void MustAE_general (string tname, Func<string,Type> getType) {
                        try {
-                               var res = Type.GetType (tname, name => {
-                                       return Assembly.Load (name);
-                               },(asm,name,ignore) => {
-                                       return (object)asm == null ? Type.GetType (name, false, ignore) : asm.GetType (name, false, ignore);
-                               }, true, false);
+                               var res = getType (tname);
                                Assert.Fail (tname);
                        } catch (ArgumentException) {}
                }
 
+               void MustAE (string typename) {
+                       MustAE_general (typename, tname => {
+                                       return Type.GetType (tname, name => {
+                                                       return Assembly.Load (name);
+                                               },(asm,name,ignore) => {
+                                                       return (object)asm == null ? Type.GetType (name, false, ignore) : asm.GetType (name, false, ignore);
+                                               }, true, false);
+                               });
+               }
+
+               void MustAEnn (string typename) {
+                       MustAE_general (typename, tname => Type.GetType (tname, null, null));
+               }
+
                void MustFNFE (string tname) {
                        try {
                                var res = Type.GetType (tname, name => {
@@ -4152,6 +4162,56 @@ namespace MonoTests.System
                        Assert.AreEqual (Type.GetType ("MonoTests.System.Foo`1[System.Int32"), null, "#15");
                }
 
+               [Test]
+               public void GetTypeNullDelegatesParseGenericCorrectly () {
+                       Assert.AreEqual (Type.GetType ("MonoTests.System.Foo`1", null, null), typeof (Foo<>), "#1");
+                       Assert.AreEqual (Type.GetType ("MonoTests.System.Foo`1[System.Int32]", null, null), typeof (Foo<int>), "#2");
+                       Assert.AreEqual (Type.GetType ("MonoTests.System.Foo`1[[System.Int32]]", null, null), typeof (Foo<int>), "#3");
+                       Assert.AreEqual (Type.GetType ("MonoTests.System.Foo`1[System.Int32][]", null, null), typeof (Foo<int>[]), "#4");
+                       Assert.AreEqual (Type.GetType ("MonoTests.System.Foo`1[System.Int32][,]", null, null), typeof (Foo<int>[,]), "#5");
+                       Assert.AreEqual (Type.GetType ("MonoTests.System.Foo`1[]", null, null), typeof (Foo<>).MakeArrayType(), "#6");
+                       Assert.AreEqual (Type.GetType ("MonoTests.System.Foo`1[,]", null, null), typeof (Foo<>).MakeArrayType (2), "#7");
+                       Assert.AreEqual (Type.GetType ("MonoTests.System.Foo`1[][]", null, null), typeof (Foo<>).MakeArrayType ().MakeArrayType (), "#8");
+
+                       MustAEnn ("MonoTests.System.Foo`1[][System.Int32]");
+                       MustAEnn ("MonoTests.System.Foo`1[");
+                       MustAEnn ("MonoTests.System.Foo`1[[");
+                       MustAEnn ("MonoTests.System.Foo`1[[]");
+                       MustAEnn ("MonoTests.System.Foo`1[,");
+                       MustAEnn ("MonoTests.System.Foo`1[*");
+                       MustAEnn ("MonoTests.System.Foo`1[System.Int32");
+               }
+
+               Dictionary<int, T> MakeDictHelper<T> (T[] arr) {
+                       return new Dictionary<int, T>();
+               }
+
+               [Test]
+               public void GetTypeAnonymousParseCorrectly () {
+                       var x = new { X = 1 };
+                       var a = new [] { x };
+                       var d = MakeDictHelper (a);
+
+                       var x_type = x.GetType ();
+                       var a_type = a.GetType ();
+                       var d_type = d.GetType ();
+
+                       Assert.AreEqual (Type.GetType (x_type.ToString ()), x_type, "#1");
+                       Assert.AreEqual (Type.GetType (x_type.ToString (), null, null), x_type, "#2");
+                       Assert.AreEqual (Type.GetType (a_type.ToString ()), a_type, "#3");
+                       Assert.AreEqual (Type.GetType (a_type.ToString (), null, null), a_type, "#4");
+                       Assert.AreEqual (Type.GetType (d_type.ToString ()), d_type, "#5");
+                       Assert.AreEqual (Type.GetType (d_type.ToString (), null, null), d_type, "#6");
+
+                       Assert.AreEqual (Type.GetType (x_type.FullName), x_type, "#7");
+                       Assert.AreEqual (Type.GetType (x_type.FullName, null, null), x_type, "#8");
+                       Assert.AreEqual (Type.GetType (a_type.FullName), a_type, "#9");
+                       Assert.AreEqual (Type.GetType (a_type.FullName, null, null), a_type, "#10");
+                       Assert.AreEqual (Type.GetType (d_type.FullName), d_type, "#11");
+                       Assert.AreEqual (Type.GetType (d_type.FullName, null, null), d_type, "#12");
+
+               }
+
 #if !MONOTOUCH && !MOBILE_STATIC
                [Test]
                [Category ("AndroidNotWorking")] // requires symbol writer

diff --git a/mono/metadata/icall.c b/mono/metadata/icall.c
index 30a1d46e9c65dc95fbc462afb5bbe2312b3d0ad0..498b95a2232dfe590ca5c1e59f507722b92c01ff 100644 (file)
--- a/mono/metadata/icall.c
+++ b/mono/metadata/icall.c
@@ -1308,13 +1308,42 @@ get_caller_no_reflection (MonoMethod *m, gint32 no, gint32 ilo, gboolean managed
        if (m->wrapper_type != MONO_WRAPPER_NONE)
                return FALSE;
 
+       if (m == *dest) {
+               *dest = NULL;
+               return FALSE;
+       }
+
        if (m->klass->image == mono_defaults.corlib && !strcmp (m->klass->name_space, "System.Reflection"))
                return FALSE;
 
+       if (!(*dest)) {
+               *dest = m;
+               return TRUE;
+       }
+       return FALSE;
+}
+
+static gboolean
+get_caller_no_system_or_reflection (MonoMethod *m, gint32 no, gint32 ilo, gboolean managed, gpointer data)
+{
+       MonoMethod **dest = (MonoMethod **)data;
+
+       /* skip unmanaged frames */
+       if (!managed)
+               return FALSE;
+
+       if (m->wrapper_type != MONO_WRAPPER_NONE)
+               return FALSE;
+
        if (m == *dest) {
                *dest = NULL;
                return FALSE;
        }
+
+       if (m->klass->image == mono_defaults.corlib && ((!strcmp (m->klass->name_space, "System.Reflection"))
+                                                       || (!strcmp (m->klass->name_space, "System"))))
+               return FALSE;
+
        if (!(*dest)) {
                *dest = m;
                return TRUE;
@@ -1330,6 +1359,7 @@ type_from_parsed_name (MonoTypeNameParse *info, MonoBoolean ignoreCase, MonoErro
        MonoType *type = NULL;
        MonoAssembly *assembly = NULL;
        gboolean type_resolve = FALSE;
+       MonoImage *rootimage = NULL;
 
        mono_error_init (error);
 
@@ -1341,10 +1371,21 @@ type_from_parsed_name (MonoTypeNameParse *info, MonoBoolean ignoreCase, MonoErro
        m = mono_method_get_last_managed ();
        dest = m;
 
-       mono_stack_walk_no_il (get_caller_no_reflection, &dest);
+       /* Ugly hack: type_from_parsed_name is called from
+        * System.Type.internal_from_name, which is called most
+        * directly from System.Type.GetType(string,bool,bool) but
+        * also indirectly from places such as
+        * System.Type.GetType(string,func,func) (via
+        * System.TypeNameParser.GetType and System.TypeSpec.Resolve)
+        * so we need to skip over all of those to find the true caller.
+        *
+        * It would be nice if we had stack marks.
+        */
+       mono_stack_walk_no_il (get_caller_no_system_or_reflection, &dest);
        if (!dest)
                dest = m;
 
+
        /*
         * FIXME: mono_method_get_last_managed() sometimes returns NULL, thus
         *        causing ves_icall_System_Reflection_Assembly_GetCallingAssembly()
@@ -1356,6 +1397,7 @@ type_from_parsed_name (MonoTypeNameParse *info, MonoBoolean ignoreCase, MonoErro
        if (dest) {
                assembly = dest->klass->image->assembly;
                type_resolve = TRUE;
+               rootimage = assembly->image;
        } else {
                g_warning (G_STRLOC);
        }
@@ -1363,21 +1405,28 @@ type_from_parsed_name (MonoTypeNameParse *info, MonoBoolean ignoreCase, MonoErro
        if (info->assembly.name)
                assembly = mono_assembly_load (&info->assembly, assembly ? assembly->basedir : NULL, NULL);
 
-
        if (assembly) {
                /* When loading from the current assembly, AppDomain.TypeResolve will not be called yet */
-               type = mono_reflection_get_type_checked (assembly->image, info, ignoreCase, &type_resolve, error);
+               type = mono_reflection_get_type_checked (rootimage, assembly->image, info, ignoreCase, &type_resolve, error);
                return_val_if_nok (error, NULL);
        }
 
+       // XXXX - aleksey -
+       //  Say we're looking for System.Generic.Dict<int, Local>
+       //  we FAIL the get type above, because S.G.Dict isn't in assembly->image.  So we drop down here.
+       //  but then we FAIL AGAIN because now we pass null as the image and the rootimage and everything
+       //  is messed up when we go to construct the Local as the type arg...
+       //
+       // By contrast, if we started with Mine<System.Generic.Dict<int, Local>> we'd go in with assembly->image
+       // as the root and then even the detour into generics would still not screw us when we went to load Local.
        if (!info->assembly.name && !type) {
                /* try mscorlib */
-               type = mono_reflection_get_type_checked (NULL, info, ignoreCase, &type_resolve, error);
+               type = mono_reflection_get_type_checked (rootimage, NULL, info, ignoreCase, &type_resolve, error);
                return_val_if_nok (error, NULL);
        }
        if (assembly && !type && type_resolve) {
                type_resolve = FALSE; /* This will invoke TypeResolve if not done in the first 'if' */
-               type = mono_reflection_get_type_checked (assembly->image, info, ignoreCase, &type_resolve, error);
+               type = mono_reflection_get_type_checked (rootimage, assembly->image, info, ignoreCase, &type_resolve, error);
                return_val_if_nok (error, NULL);
        }
 
@@ -1403,10 +1452,12 @@ ves_icall_System_Type_internal_from_name (MonoString *name,
        /* mono_reflection_parse_type() mangles the string */
        if (!parsedOk) {
                mono_reflection_free_type_info (&info);
-               g_free (str);
                if (throwOnError) {
-                       mono_set_pending_exception (mono_get_exception_argument("typeName", "failed parse"));
+                       mono_error_init (&error);
+                       mono_error_set_argument (&error, "typeName", "failed parse: %s", str);
+                       mono_error_set_pending_exception (&error);
                }
+               g_free (str);
                return NULL;
        }
 
@@ -4417,7 +4468,7 @@ ves_icall_System_Reflection_Assembly_InternalGetType (MonoReflectionAssembly *as
 
        if (module != NULL) {
                if (module->image) {
-                       type = mono_reflection_get_type_checked (module->image, &info, ignoreCase, &type_resolve, &error);
+                       type = mono_reflection_get_type_checked (module->image, module->image, &info, ignoreCase, &type_resolve, &error);
                        if (!is_ok (&error)) {
                                g_free (str);
                                mono_reflection_free_type_info (&info);
@@ -4437,7 +4488,7 @@ ves_icall_System_Reflection_Assembly_InternalGetType (MonoReflectionAssembly *as
                        if (abuilder->modules) {
                                for (i = 0; i < mono_array_length (abuilder->modules); ++i) {
                                        MonoReflectionModuleBuilder *mb = mono_array_get (abuilder->modules, MonoReflectionModuleBuilder*, i);
-                                       type = mono_reflection_get_type_checked (&mb->dynamic_image->image, &info, ignoreCase, &type_resolve, &error);
+                                       type = mono_reflection_get_type_checked (&mb->dynamic_image->image, &mb->dynamic_image->image, &info, ignoreCase, &type_resolve, &error);
                                        if (!is_ok (&error)) {
                                                g_free (str);
                                                mono_reflection_free_type_info (&info);
@@ -4452,7 +4503,7 @@ ves_icall_System_Reflection_Assembly_InternalGetType (MonoReflectionAssembly *as
                        if (!type && abuilder->loaded_modules) {
                                for (i = 0; i < mono_array_length (abuilder->loaded_modules); ++i) {
                                        MonoReflectionModule *mod = mono_array_get (abuilder->loaded_modules, MonoReflectionModule*, i);
-                                       type = mono_reflection_get_type_checked (mod->image, &info, ignoreCase, &type_resolve, &error);
+                                       type = mono_reflection_get_type_checked (mod->image, mod->image, &info, ignoreCase, &type_resolve, &error);
                                        if (!is_ok (&error)) {
                                                g_free (str);
                                                mono_reflection_free_type_info (&info);
@@ -4465,7 +4516,7 @@ ves_icall_System_Reflection_Assembly_InternalGetType (MonoReflectionAssembly *as
                        }
                }
                else {
-                       type = mono_reflection_get_type_checked (assembly->assembly->image, &info, ignoreCase, &type_resolve, &error);
+                       type = mono_reflection_get_type_checked (assembly->assembly->image, assembly->assembly->image, &info, ignoreCase, &type_resolve, &error);
                        if (!is_ok (&error)) {
                                g_free (str);
                                mono_reflection_free_type_info (&info);

diff --git a/mono/metadata/reflection-internals.h b/mono/metadata/reflection-internals.h
index 600f2c6d268906ab4c360248e961b5b394b6188e..327bb93142ce85d0579bf1bd8ae484a92e545331 100644 (file)
--- a/mono/metadata/reflection-internals.h
+++ b/mono/metadata/reflection-internals.h
@@ -10,7 +10,7 @@
 #include <mono/utils/mono-error.h>
 
 MonoType*
-mono_reflection_get_type_checked (MonoImage* image, MonoTypeNameParse *info, mono_bool ignorecase, mono_bool *type_resolve, MonoError *error);
+mono_reflection_get_type_checked (MonoImage *rootimage, MonoImage* image, MonoTypeNameParse *info, mono_bool ignorecase, mono_bool *type_resolve, MonoError *error);
 
 MonoType*
 mono_reflection_type_from_name_checked (char *name, MonoImage *image, MonoError *error);

diff --git a/mono/metadata/reflection.c b/mono/metadata/reflection.c
index be20800c24edf1f1f2ef5c132fbf3d4633c9f579..d477f51c245565a438cfa64ce3d1e469e98a2a1c 100644 (file)
--- a/mono/metadata/reflection.c
+++ b/mono/metadata/reflection.c
@@ -8629,6 +8629,7 @@ mono_reflection_get_type (MonoImage* image, MonoTypeNameParse *info, gboolean ig
 
 /**
  * mono_reflection_get_type_checked:
+ * @rootimage: the image of the currently active managed caller
  * @image: a metadata context
  * @info: type description structure
  * @ignorecase: flag for case-insensitive string compares
@@ -8639,9 +8640,9 @@ mono_reflection_get_type (MonoImage* image, MonoTypeNameParse *info, gboolean ig
  *
  */
 MonoType*
-mono_reflection_get_type_checked (MonoImage* image, MonoTypeNameParse *info, gboolean ignorecase, gboolean *type_resolve, MonoError *error) {
+mono_reflection_get_type_checked (MonoImage *rootimage, MonoImage* image, MonoTypeNameParse *info, gboolean ignorecase, gboolean *type_resolve, MonoError *error) {
        mono_error_init (error);
-       return mono_reflection_get_type_with_rootimage (image, image, info, ignorecase, type_resolve, error);
+       return mono_reflection_get_type_with_rootimage (rootimage, image, info, ignorecase, type_resolve, error);
 }
 
 

diff --git a/mono/mini/debugger-agent.c b/mono/mini/debugger-agent.c
index db15fc43917ffa629f7f881e92ed58a89602ef17..ba127514b1bacda0a733af3ee9b110c4326ecd5a 100644 (file)
--- a/mono/mini/debugger-agent.c
+++ b/mono/mini/debugger-agent.c
@@ -7258,7 +7258,7 @@ vm_commands (int command, int id, guint8 *p, guint8 *end, Buffer *buf)
                                        MonoError error;
                                        type_resolve = TRUE;
                                        /* FIXME really okay to call while holding locks? */
-                                       t = mono_reflection_get_type_checked (ass->image, &info, ignore_case, &type_resolve, &error);
+                                       t = mono_reflection_get_type_checked (ass->image, ass->image, &info, ignore_case, &type_resolve, &error);
                                        mono_error_cleanup (&error); 
                                        if (t) {
                                                g_ptr_array_add (res_classes, mono_type_get_class (t));
@@ -7692,7 +7692,7 @@ assembly_commands (int command, guint8 *p, guint8 *end, Buffer *buf)
                } else {
                        if (info.assembly.name)
                                NOT_IMPLEMENTED;
-                       t = mono_reflection_get_type_checked (ass->image, &info, ignorecase, &type_resolve, &error);
+                       t = mono_reflection_get_type_checked (ass->image, ass->image, &info, ignorecase, &type_resolve, &error);
                        if (!is_ok (&error)) {
                                mono_error_cleanup (&error); /* FIXME don't swallow the error */
                                mono_reflection_free_type_info (&info);