Fix #[pyclass(base=...)] notation #6242
Conversation
WalkthroughThe pull request refactors base class specification across PyO3 attribute macros from string literals to direct type paths. Core infrastructure in derive-impl converts base parameter handling from Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Specific areas requiring attention:
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ 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 |
849af75 to
41d497c
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
derive-impl/src/pyclass.rs(1 hunks)derive-impl/src/util.rs(2 hunks)derive/src/lib.rs(1 hunks)stdlib/src/ssl.rs(7 hunks)vm/src/builtins/bool.rs(1 hunks)vm/src/builtins/builtin_func.rs(1 hunks)vm/src/exceptions.rs(13 hunks)vm/src/stdlib/ast/pyast.rs(1 hunks)vm/src/stdlib/ctypes/array.rs(2 hunks)vm/src/stdlib/ctypes/base.rs(2 hunks)vm/src/stdlib/ctypes/function.rs(1 hunks)vm/src/stdlib/ctypes/structure.rs(1 hunks)vm/src/stdlib/ctypes/union.rs(1 hunks)vm/src/stdlib/io.rs(11 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 (runcargo 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/builtin_func.rsvm/src/builtins/bool.rsderive-impl/src/util.rsvm/src/stdlib/ctypes/array.rsvm/src/stdlib/ctypes/structure.rsderive-impl/src/pyclass.rsstdlib/src/ssl.rsvm/src/stdlib/ctypes/function.rsderive/src/lib.rsvm/src/stdlib/ctypes/base.rsvm/src/stdlib/ctypes/union.rsvm/src/exceptions.rsvm/src/stdlib/io.rsvm/src/stdlib/ast/pyast.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/builtin_func.rsvm/src/builtins/bool.rsvm/src/stdlib/ctypes/array.rsvm/src/stdlib/ctypes/structure.rsstdlib/src/ssl.rsvm/src/stdlib/ctypes/function.rsvm/src/stdlib/ctypes/base.rsvm/src/stdlib/ctypes/union.rsvm/src/exceptions.rsvm/src/stdlib/io.rsvm/src/stdlib/ast/pyast.rs
🧠 Learnings (3)
📚 Learning: 2025-09-11T05:06:59.971Z
Learnt from: CR
Repo: RustPython/RustPython PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-09-11T05:06:59.971Z
Learning: Applies to {vm,stdlib}/**/*.rs : Use RustPython macros (`pyclass`, `pymodule`, `pyfunction`, etc.) when implementing Python functionality in Rust
Applied to files:
vm/src/builtins/builtin_func.rsvm/src/builtins/bool.rsvm/src/stdlib/ctypes/array.rsvm/src/stdlib/ctypes/structure.rsvm/src/stdlib/ctypes/function.rsderive/src/lib.rsvm/src/stdlib/ctypes/base.rsvm/src/stdlib/ctypes/union.rsvm/src/exceptions.rsvm/src/stdlib/io.rsvm/src/stdlib/ast/pyast.rs
📚 Learning: 2025-09-11T05:06:59.971Z
Learnt from: CR
Repo: RustPython/RustPython PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-09-11T05:06:59.971Z
Learning: Applies to Lib/**/*.py : Minimize modifications to CPython standard library files; only make minimal changes to work around RustPython limitations
Applied to files:
vm/src/builtins/builtin_func.rsvm/src/stdlib/ctypes/array.rsvm/src/stdlib/ctypes/structure.rsstdlib/src/ssl.rsvm/src/stdlib/ctypes/function.rsderive/src/lib.rsvm/src/stdlib/ctypes/base.rsvm/src/exceptions.rsvm/src/stdlib/io.rs
📚 Learning: 2025-09-11T05:06:59.971Z
Learnt from: CR
Repo: RustPython/RustPython PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-09-11T05:06:59.971Z
Learning: In most cases, avoid editing Python code; prefer fixing bugs via Rust changes
Applied to files:
vm/src/exceptions.rs
🧬 Code graph analysis (12)
vm/src/builtins/builtin_func.rs (2)
derive/src/lib.rs (1)
pyclass(124-128)derive-impl/src/util.rs (2)
module(428-457)base(416-418)
vm/src/builtins/bool.rs (2)
derive/src/lib.rs (1)
pyclass(124-128)derive-impl/src/util.rs (2)
module(428-457)base(416-418)
vm/src/stdlib/ctypes/array.rs (2)
derive/src/lib.rs (1)
pyclass(124-128)derive-impl/src/util.rs (2)
base(416-418)module(428-457)
vm/src/stdlib/ctypes/structure.rs (2)
derive/src/lib.rs (1)
pyclass(124-128)derive-impl/src/util.rs (2)
module(428-457)base(416-418)
derive-impl/src/pyclass.rs (1)
derive-impl/src/util.rs (2)
base(416-418)syn(202-202)
stdlib/src/ssl.rs (3)
derive/src/lib.rs (1)
pyexception(139-143)derive-impl/src/lib.rs (1)
pyexception(50-56)derive-impl/src/util.rs (1)
base(416-418)
vm/src/stdlib/ctypes/function.rs (2)
derive/src/lib.rs (1)
pyclass(124-128)derive-impl/src/util.rs (2)
module(428-457)base(416-418)
vm/src/stdlib/ctypes/base.rs (1)
derive-impl/src/util.rs (2)
module(428-457)base(416-418)
vm/src/stdlib/ctypes/union.rs (1)
derive-impl/src/util.rs (1)
base(416-418)
vm/src/exceptions.rs (3)
derive/src/lib.rs (1)
pyexception(139-143)derive-impl/src/lib.rs (1)
pyexception(50-56)derive-impl/src/util.rs (1)
base(416-418)
vm/src/stdlib/io.rs (3)
derive/src/lib.rs (1)
pyclass(124-128)derive-impl/src/lib.rs (1)
pyclass(42-48)derive-impl/src/util.rs (2)
base(416-418)module(428-457)
vm/src/stdlib/ast/pyast.rs (1)
derive-impl/src/util.rs (2)
module(428-457)base(416-418)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: Run rust tests (windows-latest)
- GitHub Check: Run snippets and cpython tests (windows-latest)
- GitHub Check: Run snippets and cpython tests (macos-latest)
- GitHub Check: Ensure compilation on various targets
- GitHub Check: Run snippets and cpython tests (ubuntu-latest)
- GitHub Check: Run rust tests (macos-latest)
- GitHub Check: Check Rust code with rustfmt and clippy
🔇 Additional comments (14)
derive/src/lib.rs (1)
37-37: Documentation example correctly reflects the refactored syntax.The update to use
base = BaseClass(type path) instead of the previous string literal syntax aligns with the PR's refactoring of base class specifications across the codebase. The documentation example now accurately guides users on the correct macro usage.stdlib/src/ssl.rs (1)
249-249: LGTM! Clean refactoring from string literals to type paths.The updated
#[pyexception]attributes now use direct type paths (PyOSError,PySslError) instead of string literals. This improves type safety and aligns with the PR's objective to refactor base class specification across PyO3 macros. All referenced types are properly in scope, and the exception hierarchy is preserved correctly.Also applies to: 272-272, 281-281, 290-290, 299-299, 308-308, 317-317
vm/src/stdlib/ctypes/function.rs (1)
125-125: LGTM! Base class reference correctly updated.The change from string literal to direct type path aligns with the PR objective and macro infrastructure update. The
PyCDatatype is properly imported at line 6.vm/src/stdlib/ctypes/union.rs (1)
4-4: LGTM! Base class reference correctly updated.The change from string literal to direct type path is correct. The
PyCDatatype is properly imported at line 1.vm/src/stdlib/ctypes/structure.rs (1)
11-11: LGTM! Base class reference correctly updated.The change from string literal to direct type path is correct. The
PyCDatatype is properly imported at line 1.vm/src/stdlib/ctypes/array.rs (2)
13-13: LGTM! Base class reference correctly updated.The change from string literal to direct type path is correct. The
PyTypetype is properly imported at line 6.
50-55: LGTM! Base class reference correctly updated and typo fixed.The change from
base = "PyData"tobase = PyCDatanot only updates to the new type-path format but also fixes what appears to be a typo ("PyData"→PyCData). ThePyCDatatype is properly imported at line 11. This demonstrates a benefit of the type-path approach: compile-time verification catches naming errors.vm/src/stdlib/ctypes/base.rs (2)
160-160: LGTM! Base class reference correctly updated.The change from string literal to direct type path is correct. The
PyTypetype is properly imported at line 2.
176-181: LGTM! Base class reference correctly updated.The change from string literal to direct type path is correct. The
PyCDatatype is defined in this same file (lines 151-158), so the reference is valid.derive-impl/src/pyclass.rs (4)
307-315: LGTM: Improved type safety with syn::PathThe function signature change from
Option<String>toOption<syn::Path>for the base parameter improves type safety and aligns with the parsing logic inutil.rs. This eliminates the need for intermediate string-to-Ident conversions.
357-387: LGTM: Clean direct usage of Path in code generationThe base class handling correctly uses the
syn::Pathdirectly in bothquote_spanned!andquote!macros, eliminating the previous string-to-Ident conversion step. The span preservation in line 361 ensures good error reporting.
426-434: LGTM: Consistent usage of new Path typeThe calls to
class_meta.base()and subsequent usage ingenerate_class_defcorrectly handle the newOption<syn::Path>return type, maintaining consistency throughout the codebase.
539-570: LGTM: Exception base class handling updated correctlyThe exception implementation correctly retrieves and uses the base class as
Option<syn::Path>. The Path is properly interpolated into the#[pyclass(...)]attribute at line 570, where it will be correctly parsed by the macro.derive-impl/src/util.rs (1)
190-217: LGTM! Well-structured path parsing implementation.The new
_optional_pathmethod correctly mirrors the pattern of_optional_strand provides clear error messages. The use ofto_token_stream()followed bysyn::parse2is the idiomatic approach for parsing syn types.
| pub fn base(&self) -> Result<Option<syn::Path>> { | ||
| self.inner()._optional_path("base") | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
Verify all call sites handle the new Path return type.
The change from String to syn::Path aligns with the PR objective. Ensure all consumers of base() have been updated to work with syn::Path instead of String.
🏁 Script executed:
#!/bin/bash
# Description: Find all usages of ClassItemMeta::base() and ExceptionItemMeta::base() to verify they handle syn::Path
# Search for calls to .base() method that might need updates
rg -nP --type=rust -C5 '\.base\s*\(\s*\)'
# Also search for ExceptionItemMeta usage since it derefs to ClassItemMeta
rg -nP --type=rust -C3 'ExceptionItemMeta|ClassItemMeta'Length of output: 7276
🏁 Script executed:
#!/bin/bash
# Description: Get detailed context around base() usage to see how results are handled
# Show full context for line 426 usage in pyclass.rs
echo "=== Usage 1: Around line 426 in pyclass.rs ==="
sed -n '420,445p' derive-impl/src/pyclass.rs
echo ""
echo "=== Usage 2: Around line 539 in pyclass.rs ==="
sed -n '533,570p' derive-impl/src/pyclass.rsLength of output: 2604
🏁 Script executed:
#!/bin/bash
# Description: Find generate_class_def signature and verify type expectations
# Find generate_class_def definition
echo "=== Searching for generate_class_def definition ==="
rg -nP --type=rust -B3 'fn generate_class_def' derive-impl/src/
echo ""
echo "=== Check the full signature of generate_class_def ==="
rg -nP --type=rust -A15 'fn generate_class_def'Length of output: 1251
🏁 Script executed:
#!/bin/bash
# Description: Verify how base_class_name is used in the quote output and check for potential issues
echo "=== Full context of base_class_name usage in quote macro ==="
sed -n '539,600p' derive-impl/src/pyclass.rs
echo ""
echo "=== Search for how this generated code might be processed ==="
rg -nP --type=rust -C3 'base_class_name'Length of output: 3041
🏁 Script executed:
#!/bin/bash
# Description: Verify how pyclass attribute expects base parameter and check for compilation
echo "=== Check if there are test cases for pyexception with base ==="
fd -e rs test -o example | xargs rg -l 'pyexception.*base|base.*pyexception' 2>/dev/null || echo "No tests found"
echo ""
echo "=== Search for how pyclass macro processes the base attribute ==="
rg -nP --type=rust 'base.*=' derive-impl/src/util.rs | head -20Length of output: 303
🏁 Script executed:
#!/bin/bash
# Description: Search for similar base usage patterns and quote macro handling
echo "=== Search for all quote! blocks using base parameter ==="
rg -nP --type=rust -B5 -A5 'quote!.*base|base.*quote!' derive-impl/src/
echo ""
echo "=== Check util.rs for base handling patterns ==="
sed -n '410,425p' derive-impl/src/util.rsLength of output: 1079
Fix pyexception code path to properly handle Option<syn::Path> return type from base().
The change from String to syn::Path at line 426 is correctly handled by generate_class_def(), which expects Option<syn::Path>. However, line 570 has a critical issue: it directly interpolates the Option type into the quote macro, which will generate invalid proc macro attribute syntax like base = Some(path) or base = None.
Line 570 must follow the pattern used at lines 383-387: match on the Option<syn::Path>, unwrap when present, and provide an appropriate default when absent. Either unwrap the option explicitly before quoting (if base is required for exceptions), or conditionally quote the attribute value.
🤖 Prompt for AI Agents
In derive-impl/src/util.rs around lines 416-418 (and the pyexception emit site
near line 570), the pyexception attribute code currently interpolates an
Option<syn::Path> directly into quote causing generated attributes like `base =
Some(path)` or `base = None`; change this to match the pattern at lines
~383-387: match on the Option, map Some(path) to a token/value you will inject
(e.g. build a `base = path` TokenStream or the path token itself) and map None
to either omit the base attribute or emit an appropriate default token stream,
then use that prepared token/value in the quote invocation so the generated
attribute contains either the unwrapped path or nothing/default rather than an
Option.
Summary by CodeRabbit