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,