Implement property.name attribute #6230

ever0de · 2025-10-31T15:35:47Z

Add getter and setter for the name attribute on property objects. The getter returns the explicitly set name if available, otherwise falls back to the getter function's name. Raises AttributeError if no name is available, matching CPython 3.13 behavior.

The implementation handles edge cases:

Returns None when explicitly set to None
Propagates non-AttributeError exceptions from getter's getattr
Raises property-specific AttributeError when getter lacks name

This fix enables test_property_name in test_property.py to pass.

Summary by CodeRabbit

New Features
- Property objects now expose a readable and writable name attribute.
- When not explicitly set, name is derived from the property's getter.
Bug Fixes
- Improved error handling around property names: missing or non-existent names are treated consistently, while unexpected errors are surfaced for clearer diagnostics.

Add getter and setter for the __name__ attribute on property objects. The getter returns the explicitly set name if available, otherwise falls back to the getter function's __name__. Raises AttributeError if no name is available, matching CPython 3.13 behavior. The implementation handles edge cases: - Returns None when explicitly set to None - Propagates non-AttributeError exceptions from getter's __getattr__ - Raises property-specific AttributeError when getter lacks __name__ This fix enables test_property_name in test_property.py to pass.

coderabbitai · 2025-10-31T15:36:11Z

Walkthrough

Change makes PyProperty's name resolution return a fallible result (PyResult<Option>), treats AttributeError from underlying getters as "no name", adds public __name__ pygetset getter and setter, and updates internal callers to propagate non-AttributeError errors.

Changes

Cohort / File(s)	Summary
Property core `vm/src/builtins/property.rs`	Converted `get_property_name(&self, vm: &VirtualMachine)` to return `PyResult<Option<PyObjectRef>>`; added `name_getter(&self, vm: &VirtualMachine) -> PyResult` and `name_setter(&self, value: PyObjectRef)`; exposed `__name__` via pygetset; updated call sites (e.g., `format_property_error`) to use `?` and propagate non-AttributeError failures.

Sequence Diagram(s)

sequenceDiagram
  autonumber
  participant PyCode as Python code
  participant PyProperty as PyProperty (pygetset)
  participant VM as VirtualMachine
  participant Getter as underlying getter (optional)

  PyCode->>PyProperty: access __name__ (get)
  PyProperty->>VM: name_getter(vm)
  alt stored name present
    VM-->>PyProperty: return stored name
    PyProperty-->>PyCode: name
  else no stored name
    PyProperty->>Getter: inspect/call Getter.__name__
    alt Getter returns name
      Getter-->>PyProperty: name
      PyProperty-->>PyCode: name
    else Getter raises AttributeError
      Getter-->>PyProperty: AttributeError
      PyProperty-->>PyCode: treat as "no name" -> raise AttributeError or return None per API
    else Getter raises other error
      Getter-->>PyProperty: Other error
      PyProperty-->>PyCode: propagate error
    end
  end

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Pay attention to all updated call sites that now return PyResult<Option<...>> (notably format_property_error).
Verify pygetset wiring for __name__ (getter/setter) and that setter updates internal storage safely.
Check error classification: AttributeError should be swallowed into None; other exceptions must propagate.

Possibly related PRs

implement more property features #5828 — Prior changes to vm/src/builtins/property.rs around property name extraction; directly related to the get_property_name API and its callers.

Poem

🐰 I nudged a name from getter's nest,
Hop-scoped errors, kept the rest.
Setter plants, getter sings,
Tiny hops make clearer things.
Carrot-coded, soft and blessed. 🥕

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 66.67% which is insufficient. The required threshold is 80.00%.	You can run `@coderabbitai generate docstrings` to improve docstring coverage.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check	✅ Passed	The pull request title "Implement property.name attribute" directly and accurately describes the main change in the changeset. The raw summary confirms that the primary modifications are adding getter and setter methods for the `__name__` attribute on property objects, updating the `get_property_name` method to handle error propagation, and exposing these accessors via pygetset. The title is specific, concise, and clearly conveys the core objective to any teammate scanning the commit history, without being vague or misleading.

✨ Finishing touches

📝 Generate docstrings

🧪 Generate unit tests (beta)

Create PR with unit tests
Post copyable unit tests in a comment

📜 Recent review details

Configuration used: Path: .coderabbit.yml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 6bcb2b6 and d04d121.

📒 Files selected for processing (1)

vm/src/builtins/property.rs (3 hunks)

🧰 Additional context used

📓 Path-based instructions (2)

**/*.rs

📄 CodeRabbit inference engine (.github/copilot-instructions.md)

**/*.rs: Format Rust code with the default rustfmt style (run cargo fmt)
Run clippy and fix any warnings or lints introduced by your changes
Follow Rust best practices for error handling and memory management

Files:

vm/src/builtins/property.rs

{vm,stdlib}/**/*.rs

📄 CodeRabbit inference engine (.github/copilot-instructions.md)

Use RustPython macros (pyclass, pymodule, pyfunction, etc.) when implementing Python functionality in Rust

Files:

vm/src/builtins/property.rs

🧬 Code graph analysis (1)

vm/src/builtins/property.rs (2)

vm/src/stdlib/io.rs (5)

vm (3324-3326)

name (1562-1564)

name (2378-2378)

name (2736-2739)

name (4284-4284)

vm/src/builtins/module.rs (2)

name (120-126)

name (125-125)

🔇 Additional comments (3)

vm/src/builtins/property.rs (3)

70-94: LGTM! Error handling logic is correct.

The signature change to return PyResult<Option<PyObjectRef>> properly distinguishes between "no name available" (Ok(None)) and "error occurred" (Err). The fallback chain (explicit name → getter.name → None on AttributeError) matches CPython 3.13 behavior. Propagating non-AttributeError exceptions is the correct approach.

156-169: LGTM! Public __name__ accessors implemented correctly.

The getter properly propagates errors from get_property_name and raises an appropriate AttributeError when no name is available. The setter accepts any value without validation, which correctly matches CPython behavior (Python allows __name__ to be set to any object).

316-316: LGTM! Error propagation is correct.

The use of ? operator correctly propagates errors from get_property_name. All callers of format_property_error (lines 61, 111, 119) properly handle the PyResult return type with ?.

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 and usage tips.}

youknowone · 2025-10-31T15:41:08Z

Thank you! I thought we already had name handling on pyproperty, but it was PyProperty::get_property_name, which was not actually used for __name__. Could you check if it is also related to __name__ impl?

Consolidate duplicate logic by making name_getter() use the existing get_property_name() helper method. This eliminates code duplication and improves maintainability. Changes: - Update get_property_name() to return PyResult<Option<PyObjectRef>> to properly handle and propagate non-AttributeError exceptions - Simplify name_getter() to delegate to get_property_name() - Update format_property_error() to handle the new return type This addresses review feedback about the relationship between get_property_name() and __name__ implementation.

youknowone

Looks great. I left a style suggestion. Please apply it if it looks good

youknowone · 2025-10-31T17:00:25Z

vm/src/builtins/property.rs

+        if let Some(getter) = self.getter.read().as_ref() {
+            match getter.get_attr("__name__", vm) {
+                Ok(name) => Ok(Some(name)),
+                Err(e) => {
+                    // If it's an AttributeError from the getter, return None
+                    // Otherwise, propagate the original exception (e.g., RuntimeError)
+                    if e.class().is(vm.ctx.exceptions.attribute_error) {
+                        Ok(None)
+                    } else {
+                        Err(e)
+                    }
+                }
+            }
+        } else {
+            Ok(None)


Suggested change

if let Some(getter) = self.getter.read().as_ref() {

match getter.get_attr("__name__", vm) {

Ok(name) => Ok(Some(name)),

Err(e) => {

// If it's an AttributeError from the getter, return None

// Otherwise, propagate the original exception (e.g., RuntimeError)

if e.class().is(vm.ctx.exceptions.attribute_error) {

Ok(None)

} else {

Err(e)

}

}

}

} else {

Ok(None)

let Some(getter) = self.getter.read().as_ref() {

return Ok(None);

};

match getter.get_attr("__name__", vm) {

Ok(name) => Ok(Some(name)),

Err(e) => {

// If it's an AttributeError from the getter, return None

// Otherwise, propagate the original exception (e.g., RuntimeError)

if e.class().is(vm.ctx.exceptions.attribute_error) {

Ok(None)

} else {

Err(e)

}

}

ever0de · 2025-11-01T03:07:29Z

I've applied the review. thanks!

ever0de · 2025-11-01T03:08:29Z

vm/src/builtins/property.rs

-        {
-            return Some(name);
-        }
+        let getter = self.getter.read();


Added an extra variable because a temporary value issue prevented a single-line implementation.

youknowone requested a review from ShaharNaveh October 31, 2025 15:58

youknowone approved these changes Oct 31, 2025

View reviewed changes

style comment

d04d121

ever0de commented Nov 1, 2025

View reviewed changes

youknowone merged commit e096ce7 into RustPython:main Nov 1, 2025
22 of 23 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Implement property.name attribute #6230

Implement property.name attribute #6230

Uh oh!

ever0de commented Oct 31, 2025 •

edited by coderabbitai bot

Loading

Uh oh!

coderabbitai bot commented Oct 31, 2025 •

edited

Loading

Uh oh!

youknowone commented Oct 31, 2025

Uh oh!

youknowone left a comment

Uh oh!

youknowone Oct 31, 2025

Uh oh!

ever0de commented Nov 1, 2025

Uh oh!

ever0de Nov 1, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Implement property.__name__ attribute #6230

Implement property.__name__ attribute #6230

Uh oh!

Conversation

ever0de commented Oct 31, 2025 • edited by coderabbitai bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary by CodeRabbit

Uh oh!

coderabbitai bot commented Oct 31, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Sequence Diagram(s)

Estimated code review effort

Possibly related PRs

Poem

Pre-merge checks and finishing touches

Uh oh!

youknowone commented Oct 31, 2025

Uh oh!

youknowone left a comment

Choose a reason for hiding this comment

Uh oh!

youknowone Oct 31, 2025

Choose a reason for hiding this comment

Uh oh!

ever0de commented Nov 1, 2025

Uh oh!

ever0de Nov 1, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Implement property.name attribute #6230

Implement property.name attribute #6230

ever0de commented Oct 31, 2025 •

edited by coderabbitai bot

Loading

coderabbitai bot commented Oct 31, 2025 •

edited

Loading