GeezerVison

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

GeezerVison is showing github.com — Fix test_io on windows by youknowone · Pull Request #6387 · RustPython/RustPython

Conversation

youknowone commented Dec 9, 2025 •

edited by coderabbitai Bot

Loading

Uh oh!

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

Copy link

Copy Markdown

Member

Summary by CodeRabbit

New Features
- File I/O objects now properly clean up resources when destroyed.
- TextIOWrapper now supports iteration operations.

_{✏️ 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 Dec 9, 2025 •

edited

Loading

Uh oh!

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

Copy link

Copy Markdown

Contributor

Caution

Review failed

The pull request is closed.

Walkthrough

This pull request adds Destructor trait implementations to six IO classes in RustPython's stdlib module, enabling proper resource cleanup when these objects are garbage collected. Each class now declares Destructor in its pyclass declaration and implements slot_del to call close(). TextIOWrapper additionally gains Iterable and IterNext implementations.

Changes

Cohort / File(s)	Summary
Destructor trait additions to IO classes `crates/vm/src/stdlib/io.rs`	Added Destructor to pyclass declarations for `BufferedReader`, `BufferedWriter`, `BufferedRandom`, `BufferedRWPair`, and `FileIO`. Each now includes corresponding Destructor impl block with `slot_del` invoking `close()`.
TextIOWrapper enhancement `crates/vm/src/stdlib/io.rs`	Extended `TextIOWrapper` pyclass declaration to include Destructor, Iterable, and IterNext. Implemented matching Destructor and iteration trait blocks.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

Highly repetitive pattern applied consistently across six IO classes (BufferedReader, BufferedWriter, BufferedRandom, BufferedRWPair, TextIOWrapper, FileIO)
Straightforward destructor logic that invokes close() via slot_del
TextIOWrapper adds Iterable/IterNext implementations alongside Destructor

Areas requiring extra attention:

Verify each Destructor impl properly chains to close() without resource leaks or double-close scenarios
Confirm TextIOWrapper's Iterable/IterNext implementations align with CPython iteration semantics
Check that del paths marked as unreachable are indeed unreachable after slot_del implementation

Possibly related PRs

RustPython/RustPython#6387: Directly modifies the same io.rs pyclass declarations, adding Destructor and related trait implementations to identical IO classes.

Poem

🐰 A rabbit hops through files with glee,
Adding Destructors, clean as can be,
When resources end their day,
Close() is called right away,
No leaks for this bunny to see! 🌟

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 47.62% which is insufficient. The required threshold is 80.00%.	You can run `@coderabbitai generate docstrings` to improve docstring coverage.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title 'Fix test_io on windows' directly aligns with the main change: removing test_io from Windows-specific test skips in CI workflow, making the test run on Windows again.

📜 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 a47ecd7 and de11481.

⛔ Files ignored due to path filters (1)

Lib/test/test_io.py is excluded by !Lib/**

📒 Files selected for processing (1)

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

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.

github-actions Bot commented Dec 9, 2025

Copy link

Copy Markdown

Contributor

Code has been automatically formatted

The code in this PR has been formatted using cargo fmt --all.
Please pull the latest changes before pushing again:

git pull origin test-io

All reactions

Sorry, something went wrong.

Uh oh!

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

youknowone marked this pull request as ready for review

December 10, 2025 00:14

coderabbitai Bot reviewed

Dec 10, 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: 0

🧹 Nitpick comments (1)

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

1761-1771: Consider a macro for Destructor implementations (optional).

There are now 7 nearly identical Destructor implementations. A macro could reduce duplication:

macro_rules! impl_io_destructor {
    ($type:ty) => {
        impl Destructor for $type {
            fn slot_del(zelf: &PyObject, vm: &VirtualMachine) -> PyResult<()> {
                let _ = vm.call_method(zelf, "close", ());
                Ok(())
            }

            #[cold]
            fn del(_zelf: &Py<Self>, _vm: &VirtualMachine) -> PyResult<()> {
                unreachable!("slot_del is implemented")
            }
        }
    };
}

However, this is entirely optional — the current explicit implementations are clear and maintainable.

📜 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 ac48643 and a47ecd7.

⛔ Files ignored due to path filters (1)

Lib/test/test_io.py is excluded by !Lib/**

📒 Files selected for processing (2)

.github/workflows/ci.yaml (0 hunks)
crates/vm/src/stdlib/io.rs (10 hunks)

💤 Files with no reviewable changes (1)

.github/workflows/ci.yaml

🧰 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

🧠 Learnings (1)

📚 Learning: 2025-11-29T12:17:28.606Z

Learnt from: CR
Repo: RustPython/RustPython PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-11-29T12:17:28.606Z
Learning: Applies to **/*.rs : Use the macro system (`pyclass`, `pymodule`, `pyfunction`, etc.) when implementing Python functionality in Rust

Applied to files:

crates/vm/src/stdlib/io.rs

🔇 Additional comments (8)

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

1755-1771: LGTM!

The Destructor implementation for BufferedReader correctly follows the established pattern from _IOBase, properly swallowing errors from close() which is appropriate behavior for a destructor.

1824-1840: LGTM!

Consistent Destructor implementation for BufferedWriter.

1878-1900: LGTM!

Consistent Destructor implementation for BufferedRandom.

1944-2003: LGTM!

Consistent Destructor implementation for BufferedRWPair.

2476-2479: LGTM!

The pyclass declaration correctly includes Destructor, Iterable, and IterNext traits.

3342-3391: LGTM!

The TextIOWrapper implementations are well-designed:

Destructor: Follows the established pattern.

Iterable: Correctly checks if closed before returning self as iterator.

IterNext: Properly sets telling = false during iteration (matching CPython behavior), releases the lock during the readline call to avoid holding it during I/O, and correctly restores state on StopIteration.

4284-4292: LGTM!

The necessary imports for Destructor and related types are correctly added.

4551-4779: LGTM!

The FileIO Destructor implementation is consistent with the other I/O types and correctly ensures proper cleanup.

Sorry, something went wrong.

Uh oh!

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

All reactions

youknowone added 4 commits

December 10, 2025 09:21


          mark skip on test_io

8e7a746


          Drop for FileIO

9d4d350


          IO Desctructors

98bed0a


          Iterator

de11481

youknowone force-pushed the test-io branch from a47ecd7 to de11481 Compare

December 10, 2025 00:21

youknowone merged commit 30fb9d7 into RustPython:main

Dec 10, 2025

12 of 13 checks passed

Uh oh!

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

youknowone deleted the test-io branch

December 10, 2025 00:21

coderabbitai Bot mentioned this pull request

Dec 10, 2025

test_os, test_io on windows #6379

Merged

This was referenced Dec 28, 2025

update _pyio, test_fileio from 3.13.11 and impl more io features #6560

Merged

Upgrade test_io from 3.13.11 and fix more io tests #6565

Merged

coderabbitai Bot mentioned this pull request

Feb 5, 2026

Better codecs and fix lots of test_io and other expectedFailures #6997

Merged

Reviewers

coderabbitai[bot]

coderabbitai[bot] left review comments

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.

1 participant

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 01402520 — 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

youknowone commented Dec 9, 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 Dec 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Review failed

Walkthrough

Changes

Estimated code review effort

Possibly related PRs

Poem

Pre-merge checks and finishing touches

Uh oh!

github-actions Bot commented Dec 9, 2025

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

youknowone commented Dec 9, 2025 •

edited by coderabbitai Bot

Loading

Uh oh!

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

coderabbitai Bot commented Dec 9, 2025 •

edited

Loading

Uh oh!

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