fix: prevent infinite loop and emit a clear error when using prefixed pyclass attributes (e.g. vm::pymethod)#8098
fix: prevent infinite loop and emit a clear error when using prefixed pyclass attributes (e.g. vm::pymethod)#8098Copilot wants to merge 4 commits into
vm::pymethod)#8098Conversation
…ributes like vm::pymethod
vm::pymethod)
|
Hi, thanks for the swift fix! I had some time to dig a little deeper. The minimal fix, moving I think the open design choice is whether RustPython helper attributes inside Option A: support qualified helper attributesTreat both Sketch: fn helper_attr_ident(attr: &Attribute) -> Option<&Ident> {
attr.path().segments.last().map(|segment| &segment.ident)
}Then use that helper instead of if let Some(ident) = helper_attr_ident(attr) {
let attr_name = ident.to_string();
if attr_name == "cfg" {
cfgs.push(attr.clone());
} else if ALL_ALLOWED_NAMES.contains(&attr_name.as_str()) {
break;
}
}
iter.next();The Good:
Bad:
Option B: reject qualified helper attributes clearlyAccept Sketch: fn helper_attr_ident(attr: &Attribute) -> Option<&Ident> {
attr.path().segments.last().map(|segment| &segment.ident)
}
fn helper_attr_is_unqualified(attr: &Attribute) -> bool {
attr.path().segments.len() == 1
}Phase 1 still needs to look at the final segment so it stops before a qualified if let Some(ident) = helper_attr_ident(attr) {
let attr_name = ident.to_string();
if helper_attr_is_unqualified(attr) && attr_name == "cfg" {
cfgs.push(attr.clone());
} else if ALL_ALLOWED_NAMES.contains(&attr_name.as_str()) {
break;
}
}
iter.next();Then phase 2 can reject the qualified helper attribute explicitly: let Some(ident) = helper_attr_ident(attr) else {
continue;
};
let attr_name = ident.to_string();
if !helper_attr_is_unqualified(attr) && ALL_ALLOWED_NAMES.contains(&attr_name.as_str()) {
bail_span!(
attr,
"qualified RustPython helper attributes are not supported here; use #[{}] instead",
attr_name
);
}Good:
Bad:
|
|
@mapf0000 you are right that current behavior is wrong. Thank you for feedback. @copilot you must add a test that the new code is working correctly. also check mapf0000's suggestion about vm::pymethod. Can Rust proc-macro recognize |
Done — addressed both points (commits cover the latest two changes on top of the original fix): Namespace alias question: proc-macros receive raw token trees before name resolution, so Warning for Test: Added a runtime test in |
vm::pymethod)vm::pymethod)
|
@copilot we all know |
Added a unit test directly in
Commit: the latest commit |
| let result = | ||
| attrs_to_content_items(&[attr], |i, name| (i, name)); |
There was a problem hiding this comment.
[reviewdog-suggester] reported by reviewdog 🐶
| let result = | |
| attrs_to_content_items(&[attr], |i, name| (i, name)); | |
| let result = attrs_to_content_items(&[attr], |i, name| (i, name)); |
cargo checkwould hang indefinitely (100% CPU, no output) whenever an inner py attribute like#[pymethod]was written with a path prefix (e.g.#[vm::pymethod]) inside a#[pyclass]impl block.Root cause
attrs_to_content_itemsincrates/derive-impl/src/pyclass.rsiterated attributes with aPeekableiterator but placediter.next()after acontinue, so it was never reached whenattr.get_ident()returnedNone.get_ident()returnsNonefor multi-segment paths (vm::pymethod) — only single-segment names (pymethod) returnSome. The same attribute was peeked forever.Fix
The loops in
attrs_to_content_itemsare updated to handle multi-segment attribute paths:whileloop (which collects#[cfg]attrs before the first py item) nowbreaks when it encounters a multi-segment path whose last segment is a known py attribute name, instead of silently consuming it.forloop (which processes py items) now emits a clear compile error when it sees such an attribute, pointing the user to the unqualified form.Proc-macros receive raw token trees before name resolution, so
vmin#[vm::pymethod]is just an opaque identifier — the macro cannot know it is an alias forrustpython_vm, and the qualified form can never be supported.Testing
crates/vm/src/lib.rsdefines aTestItemstruct with#[pyclass]/#[pymethod](correct unqualified form), instantiates it, and assertsitem.value() == 42, verifying the macro-generated code is functionally correct.crates/derive-impl/src/pyclass.rsdirectly callsattrs_to_content_itemswith a#[vm::pymethod]attribute (constructed viasyn::parse_quote!) and asserts that an error is returned whose message contains both the full path (vm::pymethod) and the unqualified name (pymethod), confirming the error path is exercised and not silently ignored or looped.