Raise a proper TypeError when a property assignment cannot be performed#141
Open
jhonabreul wants to merge 3 commits into
Open
Raise a proper TypeError when a property assignment cannot be performed#141jhonabreul wants to merge 3 commits into
jhonabreul wants to merge 3 commits into
Conversation
d8e51db to
f039e68
Compare
Converter.ToManagedValue had three exits that reported failure without
setting a Python error: the final fall-through when a plain Python object
has no conversion to the target CLR class, the ClassBase exit when a
reflected class object is converted to something other than Type, and the
exit for managed values that are neither CLRObject, ClassBase, nor
MethodBinding (e.g. a MethodObject). Callers that pass setError true, such
as PropertyObject.tp_descr_set, trust the failure to carry a pending error
and return -1, so CPython raised "SystemError: error return without
exception set" instead of a useful message. Assigning a pure Python object
to a C# property of a CLR class type reproduced this. These exits now set
the conventional TypeError ("'<type>' value cannot be converted to
<target>") when setError is true and no more specific error is already
pending from a probing sub-path; setError false behavior is unchanged.
Python assignment to a CLR property resolved the setter with nonPublic: true,
so a property declared { get; private set; } was silently writable from
Python: the read-only guard in tp_descr_set never fired and reflection
invoked the private setter, mutating state the class never exposed.
Align property setter binding with the accessibility policy in
ClassManager.ShouldBindMethod (public, protected, protected internal):
CacheAccessors now discards a resolved setter that fails that check, so
assignment raises TypeError 'property is read-only'. Protected setters
remain writable from Python subclasses.
f039e68 to
f069ab4
Compare
Bump package <Version>, AssemblyVersion/AssemblyFileVersion and the perf-test baseline reference to 2.0.64.
11 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What does this implement/fix?
Fixes two related property-assignment bugs that surfaced together when a Lean Python algorithm assigned a pure-Python object to
self.universe_manager(which snake-cases ontoQCAlgorithm.UniverseManager { get; private set; }) and got the opaqueSystemError: error return without exception setinstead of an actionable error:Converter.ToManagedValuecould fail without setting a Python error even when called withsetError: true. Three exits returnedfalsesilently: the final fall-through (plain Python object with no conversion to an arbitrary CLR class), the reflected-class-object case (converting a CLR class object to something other thanType), and non-MethodBindingmanaged types (method/module objects).PropertyObject.tp_descr_settrustssetError: trueand returns-1, so CPython's release-build eval loop raisesSystemError: error return without exception setat the assignment. These exits now set the same'{type}' value cannot be converted to {obType}TypeError thetype_errorlabel already uses, preserving any more specific error a probing sub-path (e.g. a throwing implicit operator) left pending.Properties with private setters were silently writable from Python.
PropertyObject.CacheAccessorsresolved setters withGetSetMethod(nonPublic: true), so{ get; private set; }properties bypassed the"property is read-only"guard and wrote engine-internal state through reflection (convertible values succeeded silently — the Lean repro replaced the algorithm'sUniverseManagerand hung the engine). The setter is now discarded when it does not satisfyClassManager.ShouldBindMethod(the repo's accessibility policy: public, protected, protected internal), so private/internal setters read as read-only while protected setters keep working.With both fixes, the Lean repro turns from
SystemError: error return without exception setintoTypeError: property is read-only, and non-convertible assignments to writable properties reportTypeError: '<class>' value cannot be converted to <type>.Each fix is a separate commit carrying its own regression tests; the tests were verified red before each fix and green after.
Does this close any currently open issues?
No open issue; root-caused from a Lean user report of
error return without exception setduring algorithm initialization.Any other comments?
Behavioral note: code that (likely unknowingly) relied on assigning to private-setter C# properties from Python will now get
TypeError: property is read-only. This aligns property writes withClassManager.ShouldBindMethod, which already governs method/field exposure.End-to-end verification against Lean master (the original repro):
self.universe_manager = <pure python object>in a Python algorithm now fails withTypeError: property is read-onlyat the offending line instead ofSystemError: error return without exception set, and a convertible value no longer silently replaces the algorithm'sUniverseManager(which previously hung the engine on shutdown). Full embed suite on the combined branch: 974 passed, 0 failed, 8 pre-existing environment skips.