Skip to content

Reject a fifth argument to property() - #8510

Merged
youknowone merged 1 commit into
RustPython:mainfrom
jseop-lim:fix-property-arity
Aug 13, 2026
Merged

youknowone merged 1 commit into
RustPython:mainfrom
jseop-lim:fix-property-arity

Conversation

@jseop-lim

@jseop-lim jseop-lim commented Aug 13, 2026 •

Copy link
Copy Markdown
Contributor

Summary

PropertyArgs declared a fifth positional-or-keyword field, name, so the derived arity was 0..=5 and property(None, None, None, None, None) built a property object with the fifth argument stored in the __name__ slot. CPython's property.__init__ is Argument Clinic generated with maxpos = 4 and the keyword list {fget, fset, fdel, doc}, so it rejects both that call and property(name='x').

AS-IS

>>> type(property(None, None, None, None, None))
<class 'property'>
>>> type(property(name='x'))
<class 'property'>

TO-BE

>>> property(None, None, None, None, None)
TypeError: expected at most 4 arguments, got 5
>>> property(name='x')
TypeError: Unexpected keyword argument name

(matching CPython, which raises TypeError for both; the message wording still comes from FuncArgs::bind rather than CPython's property() takes at most 4 arguments (5 given), which is a separate repo-wide difference)

Changes

  • Remove the name field from PropertyArgs and the assignment that consumes it in Initializer::init, dropping the derived arity back to 0..=4.
  • Drop the now-dead name: None initializer in clone_property_with and the unused PyStrRef import. The PyProperty name slot is untouched and is still filled by __set_name__ and the __name__ setter.
  • Add regression tests in extra_tests/snippets/builtin_property.py for the five-positional-argument and name= keyword forms.

Test plan

Built and run on macOS aarch64 against upstream/main 525ba8cf0, using the CI feature set --no-default-features --features stdlib,importlib,stdio,encodings,sqlite,ssl-rustls-aws-lc,host_env.

  • On an unpatched build of that commit both forms produced a property object; with the patch both raise TypeError. Four-argument construction, the getter/setter/deleter builders and __set_name__ still behave as before, and __set_name__ still fills the name slot.
  • cargo run --release -- -m test test_property test_descr test_builtin: 331 run, 36 skipped, all pass.
  • cargo test --workspace --exclude rustpython-capi --exclude rustpython_wasm --exclude rustpython-compiler-source --exclude rustpython-venvlauncher --features threading: 1107 pass, 0 fail. cargo test in crates/capi: 102 pass.
  • extra_tests builtin snippets under RustPython: 66 of 67 pass, the new builtin_property.py cases among them. The one failure, builtin_thread.py, asserts _thread.TIMEOUT_MAX in [9223372036.0, 4294967.0] and my build reports 2147483648, which is an unrelated platform constant.
  • cargo fmt --check clean, cargo clippy -p rustpython-vm --all-targets reports nothing on the changed file, and pre-commit run --files passes on both changed files.

I used Claude Code (claude-opus-5) to draft the analysis, the patch and this description; I reviewed the change and ran every command above myself.

Summary by CodeRabbit

  • Bug Fixes
    • Corrected property handling so property names are managed consistently through standard name-assignment behavior.
    • property() now correctly rejects more than four positional arguments.
    • Unsupported name keyword arguments are now rejected.

PropertyArgs declared a fifth positional-or-keyword field, name, so the
derived arity was 0..=5 and property(None, None, None, None, None) built
a property object with the fifth argument stored in the __name__ slot.
CPython's property.__init__ is Argument Clinic generated with maxpos = 4
and the keyword list {fget, fset, fdel, doc}, so it rejects both that call
and property(name='x').

Drop the field. Nothing passed it: clone_property_with always supplied
None and copies the name separately, and the name slot is still filled by
__set_name__ and the __name__ setter.

Assisted-by: Claude Code:claude-opus-5
@coderabbitai

coderabbitai Bot commented Aug 13, 2026 •

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yml

Review profile: CHILL

Plan: Pro Plus

Run ID: 2f794607-2b34-4326-a52c-a5d82bc5c194

📥 Commits

Reviewing files that changed from the base of the PR and between 525ba8c and 83656b5.

📒 Files selected for processing (2)
  • crates/vm/src/builtins/property.rs
  • extra_tests/snippets/builtin_property.py

📝 Walkthrough

Walkthrough

The property constructor no longer accepts a fifth positional argument or the unsupported name keyword. Property cloning preserves the stored name separately. Tests cover both invalid argument forms.

Changes

Property argument validation

Layer / File(s) Summary
Property argument contract and initialization
crates/vm/src/builtins/property.rs
PropertyArgs removes the name argument. Property initialization continues to set getter, setter, deleter, and documentation state.
Property cloning and argument validation
crates/vm/src/builtins/property.rs, extra_tests/snippets/builtin_property.py
Property cloning copies the stored name separately. Tests verify that excessive positional arguments and the name keyword raise TypeError.

Estimated code review effort: 2 (Simple) | ~10 minutes

Mergeability Score: ⚪ Minimal · up to 83656

This localized change aligns property() argument handling with the documented behavior and includes regression coverage; no actionable merge-blocking risk remains after normal checks.

Suggested reviewers: shaharnaveh

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly describes the primary fix: rejecting a fifth positional argument to property().
Linked Issues check ✅ Passed The changes address issue #8460 by rejecting excess positional and name= arguments while preserving property name handling.
Out of Scope Changes check ✅ Passed All code and test changes directly support the linked issue and stated objectives.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@ShaharNaveh ShaharNaveh left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

huh, weird that cpython doesn't have a test for it.

tysm!

@youknowone youknowone added the z-ca-2026 Tag to track Contribution Academy 2026 label Aug 13, 2026

@youknowone youknowone left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@youknowone
youknowone merged commit 0b150ca into RustPython:main Aug 13, 2026
28 checks passed
@jseop-lim
jseop-lim deleted the fix-property-arity branch August 13, 2026 16:14
youknowone pushed a commit that referenced this pull request Sep 16, 2026
PropertyArgs declared a fifth positional-or-keyword field, name, so the
derived arity was 0..=5 and property(None, None, None, None, None) built
a property object with the fifth argument stored in the __name__ slot.
CPython's property.__init__ is Argument Clinic generated with maxpos = 4
and the keyword list {fget, fset, fdel, doc}, so it rejects both that call
and property(name='x').

Drop the field. Nothing passed it: clone_property_with always supplied
None and copies the name separately, and the name slot is still filled by
__set_name__ and the __name__ setter.

Assisted-by: Claude Code:claude-opus-5
@youknowone youknowone removed the z-ca-2026 Tag to track Contribution Academy 2026 label Oct 5, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

property() accepts a fifth positional argument instead of raising TypeError

3 participants