io.TextIoWrapper.reconfigure to always flush#7281
io.TextIoWrapper.reconfigure to always flush#7281youknowone merged 2 commits intoRustPython:mainfrom
io.TextIoWrapper.reconfigure to always flush#7281Conversation
📝 WalkthroughWalkthroughThe PR fixes a panic in TextIOWrapper's reentrant Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@extra_tests/snippets/stdlib_io.py`:
- Line 2: Remove the unused imports BytesIO and StringIO from the import
statement in stdlib_io.py; edit the line that currently imports BufferedReader,
BytesIO, FileIO, RawIOBase, StringIO, TextIOWrapper and delete BytesIO and
StringIO so only actually used symbols (e.g., BufferedReader, FileIO, RawIOBase,
TextIOWrapper) remain, resolving the F401 lint warnings.
- Around line 63-86: The test fails early with AttributeError because Gh6588
lacks a flush method, so TextIOWrapper.writelines() raises instead of exercising
the reentrant reconfigure-path; update the Gh6588 class to implement a
flush(self) method (e.g., forward to self.textio.flush() or a no-op) so
TextIOWrapper (created as textio via TextIOWrapper(raw, ...)) can call flush and
the test will exercise the reentrant call in write/reconfigure rather than
short-circuiting on missing flush.
| @@ -1,5 +1,5 @@ | |||
| import os | |||
| from io import BufferedReader, BytesIO, FileIO, RawIOBase, StringIO | |||
| from io import BufferedReader, BytesIO, FileIO, RawIOBase, StringIO, TextIOWrapper | |||
There was a problem hiding this comment.
Remove unused BytesIO and StringIO imports.
Line 2 introduces unused imports that are already flagged by static analysis (F401), which adds noise to linting.
Proposed fix
-from io import BufferedReader, BytesIO, FileIO, RawIOBase, StringIO, TextIOWrapper
+from io import BufferedReader, FileIO, RawIOBase, TextIOWrapper🧰 Tools
🪛 Flake8 (7.3.0)
[error] 2-2: 'io.BytesIO' imported but unused
(F401)
[error] 2-2: 'io.StringIO' imported but unused
(F401)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@extra_tests/snippets/stdlib_io.py` at line 2, Remove the unused imports
BytesIO and StringIO from the import statement in stdlib_io.py; edit the line
that currently imports BufferedReader, BytesIO, FileIO, RawIOBase, StringIO,
TextIOWrapper and delete BytesIO and StringIO so only actually used symbols
(e.g., BufferedReader, FileIO, RawIOBase, TextIOWrapper) remain, resolving the
F401 lint warnings.
| class Gh6588: | ||
| def __init__(self): | ||
| self.textio = None | ||
| self.closed = False | ||
|
|
||
| def writable(self): | ||
| return True | ||
|
|
||
| def readable(self): | ||
| return False | ||
|
|
||
| def seekable(self): | ||
| return False | ||
|
|
||
| def write(self, data): | ||
| self.textio.reconfigure(encoding="utf-8") | ||
| return len(data) | ||
|
|
||
|
|
||
| raw = Gh6588() | ||
| textio = TextIOWrapper(raw, encoding="utf-8", write_through=True) | ||
| raw.textio = textio | ||
| with assert_raises(AttributeError): | ||
| textio.writelines(["x"]) |
There was a problem hiding this comment.
This regression test currently validates missing flush, not reentrancy handling.
Gh6588 has no flush, so Line 86 can fail with AttributeError before validating the reentrant-lock path from issue #6588. This can let the panic/reentrancy bug regress undetected.
Proposed test adjustment to exercise reentrant `reconfigure` path
class Gh6588:
@@
def write(self, data):
self.textio.reconfigure(encoding="utf-8")
return len(data)
+
+ def flush(self):
+ self.textio.reconfigure(encoding="utf-8")
@@
-with assert_raises(AttributeError):
+with assert_raises(RuntimeError):
textio.writelines(["x"])🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@extra_tests/snippets/stdlib_io.py` around lines 63 - 86, The test fails early
with AttributeError because Gh6588 lacks a flush method, so
TextIOWrapper.writelines() raises instead of exercising the reentrant
reconfigure-path; update the Gh6588 class to implement a flush(self) method
(e.g., forward to self.textio.flush() or a no-op) so TextIOWrapper (created as
textio via TextIOWrapper(raw, ...)) can call flush and the test will exercise
the reentrant call in write/reconfigure rather than short-circuiting on missing
flush.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
There was a problem hiding this comment.
@ShaharNaveh is this comment correct?
No, the entire point of the test is to align the behavior to be like cpython and to get an AttributeError
2 participants
fix #6588
Summary by CodeRabbit
Bug Fixes
Tests