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 — Implement more warnings by youknowone · Pull Request #7008 · RustPython/RustPython 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 |
All reactions
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
All reactions
| getweakrefs | ||
| getweakrefs |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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).
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
All reactions
| 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
All reactions
📦 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:
|
All reactions
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Reviewers
Uh oh!
There was an error while loading. Please reload this page.
Assignees
No one assignedLabels
Projects
Milestone
No milestoneDevelopment
Successfully merging this pull request may close these issues.
Uh oh!
There was an error while loading. Please reload this page.
1 participant
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.