Update test_winsound from v3.14.3#7184
Conversation
Add PlaySound, Beep, MessageBeep functions and all SND_*/MB_* constants. Uses Windows APIs via winmm.dll, kernel32, user32.
📝 WalkthroughWalkthroughThese changes introduce a new Changes
Sequence Diagram(s)sequenceDiagram
actor Python as Python Code
participant VM as VM Runtime
participant WS as Winsound Module
participant FFI as Win32 APIs
Python->>VM: call PlaySound(sound, flags)
VM->>WS: PlaySound(args, vm)
WS->>WS: Parse arguments & validate input
alt sound is None
WS->>FFI: PlaySoundW(nullptr, flags)
else sound is bytes/memory
WS->>WS: Create buffer from bytes
WS->>FFI: PlaySoundW(buffer_ptr, flags | SND_MEMORY)
else sound is str/Path
WS->>WS: Normalize path to wide string
WS->>FFI: PlaySoundW(path_ptr, flags)
end
FFI-->>WS: Result (success or error)
WS->>WS: Convert Windows error to Python exception
WS-->>VM: PyResult
VM-->>Python: return or raise
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ 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 |
📦 Library DependenciesThe following Lib/ modules were modified. Here are their dependencies: [x] test: cpython/Lib/test/test_winsound.py dependencies: dependent tests: (no tests depend on winsound) Legend:
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
crates/vm/src/stdlib/winsound.rs (3)
131-138:vm.is_none(&fspath)check on the method object appears to be dead code.
get_method_or_type_errorreturns the bound method (or errors). The returnedfspathis the method itself, not its return value — it will never beNone. The actual call happens later on line 138. This guard can be removed without behavioral change.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/vm/src/stdlib/winsound.rs` around lines 131 - 138, Remove the redundant vm.is_none(&fspath) guard: get_method_or_type_error returns a bound method (the fspath variable) which will never be None, so the vm.is_none(&fspath) check and its Err branch should be deleted; leave the subsequent call to fspath.call((), vm)? intact so the actual return value is produced and validated. Target the block around get_method_or_type_error / vm.is_none / fspath.call in winsound.rs and remove only the is_none check and its error return.
83-168: Inconsistent error reporting:PlaySoundandBeepdiscard the Windows error code.
MessageBeep(line 202) correctly usesstd::io::Error::last_os_error().into_pyexception(vm)to propagate the Windows error, butPlaySound(lines 90, 107, 166) andBeep(line 187) raise a genericRuntimeErrorwith a hardcoded string. CPython raisesRuntimeErrorwith the actual Windows error message for all three. Consider usinglast_os_errorconsistently so callers get actionable diagnostics.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/vm/src/stdlib/winsound.rs` around lines 83 - 168, PlaySound and Beep currently return a generic vm.new_runtime_error on PlaySoundW/Beep failures; replace those error returns with the OS error conversion used by MessageBeep (i.e. use std::io::Error::last_os_error().into_pyexception(vm)) so the real Windows error is propagated. Specifically, update the three failure branches in PlaySound that check PlaySoundW (the null sound path, the SND_MEMORY buffer path, and the final path using wide.as_ptr()) to return std::io::Error::last_os_error().into_pyexception(vm) instead of vm.new_runtime_error(...), and do the same for the Beep failure branch that currently returns a generic runtime error. Ensure you call into_pyexception(vm) the same way MessageBeep does so the PyException contains the platform error message.
6-16: Usewindows-sysimports instead of manual FFI bindings.
Win32_Media_Audio,Win32_System_Diagnostics_Debug, andWin32_UI_WindowsAndMessagingare already enabled inCargo.toml, but the code declares its ownexternblocks instead of importing fromwindows-sys. The rest of the codebase (errno.rs, winreg.rs, time.rs, nt.rs, etc.) consistently usesuse windows_sys::Win32::...imports. Replace the manual FFI with:
use windows_sys::Win32::Media::Audio::PlaySoundW;use windows_sys::Win32::System::Diagnostics::Debug::Beep;use windows_sys::Win32::UI::WindowsAndMessaging::MessageBeep;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/vm/src/stdlib/winsound.rs` around lines 6 - 16, Replace the manual FFI extern blocks in the win32 module with imports from windows-sys: remove the unsafe extern "system" declarations and instead import PlaySoundW, Beep, and MessageBeep via use windows_sys::Win32::Media::Audio::PlaySoundW;, use windows_sys::Win32::System::Diagnostics::Debug::Beep;, and use windows_sys::Win32::UI::WindowsAndMessaging::MessageBeep; so the code uses the existing Cargo-enabled Win32_* feature crates rather than declaring its own bindings (references: PlaySoundW, Beep, MessageBeep, and the win32 module).crates/vm/Cargo.toml (1)
122-122:Win32_Media_Audiofeature is currently unused.The
winsoundmodule uses manual#[link(name = "winmm")] externblocks rather than importing fromwindows-sys. This feature will compile but contributes dead code to the dependency. See the related comment onwinsound.rs— either switch towindows-sysre-exports or remove this feature.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/vm/Cargo.toml` at line 122, The Cargo feature "Win32_Media_Audio" is unused because the winsound module uses manual extern #[link(name = "winmm")] blocks instead of windows-sys re-exports; either remove the unused "Win32_Media_Audio" feature from Cargo.toml or refactor the winsound module (winsound.rs / winsound) to import the relevant APIs from windows-sys (enable the feature and replace manual extern blocks with windows-sys bindings), and then update feature lists accordingly so Win32_Media_Audio is either referenced by the winsound code or deleted to avoid dead dependency code.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/vm/src/stdlib/winsound.rs`:
- Around line 179-189: The Beep implementation casts BeepArgs.duration (i32) to
u32, allowing negative durations to wrap to huge values; update fn Beep (and
validate BeepArgs.duration) to check duration is >= 0 before calling
super::win32::Beep and return vm.new_value_error("duration must be
non-negative") (or similar) when invalid; alternatively, if you want CPython
compatibility, add a comment referencing that choice and leave behavior
unchanged. Ensure the check occurs alongside the existing frequency validation
and refer to the Beep, BeepArgs, VirtualMachine, and win32::Beep symbols when
making the change.
---
Nitpick comments:
In `@crates/vm/Cargo.toml`:
- Line 122: The Cargo feature "Win32_Media_Audio" is unused because the winsound
module uses manual extern #[link(name = "winmm")] blocks instead of windows-sys
re-exports; either remove the unused "Win32_Media_Audio" feature from Cargo.toml
or refactor the winsound module (winsound.rs / winsound) to import the relevant
APIs from windows-sys (enable the feature and replace manual extern blocks with
windows-sys bindings), and then update feature lists accordingly so
Win32_Media_Audio is either referenced by the winsound code or deleted to avoid
dead dependency code.
In `@crates/vm/src/stdlib/winsound.rs`:
- Around line 131-138: Remove the redundant vm.is_none(&fspath) guard:
get_method_or_type_error returns a bound method (the fspath variable) which will
never be None, so the vm.is_none(&fspath) check and its Err branch should be
deleted; leave the subsequent call to fspath.call((), vm)? intact so the actual
return value is produced and validated. Target the block around
get_method_or_type_error / vm.is_none / fspath.call in winsound.rs and remove
only the is_none check and its error return.
- Around line 83-168: PlaySound and Beep currently return a generic
vm.new_runtime_error on PlaySoundW/Beep failures; replace those error returns
with the OS error conversion used by MessageBeep (i.e. use
std::io::Error::last_os_error().into_pyexception(vm)) so the real Windows error
is propagated. Specifically, update the three failure branches in PlaySound that
check PlaySoundW (the null sound path, the SND_MEMORY buffer path, and the final
path using wide.as_ptr()) to return
std::io::Error::last_os_error().into_pyexception(vm) instead of
vm.new_runtime_error(...), and do the same for the Beep failure branch that
currently returns a generic runtime error. Ensure you call into_pyexception(vm)
the same way MessageBeep does so the PyException contains the platform error
message.
- Around line 6-16: Replace the manual FFI extern blocks in the win32 module
with imports from windows-sys: remove the unsafe extern "system" declarations
and instead import PlaySoundW, Beep, and MessageBeep via use
windows_sys::Win32::Media::Audio::PlaySoundW;, use
windows_sys::Win32::System::Diagnostics::Debug::Beep;, and use
windows_sys::Win32::UI::WindowsAndMessaging::MessageBeep; so the code uses the
existing Cargo-enabled Win32_* feature crates rather than declaring its own
bindings (references: PlaySoundW, Beep, MessageBeep, and the win32 module).
| #[pyfunction] | ||
| fn Beep(args: BeepArgs, vm: &VirtualMachine) -> PyResult<()> { | ||
| if !(37..=32767).contains(&args.frequency) { | ||
| return Err(vm.new_value_error("frequency must be in 37 thru 32767".to_owned())); | ||
| } | ||
|
|
||
| let ok = unsafe { super::win32::Beep(args.frequency as u32, args.duration as u32) }; | ||
| if ok == 0 { | ||
| return Err(vm.new_runtime_error("Failed to beep".to_owned())); | ||
| } | ||
| Ok(()) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's check the actual code at the specified file and location
cat -n crates/vm/src/stdlib/winsound.rs | sed -n '170,195p'Repository: RustPython/RustPython
Length of output: 963
🏁 Script executed:
# Also, let's find the BeepArgs struct definition
rg "struct BeepArgs" -A 5 crates/vm/src/stdlib/winsound.rsRepository: RustPython/RustPython
Length of output: 183
🌐 Web query:
CPython winsound Beep negative duration parameter behavior
💡 Result:
In CPython, winsound.Beep(frequency, duration) does not validate that duration is non‑negative—it parses it as a C int and passes it straight through to the underlying Windows call (Beep(freq, dur) on NT-class Windows, or Sleep(dur) in the legacy path). The CPython code only range-checks frequency, not duration. [1]
On Windows, the API signature is BOOL Beep(DWORD dwFreq, DWORD dwDuration), i.e. the duration is an unsigned 32-bit DWORD. [2] That means a negative Python duration (e.g. -1) will be converted to a large unsigned value (two’s-complement wraparound), so it can turn into an extremely long beep (e.g. -1 → 4294967295 ms ≈ 49.7 days) unless the Windows call fails (in which case CPython raises RuntimeError). [1] [2]
Sources:
[1] CPython winsound.c implementation (shows no duration check; calls Beep(freq, dur) / Sleep(dur)) https://sources.debian.org/src/python2.5/2.5-5/PC/winsound.c/
[2] Microsoft Beep API signature (dwDuration is DWORD) https://learn.microsoft.com/en-us/windows/win32/api/utilapiset/nf-utilapiset-beep
Add validation for non-negative duration to prevent silent wraparound of negative values to large unsigned integers.
The duration parameter is declared as i32 (line 176) but cast to u32 (line 185) without validation. A negative value like -1 silently wraps to 4294967295 milliseconds (~49.7 days). Note that CPython's winsound.Beep exhibits the same behavior—it does not validate duration and passes negative values directly to the Windows API. If strict compatibility with CPython is desired, this behavior is expected; otherwise, add a validation check:
Proposed fix
fn Beep(args: BeepArgs, vm: &VirtualMachine) -> PyResult<()> {
if !(37..=32767).contains(&args.frequency) {
return Err(vm.new_value_error("frequency must be in 37 thru 32767".to_owned()));
}
+ if args.duration < 0 {
+ return Err(vm.new_value_error("duration must be >= 0".to_owned()));
+ }
let ok = unsafe { super::win32::Beep(args.frequency as u32, args.duration as u32) };📝 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.
| #[pyfunction] | |
| fn Beep(args: BeepArgs, vm: &VirtualMachine) -> PyResult<()> { | |
| if !(37..=32767).contains(&args.frequency) { | |
| return Err(vm.new_value_error("frequency must be in 37 thru 32767".to_owned())); | |
| } | |
| let ok = unsafe { super::win32::Beep(args.frequency as u32, args.duration as u32) }; | |
| if ok == 0 { | |
| return Err(vm.new_runtime_error("Failed to beep".to_owned())); | |
| } | |
| Ok(()) | |
| #[pyfunction] | |
| fn Beep(args: BeepArgs, vm: &VirtualMachine) -> PyResult<()> { | |
| if !(37..=32767).contains(&args.frequency) { | |
| return Err(vm.new_value_error("frequency must be in 37 thru 32767".to_owned())); | |
| } | |
| if args.duration < 0 { | |
| return Err(vm.new_value_error("duration must be >= 0".to_owned())); | |
| } | |
| let ok = unsafe { super::win32::Beep(args.frequency as u32, args.duration as u32) }; | |
| if ok == 0 { | |
| return Err(vm.new_runtime_error("Failed to beep".to_owned())); | |
| } | |
| Ok(()) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/vm/src/stdlib/winsound.rs` around lines 179 - 189, The Beep
implementation casts BeepArgs.duration (i32) to u32, allowing negative durations
to wrap to huge values; update fn Beep (and validate BeepArgs.duration) to check
duration is >= 0 before calling super::win32::Beep and return
vm.new_value_error("duration must be non-negative") (or similar) when invalid;
alternatively, if you want CPython compatibility, add a comment referencing that
choice and leave behavior unchanged. Ensure the check occurs alongside the
existing frequency validation and refer to the Beep, BeepArgs, VirtualMachine,
and win32::Beep symbols when making the change.
Summary by CodeRabbit