Repository navigation
Add _io.TextIOWrapper.detach method - #6267
Conversation
WalkthroughAdded a private Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yml Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used📓 Path-based instructions (1)**/*.rs📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
🔇 Additional comments (3)
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 |
|
Yes, the CI failures are expected at this point. |
|
Checking CPython _io_TextIOWrapper_detach_impl will give hints |
051efb1 to
05c183f
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
crates/vm/src/stdlib/io.rs (1)
122-124: Prefervm.ctx.none()over importingPyNonejust for assignment
PyNoneappears to be imported solely to settextio.buffertoNoneinTextIOWrapper.detach. That can usually be done via the context singleton (vm.ctx.none()), avoiding the extra builtins import and staying consistent with the rest of this module.If you adopt the
vm.ctx.none()pattern indetach, you can dropPyNonefrom the import list here.
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
crates/vm/src/stdlib/io.rs(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.rs: Follow the default rustfmt code style by runningcargo fmtto format Rust code
Always run clippy to lint Rust code (cargo clippy) before completing tasks and fix any warnings or lints introduced by changes
Follow Rust best practices for error handling and memory management
Use the macro system (pyclass,pymodule,pyfunction, etc.) when implementing Python functionality in Rust
Files:
crates/vm/src/stdlib/io.rs
Co-authored-by: @coderabbitai
| textio.check_closed(vm)?; | ||
| textio.telling = textio.seekable; | ||
| textio.write_pending(vm)?; | ||
| vm.call_method(&textio.buffer, "flush", ())?; |
There was a problem hiding this comment.
Could you create fn flush_inner(textio: ..., vm: ...) -> PyResult to share the actual flushing between methods?
@coderabbitai please advice about the nominal type of textio
There was a problem hiding this comment.
@youknowone The nominal type for textio in the flush_inner helper function should be &mut TextIOData.
Looking at the code, both flush and detach methods call self.lock(vm)? which returns PyMappedThreadMutexGuard<'_, TextIOData>. This guard derefs to TextIOData, so you can pass it as a mutable reference to the helper function.
Here's the suggested signature:
fn flush_inner(textio: &mut TextIOData, vm: &VirtualMachine) -> PyResult {
textio.check_closed(vm)?;
textio.telling = textio.seekable;
textio.write_pending(vm)?;
vm.call_method(&textio.buffer, "flush", ())
}Then both methods can call it like:
#[pymethod]
fn flush(&self, vm: &VirtualMachine) -> PyResult {
let mut textio = self.lock(vm)?;
flush_inner(&mut textio, vm)
}
#[pymethod]
fn detach(zelf: PyRef<Self>, vm: &VirtualMachine) -> PyResult {
let mut textio = zelf.lock(vm)?;
// Fail fast if already detached
if vm.is_none(&textio.buffer) {
return Err(vm.new_value_error("underlying buffer has been detached"));
}
flush_inner(&mut textio, vm)?;
let buffer = textio.buffer.clone();
textio.buffer = vm.ctx.none();
Ok(buffer)
}This approach cleanly separates the locking concern from the flushing logic and avoids code duplication.
Co-authored-by: @coderabbitai
As I was trying to get the regression test data uploaded to the website (it hasn't been uploaded to the website since February 2025), I ran into an issue where this method wasn't implemented. So far, I've stubbed out a dummy implementation that panics whenever this is called. Does anyone have any ideas on how to implement this method properly?
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.