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 |
youknowone
force-pushed
the
cleanup-warnings
branch
from
bdad269 to
eda8e8b
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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.
eda8e8b to
5505e77
Compare
- 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
youknowone
force-pushed
the
cleanup-warnings
branch
from
5505e77 to
d577eb2
Compare
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 );
| 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.
🧩 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_objis not an int-like object, or it overflowsPy_ssize_t),PyLong_AsSsize_t()sets an exception, and the code detects that withif (ln == -1 && PyErr_Occurred()), then:- decref’s the temporary tuple item
- returns
false, which propagates out soget_filter()returnsNULL(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)?;📝 Committable suggestion
‼️ 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) { |
🤖 Prompt for AI Agents
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.
📦 Library DependenciesThe following Lib/ modules were modified. Here are their dependencies: [x] lib: cpython/Lib/warnings.py dependencies:
dependent tests: (48 tests)
Legend:
|
1 participant
Summary by CodeRabbit
Bug Fixes
Refactor
New Features