◐ Shell
clean mode source ↗

update _pyio, test_fileio from 3.13.11 and impl more io features by youknowone · Pull Request #6560 · RustPython/RustPython

youknowone

216-244: LGTM: Well-designed EINTR trap pattern.

The trap_eintr function correctly implements PEP 475 semantics by:

  • Detecting OSError with errno == EINTR
  • Calling vm.check_signals() to handle pending signals
  • Returning Ok(None) to indicate retry needed

The WASM no-op version is appropriate since EINTR is not relevant in that environment.

693-712: LGTM: Proper bounds validation added.

The bounds check for readinto return values prevents potential issues from invalid implementations. The validation ensures the returned size is within [0, size] and provides a clear error message on violation.

723-730: LGTM: Correct EINTR retry pattern.

The EINTR-aware retry loop properly implements PEP 475 for the readall method, ensuring interrupted reads are automatically retried after signal handling.

944-957: LGTM: Proper BlockingIOError construction.

The BlockingIOError is correctly constructed with:

  • EAGAIN errno
  • Descriptive message
  • characters_written attribute set to 0

This matches CPython's behavior for non-blocking write failures.

1149-1159: LGTM: Correct partial write tracking.

The BlockingIOError construction properly tracks characters_written as written + buffer_len, allowing callers to know exactly how much data was buffered before the operation would block.

1224-1234: Verify loop condition change.

The while remaining > 0 && !self.buffer.is_empty() condition at line 1224 appears to add a buffer emptiness check. Ensure this aligns with the intended logic - specifically that the loop should exit when the buffer is empty, even if remaining > 0.

Based on the surrounding context, this appears correct, but confirming the behavior matches CPython would be helpful.

1295-1331: LGTM: Excellent EINTR handling with proper cleanup.

The EINTR-aware retry loops are correctly implemented in both branches of raw_read. Particularly good is the comment and code ensuring the buffer is always restored, even when an error occurs.

1336-1349: Excellent error handling with cause chain.

The error handling for invalid readinto return values is well-designed:

  • Attempts to convert to isize, capturing conversion errors
  • Constructs an informative OSError about the invalid return
  • Chains the original TypeError as the cause

This provides clear error context for debugging.

1391-1398: LGTM: Consistent EINTR handling.

The EINTR retry loop in read_all is consistent with the pattern used throughout this file.

1669-1672: LGTM: Proper writable check for truncate.

The check ensures truncate is only called on writable files, preventing invalid operations with a clear error message.

2506-2529: LGTM: Essential null character validation.

The embedded null character checks prevent issues when these strings are used with C APIs:

  • Line 2508: Checks encoding string for \0
  • Line 2527: Checks errors string using as_wtf8().as_bytes().contains(&0) to handle surrogates correctly

This is a critical security/correctness measure.

2635-2643: LGTM: Representable trait addition.

Adding Representable to the with clause enables the custom repr_str implementation for TextIOWrapper.

3520-3558: Excellent repr implementation with robust error handling.

The repr_str implementation for TextIOWrapper is well-designed:

  • Handles reentrant calls gracefully (line 3524-3527)
  • Checks for uninitialized state (line 3528-3530)
  • Safely retrieves optional attributes using get_attribute_opt
  • Includes relevant information: type name, name, mode, encoding

The use of let-chains for attribute extraction is clean and idiomatic.

4637-4638: LGTM: _blksize field addition.

The blksize field is properly added as AtomicCell<i64> with a sensible default of 8192 bytes (DEFAULT_BUFFER_SIZE). This aligns with storing st_blksize from file stat information.

Also applies to: 4660-4660

4673-4681: Good defensive warning for bool-as-FD.

The warning when a bool is passed as a file descriptor helps catch a common Python gotcha where bool is a subtype of int. Since using True/False as file descriptors (1/0) is almost certainly unintentional, this warning is valuable.

4732-4785: Excellent resource management for user-provided FDs.

The fd_is_own tracking properly prevents closing user-provided file descriptors when initialization fails. This is critical for correct resource management:

  • Line 4732: Tracks ownership based on whether FD was passed by user
  • Lines 4753-4755, 4767-4770, 4779-4785: On error paths, sets zelf.fd to -1 if FD is not owned, preventing cleanup
  • Lines 4759-4763: Stores st_blksize from stat for Unix systems

This prevents closing FDs that the caller is still responsible for.

4795-4813: LGTM: Clean FileIO repr implementation.

The repr implementation for FileIO is well-structured and informative:

  • Handles closed file state
  • Includes type name, name/fd, mode, and closefd flag
  • Consistent with the TextIOWrapper repr style

4844-4847: LGTM: Simple _blksize getter.

The getter implementation is straightforward and correctly loads the value from the AtomicCell.

4865-4878: LGTM: Improved readable/writable with error checking.

Changing the return type from bool to PyResult<bool> allows these methods to properly report errors when the file is closed, rather than silently returning false. This improves error reporting clarity.

4921-4951: LGTM: Comprehensive EINTR handling in read.

Both branches of the read method properly implement PEP 475:

  • Lines 4922-4931: Sized read with EINTR retry
  • Lines 4937-4946: Read-to-end with EINTR retry

Both use the correct pattern: check for EINTR, call check_signals(), and retry.

4966-4977: LGTM: Consistent EINTR handling in readinto.

The readinto method follows the same EINTR retry pattern as read, ensuring interrupted system calls are automatically retried per PEP 475.

12-19: The hardcoded EAGAIN value of 11 does not apply to WASI targets. The cfg condition any(not(target_arch = "wasm32"), target_os = "wasi") explicitly includes WASI targets, which will use libc::EAGAIN (value 6 per WASI specification). The hardcoded 11 only applies to pure WASM targets without WASI. For pure WASM environments, a placeholder errno value is acceptable since errno semantics don't apply.

Likely an incorrect or invalid review comment.