GeezerVison

The World Wide Web, restored to 800 pixels of glory.

GeezerVison is showing github.com — Add `_io.TextIOWrapper.detach` method by fanninpm · Pull Request #6267 · RustPython/RustPython

Conversation

fanninpm commented Nov 15, 2025 •

edited by coderabbitai Bot

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Contributor

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

New Features
- Added detach methods to text and general IO wrappers, enabling safe disconnection of underlying buffers and returning the detached buffer for reuse or transfer.
Bug Fixes
- Flush behavior standardized so pending writes are reliably flushed before detach or on explicit flush, reducing data-loss risk during buffer handoff.

_{✏️ Tip: You can customize this high-level summary in your review settings.}

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

coderabbitai Bot commented Nov 15, 2025 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Contributor

Walkthrough

Added a private flush_inner helper and a public detach(zelf, vm) method to the TextIOWrapper-like type: flush now delegates to flush_inner; detach flushes, verifies attachment, returns the underlying buffer, and clears the wrapper's buffer reference. (Under 50 words.)

Changes

Cohort / File(s) Summary

IO module — text IO detach & flush
crates/vm/src/stdlib/io.rs Added internal helper flush_inner(textio, vm) to perform close checks, mark tellable/seekable, flush pending writes and underlying buffer. Added public detach(zelf: PyRef<Self>, vm: &VirtualMachine) -> PyResult which validates not already detached, calls flush_inner, clones and returns the buffer, and clears the internal buffer reference. Updated TextIOWrapper.flush to call flush_inner instead of duplicating logic.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Verify flush ordering and error handling in flush_inner (check_closed → mark flags → flush pending → flush underlying).
Confirm detach prevents double-detach and that buffer cloning/return semantics are correct.
Inspect that clearing the internal buffer reference avoids subsequent use-after-detach and that flush now consistently delegates.
Check for any import adjustments or lint warnings in the modified file.

Poem

🐰 I nudged at the stream, gave a gentle shove,
I scrubbed every nibble, then sent it above;
I loosened the clasp, handed back the small cup,
Cleared my soft paws, then I hopped off — up and up! 🥕✨

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The pull request title accurately describes the main change: adding the `detach` method to `_io.TextIOWrapper`. It is concise, specific, and directly reflects the primary objective of the changeset.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

✨ Finishing touches

📝 Generate docstrings

🧪 Generate unit tests (beta)

Create PR with unit tests
Post copyable unit tests in a comment

📜 Recent review details

Configuration used: Path: .coderabbit.yml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 034de19 and 4b6935d.

📒 Files selected for processing (1)

crates/vm/src/stdlib/io.rs (3 hunks)

🧰 Additional context used

📓 Path-based instructions (1)

**/*.rs

📄 CodeRabbit inference engine (.github/copilot-instructions.md)

**/*.rs: Follow the default rustfmt code style by running cargo fmt to 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

⏰ 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)

GitHub Check: Run snippets and cpython tests (windows-latest)
GitHub Check: Run snippets and cpython tests on wasm-wasi
GitHub Check: Check the WASM package and demo
GitHub Check: Run snippets and cpython tests (ubuntu-latest)
GitHub Check: Run snippets and cpython tests (macos-latest)
GitHub Check: Run tests under miri
GitHub Check: Run rust tests (windows-latest)
GitHub Check: Run rust tests (ubuntu-latest)
GitHub Check: Run rust tests (macos-latest)
GitHub Check: Check Rust code with clippy
GitHub Check: Ensure compilation on various targets

🔇 Additional comments (3)

crates/vm/src/stdlib/io.rs (3)

2408-2414: LGTM! Clean helper implementation.

The flush_inner helper correctly extracts the flush logic as requested by the maintainer. It takes &mut TextIOData directly, avoiding the re-entrancy issues that plagued the initial detach implementation. The logic flow (check closed → update telling → write pending → flush buffer) is appropriate.

2450-2464: Excellent implementation of detach!

The detach method correctly addresses all the critical issues raised in previous reviews:

No re-entrancy: Uses flush_inner(&mut textio, vm) instead of calling vm.call_method(zelf.as_object(), "flush", ...), preventing the deadlock

Detached state check: Properly checks vm.is_none(&textio.buffer) upfront and returns a clear error message

Correct sequencing: Flushes while still attached, then detaches and returns the buffer

The logic flow ensures that if flush_inner fails, the buffer remains attached (which is the correct behavior).

Note: As mentioned in earlier discussions, other methods like seekable, readable, writable may still produce AttributeError after detach rather than a clear "buffer is detached" message. Adding a check_attached helper and calling it from those methods would be a nice follow-up enhancement, but that was acknowledged as optional.

2898-2901: LGTM! Good refactoring.

The flush method now correctly delegates to flush_inner, eliminating code duplication and ensuring consistent behavior between flush and detach.

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.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

fanninpm commented Nov 15, 2025

Copy link

Copy Markdown

Contributor Author

Yes, the CI failures are expected at this point. test_io will almost certainly crash due to the aforementioned todo!().

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

youknowone commented Nov 15, 2025

Copy link

Copy Markdown

Member

Checking CPython _io_TextIOWrapper_detach_impl will give hints

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

fanninpm added 2 commits

December 2, 2025 10:07


          Stub out _io.TextIOWrapper.detach method

4a2edf9


          Implement _io.TextIOWrapper.detach method

05c183f

fanninpm force-pushed the io-textiowrapper-detach branch from 051efb1 to 05c183f Compare

December 2, 2025 15:47

fanninpm marked this pull request as ready for review

December 2, 2025 16:14

coderabbitai Bot reviewed

Dec 2, 2025

View reviewed changes

coderabbitai Bot left a comment

Copy link

Copy Markdown

Contributor

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: 1

🧹 Nitpick comments (1)

crates/vm/src/stdlib/io.rs (1)

122-124: Prefer vm.ctx.none() over importing PyNone just for assignment

PyNone appears to be imported solely to set textio.buffer to None in TextIOWrapper.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 in detach, you can drop PyNone from the import list here.

📜 Review details

Configuration used: Path: .coderabbit.yml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between d287d1e and 05c183f.

📒 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 running cargo fmt to 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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

crates/vm/src/stdlib/io.rs Resolved

Uh oh!

There was an error while loading. Please reload this page.


          Make _io.TextIOWrapper.detach not re-enter lock

034de19

Co-authored-by: @coderabbitai

youknowone requested changes

Dec 3, 2025

View reviewed changes

youknowone left a comment •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Member

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.

Thanks! It is looking good and the missing detach seems blocking many tests.
I left a suggestion to refactor flush part

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

crates/vm/src/stdlib/io.rs Outdated

Comment on lines +2452 to +2455

		textio.check_closed(vm)?;
		textio.telling = textio.seekable;
		textio.write_pending(vm)?;
		vm.call_method(&textio.buffer, "flush", ())?;

youknowone Dec 3, 2025

Copy link

Copy Markdown

Member

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.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

coderabbitai[bot] reacted with eyes emoji

All reactions

👀 1 reaction

coderabbitai Bot Dec 3, 2025

Copy link

Copy Markdown

Contributor

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.

@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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions


          Refactor flushing logic to separate function

4b6935d

Co-authored-by: @coderabbitai

youknowone approved these changes

Dec 3, 2025

View reviewed changes

youknowone left a comment

Copy link

Copy Markdown

Member

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.

👍 Thank you!

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

youknowone merged commit 59f422d into RustPython:main

Dec 3, 2025

13 checks passed

Uh oh!

There was an error while loading. Please reload this page.

fanninpm deleted the io-textiowrapper-detach branch

December 4, 2025 02:34

ShaharNaveh mentioned this pull request

Dec 17, 2025

TextIOWrapper.detach #4072

Closed

coderabbitai Bot mentioned this pull request

Feb 28, 2026

Handle reentrant detach in TextIOWrapper.close() #7277

Merged

Reviewers

coderabbitai[bot]

coderabbitai[bot] left review comments

youknowone

youknowone approved these changes

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

2 participants

Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.

You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.

GeezerVison

Conversation

fanninpm commented Nov 15, 2025 • edited by coderabbitai Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary by CodeRabbit

Uh oh!

coderabbitai Bot commented Nov 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Estimated code review effort

Poem

Pre-merge checks and finishing touches

Uh oh!

fanninpm commented Nov 15, 2025

Uh oh!

youknowone commented Nov 15, 2025

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

youknowone left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

youknowone Dec 3, 2025

Choose a reason for hiding this comment

Uh oh!

coderabbitai Bot Dec 3, 2025

Choose a reason for hiding this comment

Uh oh!

youknowone left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

fanninpm commented Nov 15, 2025 •

edited by coderabbitai Bot

Loading

Uh oh!

There was an error while loading. Please reload this page.

coderabbitai Bot commented Nov 15, 2025 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

youknowone left a comment •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.