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 |
![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
🤖 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:
|
1 participant
Summary by CodeRabbit
New Features
Improvements
Chores