Skip to content

Allow with() in pyexception #6436

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

Jump to bottom
Merged
youknowone merged 1 commit into from
Dec 14, 2025
Merged

Allow with() in pyexception #6436

youknowone merged 1 commit into from
Dec 14, 2025

Conversation

@youknowone
Copy link
Member

@youknowone youknowone commented Dec 14, 2025 •
edited by coderabbitai bot
Loading

Summary by CodeRabbit

  • New Features

    • PyException declarations now support with(Constructor) clause for enhanced customization
    • Extra attributes in with(...) arguments are now properly preserved and propagated

  • Bug Fixes

    • Removed overly restrictive validation that previously rejected attributes on PyException implementations

✏️ Tip: You can customize this high-level summary in your review settings.

@youknowone youknowone marked this pull request as ready for review December 14, 2025 02:52
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Dec 14, 2025 •
edited
Loading

Walkthrough

The PR modifies the #[pyexception] derive macro to detect and honor with(Constructor) clauses in attributes. It introduces conditional slot generation flags, accumulates extra attributes for forwarding, and wires them into the generated code while removing previous restrictions on attributes.

Changes

Cohort / File(s) Change Summary
#[pyexception] derive macro enhancement
crates/derive-impl/src/pyclass.rs		 Reworks attribute processing to detect with(Constructor) clause, introduce has_slot_new flag, and accumulate extra_attrs. Removes unconditional attribute errors and wires extra attributes into generated #[pyclass(...)] via extra_attrs_tokens. Marks slot generation contingent on flags and adds TODO for Constructor/Initializer handling.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~30 minutes

  • Attribute parsing and accumulation logic for with(...) clauses requires careful verification
  • Conditional slot generation based on has_slot_new and has_slot_init flags—verify logic paths and backward compatibility
  • extra_attrs_tokens wiring into generated code—ensure attributes propagate correctly without breaking existing implementations
  • Macro code generation can be error-prone; pay special attention to token stream handling

Possibly related PRs

  • RustPython/RustPython#6218: Also modifies #[pyexception] derive in derive-impl/src/pyclass.rs, changing generated slot visibility and PyPayload path—may interact with these attribute/slot changes.

Suggested reviewers

  • coolreader18
  • ShaharNaveh

Poem

🐰 A macro so clever, with slots to arrange,
Constructor and attributes in harmonious change,
With flags now conditional, the logic runs true,
Extra attrs dancing—old errors bid adieu! ✨

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title 'Allow with() in pyexception' directly and clearly describes the main change: enabling the with() clause in pyexception declarations, which aligns with the changeset's core objective.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
📜 Recent review details

Configuration used: Path: .coderabbit.yml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between db71554 and da8dfad.

📒 Files selected for processing (1)
  • crates/derive-impl/src/pyclass.rs (3 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs

📄 CodeRabbit inference engine (.github/copilot-instructions.md)

**/*.rs: Follow the default rustfmt code style by running cargo fmt to format Rust code
Always run clippy to lint Rust code (cargo clippy) before completing tasks and fix any warnings or lints introduced by changes
Follow Rust best practices for error handling and memory management
Use the macro system (pyclass, pymodule, pyfunction, etc.) when implementing Python functionality in Rust

Files:

  • crates/derive-impl/src/pyclass.rs
🧬 Code graph analysis (1)
crates/derive-impl/src/pyclass.rs (2)
crates/derive-impl/src/from_args.rs (1)
  • meta (95-95)
crates/derive-impl/src/lib.rs (1)
  • pyclass (41-47)
⏰ 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). (11)
  • GitHub Check: Run rust tests (macos-latest)
  • GitHub Check: Run snippets and cpython tests (windows-latest)
  • GitHub Check: Run rust tests (windows-latest)
  • GitHub Check: Run snippets and cpython tests (macos-latest)
  • GitHub Check: Run snippets and cpython tests (ubuntu-latest)
  • GitHub Check: Run rust tests (ubuntu-latest)
  • GitHub Check: Run tests under miri
  • GitHub Check: Check Rust code with clippy
  • GitHub Check: Run snippets and cpython tests on wasm-wasi
  • GitHub Check: Ensure compilation on various targets
  • GitHub Check: Check the WASM package and demo
🔇 Additional comments (4)
crates/derive-impl/src/pyclass.rs (4)

630-630: TODO comment indicates incomplete implementation.

The comment notes that slot_new and slot_init should be properly constrained by Constructor or Initializer traits in the future. This suggests the current implementation is a work-in-progress that will require additional validation or constraints in subsequent changes.

668-672: LGTM!

The conditional generation of extra_attrs_tokens is correct. The leading comma on line 671 properly integrates the extra attributes into the existing #[pyclass(...)] attribute list.

675-675: LGTM!

The modification correctly forwards the accumulated with(...) attributes to the generated #[pyclass(...)] macro, enabling proper attribute propagation from #[pyexception] impl blocks.

582-604: Remove the reference to Unconstructible — it is not a supported pattern in the codebase and does not appear in the implementation.

The review comment correctly identifies that the code only checks for Constructor when determining whether to skip generating default slot_new. However, the claim that extract_impl_attrs treats both Constructor and Unconstructible similarly is incorrect—Unconstructible is not a supported attribute pattern in this codebase. The concern about Unconstructible should be removed from the review comment.

The actual concern worth noting is that the code only handles Constructor from the with(...) attribute, which appears to be the only supported constructor trait variant for pyexception at this time. This is not an oversight but rather the intended behavior for the current implementation.

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@youknowone youknowone merged commit 21300f6 into RustPython:main Dec 14, 2025
13 checks passed
@youknowone youknowone deleted the pyexception-with branch December 14, 2025 03:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@youknowone