Refactor warn.rs and _warnings module#7023
Conversation
📝 WalkthroughWalkthroughRemoves an internal EncodingWarning emission and refactors the warnings subsystem: Changes
Sequence Diagram(s)sequenceDiagram
participant PyCode as Python caller
participant VMWarn as vm::warn / warn_with_skip
participant WState as sys.modules["warnings"] / registry
participant Show as show_warning (hook / stderr)
PyCode->>VMWarn: warn(message, category, stack_level, ...)
VMWarn->>VMWarn: setup_context(stack_level, skip_prefixes)
VMWarn->>WState: get_warnings_filters(), get_once_registry(), get_default_action()
VMWarn->>VMWarn: get_filter(category, text, lineno, module)
alt filter is "ignore"
VMWarn->>WState: update_registry(key)
VMWarn-->>PyCode: return
else filter is "error"/"always"/"default"/"module"/"once"
VMWarn->>WState: update_registry(key) (as needed)
VMWarn->>Show: call_show_warning(WarningMessage)
Show-->>VMWarn: handled
VMWarn-->>PyCode: return
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
Sorry, something went wrong.
bdad269 to
eda8e8b
Compare
February 6, 2026 05:29
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/vm/src/warn.rs (1)
553-611: ⚠️ Potential issue | 🟡 MinorDeduplicate the stack-walk loops.
The two branches differ only by the frame-advance function; extract that difference and run a single loop to avoid duplication. As per coding guidelines, When branches differ only in a value but share common logic, extract the differing value first, then call the common logic once to avoid duplicate code.
🤖 Fix all issues with AI agents
In `@crates/vm/src/stdlib/warnings.rs`:
- Around line 11-17: The call to crate::warn::warn passes stack_level as
(stack_level as isize) which can wrap for very large usize values; change this
by performing a safe conversion from usize to isize (e.g., use isize::try_from
or usize::try_into) and handle the failure path explicitly (either clamp to
isize::MAX or return an error/early-return) before calling crate::warn::warn;
update the call site that constructs the args (the stack_level conversion in the
warn invocation and any surrounding logic that sets stack_level) so it uses the
checked conversion result and passes the resulting isize safely to
crate::warn::warn.
In `@crates/vm/src/warn.rs`:
- Around line 249-256: In normalize_module, the code uses filename.char_len() to
compute a slice index into filename.as_wtf8(), which mixes character count with
byte slicing and breaks for non-ASCII names; change to use filename.byte_len()
(or similar byte-length API) and use that byte length when computing the slice
(e.g., replace the char_len() binding with a byte_len variable and slice
as_wtf8()[..byte_len - 3]) so the ".py" strip uses byte offsets consistent with
as_wtf8(); keep the existing ends_with(b".py") check and only adjust the length
source in normalize_module.
Sorry, something went wrong.
eda8e8b to
5505e77
Compare
February 6, 2026 08:45
- Add already_warned() with filter version tracking - Add type validation for _defaultaction and _onceregistry - Use direct function names for _warnings pyattr/pyfunction - Route stdlib::warnings::warn through warn::warn - Remove spurious EncodingWarning from TextIOWrapper - Clean up comments and simplify check_matched error handling - Remove expectedFailure from 3 passing test_warnings tests
5505e77 to
d577eb2
Compare
February 6, 2026 08:48
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@crates/vm/src/warn.rs`:
- Around line 288-292: The code currently silences conversion errors by using
tmp_item[4].try_int(vm).map_or(0, ...) to set ln, which masks malformed filter
configs; change this to propagate the integer conversion error instead of
defaulting to 0 (e.g., use tmp_item[4].try_int(vm)? and then convert to
usize/isize as appropriate, or map_err to return the VM error), so that failures
in tmp_item[4].try_int(vm) bubble up rather than making ln == 0; update any
downstream handling of ln (used in the if condition with is_subclass,
check_matched, lineno) to expect a fallible conversion.
🧹 Nitpick comments (1)
crates/vm/src/warn.rs (1)
497-515:writeln!result is silently discarded.The
writeln!return value on line 506 is not handled. While this is acceptable for a best-effort warning display (matching CPython's behavior of ignoring stderr write failures), it will produce a compiler warning unless explicitly suppressed.Proposed fix
- writeln!( + let _ = writeln!( stderr, "{}:{}: {}: {}", filename, lineno, category.name(), text );
Sorry, something went wrong.
📦 Library DependenciesThe following Lib/ modules were modified. Here are their dependencies: [x] lib: cpython/Lib/warnings.py dependencies:
dependent tests: (48 tests)
Legend:
|
Sorry, something went wrong.
234bdda
into
RustPython:main
Feb 6, 2026
Summary by CodeRabbit
Bug Fixes
Refactor
New Features