Skip to content

Fix member_descriptor to match CPython behavior #6915

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
youknowone merged 1 commit into from
Feb 1, 2026
Merged

Fix member_descriptor to match CPython behavior #6915

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion Lib/test/test_inspect/test_inspect.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -672,7 +672,6 @@ def test_getfunctions(self):
('lobbest', mod.lobbest),
('spam', mod.spam)])

@unittest.expectedFailure # TODO: RUSTPYTHON
@unittest.skipIf(sys.flags.optimize >= 2,
"Docstrings are omitted with -O2 and above")
def test_getdoc(self):
Expand Down
106 changes: 61 additions & 45 deletions crates/derive-impl/src/pyclass.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1207,13 +1207,18 @@ where
let item_meta = MemberItemMeta::from_attr(ident.clone(), &item_attr)?;

let (py_name, member_item_kind) = item_meta.member_name()?;
let member_kind = match item_meta.member_kind()? {
Some(s) => match s.as_str() {
"bool" => MemberKind::Bool,
_ => unreachable!(),
},
_ => MemberKind::ObjectEx,
};
let member_kind = item_meta.member_kind()?;
if let Some(ref s) = member_kind {
match s.as_str() {
"bool" | "object" => {}
other => {
return Err(self.new_syn_error(
args.item.span(),
&format!("unknown member type '{other}'"),
));
}
}
}

// Add #[allow(non_snake_case)] for setter methods
if matches!(member_item_kind, MemberItemKind::Set) {
Expand Down Expand Up @@ -1393,37 +1398,47 @@ impl ToTokens for GetSetNursery {
}
}

/// Member kind as string, matching `rustpython_vm::builtins::descriptor::MemberKind` variants.
/// None means ObjectEx (default). Valid values: "bool", "object".
type MemberKindStr = Option<String>;

#[derive(Default)]
#[allow(clippy::type_complexity)]
struct MemberNursery {
map: HashMap<(String, MemberKind), (Option<Ident>, Option<Ident>)>,
map: HashMap<String, MemberNurseryEntry>,
validated: bool,
}

struct MemberNurseryEntry {
kind: MemberKindStr,
getter: Option<Ident>,
setter: Option<Ident>,
}

enum MemberItemKind {
Get,
Set,
}

#[derive(Eq, PartialEq, Hash)]
enum MemberKind {
Bool,
ObjectEx,
}

impl MemberNursery {
fn add_item(
&mut self,
name: String,
kind: MemberItemKind,
member_kind: MemberKind,
member_kind: MemberKindStr,
item_ident: Ident,
) -> Result<()> {
assert!(!self.validated, "new item is not allowed after validation");
let entry = self.map.entry((name.clone(), member_kind)).or_default();
let entry = self
.map
.entry(name.clone())
.or_insert_with(|| MemberNurseryEntry {
kind: member_kind,
getter: None,
setter: None,
});
Comment on lines +1431 to +1438

@coderabbitai coderabbitai Bot Jan 31, 2026

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.

⚠️ Potential issue | 🟡 Minor

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.

let func = match kind {
MemberItemKind::Get => &mut entry.0,
MemberItemKind::Set => &mut entry.1,
MemberItemKind::Get => &mut entry.getter,
MemberItemKind::Set => &mut entry.setter,
};
if func.is_some() {
bail_span!(item_ident, "Multiple member accessors with name '{}'", name);
Expand All @@ -1434,10 +1449,10 @@ impl MemberNursery {

fn validate(&mut self) -> Result<()> {
let mut errors = Vec::new();
for ((name, _), (getter, setter)) in &self.map {
if getter.is_none() {
for (name, entry) in &self.map {
if entry.getter.is_none() {
errors.push(err_span!(
setter.as_ref().unwrap(),
entry.setter.as_ref().unwrap(),
"Member '{}' is missing a getter",
name
));
Expand All @@ -1452,30 +1467,31 @@ impl MemberNursery {
impl ToTokens for MemberNursery {
fn to_tokens(&self, tokens: &mut TokenStream) {
assert!(self.validated, "Call `validate()` before token generation");
let properties = self
.map
.iter()
.map(|((name, member_kind), (getter, setter))| {
let setter = match setter {
Some(setter) => quote_spanned! { setter.span() => Some(Self::#setter)},
None => quote! { None },
};
let member_kind = match member_kind {
MemberKind::Bool => {
quote!(::rustpython_vm::builtins::descriptor::MemberKind::Bool)
}
MemberKind::ObjectEx => {
quote!(::rustpython_vm::builtins::descriptor::MemberKind::ObjectEx)
}
};
quote_spanned! { getter.span() =>
class.set_str_attr(
#name,
ctx.new_member(#name, #member_kind, Self::#getter, #setter, class),
ctx,
);
let properties = self.map.iter().map(|(name, entry)| {
let setter = match &entry.setter {
Some(setter) => quote_spanned! { setter.span() => Some(Self::#setter)},
None => quote! { None },
};
let member_kind = match entry.kind.as_deref() {
Some("bool") => {
quote!(::rustpython_vm::builtins::descriptor::MemberKind::Bool)
}
});
Some("object") => {
quote!(::rustpython_vm::builtins::descriptor::MemberKind::Object)
}
_ => {
quote!(::rustpython_vm::builtins::descriptor::MemberKind::ObjectEx)
}
};
let getter = entry.getter.as_ref().unwrap();
quote_spanned! { getter.span() =>
class.set_str_attr(
#name,
ctx.new_member(#name, #member_kind, Self::#getter, #setter, class),
ctx,
);
}
});
tokens.extend(properties);
}
}
Expand Down
92 changes: 65 additions & 27 deletions crates/vm/src/builtins/descriptor.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -170,6 +170,7 @@ impl Representable for PyMethodDescriptor {

#[derive(Debug)]
pub enum MemberKind {
Object = 6,
Bool = 14,
ObjectEx = 16,
}
Expand Down Expand Up @@ -253,11 +254,20 @@ fn calculate_qualname(descr: &PyDescriptorOwned, vm: &VirtualMachine) -> PyResul
}
}

#[pyclass(
with(GetDescriptor, Representable),
flags(BASETYPE, DISALLOW_INSTANTIATION)
)]
#[pyclass(with(GetDescriptor, Representable), flags(DISALLOW_INSTANTIATION))]
impl PyMemberDescriptor {
#[pymember]
fn __objclass__(vm: &VirtualMachine, zelf: PyObjectRef) -> PyResult {
let zelf: &Py<Self> = zelf.try_to_value(vm)?;
Ok(zelf.common.typ.clone().into())
}

#[pymember]
fn __name__(vm: &VirtualMachine, zelf: PyObjectRef) -> PyResult {
let zelf: &Py<Self> = zelf.try_to_value(vm)?;
Ok(zelf.common.name.to_owned().into())
}

#[pygetset]
fn __doc__(&self) -> Option<String> {
self.member.doc.to_owned()
Expand All @@ -276,6 +286,23 @@ impl PyMemberDescriptor {
})
}

#[pymethod]
fn __reduce__(&self, vm: &VirtualMachine) -> PyResult {
let builtins_getattr = vm.builtins.get_attr("getattr", vm)?;
Ok(vm
.ctx
.new_tuple(vec![
builtins_getattr,
vm.ctx
.new_tuple(vec![
self.common.typ.clone().into(),
vm.ctx.new_str(self.common.name.as_str()).into(),
])
.into(),
])
.into())
}

#[pyslot]
fn descr_set(
zelf: &PyObject,
Expand Down Expand Up @@ -306,6 +333,7 @@ fn get_slot_from_object(
vm: &VirtualMachine,
) -> PyResult {
let slot = match member.kind {
MemberKind::Object => obj.get_slot(offset).unwrap_or_else(|| vm.ctx.none()),
MemberKind::Bool => obj
.get_slot(offset)
.unwrap_or_else(|| vm.ctx.new_bool(false).into()),
Expand All @@ -325,25 +353,38 @@ fn set_slot_at_object(
vm: &VirtualMachine,
) -> PyResult<()> {
match member.kind {
MemberKind::Object => match value {
PySetterValue::Assign(v) => {
obj.set_slot(offset, Some(v));
}
PySetterValue::Delete => {
obj.set_slot(offset, None);
}
},
MemberKind::Bool => {
match value {
PySetterValue::Assign(v) => {
if !v.class().is(vm.ctx.types.bool_type) {
return Err(vm.new_type_error("attribute value type must be bool"));
}

obj.set_slot(offset, Some(v))
}
PySetterValue::Delete => obj.set_slot(offset, None),
};
}
MemberKind::ObjectEx => {
let value = match value {
PySetterValue::Assign(v) => Some(v),
PySetterValue::Delete => None,
PySetterValue::Delete => {
return Err(vm.new_type_error("can't delete numeric/char attribute".to_owned()));
}
};
obj.set_slot(offset, value);
}
MemberKind::ObjectEx => match value {
PySetterValue::Assign(v) => {
obj.set_slot(offset, Some(v));
}
PySetterValue::Delete => {
if obj.get_slot(offset).is_none() {
return Err(vm.new_attribute_error(member.name.clone()));
}
obj.set_slot(offset, None);
}
},
}

Ok(())
Expand All @@ -364,26 +405,23 @@ impl GetDescriptor for PyMemberDescriptor {
fn descr_get(
zelf: PyObjectRef,
obj: Option<PyObjectRef>,
cls: Option<PyObjectRef>,
_cls: Option<PyObjectRef>,
vm: &VirtualMachine,
) -> PyResult {
let descr = Self::_as_pyref(&zelf, vm)?;
match obj {
Some(x) => descr.member.get(x, vm),
None => {
// When accessed from class (not instance), for __doc__ member descriptor,
// return the class's docstring if available
// When accessed from class (not instance), check if the class has
// an attribute with the same name as this member descriptor
if let Some(cls) = cls
&& let Ok(cls_type) = cls.downcast::<PyType>()
&& let Some(interned) = vm.ctx.interned_str(descr.member.name.as_str())
&& let Some(attr) = cls_type.attributes.read().get(&interned)
{
return Ok(attr.clone());
Some(x) => {
if !x.class().fast_issubclass(&descr.common.typ) {
return Err(vm.new_type_error(format!(
"descriptor '{}' for '{}' objects doesn't apply to a '{}' object",
descr.common.name,
descr.common.typ.name(),
x.class().name()
)));
}
Ok(zelf)
descr.member.get(x, vm)
}
None => Ok(zelf),
}
}
}
Expand Down
Loading