Incorrectly using a non-generic type with type parameters now produces a helpful Python error instead of throwing NullReferenceException (#1326)

tminka · web-flow · commit d9e15a75a1f2 · 2020-12-18T13:49:32.000-08:00
Inlined GenericUtil.GenericsByName into GenericByName.
Removed unused GenericUtil.GenericsForType.
Other code quality improvements.
diff --git a/AUTHORS.md b/AUTHORS.md
@@ -60,6 +60,7 @@
 -   Sean Freitag ([@cowboygneox](https://github.com/cowboygneox))
 -   Serge Weinstock ([@sweinst](https://github.com/sweinst))
 -   Simon Mourier ([@smourier](https://github.com/smourier))
+-   Tom Minka ([@tminka](https://github.com/tminka))
 -   Viktoria Kovescses ([@vkovec](https://github.com/vkovec))
 -   Ville M. Vainio ([@vivainio](https://github.com/vivainio))
 -   Virgil Dupras ([@hsoft](https://github.com/hsoft))
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -42,6 +42,7 @@ details about the cause of the failure
 -    Made it possible to use `__len__` also on `ICollection<>` interface objects
 -    Made it possible to call `ToString`, `GetHashCode`, and `GetType` on inteface objects
 -    Fixed objects returned by enumerating `PyObject` being disposed too soon
+-    Incorrectly using a non-generic type with type parameters now produces a helpful Python error instead of throwing NullReferenceException
 
 ### Removed
 
diff --git a/src/runtime/classbase.cs b/src/runtime/classbase.cs
@@ -54,7 +54,7 @@ public virtual IntPtr type_subscript(IntPtr idx)
                 return c.pyHandle;
             }
 
-            return Exceptions.RaiseTypeError("no type matches params");
+            return Exceptions.RaiseTypeError($"{type.Namespace}.{type.Name} does not accept {types.Length} generic parameters");
         }
 
         /// <summary>
diff --git a/src/runtime/genericutil.cs b/src/runtime/genericutil.cs
@@ -9,14 +9,13 @@ namespace Python.Runtime
     /// This class is responsible for efficiently maintaining the bits
     /// of information we need to support aliases with 'nice names'.
     /// </summary>
-    internal class GenericUtil
+    internal static class GenericUtil
     {
+        /// <summary>
+        /// Maps namespace -> generic base name -> list of generic type names
+        /// </summary>
         private static Dictionary<string, Dictionary<string, List<string>>> mapping;
 
-        private GenericUtil()
-        {
-        }
-
         public static void Reset()
         {
             mapping = new Dictionary<string, Dictionary<string, List<string>>>();
@@ -25,29 +24,23 @@ public static void Reset()
         /// <summary>
         /// Register a generic type that appears in a given namespace.
         /// </summary>
+        /// <param name="t">A generic type definition (<c>t.IsGenericTypeDefinition</c> must be true)</param>
         internal static void Register(Type t)
         {
             if (null == t.Namespace || null == t.Name)
             {
                 return;
             }
 
-            Dictionary<string, List<string>> nsmap = null;
-            mapping.TryGetValue(t.Namespace, out nsmap);
-            if (nsmap == null)
+            Dictionary<string, List<string>> nsmap;
+            if (!mapping.TryGetValue(t.Namespace, out nsmap))
             {
                 nsmap = new Dictionary<string, List<string>>();
                 mapping[t.Namespace] = nsmap;
             }
-            string basename = t.Name;
-            int tick = basename.IndexOf("`");
-            if (tick > -1)
-            {
-                basename = basename.Substring(0, tick);
-            }
-            List<string> gnames = null;
-            nsmap.TryGetValue(basename, out gnames);
-            if (gnames == null)
+            string basename = GetBasename(t.Name);
+            List<string> gnames;
+            if (!nsmap.TryGetValue(basename, out gnames))
             {
                 gnames = new List<string>();
                 nsmap[basename] = gnames;
@@ -60,9 +53,8 @@ internal static void Register(Type t)
         /// </summary>
         public static List<string> GetGenericBaseNames(string ns)
         {
-            Dictionary<string, List<string>> nsmap = null;
-            mapping.TryGetValue(ns, out nsmap);
-            if (nsmap == null)
+            Dictionary<string, List<string>> nsmap;
+            if (!mapping.TryGetValue(ns, out nsmap))
             {
                 return null;
             }
@@ -75,84 +67,73 @@ public static List<string> GetGenericBaseNames(string ns)
         }
 
         /// <summary>
-        /// xxx
+        /// Finds a generic type with the given number of generic parameters and the same name and namespace as <paramref name="t"/>.
         /// </summary>
         public static Type GenericForType(Type t, int paramCount)
         {
             return GenericByName(t.Namespace, t.Name, paramCount);
         }
 
-        public static Type GenericByName(string ns, string name, int paramCount)
-        {
-            foreach (Type t in GenericsByName(ns, name))
-            {
-                if (t.GetGenericArguments().Length == paramCount)
-                {
-                    return t;
-                }
-            }
-            return null;
-        }
-
-        public static List<Type> GenericsForType(Type t)
-        {
-            return GenericsByName(t.Namespace, t.Name);
-        }
-
-        public static List<Type> GenericsByName(string ns, string basename)
+        /// <summary>
+        /// Finds a generic type in the given namespace with the given name and number of generic parameters.
+        /// </summary>
+        public static Type GenericByName(string ns, string basename, int paramCount)
         {
-            Dictionary<string, List<string>> nsmap = null;
-            mapping.TryGetValue(ns, out nsmap);
-            if (nsmap == null)
+            Dictionary<string, List<string>> nsmap;
+            if (!mapping.TryGetValue(ns, out nsmap))
             {
                 return null;
             }
 
-            int tick = basename.IndexOf("`");
-            if (tick > -1)
-            {
-                basename = basename.Substring(0, tick);
-            }
-
-            List<string> names = null;
-            nsmap.TryGetValue(basename, out names);
-            if (names == null)
+            List<string> names;
+            if (!nsmap.TryGetValue(GetBasename(basename), out names))
             {
                 return null;
             }
 
-            var result = new List<Type>();
             foreach (string name in names)
             {
                 string qname = ns + "." + name;
                 Type o = AssemblyManager.LookupTypes(qname).FirstOrDefault();
-                if (o != null)
+                if (o != null && o.GetGenericArguments().Length == paramCount)
                 {
-                    result.Add(o);
+                    return o;
                 }
             }
 
-            return result;
+            return null;
         }
 
         /// <summary>
         /// xxx
         /// </summary>
         public static string GenericNameForBaseName(string ns, string name)
         {
-            Dictionary<string, List<string>> nsmap = null;
-            mapping.TryGetValue(ns, out nsmap);
-            if (nsmap == null)
+            Dictionary<string, List<string>> nsmap;
+            if (!mapping.TryGetValue(ns, out nsmap))
             {
                 return null;
             }
-            List<string> gnames = null;
+            List<string> gnames;
             nsmap.TryGetValue(name, out gnames);
             if (gnames?.Count > 0)
             {
                 return gnames[0];
             }
             return null;
         }
+
+        private static string GetBasename(string name)
+        {
+            int tick = name.IndexOf("`");
+            if (tick > -1)
+            {
+                return name.Substring(0, tick);
+            }
+            else
+            {
+                return name;
+            }
+        }
     }
 }
diff --git a/src/tests/test_generic.py b/src/tests/test_generic.py
@@ -745,3 +745,8 @@ def test_nested_generic_class():
     """Check nested generic classes."""
     # TODO NotImplemented
     pass
+
+def test_missing_generic_type():
+    from System.Collections import IList
+    with pytest.raises(TypeError):
+        IList[bool]

Original file line number	Diff line number	Diff line change
`@@ -54,7 +54,7 @@ public virtual IntPtr type_subscript(IntPtr idx)`
`54`	`54`	`return c.pyHandle;`
`55`	`55`	`}`
`56`	`56`
`57`		`- return Exceptions.RaiseTypeError("no type matches params");`
	`57`	`+ return Exceptions.RaiseTypeError($"{type.Namespace}.{type.Name} does not accept {types.Length} generic parameters");`
`58`	`58`	`}`
`59`	`59`
`60`	`60`	`/// <summary>`