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

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions