GeezerVisonThe World Wide Web, restored to 800 pixels of glory. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Home|About|Gaming cabinet|Archives|How it works|Starter sites | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
GeezerVison is showing github.com — Refactor warn.rs and _warnings module by youknowone · Pull Request #7023 · RustPython/RustPython 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.
There was an error while loading. Please reload this page.
bdad269 to
eda8e8b
Compare
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
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.
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.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
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
5505e77 to
d577eb2
Compare
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
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.
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.
There was an error while loading. Please reload this page.
| 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.
The reason will be displayed to describe this comment to others. Learn more.
🏁 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);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:
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:
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.
- 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.
There was an error while loading. Please reload this page.
📦 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.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
- 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 an error while loading. Please reload this page.
Successfully merging this pull request may close these issues.
There was an error while loading. Please reload this page.