Fix test_io on windows by youknowone · Pull Request #6387 · RustPython/RustPython

youknowone

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