From 0fe042c803840ad929b2f7733f09d8e2ee6e80e6 Mon Sep 17 00:00:00 2001 From: Tom Minka <8955276+tminka@users.noreply.github.com> Date: Thu, 17 Dec 2020 12:50:17 +0000 Subject: [PATCH 1/4] Incorrectly using a non-generic type with type parameters now produces a helpful Python error instead of throwing NullReferenceException --- AUTHORS.md | 1 + CHANGELOG.md | 1 + src/embed_tests/TestList.cs | 35 +++++++++++++++++++++++++++++++++++ src/runtime/classbase.cs | 2 +- src/runtime/genericutil.cs | 15 +++++++++++---- 5 files changed, 49 insertions(+), 5 deletions(-) create mode 100644 src/embed_tests/TestList.cs diff --git a/AUTHORS.md b/AUTHORS.md index eeafd98e4..69e7b5c4a 100644 --- 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 index 27df6dcbe..2420d40ea 100644 --- 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/embed_tests/TestList.cs b/src/embed_tests/TestList.cs new file mode 100644 index 000000000..821864372 --- /dev/null +++ b/src/embed_tests/TestList.cs @@ -0,0 +1,35 @@ +using System; +using System.Collections.Generic; +using System.Linq; +using System.Reflection; +using System.Text; +using NUnit.Framework; +using Python.Runtime; + +namespace Python.EmbeddingTest +{ + public class TestList + { + [OneTimeSetUp] + public void SetUp() + { + PythonEngine.Initialize(); + } + + [OneTimeTearDown] + public void Dispose() + { + PythonEngine.Shutdown(); + } + + [Test] + public void MissingGenericTypeTest() + { + Assert.Throws(() => + PythonEngine.Exec($@" +from System.Collections import IList +IList[bool] +")); + } + } +} diff --git a/src/runtime/classbase.cs b/src/runtime/classbase.cs index a62e76050..7cb6938bc 100644 --- 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"); } /// diff --git a/src/runtime/genericutil.cs b/src/runtime/genericutil.cs index c583e64e2..7f588140c 100644 --- a/src/runtime/genericutil.cs +++ b/src/runtime/genericutil.cs @@ -75,20 +75,27 @@ public static List GetGenericBaseNames(string ns) } /// - /// xxx + /// Finds a generic type with the given number of generic parameters and the same name and namespace as . /// public static Type GenericForType(Type t, int paramCount) { return GenericByName(t.Namespace, t.Name, paramCount); } + /// + /// Finds a generic type in the given namespace with the given name and number of generic parameters. + /// public static Type GenericByName(string ns, string name, int paramCount) { - foreach (Type t in GenericsByName(ns, name)) + var types = GenericsByName(ns, name); + if (types != null) { - if (t.GetGenericArguments().Length == paramCount) + foreach (Type t in types) { - return t; + if (t.GetGenericArguments().Length == paramCount) + { + return t; + } } } return null; From 8e754b664a0461fe65a0563cf233ce2de59aeebc Mon Sep 17 00:00:00 2001 From: Tom Minka <8955276+tminka@users.noreply.github.com> Date: Thu, 17 Dec 2020 22:10:54 +0000 Subject: [PATCH 2/4] Inlined GenericUtil.GenericsByName into GenericByName. Removed unused GenericUtil.GenericsForType. Other code quality improvements. --- src/runtime/genericutil.cs | 98 ++++++++++++++------------------------ 1 file changed, 36 insertions(+), 62 deletions(-) diff --git a/src/runtime/genericutil.cs b/src/runtime/genericutil.cs index 7f588140c..92b847e98 100644 --- 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'. /// - internal class GenericUtil + internal static class GenericUtil { + /// + /// Maps namespace -> generic base name -> list of generic type names + /// private static Dictionary>> mapping; - private GenericUtil() - { - } - public static void Reset() { mapping = new Dictionary>>(); @@ -25,6 +24,7 @@ public static void Reset() /// /// Register a generic type that appears in a given namespace. /// + /// A generic type definition (t.IsGenericTypeDefinition must be true) internal static void Register(Type t) { if (null == t.Namespace || null == t.Name) @@ -32,22 +32,15 @@ internal static void Register(Type t) return; } - Dictionary> nsmap = null; - mapping.TryGetValue(t.Namespace, out nsmap); - if (nsmap == null) + Dictionary> nsmap; + if (!mapping.TryGetValue(t.Namespace, out nsmap)) { nsmap = new Dictionary>(); mapping[t.Namespace] = nsmap; } - string basename = t.Name; - int tick = basename.IndexOf("`"); - if (tick > -1) - { - basename = basename.Substring(0, tick); - } - List gnames = null; - nsmap.TryGetValue(basename, out gnames); - if (gnames == null) + string basename = GetBasename(t.Name); + List gnames; + if (!nsmap.TryGetValue(basename, out gnames)) { gnames = new List(); nsmap[basename] = gnames; @@ -60,9 +53,8 @@ internal static void Register(Type t) /// public static List GetGenericBaseNames(string ns) { - Dictionary> nsmap = null; - mapping.TryGetValue(ns, out nsmap); - if (nsmap == null) + Dictionary> nsmap; + if (!mapping.TryGetValue(ns, out nsmap)) { return null; } @@ -85,61 +77,31 @@ public static Type GenericForType(Type t, int paramCount) /// /// Finds a generic type in the given namespace with the given name and number of generic parameters. /// - public static Type GenericByName(string ns, string name, int paramCount) - { - var types = GenericsByName(ns, name); - if (types != null) - { - foreach (Type t in types) - { - if (t.GetGenericArguments().Length == paramCount) - { - return t; - } - } - } - return null; - } - - public static List GenericsForType(Type t) + public static Type GenericByName(string ns, string basename, int paramCount) { - return GenericsByName(t.Namespace, t.Name); - } - - public static List GenericsByName(string ns, string basename) - { - Dictionary> nsmap = null; - mapping.TryGetValue(ns, out nsmap); - if (nsmap == null) + Dictionary> nsmap; + if (!mapping.TryGetValue(ns, out nsmap)) { return null; } - int tick = basename.IndexOf("`"); - if (tick > -1) - { - basename = basename.Substring(0, tick); - } - - List names = null; - nsmap.TryGetValue(basename, out names); - if (names == null) + List names; + if (!nsmap.TryGetValue(GetBasename(basename), out names)) { return null; } - var result = new List(); 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; } /// @@ -147,13 +109,12 @@ public static List GenericsByName(string ns, string basename) /// public static string GenericNameForBaseName(string ns, string name) { - Dictionary> nsmap = null; - mapping.TryGetValue(ns, out nsmap); - if (nsmap == null) + Dictionary> nsmap; + if (!mapping.TryGetValue(ns, out nsmap)) { return null; } - List gnames = null; + List gnames; nsmap.TryGetValue(name, out gnames); if (gnames?.Count > 0) { @@ -161,5 +122,18 @@ public static string GenericNameForBaseName(string ns, string name) } return null; } + + private static string GetBasename(string name) + { + int tick = name.IndexOf("`"); + if (tick > -1) + { + return name.Substring(0, tick); + } + else + { + return name; + } + } } } From 612c0ccdbed557f731669cf24e4124d8de3ed71e Mon Sep 17 00:00:00 2001 From: Tom Minka <8955276+tminka@users.noreply.github.com> Date: Fri, 18 Dec 2020 01:29:05 +0000 Subject: [PATCH 3/4] Moved the test to Python. Added EmbeddedPythonTest with the capability to run any desired Python tests within NUnit. --- .../{TestList.cs => TestPythonTests.cs} | 22 +++++++++++++------ src/tests/test_generic.py | 5 +++++ 2 files changed, 20 insertions(+), 7 deletions(-) rename src/embed_tests/{TestList.cs => TestPythonTests.cs} (51%) diff --git a/src/embed_tests/TestList.cs b/src/embed_tests/TestPythonTests.cs similarity index 51% rename from src/embed_tests/TestList.cs rename to src/embed_tests/TestPythonTests.cs index 821864372..5026cbce5 100644 --- a/src/embed_tests/TestList.cs +++ b/src/embed_tests/TestPythonTests.cs @@ -1,5 +1,6 @@ using System; using System.Collections.Generic; +using System.IO; using System.Linq; using System.Reflection; using System.Text; @@ -8,7 +9,7 @@ namespace Python.EmbeddingTest { - public class TestList + public class TestPythonTests { [OneTimeSetUp] public void SetUp() @@ -22,14 +23,21 @@ public void Dispose() PythonEngine.Shutdown(); } - [Test] - public void MissingGenericTypeTest() + static IEnumerable MyTestCases() { - Assert.Throws(() => + yield return new[] { "test_generic", "test_missing_generic_type" }; + } + + [TestCaseSource(nameof(MyTestCases))] + public void EmbeddedPythonTest(string testFile, string testName) + { + string folder = @"..\\..\\..\\..\\tests"; PythonEngine.Exec($@" -from System.Collections import IList -IList[bool] -")); +import sys +sys.path.insert(0, '{folder}') +from {testFile} import * +{testName}() +"); } } } diff --git a/src/tests/test_generic.py b/src/tests/test_generic.py index c7e5a8d4d..c865ab32f 100644 --- 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] From 20e673f42ca62df984ee148726c0e027172636a5 Mon Sep 17 00:00:00 2001 From: Tom Minka <8955276+tminka@users.noreply.github.com> Date: Fri, 18 Dec 2020 21:05:38 +0000 Subject: [PATCH 4/4] Removed EmbeddedPythonTest --- src/embed_tests/TestPythonTests.cs | 43 ------------------------------ 1 file changed, 43 deletions(-) delete mode 100644 src/embed_tests/TestPythonTests.cs diff --git a/src/embed_tests/TestPythonTests.cs b/src/embed_tests/TestPythonTests.cs deleted file mode 100644 index 5026cbce5..000000000 --- a/src/embed_tests/TestPythonTests.cs +++ /dev/null @@ -1,43 +0,0 @@ -using System; -using System.Collections.Generic; -using System.IO; -using System.Linq; -using System.Reflection; -using System.Text; -using NUnit.Framework; -using Python.Runtime; - -namespace Python.EmbeddingTest -{ - public class TestPythonTests - { - [OneTimeSetUp] - public void SetUp() - { - PythonEngine.Initialize(); - } - - [OneTimeTearDown] - public void Dispose() - { - PythonEngine.Shutdown(); - } - - static IEnumerable MyTestCases() - { - yield return new[] { "test_generic", "test_missing_generic_type" }; - } - - [TestCaseSource(nameof(MyTestCases))] - public void EmbeddedPythonTest(string testFile, string testName) - { - string folder = @"..\\..\\..\\..\\tests"; - PythonEngine.Exec($@" -import sys -sys.path.insert(0, '{folder}') -from {testFile} import * -{testName}() -"); - } - } -}