typedef: validate the classmethod_descriptor owner and share wrap_descr_get - #981
Conversation
…cr_get
`descrobject.c classmethod_get` requires the owner argument to be a type and
a subtype of the type the descriptor was found on. `classmethod_descr_get`
checked neither, so `dict.__dict__['fromkeys'](list, ['x'])` entered
`dict.fromkeys` with `list` and raised from inside it, and
`type.__dict__['__prepare__'](list, 'X', ())` returned `{}`. The check runs
only for a `classmethod_descriptor`; `funcobject.c cm_descr_get`, the user
`@classmethod` half, has none. `classmethoddescr_call` reaches the same
validation.
The three `__get__` namespace entries added for the descriptor types
registered a fixed arity of 3, but `typeobject.c wrap_descr_get` unpacks
1..2 arguments, so `list.__dict__['append'].__get__([1])`,
`object.__dict__['__str__'].__get__(3)` and
`dict.__dict__['fromkeys'].__get__({})` were rejected. They now share a
`wrap_descr_get` helper that collapses `None` in either position and rejects
`__get__(None, None)`.
`classmethod_descriptor`'s getsets reported a foreign receiver as
"requires a 'classmethod_descriptor' object"; the sibling receiver helpers
already use the "for 'X' objects doesn't apply to a 'Y' object" wording.
extra_tests/parity_tests/classmethod_descriptor_kind.py pins the descriptor
kind, the owner validation, the `__get__` arity and the getset wording.
Assisted-by: Claude
WalkthroughThe interpreter now follows dynamic ChangesABC subclass protocol
Builtin classmethod descriptor behavior
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant wrap_descr_get
participant classmethod_descriptor
participant BoundMethod
Caller->>wrap_descr_get: call descriptor __get__
wrap_descr_get->>wrap_descr_get: validate arity and normalize arguments
wrap_descr_get->>classmethod_descriptor: validate owner and receiver
classmethod_descriptor->>BoundMethod: create bound method
BoundMethod-->>Caller: return bound method
sequenceDiagram
participant ABCMeta
participant subclass_of
participant __subclasses__
participant Iterator
ABCMeta->>subclass_of: check subclass relationship
subclass_of->>__subclasses__: lookup and call override
__subclasses__-->>Iterator: return iterable
subclass_of->>Iterator: iterate subclass values
Iterator-->>subclass_of: return value or exception
subclass_of-->>ABCMeta: return subclass result
Possibly related PRs
Poem
🚥 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. Comment |
🤖 Codex parity reviewStatic analysis of this diff vs the local RPython/PyPy sources (commit d45499e). Files in the reviewed diff1. Regressions to PyPy parity introduced by this patch
2. Other mismatches introduced by this patchNone. 3. Pre-existing mismatches (already present before this patch)
4. Structural adaptations
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
pyre/pyre-interpreter/src/typedef.rs (1)
13286-13303: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReuse the existing type-name helper.
Convert the manual
receivelookup to the same reusable helper used for the owner/type argument lookup to avoid duplicating thePyObjectRef-> type name resolution.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pyre/pyre-interpreter/src/typedef.rs` around lines 13286 - 13303, Update classmethod_descriptor_function to use the existing reusable PyObjectRef-to-type-name helper for the received object instead of manually checking is_null, calling typedef::r#type, and reading the type name. Preserve the current fallback to "object" and the existing error message behavior.pyre/pyre-interpreter/src/module/_abc/mod.rs (1)
216-247: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winPreserve attribute access failures for
_abc_registry.
getattr_str(..., "_abc_registry")can report any attribute lookup failure, while the_py_abc.py:136flow expects the normal Python attribute lookup error to become theissubclass()error. Only swallow the missing-attribute fallback case, and return other errors to avoid changing caller-visible behavior.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pyre/pyre-interpreter/src/module/_abc/mod.rs` around lines 216 - 247, Update the `_abc_registry` lookup in the surrounding subclass-check flow to distinguish a missing attribute from other `getattr_str` failures: continue with no registry only for the missing-attribute fallback, and propagate all other errors so `issubclass()` preserves normal attribute lookup behavior. Keep the existing registry list validation and recursive checks unchanged.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pyre/extra_tests/parity_tests/abc_subclasses_protocol.py`:
- Around line 52-65: Add a dedicated registry-path test in the existing ABC
subclass coverage by defining a separate class, registering it with
Base.register, and asserting issubclass(SomeClass, Base). Keep the current
inheritance assertions intact and ensure the new case exercises the
registered-class recursive branch rather than normal MRO inheritance.
In `@pyre/pyre-interpreter/src/typedef.rs`:
- Around line 12946-12971: Update wrap_descr_get in
pyre/pyre-interpreter/src/typedef.rs to pass an empty method name to the arity
validators, producing CPython’s empty-name messages for both minimum and maximum
argument failures. Update the corresponding parity assertions in
pyre/extra_tests/parity_tests/classmethod_descriptor_kind.py to expect “
expected at least 1 argument, got 0” and “ expected at most 2 arguments, got 3”;
no other sites require changes.
---
Outside diff comments:
In `@pyre/pyre-interpreter/src/module/_abc/mod.rs`:
- Around line 216-247: Update the `_abc_registry` lookup in the surrounding
subclass-check flow to distinguish a missing attribute from other `getattr_str`
failures: continue with no registry only for the missing-attribute fallback, and
propagate all other errors so `issubclass()` preserves normal attribute lookup
behavior. Keep the existing registry list validation and recursive checks
unchanged.
In `@pyre/pyre-interpreter/src/typedef.rs`:
- Around line 13286-13303: Update classmethod_descriptor_function to use the
existing reusable PyObjectRef-to-type-name helper for the received object
instead of manually checking is_null, calling typedef::r#type, and reading the
type name. Preserve the current fallback to "object" and the existing error
message behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: dc3f2c65-019c-4df1-855f-c6acfe6d1205
📒 Files selected for processing (4)
pyre/extra_tests/parity_tests/abc_subclasses_protocol.pypyre/extra_tests/parity_tests/classmethod_descriptor_kind.pypyre/pyre-interpreter/src/module/_abc/mod.rspyre/pyre-interpreter/src/typedef.rs
| class Base(metaclass=ABCMeta): | ||
| pass | ||
|
|
||
|
|
||
| class Child(Base): | ||
| pass | ||
|
|
||
|
|
||
| class Grandchild(Child): | ||
| pass | ||
|
|
||
|
|
||
| assert issubclass(Grandchild, Base) | ||
| print("OK") |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Add coverage for the _abc_registry recursion path.
This file covers the direct-hook, invalid-__subclasses__-protocol, exception-propagation, and MRO-based paths, but the registered-class recursive branch in subclass_of (_py_abc.py:135-139, registry walk in mod.rs) has no dedicated test. Add a case that calls Base.register(SomeClass) and asserts issubclass(SomeClass, Base) to cover the rooted registry-walk branch touched by this PR.
🧰 Tools
🪛 Ruff (0.16.0)
[warning] 52-52: Base is an abstract base class, but it has no abstract methods or properties
(B024)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pyre/extra_tests/parity_tests/abc_subclasses_protocol.py` around lines 52 -
65, Add a dedicated registry-path test in the existing ABC subclass coverage by
defining a separate class, registering it with Base.register, and asserting
issubclass(SomeClass, Base). Keep the current inheritance assertions intact and
ensure the new case exercises the registered-class recursive branch rather than
normal MRO inheritance.
| /// `typeobject.c wrap_descr_get` — the `__get__` namespace entry every | ||
| /// descriptor type shares. `PyArg_UnpackTuple(args, "__get__", 1, 2)` makes | ||
| /// the owner optional and `None` collapses to the absent operand in both | ||
| /// positions, so `list.__dict__['append'].__get__([1])` binds while | ||
| /// `__get__(None, None)` is rejected before the type's own binder runs. | ||
| fn wrap_descr_get( | ||
| args: &[PyObjectRef], | ||
| bind: fn(PyObjectRef, PyObjectRef, PyObjectRef) -> crate::PyResult, | ||
| ) -> crate::PyResult { | ||
| crate::type_methods::arity_at_least(args, "__get__", 1)?; | ||
| crate::type_methods::arity_at_most(args, "__get__", 2)?; | ||
| let collapse = |value: PyObjectRef| { | ||
| if value.is_null() || unsafe { pyre_object::is_none(value) } { | ||
| PY_NULL | ||
| } else { | ||
| value | ||
| } | ||
| }; | ||
| let obj = collapse(args[1]); | ||
| let w_type = collapse(args.get(2).copied().unwrap_or(PY_NULL)); | ||
| if obj.is_null() && w_type.is_null() { | ||
| return Err(crate::PyError::type_error("__get__(None, None) is invalid")); | ||
| } | ||
| bind(args[0], obj, w_type) | ||
| } | ||
|
|
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
# Inspect the relevant Rust implementation and test assertions without running repo code.
sed -n '12930,12980p' pyre/pyre-interpreter/src/typedef.rs || true
printf '\n--- test file ---\n'
sed -n '115,150p' pyre/extra_tests/parity_tests/classmethod_descriptor_kind.py || true
# Search for arity helpers and any __get__ call sites.
printf '\n--- arity helpers ---\n'
rg -n "fn arity_at_*(|\"__get__\"|PyArg_UnpackTuple|wrap_descr_get)" pyre/pyre-interpreter/src pyre -S || trueRepository: youknowone/pyre
Length of output: 3805
🌐 Web query:
Python CPython CAPI PyArg_UnpackTuple empty name message TypeError "expected at most 2 arguments"
💡 Result:
In the CPython C API, the PyArg_UnpackTuple function uses the name parameter to identify the function or method in the generated TypeError message when argument unpacking fails [1]. If you pass an empty string ("") as the name, the resulting TypeError message will begin with a leading space (or appear to have no function name), often reading like " expected at most 2 arguments" [2]. This occurs because PyArg_UnpackTuple formats its error message by concatenating the provided name with a description of the argument mismatch [2]. Passing an empty string is generally considered a misuse of the API [2]. To fix this, you should provide the actual name of the function or method being implemented, which helps ensure that Python's error reporting is clear and descriptive for the user [2][1]. If no specific function name is appropriate, it is often recommended to use a descriptive identifier or, in some cases, verify if the implementation approach is correct [2]. Recent CPython development has seen efforts to replace such empty name usages with proper function names to improve error clarity [2].
Citations:
- 1: https://stackoverflow.com/questions/21907473/what-is-parameter-name-in-pyarg-unpacktuple-python-c-api-for
- 2:
PyArg_UnpackTupleis used withname=""python/cpython#123446
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
git ls-files | rg 'type_methods\.rs|typeobject\.c|typedef\.rs|classmethod_descriptor_kind\.py' || true
printf '\n--- arity helpers ---\n'
cat -n pyre/pyre-interpreter/src/type_methods.rs | sed -n '155,220p'
printf '\n--- all wrap_descr_get callers/usages ---\n'
rg -n "wrap_descr_get|arity_at_least|arity_at_most" pyre/pyre-interpreter/src pyre -S || trueRepository: youknowone/pyre
Length of output: 17833
🌐 Web query:
CPython CPython typeobject.c wrap_descr_get PyArg_UnpackTuple args "", __get__
💡 Result:
In the CPython source code, wrap_descr_get is a function used to handle the get method for objects that are implemented as wrapper descriptors [1]. These descriptors typically represent slots or methods defined in C that are exposed to Python [1][2]. When a user calls get on such a wrapper descriptor in Python, the underlying C-level tp_descr_get function must be invoked [1]. The wrap_descr_get function acts as an adapter, translating the Python-level arguments into the format required by the C implementation [1]. The reference to PyArg_UnpackTuple(args, "", 1, 2, &obj, &type) inside wrap_descr_get is the mechanism used to parse the arguments passed to the get method [3][4]. Specifically: 1. Arguments: The tuple args contains the arguments passed to the Python-level get call. 2. Argument Parsing: PyArg_UnpackTuple is configured to expect a minimum of 1 argument (the instance obj) and a maximum of 2 arguments (the instance obj and the owner type type) [3][4]. 3. Optionality: Because PyArg_UnpackTuple specifies a minimum of 1 and maximum of 2, the type argument (the owner) is effectively optional at the Python level [5][6]. This has historically led to discrepancies where the generated text_signature for these descriptors did not perfectly match this flexible argument parsing behavior [3][4]. In summary, wrap_descr_get serves as the bridge for invoking C-level tp_descr_get implementations when accessed via the descriptor protocol, with PyArg_UnpackTuple ensuring the instance and owner arguments are correctly extracted [1][3][4].
Citations:
- 1: https://stackoverflow.com/questions/79407071/understanding-descriptor-protocol-for-wrapper-descriptor-itself
- 2: https://gopy.tamnd.com/docs/annotations/objects/descrobject_detail
- 3: Incorrect __text_signature__ for the __get__ slot wrapper python/cpython#80160
- 4: https://bugs.python.org/issue35979
- 5: gh-93021: Fix __text_signature__ for __get__ python/cpython#93023
- 6: [3.11] gh-93021: Fix __text_signature__ for __get__ (GH-93023) python/cpython#94085
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
python3 - <<'PY'
import sys
print("python", sys.version)
for descr in (
list.__dict__["append"],
object.__dict__["__str__"],
dict.__dict__["fromkeys"],
):
try:
descr.__get__()
except TypeError as exc:
print("0-arg:", exc, repr(str(exc)))
else:
print("0-arg: accepted")
try:
descr.__get__(1, 2, 3)
except TypeError as exc:
print("3-arg:", exc, repr(str(exc)))
else:
print("3-arg: accepted")
try:
descr.__get__(None, None)
except TypeError as exc:
print("None,None:", exc, repr(str(exc)))
else:
print("None,None: accepted")
PYRepository: youknowone/pyre
Length of output: 962
🌐 Web query:
CPython 3.9 Objects typeobject.c wrap_descr_get PyArg_UnpackTuple args "",
💡 Result:
In CPython 3.9, the wrap_descr_get function is a C-level helper used to translate calls to the get descriptor method into the underlying C tp_descr_get slot implementation [1]. When a type implemented in C provides a get method for its descriptors, CPython creates a "wrapper descriptor" [1]. The wrap_descr_get function facilitates this by handling the argument parsing required when that descriptor is accessed from Python [1]. Specifically, it uses PyArg_UnpackTuple to ensure the correct number of arguments (the instance and owner type) are passed to the descriptor's get method [2][3]. The specific call structure typically appears as: PyArg_UnpackTuple(args, "", 1, 2, &obj, &type) [2][3] In this context: 1. args: The tuple of arguments passed from the Python level [4]. 2. "": An empty string is passed as the name parameter [5]. Historically, this led to ambiguous error messages in some versions of CPython, as the error reporting mechanism uses this name when argument unpacking fails (e.g., producing " expected at least 1 argument" instead of a named descriptor error) [5]. 3. 1, 2: Specifies that the get method expects a minimum of 1 argument (the instance) and a maximum of 2 (the instance and the owner class) [2][3]. 4. &obj, &type: Pointers to PyObject variables that receive the unpacked arguments [4]. This implementation is internal to CPython's type system (found in Objects/typeobject.c) and is designed to bridge the gap between the Python-level descriptor protocol and the low-level C API [1][6].
Citations:
- 1: https://stackoverflow.com/questions/79407071/understanding-descriptor-protocol-for-wrapper-descriptor-itself
- 2: Incorrect __text_signature__ for the __get__ slot wrapper python/cpython#80160
- 3: https://bugs.python.org/issue35979
- 4: https://docs.python.org/release/3.9.14/c-api/arg.html
- 5:
PyArg_UnpackTupleis used withname=""python/cpython#123446 - 6: https://docs.python.org/release/3.9.22/c-api/typeobj.html
Match CPython’s empty-name arity messages for __get__. wrap_descr_get currently emits __get__ expected..., but CPython passes an empty name for this unpacking path, producing expected at least 1 argument, got 0 and expected at most 2 arguments, got 3. Update wrap_descr_get and the parity assertions to use/match the empty-name message so the test does not pin divergent behavior.
📍 Affects 2 files
pyre/pyre-interpreter/src/typedef.rs#L12946-L12971(this comment)pyre/extra_tests/parity_tests/classmethod_descriptor_kind.py#L125-L146
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pyre/pyre-interpreter/src/typedef.rs` around lines 12946 - 12971, Update
wrap_descr_get in pyre/pyre-interpreter/src/typedef.rs to pass an empty method
name to the arity validators, producing CPython’s empty-name messages for both
minimum and maximum argument failures. Update the corresponding parity
assertions in pyre/extra_tests/parity_tests/classmethod_descriptor_kind.py to
expect “ expected at least 1 argument, got 0” and “ expected at most 2
arguments, got 3”; no other sites require changes.
1 participant
Follow-up to #970, which merged while this work was in flight.
typedef: validate theclassmethod_descriptorowner and sharewrap_descr_getThree defects the #970 review found, verified against a CPython 3.14.2 oracle
before and after.
The owner argument was not validated.
descrobject.c classmethod_getrequires it to be a type and a subtype of the type the descriptor was found
on:
dict.__dict__['fromkeys'](list, ['x'])TypeError: descriptor 'fromkeys' requires a subtype of 'dict' but received 'list'TypeError: list indices must be integers or slices, not str— the call reacheddict.fromkeyswithlisttype.__dict__['__prepare__'](list, 'X', ())TypeError{}dict.__dict__['fromkeys'].__get__(None, list)TypeErrordict.__dict__['fromkeys'].__get__(None, 3)needs a type, not a 'int' as arg 2The check is gated on
is_classmethod_descriptor:funcobject.c cm_descr_get,the user
@classmethodhalf that sharesclassmethod_descr_get, has none.classmethoddescr_callreaches the same validation, mirroring its delegation toclassmethod_get.__get__took a fixed three arguments.typeobject.c wrap_descr_getunpacks 1..2, so
list.__dict__['append'].__get__([1]),object.__dict__['__str__'].__get__(3)anddict.__dict__['fromkeys'].__get__({})all raised__get__ expected 2 arguments, got 1. All three descriptor types now share awrap_descr_gethelper that collapsesNonein either position and rejects__get__(None, None).The
classmethod_descriptorgetsets used the wrong receiver-mismatchwording —
descriptor '__objclass__' requires a 'classmethod_descriptor' objectwhere the sibling receiver helpers already producedescriptor '__objclass__' for 'classmethod_descriptor' objects doesn't apply to a 'int' object.extra_tests/parity_tests/classmethod_descriptor_kind.pypins the descriptorkind, both binding halves, the owner validation, the
__get__arity and thegetset wording.
Two review findings that do not hold
bh_load_global_fnresolving from the wrong namespace after the arg-0exemption. The helper already discarded
namespace_ptrbefore #970 —let _ = namespace_ptr;is untouched by that branch — so the exemption decidesonly whether the walk aborts, never which namespace is read. Measured the named
scenario: one code object driven through 200k iterations under two different
__globals__yields('module', 'alt')on dynasm withloops_compiled=4,identical to CPython and to
PYRE_JIT=0. The custom-__builtins__variant doesdiverge from CPython, but identically with the JIT off, so it is a pre-existing
interpreter gap rather than a JIT defect.
descr_reduceneeding its fields rooted across the allocations. Both valuescome from non-moving allocators — the owner is a
W_TypeObject(
w_type_new_builtin) andw_str_newallocates through off-GClltype::malloc_typed(w_str_new_managedis the collectable sibling). Beyondthat,
w_tuple_newroots its own items:w_tuple_new_array_backedpins eachone and fills the items block from the relocated shadow-stack slots, and
w_specialised_tuple_oo_newdoes the same for the arity-2 path.One divergence is left alone:
method_descriptor.__objclass__and__name__report
doesn't apply to 'int' objectwithout CPython's article. That isMember.typecheck(pypy/interpreter/typedef.py:498-502) verbatim.stdlib: honor ABC__subclasses__protocol_abcplus a parity script.Gates
Run on the pre-rebase tree (this branch was rebased onto
1ac12147314after theruns, and the commit contents are byte-identical across the rebase):
extra_tests/parity_tests/run.py— all pass on cpython, dynasm and craneliftcpython_tests/run.py --backend dynasm— 46 pass, 0 fail, no regressionscheck.py --backend dynasm— 5 failed, 361 passedcheck.py --backend cranelift— 5 failed, 361 passedBoth backends fail on exactly the five benches #970 documents as main's own
drift against stale #947 baselines — closure_freevar_branch_resume,
exception_args_virtual, exception_multi_handler_warmup,
exception_reraise_tb_depth_jitstress and list_length_hint_validate.
🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Tests