Fix member_descriptor to match CPython behavior #6915
Merged
Conversation
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
Contributor
📝 WalkthroughWalkthroughRefactored Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 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 |
descriptor.rs: - Add __objclass__, __name__ (pymember) and __reduce__ (pymethod) - Add type check (descr_check) in descr_get; simplify None branch - Remove incorrect BASETYPE flag - Add MemberKind::Object (_Py_T_OBJECT = 6) - Prevent Bool slot deletion (TypeError) - Raise AttributeError on ObjectEx deletion when already None pyclass.rs (derive-impl): - Remove duplicate MemberKind enum; use MemberKindStr (Option<String>) - Simplify MemberNursery map key from (String, MemberKind) to String - Support #[pymember(type="object")] for _Py_T_OBJECT semantics test_inspect.py: - Remove expectedFailure from test_getdoc (now passing)
Contributor
📦 Library DependenciesThe following Lib/ modules were modified. Here are their dependencies: [x] lib: cpython/Lib/inspect.py dependencies:
dependent tests: (43 tests)
Legend:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@crates/derive-impl/src/pyclass.rs`:
- Around line 1431-1438: The current add_item logic inserts or reuses a
MemberNurseryEntry but silently ignores a differing member_kind when a getter
and setter declare different types; update add_item to validate that if an
existing MemberNurseryEntry (from self.map.entry(name)) has a different kind
than the incoming member_kind you detect the mismatch and emit a clear
error/diagnostic (or return Err) rather than silently overwriting/ignoring it.
Locate the creation/lookup code around MemberNurseryEntry and the map.entry(...)
call in add_item, compare entry.kind with the new member_kind, and produce a
compile-time friendly failure that references the member name and both kinds so
the user can fix the #[pymember(type="...")] mismatch.
Comment on lines
+1431
to
+1438
| let entry = self | ||
| .map | ||
| .entry(name.clone()) | ||
| .or_insert_with(|| MemberNurseryEntry { | ||
| kind: member_kind, | ||
| getter: None, | ||
| setter: None, | ||
| }); |
Contributor
There was a problem hiding this comment.
Potential silent inconsistency when getter and setter specify different type values.
If a getter specifies #[pymember(type="bool")] and the setter specifies #[pymember(setter, type="object")], the first one processed wins and the second's member_kind is silently ignored. Consider adding validation in add_item to detect and report mismatched kinds:
🛡️ Suggested validation
let entry = self
.map
.entry(name.clone())
.or_insert_with(|| MemberNurseryEntry {
kind: member_kind,
getter: None,
setter: None,
});
+ // Validate that getter and setter have consistent member_kind
+ if entry.kind != member_kind && member_kind.is_some() && entry.kind.is_some() {
+ bail_span!(item_ident, "Member '{}' has inconsistent type specifications", name);
+ }
+ // If entry was created with None kind, allow later specification
+ if entry.kind.is_none() && member_kind.is_some() {
+ entry.kind = member_kind;
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| let entry = self | |
| .map | |
| .entry(name.clone()) | |
| .or_insert_with(|| MemberNurseryEntry { | |
| kind: member_kind, | |
| getter: None, | |
| setter: None, | |
| }); | |
| let entry = self | |
| .map | |
| .entry(name.clone()) | |
| .or_insert_with(|| MemberNurseryEntry { | |
| kind: member_kind, | |
| getter: None, | |
| setter: None, | |
| }); | |
| // Validate that getter and setter have consistent member_kind | |
| if entry.kind != member_kind && member_kind.is_some() && entry.kind.is_some() { | |
| bail_span!(item_ident, "Member '{}' has inconsistent type specifications", name); | |
| } | |
| // If entry was created with None kind, allow later specification | |
| if entry.kind.is_none() && member_kind.is_some() { | |
| entry.kind = member_kind; | |
| } |
🤖 Prompt for AI Agents
In `@crates/derive-impl/src/pyclass.rs` around lines 1431 - 1438, The current
add_item logic inserts or reuses a MemberNurseryEntry but silently ignores a
differing member_kind when a getter and setter declare different types; update
add_item to validate that if an existing MemberNurseryEntry (from
self.map.entry(name)) has a different kind than the incoming member_kind you
detect the mismatch and emit a clear error/diagnostic (or return Err) rather
than silently overwriting/ignoring it. Locate the creation/lookup code around
MemberNurseryEntry and the map.entry(...) call in add_item, compare entry.kind
with the new member_kind, and produce a compile-time friendly failure that
references the member name and both kinds so the user can fix the
#[pymember(type="...")] mismatch.
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.
descriptor.rs:
pyclass.rs (derive-impl):
Summary by CodeRabbit
New Features
Improvements
✏️ Tip: You can customize this high-level summary in your review settings.