Skip to content

Converter failure contract conflates 'no match' with 'user code threw', forcing error-indicator smuggling under setError: false #139

Description

@jhonabreul

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:

  1. 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.
  2. 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.

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Fields

    No fields configured for issues without a type.

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions