Refactor warn.rs and _warnings module - #7023
Conversation
Sorry, something went wrong.
📝 Walkthrough
WalkthroughRemoves an internal EncodingWarning emission and refactors the warnings subsystem: warn() now returns unit and delegates through new helpers (registry/filter/module normalization and context setup); adds two interned constant names to the VM context. No exported Python API signatures changed beyond warn semantics. 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
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. ❤️ ShareComment @coderabbitai help to get the list of available commands and usage tips. |
Sorry, something went wrong.
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 | 🟡 Minor
Deduplicate 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.
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.
- 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
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agentsIn `@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.
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.
| let is_subclass = category.is_subclass(&tmp_item[2], vm)?; | ||
| let good_mod = check_matched(&tmp_item[3], &module, vm)?; | ||
| let ln: usize = tmp_item[4].try_int(vm).map_or(0, |v| v.as_u32_mask() as _); | ||
|
|
||
| if good_msg && good_mod && is_subclass && (ln == 0 || lineno == ln) { | ||
| if good_msg && is_subclass && good_mod && (ln == 0 || lineno == ln) { |
There was a problem hiding this comment.
⚠️ Potential issue | 🟠 Major
🧩 Analysis chain🏁 Script executed:
cat -n crates/vm/src/warn.rs | sed -n '280,300p'Repository: RustPython/RustPython
Length of output: 934
🌐 Web query:
CPython _warnings.c get_filter function lineno error handling
💡 Result:
In current CPython main, get_filter() itself doesn’t “parse” lineno; it receives lineno as a Py_ssize_t argument and delegates filter matching to filter_search(). (chromium.googlesource.com)
The lineno error handling you’re asking about is for the filter entry’s stored line number (ln_obj, i.e., the 5th element of each filter tuple). In filter_search() CPython does:
- ln = PyLong_AsSsize_t(ln_obj);
- If conversion fails (e.g., ln_obj is not an int-like object, or it overflows Py_ssize_t), PyLong_AsSsize_t() sets an exception, and the code detects that with if (ln == -1 && PyErr_Occurred()), then:
- decref’s the temporary tuple item
- returns false, which propagates out so get_filter() returns NULL (error). (chromium.googlesource.com)
So, the behavior is: invalid/overflowing ln_obj → exception from PyLong_AsSsize_t is propagated unchanged; there’s no custom warning-filter-specific error message for “bad lineno”, just the generic conversion error. (chromium.googlesource.com)
Citations:
- 1: https://chromium.googlesource.com/external/github.com/python/cpython/%2B/refs/heads/main/Python/_warnings.c
- 2: https://chromium.googlesource.com/external/github.com/python/cpython/%2B/refs/heads/main/Python/_warnings.c
- 3: https://chromium.googlesource.com/external/github.com/python/cpython/%2B/refs/heads/main/Python/_warnings.c
Silently defaulting ln to 0 on try_int failure masks malformed filter configurations.
Line 290: when tmp_item[4] fails integer conversion, map_or(0, ...) silently treats it as "match any line number." CPython's filter_search() in _warnings.c calls PyLong_AsSsize_t() which propagates conversion errors instead. This behavioral difference could hide filter configuration bugs.
Proposed fix- let ln: usize = tmp_item[4].try_int(vm).map_or(0, |v| v.as_u32_mask() as _);
+ let ln: usize = tmp_item[4].try_int(vm).map(|v| v.as_u32_mask() as usize)?;‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let is_subclass = category.is_subclass(&tmp_item[2], vm)?; | |
| let good_mod = check_matched(&tmp_item[3], &module, vm)?; | |
| let ln: usize = tmp_item[4].try_int(vm).map_or(0, |v| v.as_u32_mask() as _); | |
| if good_msg && good_mod && is_subclass && (ln == 0 || lineno == ln) { | |
| if good_msg && is_subclass && good_mod && (ln == 0 || lineno == ln) { | |
| let is_subclass = category.is_subclass(&tmp_item[2], vm)?; | |
| let good_mod = check_matched(&tmp_item[3], &module, vm)?; | |
| let ln: usize = tmp_item[4].try_int(vm).map(|v| v.as_u32_mask() as usize)?; | |
| if good_msg && is_subclass && good_mod && (ln == 0 || lineno == ln) { |
In `@crates/vm/src/warn.rs` around lines 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.
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.
Summary by CodeRabbit
Bug Fixes
Refactor
New Features