Repository navigation
Report converter failure causes explicitly instead of through the error indicator - #157
Conversation
…or indicator Converter.ToManagedValue raised a TypeError for a throwing implicit operator (and for a deleted type) even when called with setError: false, so that MethodBinder.Invoke could surface it as the bind-failure reason. Every other caller that converts with setError: false and expects a clean error indicator could leak that error (ArrayObject.sq_contains, MetaType.sq_contains, PyObject's IConvertible.ToType and dynamic TryConvert, LookUpObject.Contains). The converter now reports that cause through an out conversionError and only raises it when setError is true. MethodBinder.Bind keeps the cause of the last conversion it attempted and Invoke raises it as the same TypeError when no overload matches, so the bind-failure messages are unchanged. LookUpObject.Contains also returned 0 after setting a TypeError, and went on to call ContainsKey with whatever the failed conversion left in its result, which crashed the process when that was a value of the wrong type. It now returns -1 so the TypeError is raised. Co-Authored-By: Claude Opus 5.5 <[email protected]>
There was a problem hiding this comment.
Approving. The converter now reports why a conversion failed through conversionError instead of leaving it on the Python error indicator. setError: false now really means nothing is raised, and the bind-failure messages are the same as before.
What I checked
-
Binder messages unchanged.
Bindnow resetsconversionErrorat the start of each parameter.mastercleared the pending error at about the same point: in theop == nullbranch and afterPyObject_Typewhenmethods.Count > 1. With a single candidate, the failing parameter ends the search, so there is nothing to compare. Every way out matches the old pending-error behavior:- a later candidate rejected by
typematchbefore converting → generic message - the decimal/float probe failing with no implicit fallback → specific message
- a probe failing but a later
ToManagedsucceeding → the cause is overwritten with null - a full match →
binding != null, so the cause is ignored
The only paths in
Bindthat return null after a match set their own error.ClassObject's base-constructorBinduses an empty argument tuple, so it is unaffected. - a later candidate rejected by
-
Nested conversions. The discarded causes are in the
KeyValuePairkey/value conversion and theMakeListelement conversion. Onmasterthose errors never reachedInvokeanyway:type_errorandMakeListcallExceptions.Clear()whensetErroris false. So discarding them changes no message, as the description says. -
Other
setError: falsecallers:ArrayObject/MetaTypesq_contains,IConvertible.ToType, dynamicTryConvert,TupleCodecs,TryAsManagedObject, and the closest-overload diagnosis. None of them read the pending error on purpose, so each one only loses the leak. -
LookUpObject.Contains: the addedreturn -1was missing. Onmaster, a key that fails to convert returned 0/1 with aTypeErrorset. Pure Python code carries on, but the error comes back as aSystemErrorat the next C boundary, so no caller could rely on gettingFalse. If the leftover value had the wrong type, the call crashed the process instead.MetaType.sq_containsalready raises the sameTypeError, so the two are now consistent. -
API: only
internalsignatures change. The onlyInternalsVisibleTotargets are this repo's own test assemblies, so downstream code is unaffected. -
Cost: one extra
outargument per conversion. Messages are still built only on failure.
Evidence: CI is green: Build and Test on Python 3.8–3.14, lean-python-unit-tests and lean-python-regression. The new tests cover both implicit-operator paths with setError true and false, the leak-prone callers (three of the four fail on master), and the in crash. ImplicitConversionErrorHandling still passes. The description also covers an end-to-end algorithm run, comparing it with master. As the description says, the deleted-type branch has no test because it needs a domain reload. It follows the same pattern as the operator catches, which are tested.
Other repositories: no source changes are needed. Lean picks this up with its next pythonnet package update.
Risk: risk:low. The runtime change is small and easy to revert. It keeps the existing behavior on every binder path above. Its one visible change replaces a leaked error or a process crash with the TypeError the code was already trying to raise. It is backed by the new tests, green CI including Lean's suites, and an end-to-end run.
Closes #139
What changed
Converter.ToManagedValueraised aTypeErrorfor a throwing implicit operator, and for a deleted type, even when called withsetError: false. It did that soMethodBinder.Invokecould show the cause instead of the generic "No method matches given arguments". Any other caller that converts withsetError: falseand expects a clean error indicator could therefore leak that error into the next unrelated Python call, or fail a successful call withSystemError: ... returned a result with an exception set.ToManaged/ToManagedValuetake a newout string? conversionErrorand report the cause through it (the two implicit-operator catches and the deleted-type branch). The cause is raised only whensetErroris true, sosetError: falseno longer raises on these paths. The existing 4-argument overloads keep their signatures and discard the cause.Bindreturns the cause of the last argument conversion it attempted. It is reset per parameter at the same point where the binder already calledExceptions.Clear(), so it is the cause a pending error would have carried before. When no overload matches,Invokeraises it as the sameTypeError. The bind-failure messages are unchanged, includingFailed to implicitly convert X to Y(ImplicitConversionErrorHandling) and the generic no-match message.LookUpObject.Contains(inon types withContainsKey/Count): it set aTypeErrorbut returned 0. Then it calledContainsKeywith whatever the failed conversion had left in its result. When that was a value of the wrong type (for example, the intermediate string from a throwingimplicit operator T(string)), reflection threw an unhandledArgumentExceptionthat crashed the process. It now returns -1, sokey in objraises theTypeError.The defensive
Exceptions.Clear()calls added in #138 are kept, and their comments are updated. They still protect against sub-paths outside the converter, such as a user decoder that raises and returns false. The private collection helpers (ToArray,ToList,MakeList,ToEnum) andToPrimitivedon't carry the cause. They already cleared element failures whensetErroris false, so carrying the cause through them would surface new messages rather than keep the current ones.Tests
New embedding tests:
TestConverter.ThrowingImplicitConversionOnlyLeavesErrorWhenSettingErrors: covers both implicit-operator paths withsetErrortrue and false. It checks the reported cause, and that an error is pending only whensetErroris true, with the same message.ThrowingImplicitConversionDoesNotLeakFromConvertibleToType,...FromDynamicConversionand...FromArrayContainsall fail onmasterwith a pending error.ClassManagerTests.ContainsWithKeyOfWrongTypeRaisesTypeError: covers a look-up container and a dynamic one. Onmastertheinreturns False with the error left pending, and the throwing-implicit-key case crashes the test host.Results:
dotnet test --runtime any-x64 src/embed_tests/ --filter "FullyQualifiedName!~SetPythonPath": 1038 passed, 8 skipped, 0 failed. The isolatedSetPythonPathrun: 1 passed.pytest --runtime netcore tests: 477 passed, 23 skipped, 1 failed. The failure istest_module.py::test_explicit_assembly_load(assembly not found in this environment), which fails the same way onmaster.dotnet test --runtime any-x64 src/python_tests_runner/: 1 passed, 1 failed (No module named 'tests.utils', an environment path issue that also fails onmaster).master) built against thisPython.Runtime.dll, with the test selection of theLean Python Unit Testsworkflow: 39220 passed, 100 skipped, 0 failed.master:self.securities.contains_key("BAD TICKER")(Symbol's implicit operator from string throws) raisesTypeError: Failed to implicitly convert System.String to QuantConnect.Symbolon both builds.contains_key("SPY")still returns True, and the backtest trades and completes normally."BAD TICKER" in self.securitiescrashes the process onmasterwith theArgumentExceptionabove. With this change it raisesTypeError: invalid parameter type for sq_contains: ...and the algorithm continues.The deleted-type branch gets the same treatment as the implicit-operator catches, but no test covers it, because building a deleted type needs a domain reload.
Internal signatures change (
Converter.ToManaged/ToManagedValueoverloads,MethodBinder.Bind). Public API and bind-failure messages are unchanged. The visible behavior changes are: no stray pending errors after failedsetError: falseconversions, andTypeErrorinstead of a leaked error or crash frominon look-up types.🤖 Generated with Claude Code