From 1742e9a0d3ac3f71e667ff9210a99574abef090d Mon Sep 17 00:00:00 2001 From: "quantconnect-kronos[bot]" <346399058+quantconnect-kronos[bot]@users.noreply.github.com> Date: Fri, 30 Oct 2026 13:49:04 +0000 Subject: [PATCH] Report converter failure causes explicitly instead of through the error indicator Converter.ToManagedValue raised a TypeError for a throwing implicit operator (and for a deleted type) even when called with setError: false, so that MethodBinder.Invoke could surface it as the bind-failure reason. Every other caller that converts with setError: false and expects a clean error indicator could leak that error (ArrayObject.sq_contains, MetaType.sq_contains, PyObject's IConvertible.ToType and dynamic TryConvert, LookUpObject.Contains). The converter now reports that cause through an out conversionError and only raises it when setError is true. MethodBinder.Bind keeps the cause of the last conversion it attempted and Invoke raises it as the same TypeError when no overload matches, so the bind-failure messages are unchanged. LookUpObject.Contains also returned 0 after setting a TypeError, and went on to call ContainsKey with whatever the failed conversion left in its result, which crashed the process when that was a value of the wrong type. It now returns -1 so the TypeError is raised. Co-Authored-By: Claude Opus 5.5 --- src/embed_tests/ClassManagerTests.cs | 25 +++++++++++ src/embed_tests/TestConverter.cs | 66 ++++++++++++++++++++++++++++ src/runtime/Converter.cs | 53 ++++++++++++++-------- src/runtime/MethodBinder.cs | 32 +++++++++----- src/runtime/PythonException.cs | 2 +- src/runtime/TypeManager.cs | 4 +- src/runtime/Types/LookUpObject.cs | 1 + 7 files changed, 151 insertions(+), 32 deletions(-) diff --git a/src/embed_tests/ClassManagerTests.cs b/src/embed_tests/ClassManagerTests.cs index f1af2c22a..243ae8efa 100644 --- a/src/embed_tests/ClassManagerTests.cs +++ b/src/embed_tests/ClassManagerTests.cs @@ -1159,6 +1159,31 @@ def contains(dictionary, key): Assert.IsFalse(result); } + [TestCase(typeof(TestKeyValueContainer), 1)] + [TestCase(typeof(DynamicClassDictionary), 1)] + // the failed conversion of the key leaves a string behind, which ContainsKey can't take + [TestCase(typeof(TestKeyValueContainer), "key")] + public void ContainsWithKeyOfWrongTypeRaisesTypeError(Type dictType, object key) + { + using var _ = Py.GIL(); + + var module = PyModule.FromString("ContainsWithKeyOfWrongTypeRaisesTypeError", $@" +def contains(dictionary, key): + try: + key in dictionary + except TypeError as e: + return str(e) +"); + + using var contains = module.GetAttr("contains"); + using var pyDictionary = Activator.CreateInstance(dictType).ToPython(); + using var pyKey = key.ToPython(); + + var error = contains.Invoke(pyDictionary, pyKey).As(); + StringAssert.StartsWith("invalid parameter type for sq_contains", error); + Assert.IsFalse(Exceptions.ErrorOccurred()); + } + [TestCaseSource(nameof(IDictionaryContainsTestCases))] public void CanCheckIfNoneIsInDictionary(Type dictType) { diff --git a/src/embed_tests/TestConverter.cs b/src/embed_tests/TestConverter.cs index 98aa43280..746d13044 100644 --- a/src/embed_tests/TestConverter.cs +++ b/src/embed_tests/TestConverter.cs @@ -718,6 +718,72 @@ from System import Uri Exceptions.Clear(); } + // The implicit operators of ErroredImplicitConversion throw: the conversion reports why, + // and only raises it when asked to set errors + [TestCase(true, true)] + [TestCase(false, true)] + [TestCase(true, false)] + [TestCase(false, false)] + public void ThrowingImplicitConversionOnlyLeavesErrorWhenSettingErrors(bool setError, bool fromClrObject) + { + using var _ = Py.GIL(); + + using var pyValue = fromClrObject ? new TestMethodBinder.ErroredImplicitConversion().ToPython() : "value".ToPython(); + var targetType = fromClrObject ? typeof(string) : typeof(TestMethodBinder.ErroredImplicitConversion); + var expectedError = fromClrObject + ? "Failed to implicitly convert Python.EmbeddingTest.TestMethodBinder+ErroredImplicitConversion to System.String" + : "Failed to implicitly convert System.String to Python.EmbeddingTest.TestMethodBinder+ErroredImplicitConversion"; + + Assert.IsFalse(Converter.ToManaged(pyValue, targetType, out var _, setError, out var _, out var conversionError)); + Assert.AreEqual(expectedError, conversionError); + Assert.AreEqual(setError, Exceptions.ErrorOccurred()); + if (setError) + { + var error = PythonException.FetchCurrentRaw(); + Assert.AreEqual(expectedError, error.Message); + } + } + + [Test] + public void ThrowingImplicitConversionDoesNotLeakFromConvertibleToType() + { + using var _ = Py.GIL(); + + using var pyValue = new TestMethodBinder.ErroredImplicitConversion().ToPython(); + Assert.Throws(() => ((IConvertible)pyValue).ToType(typeof(string), null)); + Assert.IsFalse(Exceptions.ErrorOccurred()); + } + + [Test] + public void ThrowingImplicitConversionDoesNotLeakFromDynamicConversion() + { + using var _ = Py.GIL(); + + dynamic pyValue = new TestMethodBinder.ErroredImplicitConversion().ToPython(); + Assert.Throws(() => { var converted = (string)pyValue; }); + Assert.IsFalse(Exceptions.ErrorOccurred()); + pyValue.Dispose(); + } + + [Test] + public void ThrowingImplicitConversionDoesNotLeakFromArrayContains() + { + using var _ = Py.GIL(); + + var module = PyModule.FromString("ThrowingImplicitConversionArrayContainsModule", @" +from clr import AddReference +AddReference('Python.EmbeddingTest') +from System import Array, String +from Python.EmbeddingTest import TestMethodBinder + +def contains(): + return TestMethodBinder.ErroredImplicitConversion() in Array[String](['value']) +"); + using var contained = module.GetAttr("contains").Invoke(); + Assert.IsFalse(contained.As()); + Assert.IsFalse(Exceptions.ErrorOccurred()); + } + [TestCase(true)] [TestCase(false)] public void FailedConversionOfNonClrValueManagedTypesOnlyLeavesErrorWhenSettingErrors(bool setError) diff --git a/src/runtime/Converter.cs b/src/runtime/Converter.cs index e3c210144..47e592d16 100644 --- a/src/runtime/Converter.cs +++ b/src/runtime/Converter.cs @@ -479,7 +479,7 @@ internal static bool ToManaged(BorrowedReference value, Type type, out object? result, bool setError) { var usedImplicit = false; - return ToManaged(value, type, out result, setError, out usedImplicit); + return ToManaged(value, type, out result, setError, out usedImplicit, out _); } /// /// Return a managed object for the given Python object, taking funny @@ -488,29 +488,37 @@ internal static bool ToManaged(BorrowedReference value, Type type, /// A Python object /// The desired managed type /// Receives the managed object - /// If true, call Exceptions.SetError with the reason for failure. + /// + /// If true, call Exceptions.SetError with the reason for failure. + /// If false, a failed conversion doesn't raise a Python error. + /// + /// + /// Receives why the conversion failed when the value matched the type but converting + /// it threw (e.g. a throwing implicit operator), whether or not is set + /// /// True on success internal static bool ToManaged(BorrowedReference value, Type type, - out object? result, bool setError, out bool usedImplicit) + out object? result, bool setError, out bool usedImplicit, out string? conversionError) { if (type.IsByRef) { type = type.GetElementType(); } - return Converter.ToManagedValue(value, type, out result, setError, out usedImplicit); + return Converter.ToManagedValue(value, type, out result, setError, out usedImplicit, out conversionError); } internal static bool ToManagedValue(BorrowedReference value, Type obType, out object result, bool setError) { var usedImplicit = false; - return ToManagedValue(value, obType, out result, setError, out usedImplicit); + return ToManagedValue(value, obType, out result, setError, out usedImplicit, out _); } internal static bool ToManagedValue(BorrowedReference value, Type obType, - out object? result, bool setError, out bool usedImplicit) + out object? result, bool setError, out bool usedImplicit, out string? conversionError) { usedImplicit = false; + conversionError = null; if (obType == typeof(PyObject)) { result = new PyObject(value); @@ -562,9 +570,11 @@ internal static bool ToManagedValue(BorrowedReference value, Type obType, catch { // Failed to convert using implicit conversion, must catch the error to stop program from exploding on Linux - // Raised even when setError is false: MethodBinder.Invoke surfaces a pending - // error as the binding failure reason instead of its generic no-match message - Exceptions.RaiseTypeError($"Failed to implicitly convert {type} to {obType}"); + conversionError = $"Failed to implicitly convert {type} to {obType}"; + if (setError) + { + Exceptions.RaiseTypeError(conversionError); + } return false; } } @@ -587,7 +597,11 @@ internal static bool ToManagedValue(BorrowedReference value, Type obType, if (!cb.type.Valid) { - Exceptions.SetError(Exceptions.TypeError, cb.type.DeletedMessage); + conversionError = cb.type.DeletedMessage; + if (setError) + { + Exceptions.SetError(Exceptions.TypeError, conversionError); + } return false; } result = cb.type.Value; @@ -737,7 +751,7 @@ internal static bool ToManagedValue(BorrowedReference value, Type obType, var underlyingType = Nullable.GetUnderlyingType(obType); if (underlyingType != null) { - return ToManagedValue(value, underlyingType, out result, setError, out usedImplicit); + return ToManagedValue(value, underlyingType, out result, setError, out usedImplicit, out conversionError); } TypeCode typeCode = Type.GetTypeCode(obType); @@ -758,7 +772,7 @@ internal static bool ToManagedValue(BorrowedReference value, Type obType, var opImplicit = obType.GetMethod("op_Implicit", new[] { obType }); if (opImplicit != null) { - if (ToManagedValue(value, opImplicit.ReturnType, out result, setError, out usedImplicit)) + if (ToManagedValue(value, opImplicit.ReturnType, out result, setError, out usedImplicit, out conversionError)) { opImplicit = obType.GetMethod("op_Implicit", new[] { result.GetType() }); if (opImplicit != null) @@ -770,9 +784,11 @@ internal static bool ToManagedValue(BorrowedReference value, Type obType, catch { // Failed to convert using implicit conversion, must catch the error to stop program from exploding on Linux - // Raised even when setError is false: MethodBinder.Invoke surfaces a pending - // error as the binding failure reason instead of its generic no-match message - Exceptions.RaiseTypeError($"Failed to implicitly convert {result.GetType()} to {obType}"); + conversionError = $"Failed to implicitly convert {result.GetType()} to {obType}"; + if (setError) + { + Exceptions.RaiseTypeError(conversionError); + } return false; } } @@ -1051,11 +1067,11 @@ internal static bool ToPrimitive(BorrowedReference value, Type obType, out objec // references returned through key, dicValue are borrowed. if (Runtime.PyDict_Next(value, out pos, out key, out dicValue) != 0) { - if (!ToManaged(key, typeArguments[0], out var convertedKey, setError, out usedImplicit)) + if (!ToManaged(key, typeArguments[0], out var convertedKey, setError, out usedImplicit, out _)) { goto type_error; } - if (!ToManaged(dicValue, typeArguments[1], out var convertedValue, setError, out usedImplicit)) + if (!ToManaged(dicValue, typeArguments[1], out var convertedValue, setError, out usedImplicit, out _)) { goto type_error; } @@ -1656,8 +1672,7 @@ private static IList MakeList(BorrowedReference value, NewReference IterObject, if (!Converter.ToManaged(item.Borrow(), elementType, out var obj, setError)) { - // some element conversion paths raise even when setError is false - // (e.g. failed implicit operators, deleted types) + // a conversion sub-path may have left an error pending (e.g. a raising user decoder) if (!setError) { Exceptions.Clear(); diff --git a/src/runtime/MethodBinder.cs b/src/runtime/MethodBinder.cs index 803ee2f23..5ec3edc66 100644 --- a/src/runtime/MethodBinder.cs +++ b/src/runtime/MethodBinder.cs @@ -470,11 +470,17 @@ internal static int ArgPrecedence(Type t, bool isOperatorMethod) /// internal Binding Bind(BorrowedReference inst, BorrowedReference args, BorrowedReference kw) { - return Bind(inst, args, kw, null); + return Bind(inst, args, kw, null, out _); } - internal Binding Bind(BorrowedReference inst, BorrowedReference args, BorrowedReference kw, MethodBase info) + /// + /// When no overload matches, receives why the last argument conversion attempted failed, + /// if converting it threw (e.g. a throwing implicit operator) + /// + internal Binding Bind(BorrowedReference inst, BorrowedReference args, BorrowedReference kw, MethodBase info, out string? conversionError) { + conversionError = null; + // If we have KWArgs create dictionary and collect them. // An empty kwargs dict (e.g. calling with **{}) is equivalent to no kwargs at all, // so we only create the dictionary if there are actual keyword arguments, @@ -597,6 +603,8 @@ internal Binding Bind(BorrowedReference inst, BorrowedReference args, BorrowedRe } } + // Only a conversion that rejects the last candidate tried explains why nothing matched + conversionError = null; NewReference tempObject = default; // At this point, if op is IntPtr.Zero we don't have a KWArg and are not using default @@ -687,7 +695,7 @@ internal Binding Bind(BorrowedReference inst, BorrowedReference args, BorrowedRe if (underlyingType == typeof(decimal) || underlyingType == typeof(double)) { clrtype = parameter.ParameterType; - typematch = Converter.ToManaged(op, clrtype, out arg, false); + typematch = Converter.ToManaged(op, clrtype, out arg, false, out _, out conversionError); if (typematch) { implicitConversions++; @@ -700,7 +708,7 @@ internal Binding Bind(BorrowedReference inst, BorrowedReference args, BorrowedRe else if (Runtime.PyFloat_Check(op) && argtypecode.IsInteger() && !underlyingType.IsEnum) { clrtype = parameter.ParameterType; - typematch = Converter.ToManaged(op, clrtype, out arg, false); + typematch = Converter.ToManaged(op, clrtype, out arg, false, out _, out conversionError); if (typematch) { implicitConversions++; @@ -745,7 +753,7 @@ internal Binding Bind(BorrowedReference inst, BorrowedReference args, BorrowedRe outs++; } - if (!Converter.ToManaged(op, clrtype, out arg, false)) + if (!Converter.ToManaged(op, clrtype, out arg, false, out _, out conversionError)) { tempObject.Dispose(); margs = null; @@ -812,10 +820,8 @@ internal Binding Bind(BorrowedReference inst, BorrowedReference args, BorrowedRe if (matches.Count > 0 || (matchesUsingImplicitConversion != null && matchesUsingImplicitConversion.Count > 0)) { - // A rejected overload's conversion probe may have raised a Python error - // even though probing converts with setError: false (e.g. a throwing - // implicit operator raises unconditionally so Invoke can surface it as - // the failure reason when nothing matches). Once an overload matched, + // A rejected overload's conversion probe may have left a Python error + // pending (e.g. a raising user decoder). Once an overload matched, // that error must not survive the successful call: CPython would fail // it with "SystemError: ... returned a result with an error set". Exceptions.Clear(); @@ -1027,12 +1033,18 @@ internal virtual NewReference Invoke(BorrowedReference inst, BorrowedReference a internal virtual NewReference Invoke(BorrowedReference inst, BorrowedReference args, BorrowedReference kw, MethodBase info, MethodInfo[] methodinfo) { - Binding binding = Bind(inst, args, kw, info); + Binding binding = Bind(inst, args, kw, info, out var conversionError); object result; IntPtr ts = IntPtr.Zero; if (binding == null) { + // A conversion that threw has a more specific cause than the generic no-match message + if (conversionError != null && !Exceptions.ErrorOccurred()) + { + Exceptions.RaiseTypeError(conversionError); + } + // If we already have an exception pending, don't create a new one if (!Exceptions.ErrorOccurred()) { diff --git a/src/runtime/PythonException.cs b/src/runtime/PythonException.cs index 89737855e..129668239 100644 --- a/src/runtime/PythonException.cs +++ b/src/runtime/PythonException.cs @@ -142,7 +142,7 @@ internal static Exception FetchCurrent() return null; } - if (Converter.ToManagedValue(pyInfo.Borrow(), typeof(ExceptionDispatchInfo), out object? result, setError: false, out var _)) + if (Converter.ToManagedValue(pyInfo.Borrow(), typeof(ExceptionDispatchInfo), out object? result, setError: false, out var _, out _)) { return (ExceptionDispatchInfo)result!; } diff --git a/src/runtime/TypeManager.cs b/src/runtime/TypeManager.cs index f526d87a6..e20b8c5ba 100644 --- a/src/runtime/TypeManager.cs +++ b/src/runtime/TypeManager.cs @@ -385,7 +385,7 @@ internal static NewReference CreateSubType(BorrowedReference py_name, BorrowedRe { if (Exceptions.ErrorOccurred()) return default; } - else if (!Converter.ToManagedValue(assemblyPtr, typeof(string), out assembly, true, out var _)) + else if (!Converter.ToManagedValue(assemblyPtr, typeof(string), out assembly, true, out var _, out _)) { return Exceptions.RaiseTypeError("Couldn't convert __assembly__ value to string"); } @@ -397,7 +397,7 @@ internal static NewReference CreateSubType(BorrowedReference py_name, BorrowedRe { if (Exceptions.ErrorOccurred()) return default; } - else if (!Converter.ToManagedValue(pyNamespace, typeof(string), out namespaceStr, true, out var _)) + else if (!Converter.ToManagedValue(pyNamespace, typeof(string), out namespaceStr, true, out var _, out _)) { return Exceptions.RaiseTypeError("Couldn't convert __namespace__ value to string"); } diff --git a/src/runtime/Types/LookUpObject.cs b/src/runtime/Types/LookUpObject.cs index c2f9cd885..6af646842 100644 --- a/src/runtime/Types/LookUpObject.cs +++ b/src/runtime/Types/LookUpObject.cs @@ -111,6 +111,7 @@ internal static int Contains(BorrowedReference ob, BorrowedReference v, Dictiona { Exceptions.SetError(Exceptions.TypeError, $"invalid parameter type for sq_contains: should be {Converter.GetTypeByAlias(v)}, found {parameters[0].ParameterType}"); + return -1; } // If the argument is None, we return false. Python allows using None as key,