Implement more warnings - #7008
Conversation
π WalkthroughWalkthroughRefactors and expands the warnings subsystem: adds VM-level warning state and lock APIs, broadens warn/warn_explicit to accept PyObject messages, adds PYTHONWARNINGS parsing, simplifies frame traversal, and updates call sites to pass message.into() conversions; many changes affect warn routing, registry, and filter matching. Changes
Sequence Diagram(s)sequenceDiagram
participant Env as Settings / Env
participant WarnAPI as warn() / warn_explicit
participant FrameWalk as Frame Walker
participant Filter as Filter Matcher
participant Registry as once_registry
participant Show as call_show_warning / _showwarnmsg
Env->>WarnAPI: provide message, category, skip_file_prefixes
WarnAPI->>FrameWalk: walk stack (skip internals/prefixes)
FrameWalk-->>WarnAPI: calling context (filename, lineno, module)
WarnAPI->>Filter: evaluate filters for category & message
Filter-->>WarnAPI: action (error/ignore/once/always/default/module)
alt error
WarnAPI->>Env: raise as exception
else once
WarnAPI->>Registry: check/record
Registry-->>WarnAPI: already_warned? / recorded
alt not recorded
WarnAPI->>Show: call_show_warning with text & instance
Show-->>Env: emit via _showwarnmsg
end
else emit
WarnAPI->>Show: call_show_warning
Show-->>Env: emit via _showwarnmsg
end
Estimated code review effortπ― 4 (Complex) | β±οΈ ~45 minutes Possibly related PRs
Poem
π₯ Pre-merge checks | β 2 | β 1β Failed checks (1 inconclusive)
β Passed checks (2 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 |
There was a problem hiding this comment.
Actionable comments posted: 2
π€ Fix all issues with AI agents
In @.cspell.dict/python-more.txt:
- Around line 102-103: The dictionary contains a duplicate entry "getweakrefs"
on consecutive lines; remove the redundant line so only a single "getweakrefs"
entry remains (i.e., keep one occurrence and delete the duplicate to ensure
unique entries).
In `@crates/vm/src/warn.rs`:
- Around line 88-94: The release_lock implementation can underflow because load
+ fetch_sub races; replace the two-step logic in release_lock to an atomic
compare-and-swap loop (e.g., using AtomicUsize::fetch_update or a CAS loop) on
self.lock_count: read current, if zero return false, otherwise attempt to set
current - 1 and retry on contention; return true only when the decrement
succeeded so concurrent releases cannot drive the counter to usize::MAX.
| getweakrefs | ||
| getweakrefs |
There was a problem hiding this comment.
Remove duplicate entry.
The term "getweakrefs" appears twice on consecutive lines. Dictionary files should contain unique entries only.
π§ Proposed fix
getweakrefcount
-getweakrefs
getweakrefs
getwindowsversionπ 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.
| getweakrefs | |
| getweakrefs | |
| getweakrefcount | |
| getweakrefs | |
| getwindowsversion |
π€ Prompt for AI Agents
In @.cspell.dict/python-more.txt around lines 102 - 103, The dictionary contains
a duplicate entry "getweakrefs" on consecutive lines; remove the redundant line
so only a single "getweakrefs" entry remains (i.e., keep one occurrence and
delete the duplicate to ensure unique entries).
| pub fn release_lock(&self) -> bool { | ||
| let prev = self.lock_count.load(Ordering::SeqCst); | ||
| if prev == 0 { | ||
| return false; | ||
| } | ||
| self.lock_count.fetch_sub(1, Ordering::SeqCst); | ||
| true |
There was a problem hiding this comment.
Avoid lock_count underflow on concurrent release.
load + fetch_sub can race, causing an underflow to usize::MAX if two releases happen around the same time. Use fetch_update/CAS to make the decrement conditional.
Proposed fix
- pub fn release_lock(&self) -> bool {
- let prev = self.lock_count.load(Ordering::SeqCst);
- if prev == 0 {
- return false;
- }
- self.lock_count.fetch_sub(1, Ordering::SeqCst);
- true
- }
+ pub fn release_lock(&self) -> bool {
+ self.lock_count
+ .fetch_update(Ordering::SeqCst, Ordering::SeqCst, |v| {
+ if v == 0 { None } else { Some(v - 1) }
+ })
+ .is_ok()
+ }π 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.
| pub fn release_lock(&self) -> bool { | |
| let prev = self.lock_count.load(Ordering::SeqCst); | |
| if prev == 0 { | |
| return false; | |
| } | |
| self.lock_count.fetch_sub(1, Ordering::SeqCst); | |
| true | |
| pub fn release_lock(&self) -> bool { | |
| self.lock_count | |
| .fetch_update(Ordering::SeqCst, Ordering::SeqCst, |v| { | |
| if v == 0 { None } else { Some(v - 1) } | |
| }) | |
| .is_ok() | |
| } |
π€ Prompt for AI Agents
In `@crates/vm/src/warn.rs` around lines 88 - 94, The release_lock implementation
can underflow because load + fetch_sub races; replace the two-step logic in
release_lock to an atomic compare-and-swap loop (e.g., using
AtomicUsize::fetch_update or a CAS loop) on self.lock_count: read current, if
zero return false, otherwise attempt to set current - 1 and retry on contention;
return true only when the decrement succeeded so concurrent releases cannot
drive the counter to usize::MAX.
π¦ Library DependenciesThe following Lib/ modules were modified. Here are their dependencies: [ ] lib: cpython/Lib/site.py dependencies:
dependent tests: (1 tests)
[x] lib: cpython/Lib/warnings.py dependencies:
dependent tests: (48 tests)
Legend:
|
Summary by CodeRabbit
New Features
Improvements
Chores