Repository navigation
Converter failure contract conflates 'no match' with 'user code threw', forcing error-indicator smuggling under setError: false #139
Description
Context
#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: falsecorrectly 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 aTypeErroron the indicator even whensetErroris false (the implicit-conversion catches inConverter.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-stateErrorBehavior { None, Set, Capture }) throughToManaged/ToManagedValue/ToPrimitiveand 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.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).- With that in place,
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.
Activity