Raise a proper TypeError when a property assignment cannot be performed (#141)

* Set a TypeError when a conversion to a CLR class fails with setError

Converter.ToManagedValue had three exits that reported failure without
setting a Python error: the final fall-through when a plain Python object
has no conversion to the target CLR class, the ClassBase exit when a
reflected class object is converted to something other than Type, and the
exit for managed values that are neither CLRObject, ClassBase, nor
MethodBinding (e.g. a MethodObject). Callers that pass setError true, such
as PropertyObject.tp_descr_set, trust the failure to carry a pending error
and return -1, so CPython raised "SystemError: error return without
exception set" instead of a useful message. Assigning a pure Python object
to a C# property of a CLR class type reproduced this. These exits now set
the conventional TypeError ("'<type>' value cannot be converted to
<target>") when setError is true and no more specific error is already
pending from a probing sub-path; setError false behavior is unchanged.

* Update version to 2.0.64

Bump package <Version>, AssemblyVersion/AssemblyFileVersion and the
perf-test baseline reference to 2.0.64.

Author: jhonabreul

src/embed_tests/TestConverter.cs

@@ -546,6 +546,75 @@ public void OverflowConversionOnlyLeavesErrorWhenSettingErrors(bool setError)
             Assert.AreEqual(setError, Exceptions.ErrorOccurred());
             Exceptions.Clear();
         }
+
+        [TestCase(true)]
+        [TestCase(false)]
+        public void FailedConversionToClrClassOnlyLeavesErrorWhenSettingErrors(bool setError)
+        {
+            using var _ = Py.GIL();
+
+            var module = PyModule.FromString("FailedConversionToClrClassModule", @"
+class PurePythonValue:
+    pass
+");
+            using var instance = module.GetAttr("PurePythonValue").Invoke();
+
+            Assert.IsFalse(Converter.ToManaged(instance, typeof(UriBuilder), out var result, setError));
+            Assert.IsNull(result);
+            Assert.AreEqual(setError, Exceptions.ErrorOccurred());
+            if (setError)
+            {
+                Assert.IsTrue(Exceptions.ExceptionMatches(Exceptions.TypeError));
+            }
+            Exceptions.Clear();
+        }
+
+        [TestCase(true)]
+        [TestCase(false)]
+        public void FailedConversionOfClassObjectOnlyLeavesErrorWhenSettingErrors(bool setError)
+        {
+            using var _ = Py.GIL();
+
+            var module = PyModule.FromString("FailedConversionOfClassObjectModule", @"
+from System import Uri
+");
+            using var classObject = module.GetAttr("Uri");
+
+            Assert.IsFalse(Converter.ToManaged(classObject, typeof(UriBuilder), out var result, setError));
+            Assert.IsNull(result);
+            Assert.AreEqual(setError, Exceptions.ErrorOccurred());
+            if (setError)
+            {
+                Assert.IsTrue(Exceptions.ExceptionMatches(Exceptions.TypeError));
+            }
+            Exceptions.Clear();
+        }
+
+        [TestCase(true)]
+        [TestCase(false)]
+        public void FailedConversionOfNonClrValueManagedTypesOnlyLeavesErrorWhenSettingErrors(bool setError)
+        {
+            using var _ = Py.GIL();
+
+            // a CLR namespace module is a managed type that is neither a CLRObject,
+            // a class, nor a method binding; a class attribute access yields a
+            // MethodBinding that reaches the final conversion fall-through
+            using var systemModule = Py.Import("System");
+            using var uriClass = systemModule.GetAttr("Uri");
+            using var compareBinding = uriClass.GetAttr("Compare");
+
+            foreach (var value in new[] { systemModule, compareBinding })
+            {
+                Assert.IsFalse(Converter.ToManaged(value, typeof(UriBuilder), out var result, setError));
+                Assert.IsNull(result);
+                Assert.AreEqual(setError, Exceptions.ErrorOccurred());
+                if (setError)
+                {
+                    Assert.IsTrue(Exceptions.ExceptionMatches(Exceptions.TypeError));
+                }
+                Exceptions.Clear();
+            }
+        }
     }
 
     public interface IGetList

src/embed_tests/TestPropertyAccess.cs

@@ -52,6 +52,8 @@ public class Fixture
             public static readonly string PublicStaticReadOnlyField = "Default value";
             protected static readonly string ProtectedStaticReadOnlyField = "Default value";
 
+            public Fixture ObjectProperty { get; set; }
+
             public static Fixture Create()
             {
                 return new Fixture();
@@ -291,6 +293,34 @@ def SetValue(self, fixture):
             }
         }
 
+        [Test]
+        public void TestSetPropertyNonConvertibleValueRaisesTypeError()
+        {
+            dynamic model = PyModule.FromString("module", @"
+from clr import AddReference
+AddReference(""System"")
+AddReference(""Python.EmbeddingTest"")
+
+from Python.EmbeddingTest import *
+
+class PurePythonValue:
+    pass
+
+class TestSetPropertyNonConvertibleValueRaisesTypeError:
+    def SetValue(self, fixture):
+        fixture.ObjectProperty = PurePythonValue()
+").GetAttr("TestSetPropertyNonConvertibleValueRaisesTypeError").Invoke();
+
+            var fixture = new Fixture();
+
+            using (Py.GIL())
+            {
+                var exception = Assert.Throws<PythonException>(() => model.SetValue(fixture));
+                Assert.AreEqual("TypeError", exception.Type.Name);
+                StringAssert.Contains("value cannot be converted to", exception.Message);
+            }
+        }
+
         [Test]
         public void TestGetPublicReadOnlyPropertyFailsWhenAccessedOnClass()
         {

src/perf_tests/Python.PerformanceTests.csproj

@@ -13,7 +13,7 @@
       <IncludeAssets>runtime; build; native; contentfiles; analyzers; buildtransitive</IncludeAssets>
     </PackageReference>
     <PackageReference Include="Microsoft.NET.Test.Sdk" Version="16.*" />
-    <PackageReference Include="quantconnect.pythonnet" Version="2.0.63" GeneratePathProperty="true">
+    <PackageReference Include="quantconnect.pythonnet" Version="2.0.64" GeneratePathProperty="true">
       <IncludeAssets>compile</IncludeAssets>
     </PackageReference>
   </ItemGroup>
@@ -25,7 +25,7 @@
   </Target>
 
   <Target Name="CopyBaseline" AfterTargets="Build">
-    <Copy SourceFiles="$(NuGetPackageRoot)quantconnect.pythonnet\2.0.63\lib\net10.0\Python.Runtime.dll" DestinationFolder="$(OutDir)baseline" />
+    <Copy SourceFiles="$(NuGetPackageRoot)quantconnect.pythonnet\2.0.64\lib\net10.0\Python.Runtime.dll" DestinationFolder="$(OutDir)baseline" />
   </Target>
 
   <Target Name="CopyNewBuild" AfterTargets="Build">

src/runtime/Converter.cs

@@ -526,6 +526,7 @@ internal static bool ToManagedValue(BorrowedReference value, Type obType,
                     // The value being converted is a class type, so it will only succeed if it's being converted into a Type
                     if (obType != typeof(Type))
                     {
+                        SetConversionError(value, obType, setError);
                         return false;
                     }
 
@@ -540,6 +541,7 @@ internal static bool ToManagedValue(BorrowedReference value, Type obType,
                 // Method bindings will be handled below along with actual Python callables
                 if (mt is not MethodBinding)
                 {
+                    SetConversionError(value, obType, setError);
                     return false;
                 }
             }
@@ -723,9 +725,26 @@ internal static bool ToManagedValue(BorrowedReference value, Type obType,
                 }
             }
 
+            SetConversionError(value, obType, setError);
             return false;
         }
 
+        /// <summary>
+        /// Sets a TypeError for a failed conversion, unless the caller did not ask for
+        /// errors or a probing sub-path already raised a more specific error that must
+        /// be surfaced instead. Callers that report failure with setError true must
+        /// always leave a Python error pending, otherwise CPython turns the bare error
+        /// return into a "SystemError: error return without exception set".
+        /// </summary>
+        static void SetConversionError(BorrowedReference value, Type obType, bool setError)
+        {
+            if (setError && !Exceptions.ErrorOccurred())
+            {
+                string tpName = Runtime.PyObject_GetTypeName(value);
+                Exceptions.SetError(Exceptions.TypeError, $"'{tpName}' value cannot be converted to {obType}");
+            }
+        }
+
         static bool EncodableByUser(Type type, object value)
         {
             // When no encoders are registered (the common case) skip the type

src/runtime/Properties/AssemblyInfo.cs

@@ -4,5 +4,5 @@
 [assembly: InternalsVisibleTo("Python.EmbeddingTest, PublicKey=00240000048000009400000006020000002400005253413100040000110000005ffd8f49fb44ab0641b3fd8d55e749f716e6dd901032295db641eb98ee46063cbe0d4a1d121ef0bc2af95f8a7438d7a80a3531316e6b75c2dae92fb05a99f03bf7e0c03980e1c3cfb74ba690aca2f3339ef329313bcc5dccced125a4ffdc4531dcef914602cd5878dc5fbb4d4c73ddfbc133f840231343e013762884d6143189")]
 [assembly: InternalsVisibleTo("Python.Test, PublicKey=00240000048000009400000006020000002400005253413100040000110000005ffd8f49fb44ab0641b3fd8d55e749f716e6dd901032295db641eb98ee46063cbe0d4a1d121ef0bc2af95f8a7438d7a80a3531316e6b75c2dae92fb05a99f03bf7e0c03980e1c3cfb74ba690aca2f3339ef329313bcc5dccced125a4ffdc4531dcef914602cd5878dc5fbb4d4c73ddfbc133f840231343e013762884d6143189")]
 
-[assembly: AssemblyVersion("2.0.63")]
-[assembly: AssemblyFileVersion("2.0.63")]
+[assembly: AssemblyVersion("2.0.64")]
+[assembly: AssemblyFileVersion("2.0.64")]

src/runtime/Python.Runtime.csproj

@@ -5,7 +5,7 @@
     <RootNamespace>Python.Runtime</RootNamespace>
     <AssemblyName>Python.Runtime</AssemblyName>
     <PackageId>QuantConnect.pythonnet</PackageId>
-    <Version>2.0.63</Version>
+    <Version>2.0.64</Version>
     <GenerateAssemblyInfo>false</GenerateAssemblyInfo>
     <PackageLicenseFile>LICENSE</PackageLicenseFile>
     <RepositoryUrl>https://github.com/pythonnet/pythonnet</RepositoryUrl>

tests/test_array.py

@@ -761,8 +761,6 @@ def test_null_array():
         ob = Test.NullArrayTest()
         _ = ob.items["wrong"]
 
-# TODO: Error Type should be TypeError for all cases
-# Currently throws SystemErrors instead
 def test_interface_array():
     """Test interface arrays."""
     from Python.Test import Spam
@@ -789,15 +787,15 @@ def test_interface_array():
     items[0] = None
     assert items[0] is None
 
-    with pytest.raises(SystemError):
+    with pytest.raises(TypeError):
         ob = Test.InterfaceArrayTest()
         ob.items[0] = 99
 
     with pytest.raises(TypeError):
         ob = Test.InterfaceArrayTest()
         _ = ob.items["wrong"]
 
-    with pytest.raises(SystemError):
+    with pytest.raises(TypeError):
         ob = Test.InterfaceArrayTest()
         ob.items["wrong"] = "wrong"
 
@@ -828,7 +826,7 @@ def test_typed_array():
     items[0] = None
     assert items[0] is None
 
-    with pytest.raises(SystemError):
+    with pytest.raises(TypeError):
         ob = Test.TypedArrayTest()
         ob.items[0] = 99
 

tests/test_delegate.py

@@ -279,9 +279,9 @@ def test_invalid_object_delegate():
 
     d = ObjectDelegate(hello_func)
     ob = DelegateTest()
-    # QuantConnect fork: a mismatched delegate return surfaces as a .NET
-    # InvalidOperationException rather than a Python SystemError.
-    with pytest.raises(System.InvalidOperationException):
+    # The mismatched delegate return fails its conversion to the expected
+    # type, which surfaces as a TypeError describing the failed conversion.
+    with pytest.raises(TypeError):
         ob.CallObjectDelegate(d)
 
 def test_out_int_delegate():