Obj: refuse to auto-vivify dunder and sunder attributes#185
Open
Deln0r wants to merge 1 commit into
Open
Conversation
Obj.__getattr__ currently creates an empty child Obj for any
attribute that is missing from the backing dict. This is convenient
for keyword-escape access like obj.from_ but it interacts badly with
Python introspection tooling:
- inspect.unwrap probes ``.__wrapped__``, gets a fresh Obj back,
follows it, gets another fresh Obj... and the unwrap loop in
CPython 3.13 terminates with a wrapper-loop ValueError, but the
Obj has now been mutated to hold a recursive ``__wrapped``
entry, and any subsequent serialize()/__repr__() call recurses
until RecursionError. See mautrix#176 for the original traceback.
- IPython's display path probes
``_ipython_canary_method_should_not_exist_`` (a sunder name) on
every object it displays, leaving the same kind of stray entry.
Refuse to create attributes whose name starts AND ends with an
underscore so __getattr__ stays useful for the documented
keyword-escape (single trailing underscore, e.g. ``from_``) but
returns AttributeError for dunder / sunder probes. Defined dunder
methods (__init__, __repr__, ...) are unaffected because __getattr__
is only called when normal attribute lookup has already failed.
Adds obj_test.py with five regression cases covering normal access,
the keyword-escape, dunder probe, sunder probe, and the original
inspect.unwrap end-to-end scenario.
Closes mautrix#176.
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.
Closes #176.
Problem
Obj.__getattr__auto-vivifies missing attributes into empty childObjinstances. That is intentional for the documented keyword-escape pattern (obj.from_maps to thefromkey), but it interacts badly with Python introspection tooling:inspect.unwrap(obj)probes__wrapped__, gets a freshObjback, follows it, gets another freshObj, and the underlyingObjnow holds a recursive__wrappedentry. Any subsequentserialize()or__repr__()call recurses untilRecursionError— exactly the traceback in the issue._ipython_canary_method_should_not_exist_(a sunder name) on every object it displays, leaving the same kind of stray entry.Fix
Refuse to create attributes whose name starts AND ends with an underscore. The keyword-escape case (single trailing underscore, e.g.
from_) is unaffected because the leading underscore is absent. Defined dunder methods (__init__,__repr__, …) are unaffected because__getattr__is only called when normal attribute lookup has already failed.This is the exact shape proposed by the issue author.
Test plan
A new
mautrix/types/util/obj_test.pycovers:test_obj_basic_attribute_accessobj.name = ...access still workstest_obj_trailing_underscore_keyword_escapeobj.from_still maps to thefromkey (no regression)test_obj_dunder_attribute_raisesobj.__wrapped__raises and does NOT pollute__dict__test_obj_sunder_attribute_raisesobj._ipython_canary_method_should_not_exist_raisestest_obj_inspect_unwrap_does_not_recurseinspect.unwrap(o) is o,o.serialize() == {}