Let a shadowing class attribute win over the exception setattr/delattr arms - #1967
Merged
Merged
Conversation
Unset OSError.characters_written continues MRO lookup so
`class E(OSError): characters_written = 42` reads 42. The OSError
getset still raises AttributeError("characters_written").
traceback() constructor and tb_next setter TypeError text match
the 3.14 named-argument form.
Assisted-by: Claude
…r arms
`object_setattr` and `object_delattr` walk the type MRO into `w_descr`
before the store. A subclass that shadows an exception slot name with a
plain class attribute (`class E(OSError): errno = 99`) contributes no
`__set__`/`__delete__`, so `descr__setattr__` continues to `setdictvalue`
and `descr__delattr__` to `deldictvalue`. The `if is_exception(obj)` arms
below both fired by name regardless and reached the interpreter slot, so
the assignment never got to the instance dict and the removal never took
the dict entry back out:
class E(OSError): errno = 99
e = E(1, 'm'); e.errno = 'SET'
e.errno, e.__dict__ # was (99, {}), now ('SET', {'errno': 'SET'})
del e.errno
e.errno, e.__dict__ # was (None, {'errno': None}), now (99, {})
The arms stand in for a `GetSetProperty.__set__` / `__delete__`, which
only run when the MRO resolved to that descriptor, so gate both on the
walk having found nothing. Every exception `GetSetProperty` carries the
one shared `exception_getset_fset` / `exception_getset_fdel`, which call
`exception_attr_set` / `exception_attr_delete` themselves, so the
unshadowed paths are unchanged: `OSError().errno = 5` still writes the
slot through the descriptor, `BaseExceptionGroup(...).message = 'z'` still
raises `readonly attribute`, `del OSError().errno` still resets the slot,
and an undeclared name still reaches `setdictvalue` with `w_descr` unset.
Measured over 32 exception slot attributes across 9 classes under a
shadowing subclass, five patterns each (read, set-then-read, check
`__dict__`, set-del-read, del-unset). `pypy3` and CPython 3.14 agree on
the first four, 144 cases: pyre differed on 62 before this and none
after, so the arms were a pyre-only deviation with no upstream
counterpart rather than a spec question.
On the fifth, `del` of a name that was never set, the two upstreams
disagree and pyre matched neither: it reset the slot and reported the
delete as having succeeded. It now reaches the shared
`raiseattrerror(obj, name, w_descr)` terminal that `descr__delattr__`
ends in, which reports a found-but-non-data descriptor as
`'E' object attribute 'errno' is read-only` -- PyPy's message, where 3.14
says `'E' object has no attribute 'errno'`. That difference is
`raiseattrerror`'s own shape, shared with the setattr terminal, and is
left as it stands.
Assisted-by: Claude
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Advanced Run ID: 📒 Files selected for processing (3)
Included review availability: This review used your included allowance. Your plan provides up to 2 included reviews per hour; 0 remain after this review. WalkthroughThe changes update exception attribute lookup and descriptor handling. They also revise argument and type error messages for traceback construction and ChangesException attribute handling
Traceback error reporting
Priority: ⬇️ Low Estimated code review effort: 2 (Simple) | ~10 minutes Change: Bug fix Merge Risk: ⚪ Minimal · up to The intended exception attribute behavior is supported by the inspected lookup paths, and no actionable issue remains before merge. Architecture SummaryArchitecture risk: 🔵 Low · up to The change affects 1 system. Changed systems: Architecture concerns Review detailsSystems and components
Before / after behavior
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. A rabbit checks the traceback call, Comment |
🤖 Codex parity reviewStatic analysis of this diff vs the local RPython/PyPy sources (commit 1fb3764). Files in the reviewed diffCodex did not produce a report (exit 1). Last log lines: |
This was referenced Sep 26, 2026
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
1 participant
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.
characters_writtenread side (first commit)#1959 landed a version of the unset-
characters_writtengetter that returnedErrdirectly, which short-circuits the MRO lookup so a subclass classattribute could not win:
The follow-up that fixed it (
Ok(PY_NULL), letting the caller continue, withthe raise staying in
exception_getset_fgetwhere the descriptor is) wasdropped by #1959's squash, so it is recovered here. The same commit carries the
three
traceback()/tb_nextmessages from that review.object_setattrandobject_delattrwalk the type MRO intow_descrbeforethe store, then run an
if is_exception(obj)arm that reaches the interpreterslot by name. The arm fired regardless of what the walk found, so a subclass
that shadows an exception slot name with a plain class attribute lost the
write entirely:
The arms stand in for a
GetSetProperty.__set__/__delete__, which only runwhen the MRO resolved to that descriptor.
descr__setattr__continues tosetdictvalueanddescr__delattr__todeldictvaluewhen the walk found anon-data descriptor, so both arms are now gated on
w_descr.is_none(). Theread side already had this ordering — its exception arm lives in
object_getattr_miss, after the walk — which is why only the two write pathswere affected.
The unshadowed paths are unchanged, because every exception
GetSetPropertycarries the one shared
exception_getset_fset/exception_getset_fdelandthose call
exception_attr_set/exception_attr_deletethemselves:OSError().errno = 5still writes the slot through the descriptor,BaseExceptionGroup(...).message = 'z'still raisesreadonly attribute,del OSError().errnostill resets the slot, and an undeclared name stillreaches
setdictvaluewithw_descrunset.Measurement
32 exception slot attributes across 9 classes under a shadowing subclass, five
patterns each — read, set-then-read, check
__dict__, set-del-read, del-unset.pypy3== CPython 3.14del-unset)pypy3Where both upstreams agree, pyre was alone — the arms were a pyre-only
deviation with no upstream counterpart, not a spec question.
On
delof a name that was never set the two upstreams disagree, and pyrepreviously matched neither: it reset the slot and reported the delete as having
succeeded. It now reaches the shared
raiseattrerror(obj, name, w_descr)terminal
descr__delattr__ends in, which reports a found-but-non-datadescriptor as
'E' object attribute 'errno' is read-only. 3.14 says'E' object has no attribute 'errno'there. That remaining difference israiseattrerror's own message shape, shared with the setattr terminal, and isleft as it stands.
— commented by Claude
Summary by CodeRabbit
characters_writtenvalue. Missing values produce a more specific error.tb_nextvalues provide clearer details.