Validate TextIOWrapper chunk size#8265
Conversation
Match CPython for non-positive, oversized, and index-compatible values. Assisted-by: Codex:GPT-5
📝 WalkthroughWalkthroughChangesThe TextIOWrapper chunk size validation
Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
crates/vm/src/stdlib/_io.rs (1)
3208-3230: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSetter logic matches CPython's
PyNumber_AsSsize_tsemantics.The order of checks (
try_index→TypeErrorfor non-index types;try_to_primitive::<isize>→ValueErrorfor overflow;<= 0→ValueError) matches CPython's_CHUNK_SIZEsetter, which callsPyNumber_AsSsize_t(arg, PyExc_ValueError)(converting overflow intoValueErrorinstead of the defaultOverflowError) followed by a<= 0check. The hardcoded'int'in the overflow message is also consistent with CPython, since by that point the value has already been converted via__index__.One small note: since
chunk_size > 0is already guaranteed by the preceding check,usize::try_from(chunk_size)can never fail on any supported platform (usize::MAX >= isize::MAX), making the.map_err(...)branch unreachable dead code. Not required, butchunk_size as usizewould be simpler.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/vm/src/stdlib/_io.rs` around lines 3208 - 3230, In set_chunksize, simplify the conversion after the chunk_size <= 0 validation by using a direct conversion to usize instead of usize::try_from with an unreachable error mapping; preserve the existing validation and error behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@extra_tests/snippets/stdlib_io.py`:
- Around line 90-104: Replace both `assert False` statements in the invalid
`_CHUNK_SIZE` test loops with explicit `raise AssertionError(...)` calls,
preserving the existing failure messages and exception expectations so the
checks remain active under optimized Python execution.
---
Nitpick comments:
In `@crates/vm/src/stdlib/_io.rs`:
- Around line 3208-3230: In set_chunksize, simplify the conversion after the
chunk_size <= 0 validation by using a direct conversion to usize instead of
usize::try_from with an unreachable error mapping; preserve the existing
validation and error behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 9a755fa4-7822-40d3-a03b-48d68a59e3ba
📒 Files selected for processing (2)
crates/vm/src/stdlib/_io.rsextra_tests/snippets/stdlib_io.py
ShaharNaveh
left a comment
There was a problem hiding this comment.
tysm:)
welcome to the project
Summary
TextIOWrapper._CHUNK_SIZEsetter가 CPython과 동일하게 값을 검증하도록 수정합니다.기존에는
usize로 바로 변환되면서 다음 값들이 정상적으로 설정될 수 있었습니다.0isize범위를 벗어난 큰 정수이번 변경에서는 다음 동작을 적용합니다.
ValueError발생TypeError발생isize범위를 벗어나는 정수는ValueError발생__index__()를 제공하는 객체는 정상적으로 허용동작을 검증하기 위한 extra snippet 테스트도 추가했습니다.
Tests
cd extra_tests pytest -v snippets/stdlib_io.pySummary by CodeRabbit
Bug Fixes
TextIOWrapper._CHUNK_SIZE.__index__are accepted correctly._CHUNK_SIZEcontinues to raise an attribute error.Tests