Add callable validation to `codecs.register_error` #6229

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 RustPython:main from ever0de:fix-codecs-register-error-validation

Oct 30, 2025

Contributor

ever0de commented Oct 30, 2025 •

edited by coderabbitai bot

Loading

Validate that the handler argument passed to codecs.register_error is callable, raising TypeError with message 'handler must be callable' if it is not.

This fix enables test_badregistercall in test_codeccallbacks.py to pass.

Summary by CodeRabbit

Bug Fixes
- Improved error handler registration with enhanced validation to ensure handlers are callable, returning appropriate error messages when validation fails.


          Add callable validation to codecs.register_error

5a8d149

Validate that the handler argument passed to codecs.register_error
is callable, raising TypeError with message 'handler must be callable'
if it is not. This matches CPython behavior.

This fix enables test_badregistercall in test_codeccallbacks.py to pass.

Contributor

coderabbitai bot commented Oct 30, 2025 •

edited

Loading

Walkthrough

The register_error function in the codecs stdlib module is updated to return PyResult<()> instead of unit type, adding runtime validation that checks whether the provided handler is callable, and raising a TypeError if validation fails.

Changes

Cohort / File(s)	Summary
Error handler validation `vm/src/stdlib/codecs.rs`	Function `register_error` now returns `PyResult<()>`, validates that the handler argument is callable, and returns `TypeError` if not; previously returned unit type without validation.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~8 minutes

Verify the callability check implementation matches Python's semantics for detecting callable objects
Confirm error message and exception type align with standard Python behavior
Ensure Ok(()) return path correctly indicates successful handler registration

Poem

🐰 A handler arrives at the gate,
We check if it's callable—don't be late!
TypeError if mischief we find,
PyResult returns peace of mind,
Validation's the key to codecs so great! 🔑

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 "Add callable validation to `codecs.register_error`" directly and clearly describes the main change in the changeset. The primary modification is adding runtime validation to ensure the handler argument is callable, returning a TypeError if it is not, which is exactly what the title conveys. The title is concise, specific with the function name and module, and avoids vague terms or unnecessary details. It accurately represents the purpose of the changes and would help a teammate quickly understand the primary intent.
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.}

coderabbitai bot reviewed

View reviewed changes

Contributor

coderabbitai bot left a comment

Actionable comments posted: 0

🧹 Nitpick comments (1)

vm/src/stdlib/codecs.rs (1)

70-78: LGTM! The callable validation correctly matches CPython behavior.

The implementation properly validates that the handler is callable before registration, and the error message matches CPython's expected output. The return type change to PyResult<()> is consistent with other registration functions in this module (register and unregister).

Minor observation: Line 72 uses .to_owned() to convert the string literal. If new_type_error accepts impl Into<String>, you could simplify this to just pass the string literal directly, but this is a very minor style point.

📜 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 614028f and 5a8d149.

⛔ Files ignored due to path filters (1)

Lib/test/test_codeccallbacks.py is excluded by !Lib/**

📒 Files selected for processing (1)

vm/src/stdlib/codecs.rs (1 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:

vm/src/stdlib/codecs.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/stdlib/codecs.rs

🧬 Code graph analysis (1)

vm/src/stdlib/codecs.rs (1)

vm/src/codecs.rs (1)

register_error (341-343)

⏰ 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: Check the WASM package and demo
GitHub Check: Run snippets and cpython tests (ubuntu-latest)
GitHub Check: Run rust tests (ubuntu-latest)
GitHub Check: Run snippets and cpython tests (macos-latest)
GitHub Check: Run snippets and cpython tests on wasm-wasi
GitHub Check: Run tests under miri
GitHub Check: Run snippets and cpython tests (windows-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

youknowone approved these changes

View reviewed changes

Member

youknowone left a comment

👍

youknowone merged commit 9e7d291 into RustPython:main

12 checks passed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet