Windows: msvcrt, winreg follow-ups (os.utime, text-mode CRLF) - #806
Conversation
Replace the no-op stub with a real utime: parse (path, times=None, *, ns=None, dir_fd=None, follow_symlinks=True), convert times/ns to a Duration (both None means now), and call host_env set_file_times on Windows / set_file_times_at on unix.
TextIOWrapper.write now substitutes the write newline for '\n' when newline is None (Windows: '\r\n'; unix: no translation), matching the writenl rule. Explicit '\r'/'\r\n' were already handled; '' and '\n' stay verbatim.
… import msvcrt: new module over host_env::msvcrt (getch/getwch family, putch, ungetch, kbhit, locking, setmode, open_osfhandle, get_osfhandle, heapmin, SetErrorMode; LK_*/SEM_* constants). winreg: install real key operations over host_env::winreg behind a PyHKEY handle object (Open/Create/Close/Enum/Query/Set/Delete key and value, connect_registry, expand_environment_strings, reflection). The predefined HKEY_* roots expose the full sign-extended pointer values. _winapi: add the process/priority/GetStdHandle constants and the CloseHandle/WaitForSingleObject/GetExitCodeProcess functions that subprocess captures as default arguments at import once msvcrt exists. Cargo: enable windows-sys Win32_System_Threading and add the widestring dependency the winreg wrappers take.
WalkthroughThe interpreter adds Windows ChangesPlatform standard-library support
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant PythonCaller
participant WinregModule
participant HostWinreg
PythonCaller->>WinregModule: OpenKey or SetValueEx
WinregModule->>HostWinreg: invoke Win32 registry operation
HostWinreg-->>WinregModule: return handle, data, or error
WinregModule-->>PythonCaller: return PyHKEY, converted value, or PyError
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d48c45790e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| host_reg::REG_MULTI_SZ => { | ||
| let len = unsafe { w_list_len(value) }; |
There was a problem hiding this comment.
When SetValueEx receives REG_MULTI_SZ with None or any non-list value, this calls the unsafe w_list_len accessor on an object that is not a W_ListObject; its documented precondition is violated, so malformed Python input can cause invalid layout reads or crash the interpreter instead of raising TypeError (and None should encode an empty value). Check the value type before using list internals.
AGENTS.md reference: AGENTS.md:L194-L195
Useful? React with 👍 / 👎.
| host_reg::REG_SZ | host_reg::REG_EXPAND_SZ => { | ||
| if unsafe { !is_str(value) } { | ||
| return Err(crate::PyError::type_error("value must be a string")); | ||
| } | ||
| Ok(encode_utf16z(unsafe { w_str_get_value(value) })) |
There was a problem hiding this comment.
For a REG_SZ or REG_EXPAND_SZ value containing a lone surrogate, w_str_get_value panics because the backing WTF-8 is not valid Rust UTF-8; such values are explicitly exercised by the upstream winreg tests and must round-trip with surrogate-preserving UTF-16 rather than terminating the interpreter. Use the surrogate-aware WTF-8 accessor/conversion here.
AGENTS.md reference: AGENTS.md:L194-L195
Useful? React with 👍 / 👎.
| host_reg::REG_QWORD => Ok((crate::baseobjspace::int_w(value)? as u64) | ||
| .to_le_bytes() | ||
| .to_vec()), |
There was a problem hiding this comment.
Valid registry QWORDs above i64::MAX, such as the upstream test value 0xFEDCBA9876543210, cannot be written because int_w raises OverflowError; the paired read path also casts these values to i64 and returns a negative Python integer. Convert through an unsigned 64-bit/bigint representation so the complete REG_QWORD range round-trips.
AGENTS.md reference: AGENTS.md:L194-L195
Useful? React with 👍 / 👎.
| for chunk in units.split(|&c| c == 0) { | ||
| if chunk.is_empty() { | ||
| break; |
There was a problem hiding this comment.
A valid REG_MULTI_SZ may contain empty elements, but this loop treats the first empty chunk as the terminator; consequently values such as ["", "", ""], covered by both bundled winreg test suites, are read back as an empty list. Only the final terminator should be removed while preserving interior and leading empty strings.
AGENTS.md reference: AGENTS.md:L194-L195
Useful? React with 👍 / 👎.
| crate::py_class! { | ||
| "PyHKEY", | ||
| methods: { |
There was a problem hiding this comment.
Handles returned by OpenKey, CreateKey, and ConnectRegistry are never closed when their wrapper becomes unreachable because this class registers no finalizer; code that relies on normal PyHKEY ownership semantics will therefore leak one Windows registry handle per object and can exhaust handles in a long-running process. The corresponding PyPy W_HKEY registers _finalize_ to call Close, so the port needs equivalent automatic cleanup.
AGENTS.md reference: AGENTS.md:L194-L195
Useful? React with 👍 / 👎.
| pos.len() | ||
| ))); | ||
| } | ||
| crate::builtins::kwarg_reject_unknown(kwargs, &["ns", "dir_fd", "follow_symlinks"], "utime")?; |
There was a problem hiding this comment.
The public signature allows os.utime(path, times=(atime, mtime)), and the bundled test_os.py exercises exactly that call, but times is omitted from the accepted keyword list and is rejected as unexpected before parsing. Add it as a positional-or-keyword slot while still detecting duplicate positional and keyword values.
AGENTS.md reference: AGENTS.md:L194-L195
Useful? React with 👍 / 👎.
| let dur_from_secs = |v: PyObjectRef| -> Result<std::time::Duration, crate::PyError> { | ||
| let f = crate::builtins::builtin_float(&[v])?; | ||
| let secs = unsafe { pyre_object::w_float_get_value(f) }; | ||
| std::time::Duration::try_from_secs_f64(secs) | ||
| .map_err(|_| crate::PyError::value_error("utime: timestamp out of range")) |
There was a problem hiding this comment.
When callers set a legitimate pre-1970 timestamp, Duration::try_from_secs_f64 rejects the negative value (and the ns path explicitly rejects negative nanoseconds), although os.utime and the underlying signed timespec support dates before the epoch. Represent timestamps as signed seconds plus nanoseconds, as the corresponding PyPy conversion does, rather than as an unsigned Duration.
AGENTS.md reference: AGENTS.md:L194-L195
Useful? React with 👍 / 👎.
🤖 Codex parity reviewStatic analysis of this diff vs the local RPython/PyPy sources (commit d48c457). Files in the reviewed diff1. Regressions to PyPy parity introduced by this patchNone. 2. Other mismatches introduced by this patch
3. Pre-existing mismatches (already present before this patch)
4. Structural adaptations
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pyre/pyre-interpreter/src/module/_winapi/mod.rs`:
- Around line 26-32: Update last_os_error() to preserve the raw native
GetLastError() value when constructing the OSError, while retaining any POSIX
errno mapping needed for compatibility. Ensure failures from CloseHandle and
GetExitCodeProcess expose the original Windows code through winerror, matching
PyErr_SetFromWindowsErr semantics.
In `@pyre/pyre-interpreter/src/module/posix/interp_posix.rs`:
- Around line 1389-1403: Align the advertised `os.supports_fd` capabilities with
`utime_impl`: either add integer file-descriptor handling that routes through
`futimens`/`futimes`, or remove `HAVE_FUTIMENS` and `HAVE_FUTIMES` from
`_have_functions` so `os.utime` is no longer advertised as fd-capable.
- Around line 1412-1422: Extract the duplicated dir_fd parsing into a shared
helper near rename_impl, preserving None handling, integer conversion, and the
existing type-name lookup for TypeError messages. Update both rename_impl and
the shown caller to use this helper with their respective keyword name, removing
the local kwarg_get/present validation blocks.
- Around line 1437-1449: Update the utime timestamp conversion around
dur_from_secs and dur_from_ns to preserve negative, pre-1970 timestamps instead
of rejecting them. Replace the unsigned Duration representation throughout the
related host set_file_times* call path with a signed offset or equivalent
signed-seconds/nanoseconds representation, while retaining range validation and
existing handling for nonnegative timestamps.
- Around line 1465-1471: Update the `(None, None)` branch in the `os.utime`
timestamp handling to use the kernel/host time-setting API’s `UTIME_NOW`
representation when supported, instead of calling `SystemTime::now()` and
constructing a `Duration`. Preserve the existing explicit timestamp behavior for
all other cases.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 165a97b7-5f23-49c4-9583-ec3877e98b92
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (8)
pyre/pyre-interpreter/Cargo.tomlpyre/pyre-interpreter/src/importing.rspyre/pyre-interpreter/src/module/_io/textio.rspyre/pyre-interpreter/src/module/_winapi/mod.rspyre/pyre-interpreter/src/module/mod.rspyre/pyre-interpreter/src/module/msvcrt/mod.rspyre/pyre-interpreter/src/module/posix/interp_posix.rspyre/pyre-interpreter/src/module/winreg/mod.rs
| /// Map the current thread's last OS error to an `OSError`. | ||
| fn last_os_error() -> crate::PyError { | ||
| crate::PyError::os_error_syscall( | ||
| crate::builtins::io_error_posix_errno(&std::io::Error::last_os_error(), 0), | ||
| pyre_object::PY_NULL, | ||
| ) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
Does CPython's _winapi.CloseHandle/GetExitCodeProcess raise OSError with winerror set to the raw Win32 error code?
💡 Result:
Yes, CPython's _winapi.CloseHandle and _winapi.GetExitCodeProcess functions raise OSError when the underlying Win32 API call fails, and this exception includes the raw Windows error code [1][2]. When these functions encounter a failure, they typically use PyErr_SetFromWindowsErr or similar internal CPython helpers [3][4][5]. This mechanism performs the following actions: 1. It retrieves the current Win32 error code using GetLastError [3][5]. 2. It constructs an OSError instance where the winerror attribute is set to the raw Windows error code (e.g., 6 for ERROR_INVALID_HANDLE) [1][2]. 3. It maps this Windows error code to a corresponding POSIX-style errno value, which is stored in the errno attribute of the OSError [6][7]. For example, if _winapi.CloseHandle is called with an invalid handle, it fails with ERROR_INVALID_HANDLE (code 6), resulting in an OSError with [WinError 6] The handle is invalid [1][2]. While the exception message and errno attribute reflect a POSIX-style translation, the winerror attribute provides access to the original Win32 error code [6][7].
Citations:
- 1: asyncio: _ProactorBasePipeTransport._call_connection_lost leaks OSError [WinError 6] from PipeHandle.close() on Windows python/cpython#149388
- 2: https://stackoverflow.com/questions/47535676/oserror-winerror-6-the-handle-is-invalid-when-calling-subprocess-from-python
- 3: https://github.com/python/cpython/blob/v3.10.2/Modules/_winapi.c
- 4: https://github.com/python/cpython/blob/b8f4163da30e16c7cd58fe04f4b17e38d53cd57e/Modules/_winapi.c
- 5: https://github.com/python/cpython/blob/master/Modules/_winapi.c
- 6: ctypes.WinError & OSError python/cpython#67150
- 7: https://github.com/python/cpython/blob/master/PC/errmap.h
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 'Files matching _winapi/mod.rs:\n'
fd -a 'mod.rs$' . | grep 'pyre-interpreter/src/module/_winapi/mod.rs' || true
printf '\nRelevant source around last_os_error usages:\n'
rg -n "last_os_error|io_error_posix_errno|os_error_syscall|CloseHandle|GetExitCodeProcess|WAIT_TIMEOUT" pyre/pyre-interpreter/src/module/_winapi/mod.rs -C 4
printf '\nDefinitions/usages of WinAPI error helpers:\n'
rg -n "io_error_posix_errno|os_error_syscall|last_os_error|Win32|WindowsError|winerror|errno" pyre/pyre-interpreter/src | head -120Repository: youknowone/pyre
Length of output: 2014
🌐 Web query:
CPython Modules _winapi.c CloseHandle GetLastError PyErr_SetFromWindowsErr OSError winerror
💡 Result:
In the CPython source code, specifically within the Modules/_winapi.c module, the _winapi.CloseHandle function provides a direct wrapper around the Win32 API CloseHandle function [1][2][3]. The implementation in _winapi_CloseHandle_impl works as follows: 1. It calls CloseHandle(handle) wrapped within Py_BEGIN_ALLOW_THREADS and Py_END_ALLOW_THREADS to release the GIL during the potentially blocking system call [1][2][3]. 2. If CloseHandle fails (i.e., returns FALSE), the function calls PyErr_SetFromWindowsErr(0) [1][2][3]. The PyErr_SetFromWindowsErr(int ierr) function is a CPython C-API convenience function that raises an OSError [4][5]. By passing 0 as the ierr argument, CPython automatically invokes the Win32 GetLastError() function to retrieve the most recent error code [4][5]. It then formats this error into an OSError exception where: - The winerror attribute is set to the error code returned by GetLastError() [4][5]. - The strerror attribute contains the descriptive error message obtained via the Win32 FormatMessage function [4][5]. A common manifestation of this behavior is OSError: [WinError 6] The handle is invalid, which occurs when CloseHandle is called on a handle that has already been closed or is otherwise invalid [6][7]. Because this is implemented as a standard exception, it propagates up the call stack unless explicitly caught by the Python code initiating the handle operation [6]. Top results: [1], [4], [6]
Citations:
- 1: https://github.com/python/cpython/blob/master/Modules/_winapi.c
- 2: https://github.com/python/cpython/blob/v3.10.2/Modules/_winapi.c
- 3: https://github.com/python/cpython/blob/b8f4163da30e16c7cd58fe04f4b17e38d53cd57e/Modules/_winapi.c
- 4: https://docs.python.org/3/c-api/exceptions.html
- 5: https://github.com/python/cpython/blob/main/Doc/c-api/exceptions.rst
- 6: asyncio: _ProactorBasePipeTransport._call_connection_lost leaks OSError [WinError 6] from PipeHandle.close() on Windows python/cpython#149388
- 7: https://bugs.python.org/issue36067
🌐 Web query:
CPython Modules _winapi.c GetExitCodeProcess GetLastError PyErr_SetFromWindowsErr OSError winerror
💡 Result:
In CPython, the _winapi module provides a thin wrapper around the Windows API function GetExitCodeProcess [1][2]. When _winapi.GetExitCodeProcess is called from Python, it executes the following C logic defined in Modules/_winapi.c [1][3]: 1. It invokes the Win32 API function GetExitCodeProcess [1][4]. 2. If the function fails (returning zero), it retrieves the extended error information using the Win32 GetLastError function [1][4]. 3. It then calls PyErr_SetFromWindowsErr(GetLastError), which raises an OSError exception [1][5]. This convenience function automatically translates the Windows error code into a Python OSError with the winerror attribute populated [5][6]. 4. On failure, the function also sets the return value to a sentinel (PY_DWORD_MAX) [1][7]. The PyErr_SetFromWindowsErr function is a C-API utility that formats the Windows error message using the system's FormatMessage function and constructs the OSError instance with the appropriate winerror and strerror attributes [5][6]. This ensures that when the underlying Windows API fails—for example, due to an invalid handle—the Python caller receives a descriptive exception rather than an silent failure or invalid return value [5][8].
Citations:
- 1: https://github.com/python/cpython/blob/master/Modules/_winapi.c
- 2: https://github.com/python/cpython/blob/v3.10.2/Modules/_winapi.c
- 3: https://github.com/python/cpython/blob/48b069a003ba6c684a9ba78493fbbec5e89f10b8/Modules/_winapi.c
- 4: https://learn.microsoft.com/en-us/windows/win32/api/processthreadsapi/nf-processthreadsapi-getexitcodeprocess
- 5: https://docs.python.org/3/c-api/exceptions.html
- 6: https://github.com/python/cpython/blob/main/Doc/c-api/exceptions.rst
- 7: https://github.com/python/cpython/blob/b8f4163da30e16c7cd58fe04f4b17e38d53cd57e/Modules/_winapi.c
- 8: https://bugs.python.org/issue36067
Preserve the native Win32 error code in _winapi failures.
last_os_error() converts every failure to a POSIX errno before raising OSError, so CloseHandle and GetExitCodeProcess failures lose the original GetLastError() value. Windows OSError/PyErr_SetFromWindowsErr semantics expose that native code via winerror; keep the raw Windows error alongside any POSIX mapping for these calls.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pyre/pyre-interpreter/src/module/_winapi/mod.rs` around lines 26 - 32, Update
last_os_error() to preserve the raw native GetLastError() value when
constructing the OSError, while retaining any POSIX errno mapping needed for
compatibility. Ensure failures from CloseHandle and GetExitCodeProcess expose
the original Windows code through winerror, matching PyErr_SetFromWindowsErr
semantics.
| fn utime_impl(args: &[PyObjectRef]) -> Result<PyObjectRef, crate::PyError> { | ||
| let (pos, kwargs) = crate::builtins::split_builtin_kwargs(args); | ||
| if pos.is_empty() { | ||
| return Err(crate::PyError::type_error( | ||
| "utime() missing required argument 'path' (pos 1)", | ||
| )); | ||
| } | ||
| if pos.len() > 2 { | ||
| return Err(crate::PyError::type_error(format!( | ||
| "utime() takes from 1 to 2 positional arguments but {} were given", | ||
| pos.len() | ||
| ))); | ||
| } | ||
| crate::builtins::kwarg_reject_unknown(kwargs, &["ns", "dir_fd", "follow_symlinks"], "utime")?; | ||
| let path = extract_path(pos[0])?; |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
utime now rejects fd arguments while _have_functions still advertises HAVE_FUTIMENS/HAVE_FUTIMES.
Lines 589-590 keep both macros in _have_functions, so os.py puts os.utime into os.supports_fd. utime_impl unconditionally runs extract_path(pos[0]), so a caller that follows that capability bit (e.g. shutil.copystat on an open fd) now gets a TypeError instead of the previous silent no-op. Either accept an integer path and route to futimens, or drop the two macros from _have_functions.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pyre/pyre-interpreter/src/module/posix/interp_posix.rs` around lines 1389 -
1403, Align the advertised `os.supports_fd` capabilities with `utime_impl`:
either add integer file-descriptor handling that routes through
`futimens`/`futimes`, or remove `HAVE_FUTIMENS` and `HAVE_FUTIMES` from
`_have_functions` so `os.utime` is no longer advertised as fd-capable.
| let dir_fd = match crate::builtins::kwarg_get(kwargs, "dir_fd").and_then(present) { | ||
| Some(v) => { | ||
| if !unsafe { pyre_object::is_int(v) } { | ||
| return Err(crate::PyError::type_error( | ||
| "argument should be integer or None, not object", | ||
| )); | ||
| } | ||
| Some(unsafe { pyre_object::w_int_get_value(v) } as i32) | ||
| } | ||
| None => None, | ||
| }; |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Reuse rename_impl's dir_fd extractor instead of duplicating it with a degraded message.
This block repeats the dir_fd logic from rename_impl (Lines 1324-1339) but hardcodes not object where the other site reports the real type name. Extract a shared helper so the TypeError text stays consistent.
♻️ Suggested shared helper
fn kwarg_dir_fd(
kwargs: &[(&str, PyObjectRef)],
name: &str,
) -> Result<Option<i32>, crate::PyError> {
match crate::builtins::kwarg_get(kwargs, name) {
Some(v) if !unsafe { pyre_object::is_none(v) } => {
if !unsafe { pyre_object::is_int(v) } {
let type_name = crate::typedef::r#type(v)
.map(|t| unsafe { pyre_object::typeobject::w_type_get_name(t.as_ptr()) })
.unwrap_or("object");
return Err(crate::PyError::type_error(format!(
"argument should be integer or None, not {type_name}"
)));
}
Ok(Some((unsafe { pyre_object::w_int_get_value(v) }) as i32))
}
_ => Ok(None),
}
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pyre/pyre-interpreter/src/module/posix/interp_posix.rs` around lines 1412 -
1422, Extract the duplicated dir_fd parsing into a shared helper near
rename_impl, preserving None handling, integer conversion, and the existing
type-name lookup for TypeError messages. Update both rename_impl and the shown
caller to use this helper with their respective keyword name, removing the local
kwarg_get/present validation blocks.
| let dur_from_secs = |v: PyObjectRef| -> Result<std::time::Duration, crate::PyError> { | ||
| let f = crate::builtins::builtin_float(&[v])?; | ||
| let secs = unsafe { pyre_object::w_float_get_value(f) }; | ||
| std::time::Duration::try_from_secs_f64(secs) | ||
| .map_err(|_| crate::PyError::value_error("utime: timestamp out of range")) | ||
| }; | ||
| let dur_from_ns = |v: PyObjectRef| -> Result<std::time::Duration, crate::PyError> { | ||
| let n = crate::builtins::space_index_w(v)?; | ||
| if n < 0 { | ||
| return Err(crate::PyError::value_error("utime: timestamp out of range")); | ||
| } | ||
| Ok(std::time::Duration::from_nanos(n as u64)) | ||
| }; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Pre-1970 timestamps are rejected instead of applied.
Duration is unsigned, so dur_from_secs fails try_from_secs_f64 for any negative times value and dur_from_ns explicitly rejects n < 0. CPython/PyPy accept negative timestamps (os.utime(p, (-1, -1)) sets 1969-12-31 on Linux and Windows), and callers such as tarfile/shutil restoring old archives will now see a spurious ValueError: utime: timestamp out of range. The host set_file_times* signatures need a signed offset (or an (i64 secs, u32 nsec) pair) to represent this.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pyre/pyre-interpreter/src/module/posix/interp_posix.rs` around lines 1437 -
1449, Update the utime timestamp conversion around dur_from_secs and dur_from_ns
to preserve negative, pre-1970 timestamps instead of rejecting them. Replace the
unsigned Duration representation throughout the related host set_file_times*
call path with a signed offset or equivalent signed-seconds/nanoseconds
representation, while retaining range validation and existing handling for
nonnegative timestamps.
| (None, None) => { | ||
| let now = std::time::SystemTime::now() | ||
| .duration_since(std::time::UNIX_EPOCH) | ||
| .unwrap_or(std::time::Duration::ZERO); | ||
| (now, now) | ||
| } | ||
| }; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔵 Trivial | ⚖️ Poor tradeoff
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Locate the host_env timestamp APIs and check for a UTIME_NOW / Option<Duration> variant.
fd -t f -e rs . --exec rg -nP -C6 '\bfn\s+set_file_times(_at)?\s*[(<]' {} \;Repository: youknowone/pyre
Length of output: 2043
🏁 Script executed:
#!/bin/bash
set -u
echo "Repository present:"
pwd
git status --short | head -20 || true
echo
echo "Find target file candidates:"
fd -t f 'interp_posix\.rs$' . || find . -path '*posix*interp_posix.rs' -print
echo
echo "Find host_env files by name:"
fd -t f -i 'host.*env|posix.*env|set_file_times' . || true
echo
echo "Search for set_file_times symbols in tracked rs files:"
git ls-files '*.rs' | xargs rg -n -C5 'set_file_times|FILE_TIMES|times_at|times = None' || trueRepository: youknowone/pyre
Length of output: 2014
🌐 Web query:
CPython os.utime times=None ns=None utimensat UTIME_NOW
💡 Result:
In CPython, when calling os.utime(path, times=None, ns=None), the default behavior is to set both the access and modification times of the file to the current time [1]. This operation is functionally equivalent to the Unix touch command [1]. Regarding the implementation details and the use of utimensat with UTIME_NOW: 1. Mechanism: When times=None and ns=None are passed (or when neither argument is provided), CPython's internal implementation (located in Modules/posixmodule.c) attempts to use the most precise system-level API available on the platform [2][3]. 2. Utimensat usage: On modern POSIX-compliant systems where utimensat is available, CPython often utilizes it to perform the operation [2][3]. When the times are meant to be set to the "current time," the internal C implementation may use the UTIME_NOW constant (specifically for the tv_nsec field of the timespec structure) [4][5][3]. 3. Evolution: Historically, CPython used the older utime or utimes system calls, which had microsecond resolution [2][3]. Since Python 3.3, support was added for higher-resolution nanosecond timestamps, and the implementation was updated to prefer utimensat and related functions that support these nanosecond-precision flags (UTIME_NOW and UTIME_OMIT) where supported by the operating system [2][3]. 4. API Constraints: It is important to note that ns=None is not a valid argument; if the ns keyword is used, it must be a 2-tuple of integers representing nanoseconds [6]. The times=None behavior is the standard mechanism to trigger the "set to now" functionality [1]. In summary, when you invoke os.utime(path) or os.utime(path, times=None), CPython effectively triggers a "set to current time" operation that leverages UTIME_NOW via utimensat on supported platforms [1][5][3].
Citations:
- 1: https://hg.python.org/cpython/rev/bba131e48852
- 2: https://bugs.python.org/issue12904
- 3: https://mail.python.org/pipermail/python-checkins/2011-September/107782.html
- 4: add st_*time_ns fields to os.stat(), add ns keyword to os.*utime*(), os.*utimens*() expects a number of nanoseconds python/cpython#58335
- 5: add st_*time_ns fields to os.stat(), add ns keyword to os.*utime*(), os.*utimens*() expects a number of nanoseconds python/cpython#58335
- 6: https://bugs.python.org/issue15382
Route the both-None case through a kernel-supplied timestamp.
os.utime(path, times=None, ns=None) is intended to let the filesystem supply the timestamp rather than baking SystemTime::now() into the filesystem metadata. If the host time-setting API can express UTIME_NOW, pass that here instead of reading SystemTime::now() and clamping Duration::ZERO from epoch.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pyre/pyre-interpreter/src/module/posix/interp_posix.rs` around lines 1465 -
1471, Update the `(None, None)` branch in the `os.utime` timestamp handling to
use the kernel/host time-setting API’s `UTIME_NOW` representation when
supported, instead of calling `SystemTime::now()` and constructing a `Duration`.
Preserve the existing explicit timestamp behavior for all other cases.
1 participant
Follow-ups to the nt module (#780).
os.utime(was a no-op stub) — parses(path, times=None, *, ns=None, dir_fd=None, follow_symlinks=True), converts times/ns to aDuration(bothNone= now), and calls host_envset_file_times(Windows) /set_file_times_at(unix).\nto the platform newline whennewline=None(Windows\r\n), matching the writenl rule; explicit\r/\r\nwere already handled,''/\nstay verbatim.host_env::msvcrt(getch/getwch family, putch, ungetch, kbhit, locking, setmode, open_osfhandle, get_osfhandle, heapmin, SetErrorMode).host_env::winregbehind aPyHKEYhandle object; predefinedHKEY_*roots expose the full sign-extended pointer values.CloseHandle/WaitForSingleObject/GetExitCodeProcessthatsubprocesscaptures as default args at import oncemsvcrtexists.All OS bindings go through
rustpython_host_env. Verified locally:check.py --backend dynasm322/322, JIT on/off smoke test for all four features passes.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
msvcrt, including console, file-descriptor, locking, and error-mode operations.os.utimesupport for updating file timestamps.Bug Fixes