You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
#138 fixed a family of leaked-Python-error bugs (TryAs*, converter collection paths, MethodBinder overload probing) that all share one root cause: the converter's failure contract is ambiguous, and the Python error indicator is being used as an undeclared side channel.
The design flaw
Converter.ToManaged/ToManagedValue/ToPrimitive report failure as a bool, with a setError flag that nominally controls whether the Python error indicator is set. But a boolean cannot distinguish two very different failures:
Didn't match — the Python value doesn't fit the target type. Expected and benign; overload probing hits this dozens of times per call. setError: false correctly suppresses errors here.
Matched but user code blew up — e.g. a throwing implicit operator (op_Implicit). Here the converter holds information that exists nowhere else (the CLR exception it just swallowed) and has no channel to return it, so it raises a TypeError on the indicator even when setError is false (the implicit-conversion catches in Converter.ToManagedValue).
MethodBinder.Invoke deliberately reads that smuggled error on the no-match path and surfaces it instead of the generic "No method matches given arguments" message (covered by ImplicitConversionErrorHandling). The result is a contradictory contract: MethodBinder probes with setError: false yet relies on the converter setting errors anyway.
Because the effective contract is "setError: false means usually no error pending", every caller that reasonably assumes false means a clean indicator is a latent leak. That's exactly what #138 had to patch, case by case: TryAsManagedObject clears before returning false, MakeList clears its element-conversion failures, and Bind clears once an overload matches. Since #137 pins thread states, any such leak survives Py.GIL scopes and poisons the next unrelated Python call on the thread (or fails a successful call with SystemError: ... returned a result with an error set).
Known remaining callers that assume setError: false leaves a clean indicator and could leak if a raising path fires (throwing implicit operator, deleted type at Converter.ToManagedValue's ClassBase branch, or a user IPyObjectDecoder that raises and returns false): ArrayObject.sq_contains, MetaType.sq_contains (earlier probe raises, later probe succeeds), PyObject.IConvertible.ToType, dynamic PyObject.TryConvert, and LookUpObject.Contains (which also sets an error and returns 0 instead of -1).
Recommendation
Make the second failure kind an explicit channel instead of smuggling it through the error indicator:
Thread an out Exception? conversionError (or a tri-state ErrorBehavior { None, Set, Capture }) through ToManaged/ToManagedValue/ToPrimitive and their private helpers (ToArray, ToList, MakeList, ToEnum).
The implicit-conversion catches populate the captured cause instead of calling Exceptions.RaiseTypeError; same for the deleted-type branch.
MethodBinder.Bind keeps the captured cause of the last rejected candidate; Invoke uses it on the no-match path to produce the same specific TypeError it produces today (preserving the ImplicitConversionErrorHandling contract and the message format Lean parses).
With that in place, setError: false genuinely guarantees an untouched indicator, and the defensive Exceptions.Clear() calls added in Clear pending Python error when a Try-style conversion fails #138 (and the ones the remaining callers above would otherwise need) become unnecessary.
Most call sites would just pass a discard, so the refactor is wide but mechanical. The main risk area is MethodBinder's no-match message content, which downstream Lean parses — the captured-cause path must reproduce it exactly.
Context
#138 fixed a family of leaked-Python-error bugs (
TryAs*, converter collection paths,MethodBinderoverload probing) that all share one root cause: the converter's failure contract is ambiguous, and the Python error indicator is being used as an undeclared side channel.The design flaw
Converter.ToManaged/ToManagedValue/ToPrimitivereport failure as abool, with asetErrorflag that nominally controls whether the Python error indicator is set. But a boolean cannot distinguish two very different failures:setError: falsecorrectly suppresses errors here.op_Implicit). Here the converter holds information that exists nowhere else (the CLR exception it just swallowed) and has no channel to return it, so it raises aTypeErroron the indicator even whensetErroris false (the implicit-conversion catches inConverter.ToManagedValue).MethodBinder.Invokedeliberately reads that smuggled error on the no-match path and surfaces it instead of the generic "No method matches given arguments" message (covered byImplicitConversionErrorHandling). The result is a contradictory contract:MethodBinderprobes withsetError: falseyet relies on the converter setting errors anyway.Because the effective contract is "
setError: falsemeans usually no error pending", every caller that reasonably assumesfalsemeans a clean indicator is a latent leak. That's exactly what #138 had to patch, case by case:TryAsManagedObjectclears before returningfalse,MakeListclears its element-conversion failures, andBindclears once an overload matches. Since #137 pins thread states, any such leak survivesPy.GILscopes and poisons the next unrelated Python call on the thread (or fails a successful call withSystemError: ... returned a result with an error set).Known remaining callers that assume
setError: falseleaves a clean indicator and could leak if a raising path fires (throwing implicit operator, deleted type atConverter.ToManagedValue'sClassBasebranch, or a userIPyObjectDecoderthat raises and returns false):ArrayObject.sq_contains,MetaType.sq_contains(earlier probe raises, later probe succeeds),PyObject.IConvertible.ToType, dynamicPyObject.TryConvert, andLookUpObject.Contains(which also sets an error and returns 0 instead of -1).Recommendation
Make the second failure kind an explicit channel instead of smuggling it through the error indicator:
out Exception? conversionError(or a tri-stateErrorBehavior { None, Set, Capture }) throughToManaged/ToManagedValue/ToPrimitiveand their private helpers (ToArray,ToList,MakeList,ToEnum).Exceptions.RaiseTypeError; same for the deleted-type branch.MethodBinder.Bindkeeps the captured cause of the last rejected candidate;Invokeuses it on the no-match path to produce the same specificTypeErrorit produces today (preserving theImplicitConversionErrorHandlingcontract and the message format Lean parses).setError: falsegenuinely guarantees an untouched indicator, and the defensiveExceptions.Clear()calls added in Clear pending Python error when a Try-style conversion fails #138 (and the ones the remaining callers above would otherwise need) become unnecessary.Most call sites would just pass a discard, so the refactor is wide but mechanical. The main risk area is
MethodBinder's no-match message content, which downstream Lean parses — the captured-cause path must reproduce it exactly.