Report converter failure causes explicitly instead of through the error indicator

src/embed_tests/ClassManagerTests.cs

@@ -1159,6 +1159,31 @@ def contains(dictionary, key):
             Assert.IsFalse(result);
         }
 
+        [TestCase(typeof(TestKeyValueContainer<string, string>), 1)]
+        [TestCase(typeof(DynamicClassDictionary<string, string>), 1)]
+        // the failed conversion of the key leaves a string behind, which ContainsKey can't take
+        [TestCase(typeof(TestKeyValueContainer<TestMethodBinder.ErroredImplicitConversion, string>), "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<string>();
+            StringAssert.StartsWith("invalid parameter type for sq_contains", error);
+            Assert.IsFalse(Exceptions.ErrorOccurred());
+        }
+
         [TestCaseSource(nameof(IDictionaryContainsTestCases))]
         public void CanCheckIfNoneIsInDictionary(Type dictType)
         {

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<InvalidCastException>(() => ((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<Microsoft.CSharp.RuntimeBinder.RuntimeBinderException>(() => { 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<bool>());
+            Assert.IsFalse(Exceptions.ErrorOccurred());
+        }
+
         [TestCase(true)]
         [TestCase(false)]
         public void FailedConversionOfNonClrValueManagedTypesOnlyLeavesErrorWhenSettingErrors(bool setError)

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 _);
         }
         /// <summary>
         /// Return a managed object for the given Python object, taking funny
@@ -488,29 +488,37 @@ internal static bool ToManaged(BorrowedReference value, Type type,
         /// <param name="value">A Python object</param>
         /// <param name="type">The desired managed type</param>
         /// <param name="result">Receives the managed object</param>
-        /// <param name="setError">If true, call <c>Exceptions.SetError</c> with the reason for failure.</param>
+        /// <param name="setError">
+        /// If true, call <c>Exceptions.SetError</c> with the reason for failure.
+        /// If false, a failed conversion doesn't raise a Python error.
+        /// </param>
+        /// <param name="conversionError">
+        /// Receives why the conversion failed when the value matched the type but converting
+        /// it threw (e.g. a throwing implicit operator), whether or not <paramref name="setError"/> is set
+        /// </param>
         /// <returns>True on success</returns>
         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();

src/runtime/MethodBinder.cs

@@ -470,11 +470,17 @@ internal static int ArgPrecedence(Type t, bool isOperatorMethod)
         /// </summary>
         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)
+        /// <param name="conversionError">
+        /// When no overload matches, receives why the last argument conversion attempted failed,
+        /// if converting it threw (e.g. a throwing implicit operator)
+        /// </param>
+        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())
                 {

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!;
             }

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");
                     }