Skip to content

fix: prevent infinite loop and emit a clear error when using prefixed pyclass attributes (e.g. vm::pymethod)#8098

Open
Copilot wants to merge 4 commits into
mainfrom
copilot/fix-infinite-loop-macro-expansion
Open

fix: prevent infinite loop and emit a clear error when using prefixed pyclass attributes (e.g. vm::pymethod)#8098
Copilot wants to merge 4 commits into
mainfrom
copilot/fix-infinite-loop-macro-expansion

Conversation

Copilot AI commented Jun 13, 2026

Copy link
Copy Markdown
Contributor

cargo check would 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_items in crates/derive-impl/src/pyclass.rs iterated attributes with a Peekable iterator but placed iter.next() after a continue, so it was never reached when attr.get_ident() returned None. get_ident() returns None for multi-segment paths (vm::pymethod) — only single-segment names (pymethod) return Some. The same attribute was peeked forever.

Fix

The loops in attrs_to_content_items are updated to handle multi-segment attribute paths:

  • The while loop (which collects #[cfg] attrs before the first py item) now breaks when it encounters a multi-segment path whose last segment is a known py attribute name, instead of silently consuming it.
  • The for loop (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 vm in #[vm::pymethod] is just an opaque identifier — the macro cannot know it is an alias for rustpython_vm, and the qualified form can never be supported.

// Using a path-qualified inner attribute now produces a clear error:
#[vm::pyclass]
impl Item {
    #[vm::pymethod]  // error: found `#[vm::pymethod]`, use `#[pymethod]` instead;
                     //        proc-macros cannot resolve namespace aliases
    fn value(&self) -> i64 { self.value }
}

Testing

  • A runtime test in crates/vm/src/lib.rs defines a TestItem struct with #[pyclass]/#[pymethod] (correct unqualified form), instantiates it, and asserts item.value() == 42, verifying the macro-generated code is functionally correct.
  • A unit test in crates/derive-impl/src/pyclass.rs directly calls attrs_to_content_items with a #[vm::pymethod] attribute (constructed via syn::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.

Copilot AI changed the title [WIP] Fix infinite loop in macro expansion for pyclass and pymethod fix: prevent infinite loop when using prefixed pyclass attributes (e.g. vm::pymethod) Jun 13, 2026
Copilot AI requested a review from youknowone June 13, 2026 23:42
@youknowone youknowone marked this pull request as ready for review June 14, 2026 05:09
@mapf0000

Copy link
Copy Markdown

Hi, thanks for the swift fix! I had some time to dig a little deeper.

The minimal fix, moving iter.next() so the iterator always advances, prevents
the proc-macro hang. That is good, but by itself it can turn qualified helper
attributes such as #[vm::pymethod] into a silent no-op: the compiler no longer
hangs, but the method is not registered either.

I think the open design choice is whether RustPython helper attributes inside
the macro body should support qualified paths.

Option A: support qualified helper attributes

Treat both #[pymethod] and #[vm::pymethod] as pymethod, by recognizing the
final path segment.

Sketch:

fn helper_attr_ident(attr: &Attribute) -> Option<&Ident> {
    attr.path().segments.last().map(|segment| &segment.ident)
}

Then use that helper instead of get_ident() in the pyclass.rs and
pymodule.rs attribute scans:

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 AttributeExt helpers in util.rs also need the same path handling, since
recognized qualified helper attributes will later flow through
ident_and_promoted_nested().

Good:

  • Matches user expectations when rustpython_vm is imported as vm.
  • Avoids both the hang and the silent skip.
  • Makes inner helper attributes feel consistent with outer macros like
    #[vm::pyclass].
  • Supports arbitrary crate aliases, not just vm.

Bad:

  • A proc macro cannot prove that vm resolves to rustpython_vm; it only sees
    tokens.
  • Any attribute path ending in a RustPython helper name, for example
    #[other::pymethod], would be treated as that helper inside RustPython macro
    contexts.

Option B: reject qualified helper attributes clearly

Accept #[pymethod], but emit a diagnostic for #[vm::pymethod].

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
helper attribute and lets phase 2 report the error:

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:

  • More conservative.
  • Avoids consuming unrelated attribute paths that merely end in pymethod.
  • Keeps the supported syntax narrower and easier to reason about.

Bad:

  • Less ergonomic, especially because users can already write qualified outer
    macros like #[vm::pyclass].
  • Requires users to mix qualified and unqualified RustPython attributes.
  • Still needs explicit detection to avoid silent skips.

@youknowone

Copy link
Copy Markdown
Member

@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 vm as a symbol of imported namespace? e.g. use rustpython_vm must allow rustpython_vm::pyemthod and use rustpython_vm as vm must allow vm::pymethod. It doesn't likely to be. If that's impossible, when {}::pymethod exists, we must warn user to use #[pymethod] rather than the wrong namespace notation.

Copilot AI commented Jun 15, 2026

Copy link
Copy Markdown
Contributor Author

@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::pyme...

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 vm in #[vm::pymethod] is just an opaque identifier — the macro has no way to know it's an alias for rustpython_vm. The qualified form cannot be supported.

Warning for #[vm::pymethod]: Both loops in attrs_to_content_items now detect when a multi-segment attribute path ends with a known py* name. The while loop breaks (so the attribute isn't silently consumed) and the for loop emits a compile error that names the full path written and the correct unqualified form, e.g.:

error: found `#[vm::pymethod]`, use `#[pymethod]` instead; proc-macros cannot resolve namespace aliases

Test: Added a runtime test in crates/vm/src/lib.rs that defines a TestItem struct with #[pyclass]/#[pymethod], instantiates it, and asserts item.value() == 42, verifying the macro-generated code is functionally correct. For compile-fail coverage of the new error (the #[vm::pymethod] → error path), that would need trybuild infrastructure which isn't currently set up in the repo.

Copilot AI changed the title fix: prevent infinite loop when using prefixed pyclass attributes (e.g. vm::pymethod) fix: prevent infinite loop and emit a clear error when using prefixed pyclass attributes (e.g. vm::pymethod) Jun 15, 2026
@youknowone

Copy link
Copy Markdown
Member

@copilot we all know #[pymehtod] works well because the entire codebase is using it. you must test using #[something::pymethod] actually fails

Copilot AI commented Jun 16, 2026

Copy link
Copy Markdown
Contributor Author

@copilot we all know #[pymehtod] works well because the entire codebase is using it. you must test using #[something::pymethod] actually fails

Added a unit test directly in crates/derive-impl/src/pyclass.rs that calls attrs_to_content_items with a syn::parse_quote!(#[vm::pymethod]) attribute and asserts:

  1. The result is an Err (not an infinite loop or silent success)
  2. The error message contains both the full path (vm::pymethod) and the unqualified name (pymethod)

Commit: the latest commit test: add unit test in derive-impl verifying path-qualified py attrs return an error.

Comment on lines +2086 to +2087
let result =
attrs_to_content_items(&[attr], |i, name| (i, name));

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.

[reviewdog-suggester] reported by reviewdog 🐶

Suggested change
let result =
attrs_to_content_items(&[attr], |i, name| (i, name));
let result = attrs_to_content_items(&[attr], |i, name| (i, name));

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.

3 participants