GeezerVison

The World Wide Web, restored to 800 pixels of glory.

GeezerVison is showing github.com — Allow #[pyexception] on module attr by youknowone · Pull Request #6218 · RustPython/RustPython

Conversation

youknowone commented Oct 26, 2025 •

edited by coderabbitai Bot

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Member

Summary by CodeRabbit

New Features
- Introduced specialized SSL exception types (SSLCertVerificationError, SSLZeroReturnError, SSLWantReadError, SSLWantWriteError, SSLSyscallError, SSLEOFError) for improved error handling and compatibility with Python's ssl module.
Refactor
- Enhanced internal exception framework to support custom exception hierarchies with improved attribute handling.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions


          Expand #[pyexception] macro working with module attr

c8ff4b9

coderabbitai Bot commented Oct 26, 2025 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Contributor

Walkthrough

Introduces support for a new #[pyexception] attribute in the derive-impl framework to define exception types, refactors the SSL module to use a structured exception hierarchy with PySslError as the base, makes exception slot methods public, and extends exception metadata validation to support module attributes.

Changes

Cohort / File(s)	Summary
Derive infrastructure for pyexception support `derive-impl/src/pymodule.rs`, `derive-impl/src/util.rs`	Adds Exception variant to attribute name enum, wires it through Display and FromStr conversions, treats pyexception like pyclass in module item creation, includes Exception in validation flow, and adds "module" to ALLOWED_NAMES for ExceptionItemMeta.
Exception slot method visibility `derive-impl/src/pyclass.rs`, `vm/src/exceptions.rs`	Changes `slot_new` and `slot_init` from private/`pub(crate)` to `pub` visibility in PyException and PyOSError implementations; adds new PyPayload implementation path for PyException when no ctx_name is provided.
SSL module exception hierarchy refactoring `stdlib/src/ssl.rs`	Replaces ad-hoc SSL exception creation with a structured hierarchy: introduces PySslError (base, derives from PyOSError) and six subclasses (PySslCertVerificationError, PySslZeroReturnError, PySslWantReadError, PySslWantWriteError, PySslSyscallError, PySslEOFError), all marked as pyexceptions; updates all error construction sites to use the new exception classes and adds __str__ implementation for PySslError.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

stdlib/src/ssl.rs: Requires careful review of the exception hierarchy refactoring, all error construction points, and the __str__ implementation for correctness and consistency across the module. Verify that all old ssl_error(vm) calls have been properly replaced.
derive-impl/src/pymodule.rs and derive-impl/src/util.rs: Review the integration of Exception variant into the attribute parsing and validation pipeline to ensure it follows the same pattern as Class and Function.
Visibility changes: Verify that making slot methods public in derive-impl and vm/src/exceptions.rs does not expose internal implementation details unintentionally.

Possibly related PRs

Impl PyAttributeError args #5805: Modifies exception slot implementations in vm/src/exceptions.rs, including slot_init and slot_new visibility and exception slot implementations, directly overlapping with exception infrastructure changes in this PR.

Poem

🐰 Exceptions now have a home, a hierarchy so grand,
From PySslError's base, the subclasses expand,
The #[pyexception] magic makes definitions neat,
Public slots and module attributes, the feature's complete! ✨

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check	✅ Passed	The PR title "Allow #[pyexception] on module attr" directly describes the primary objective of the changeset. The modifications across multiple files demonstrate this core feature: derive-impl/src/pymodule.rs introduces an Exception variant to support pyexception attributes in modules, derive-impl/src/util.rs adds "module" to the allowed attribute names for ExceptionItemMeta, and stdlib/src/ssl.rs exemplifies the new capability with public pyexception definitions. The title is specific, concise, and clearly communicates the main change without being vague or misleading.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

✨ Finishing touches

📝 Generate docstrings

🧪 Generate unit tests (beta)

Create PR with unit tests
Post copyable unit tests in a comment

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.}

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.


          pyssl errors

ded211d

youknowone force-pushed the pyexcetion branch from 810fc12 to ded211d Compare

October 27, 2025 00:37

youknowone marked this pull request as ready for review

October 27, 2025 00:37

youknowone requested review from ShaharNaveh and coolreader18

October 27, 2025 00:38

coderabbitai Bot reviewed

Oct 27, 2025

View reviewed changes

coderabbitai Bot left a comment

Copy link

Copy Markdown

Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (2)

stdlib/src/ssl.rs (2)

206-226: New SSL exception hierarchy (SSLError + subclasses) — well structured.

Base SSLError derives from PyOSError (CPython parity).
str prefers strerror then args — sensible default.

Minor: you could fallback to exc.str(vm) instead of str(args) for perfect parity, but current code is fine.

Also applies to: 230-236, 239-245, 248-254, 257-263, 266-272, 275-281

1982-2012: convert_openssl_error/convert_ssl_error are solid; add ZERO_RETURN mapping.

Map ssl::ErrorCode::ZERO_RETURN to SSLZeroReturnError for better CPython parity (clean shutdown). Suggested patch:

@@ fn convert_ssl_error(vm: &VirtualMachine, e: impl std::borrow::Borrow<ssl::Error>) -> PyBaseExceptionRef {
-        let (cls, msg) = match e.code() {
+        let (cls, msg) = match e.code() {
             ssl::ErrorCode::WANT_READ => (
                 PySslWantReadError::class(&vm.ctx).to_owned(),
                 "The operation did not complete (read)",
             ),
             ssl::ErrorCode::WANT_WRITE => (
                 PySslWantWriteError::class(&vm.ctx).to_owned(),
                 "The operation did not complete (write)",
             ),
+            ssl::ErrorCode::ZERO_RETURN => (
+                PySslZeroReturnError::class(&vm.ctx).to_owned(),
+                "TLS/SSL connection has been closed (EOF)",
+            ),
             ssl::ErrorCode::SYSCALL => match e.io_error() {
                 Some(io_err) => return io_err.to_pyexception(vm),
                 None => (
                     PySslSyscallError::class(&vm.ctx).to_owned(),
                     "EOF occurred in violation of protocol",
                 ),
             },
             ssl::ErrorCode::SSL => match e.ssl_error() {
                 Some(e) => return convert_openssl_error(vm, e.clone()),
                 None => (
                     PySslError::class(&vm.ctx).to_owned(),
                     "A failure in the SSL library occurred",
                 ),
             },
             _ => (
                 PySslError::class(&vm.ctx).to_owned(),
                 "A failure in the SSL library occurred",
             ),
         };

Run your SSL tests that exercise graceful close to verify behavior change.

Also applies to: 2034-2068

📜 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 2e7a8b4 and ded211d.

📒 Files selected for processing (5)

derive-impl/src/pyclass.rs (3 hunks)
derive-impl/src/pymodule.rs (5 hunks)
derive-impl/src/util.rs (1 hunks)
stdlib/src/ssl.rs (11 hunks)
vm/src/exceptions.rs (2 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 (run cargo fmt)
Run clippy and fix any warnings or lints introduced by your changes
Follow Rust best practices for error handling and memory management

Files:

derive-impl/src/util.rs
derive-impl/src/pymodule.rs
derive-impl/src/pyclass.rs
stdlib/src/ssl.rs
vm/src/exceptions.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:

stdlib/src/ssl.rs
vm/src/exceptions.rs

🧬 Code graph analysis (4)

derive-impl/src/pymodule.rs (1)

derive-impl/src/util.rs (1)

attr_name (106-108)

derive-impl/src/pyclass.rs (2)

vm/src/exceptions.rs (12)

class (43-45)

slot_new (1453-1465)

slot_new (1468-1470)

slot_init (1272-1279)

slot_init (1314-1330)

slot_init (1349-1369)

slot_init (1473-1489)

slot_init (1655-1687)

slot_init (1749-1756)

slot_init (1791-1805)

slot_init (1842-1855)

slot_init (1893-1905)

vm/src/class.rs (1)

static_type (25-33)

stdlib/src/ssl.rs (3)

derive/src/lib.rs (1)

pyexception (139-143)

derive-impl/src/lib.rs (1)

pyexception (50-56)

vm/src/exceptions.rs (10)

__str__ (637-644)

__str__ (1405-1415)

__str__ (1492-1521)

__str__ (1690-1734)

__str__ (1808-1831)

__str__ (1858-1882)

__str__ (1908-1928)

class (43-45)

args (567-569)

args (573-573)

vm/src/exceptions.rs (1)

vm/src/types/slot.rs (4)

slot_new (787-790)

slot_new (810-812)

slot_new (821-823)

slot_init (836-840)

⏰ 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). (8)

GitHub Check: Run snippets and cpython tests (macos-latest)
GitHub Check: Run snippets and cpython tests (ubuntu-latest)
GitHub Check: Run snippets and cpython tests (windows-latest)
GitHub Check: Run rust tests (ubuntu-latest)
GitHub Check: Check Rust code with rustfmt and clippy
GitHub Check: Run rust tests (windows-latest)
GitHub Check: Run rust tests (macos-latest)
GitHub Check: Ensure compilation on various targets

🔇 Additional comments (10)

derive-impl/src/util.rs (1)

449-451: ALLOWED_NAMES now includes "module" — good alignment with pyclass metadata.

This enables #[pyexception(module = ...)] parsing consistently. No issues.

derive-impl/src/pymodule.rs (3)

19-20: Add AttrName::Exception with Display/FromStr — looks correct.

Enum wiring is complete and error strings remain coherent.

Also applies to: 28-29, 37-43

239-243: Treat pyexception like pyclass in module items.

Correct: both define types. This keeps generation paths uniform.

309-316: Allow #[pyexception] to follow #[pyattr].

The diagnostic string is updated accordingly. Good UX improvement.

derive-impl/src/pyclass.rs (3)

554-560: PyException PyPayload fallback via StaticType — good default.

Removes ctx dependency for external exceptions. Ensure new_class initialization runs before first class() call, otherwise StaticType::static_type() panics. Please verify module init order.

622-629: Make generated slot_new public.

Necessary for cross-crate calls like ::Base::slot_new when Base lives in vm. Good.

643-650: Make generated slot_init public.

Same rationale as slot_new. Looks good.

vm/src/exceptions.rs (1)

1452-1469: Expose PyOSError slot_new/slot_init as public.

Enables derive-generated calls from other crates. Safe and consistent with the new macro behavior.

Also applies to: 1473-1489

stdlib/src/ssl.rs (2)

41-44: Import AsObject/PyOSError — required for new exception plumbing.

Imports are correct and used.

642-647: Consistent use of PySslError for module errors.

Raised errors now consistently use the new SSL classes. Good consistency upgrade.

Also applies to: 771-782, 963-972, 1418-1421, 1451-1454, 1883-1886

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

ShaharNaveh approved these changes

Oct 27, 2025

View reviewed changes

youknowone merged commit 517b55b into RustPython:main

Oct 27, 2025

1 check passed

Uh oh!

There was an error while loading. Please reload this page.

youknowone deleted the pyexcetion branch

October 27, 2025 09:37

coderabbitai Bot mentioned this pull request

Dec 14, 2025

Allow with() in pyexception #6436

Merged

coderabbitai Bot mentioned this pull request

Dec 28, 2025

update _pyio, test_fileio from 3.13.11 and impl more io features #6560

Merged

coderabbitai Bot mentioned this pull request

Feb 3, 2026

Support #[cfg] in pymodule with #6975

Merged

Reviewers

coderabbitai[bot]

coderabbitai[bot] left review comments

ShaharNaveh

ShaharNaveh approved these changes

coolreader18

Awaiting requested review from coolreader18

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

2 participants

Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.

You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.

GeezerVison

Conversation

youknowone commented Oct 26, 2025 • edited by coderabbitai Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary by CodeRabbit

Uh oh!

coderabbitai Bot commented Oct 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Estimated code review effort

Possibly related PRs

Poem

Pre-merge checks and finishing touches

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

youknowone commented Oct 26, 2025 •

edited by coderabbitai Bot

Loading

Uh oh!

There was an error while loading. Please reload this page.

coderabbitai Bot commented Oct 26, 2025 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.