Update rustyline & socket2 #6074
Conversation
WalkthroughThis change updates dependency versions in Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~2 minutes Poem
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. ✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (3)
Cargo.toml(1 hunks)stdlib/Cargo.toml(1 hunks)vm/src/readline.rs(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs
📄 CodeRabbit Inference Engine (.github/copilot-instructions.md)
**/*.rs: Follow the default rustfmt code style (cargo fmtto format)
Always run clippy to lint code (cargo clippy) before completing tasks. Fix any warnings or lints that are introduced by your 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:
vm/src/readline.rs
🧠 Learnings (6)
📓 Common learnings
Learnt from: CR
PR: RustPython/RustPython#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-07-19T03:16:56.511Z
Learning: Applies to **/*.rs : Follow Rust best practices for error handling and memory management
📚 Learning: in cargo build scripts (build.rs), the environment variable cargo_cfg_target_os is guaranteed to exi...
Learnt from: arihant2math
PR: RustPython/RustPython#5790
File: build.rs:2-2
Timestamp: 2025-06-28T16:31:03.991Z
Learning: In Cargo build scripts (build.rs), the environment variable CARGO_CFG_TARGET_OS is guaranteed to exist and is automatically set by Cargo during the build process, making unwrap() safe to use when accessing this variable.
Applied to files:
Cargo.toml
📚 Learning: in rustpython's stat module implementation, platform-specific constants like sf_supported and sf_syn...
Learnt from: moreal
PR: RustPython/RustPython#5847
File: vm/src/stdlib/stat.rs:547-567
Timestamp: 2025-06-27T14:47:28.810Z
Learning: In RustPython's stat module implementation, platform-specific constants like SF_SUPPORTED and SF_SYNTHETIC should be conditionally declared only for the platforms where they're available (e.g., macOS), following CPython's approach of optional declaration using #ifdef checks rather than providing fallback values for other platforms.
Applied to files:
Cargo.toml
📚 Learning: in rustpython's stat module implementation, platform-specific constants like sf_supported and sf_syn...
Learnt from: moreal
PR: RustPython/RustPython#5847
File: vm/src/stdlib/stat.rs:547-567
Timestamp: 2025-06-27T14:47:28.810Z
Learning: In RustPython's stat module implementation, platform-specific constants like SF_SUPPORTED and SF_SYNTHETIC should be conditionally declared only for the platforms where they're available (e.g., macOS), following CPython's approach of optional declaration rather than providing fallback values for other platforms.
Applied to files:
Cargo.toml
📚 Learning: applies to **/*.rs : use the macro system (`pyclass`, `pymodule`, `pyfunction`, etc.) when implement...
Learnt from: CR
PR: RustPython/RustPython#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-07-19T03:16:56.511Z
Learning: Applies to **/*.rs : Use the macro system (`pyclass`, `pymodule`, `pyfunction`, etc.) when implementing Python functionality in Rust
Applied to files:
Cargo.toml
📚 Learning: applies to **/*.rs : follow rust best practices for error handling and memory management...
Learnt from: CR
PR: RustPython/RustPython#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-07-19T03:16:56.511Z
Learning: Applies to **/*.rs : Follow Rust best practices for error handling and memory management
Applied to files:
Cargo.tomlvm/src/readline.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). (4)
- GitHub Check: Run snippets and cpython tests (ubuntu-latest)
- GitHub Check: Run snippets and cpython tests (windows-latest)
- GitHub Check: Run snippets and cpython tests (macos-latest)
- GitHub Check: Run rust tests (windows-latest)
🔇 Additional comments (2)
Cargo.toml (1)
206-208: No legacy rustyline APIs detectedI searched for all known removed items from the v15→v17 bump and found no references in the workspace:
- ReadlineError variants (
WindowResized,WinResize,Utf8Error,Errno)history::FileHistoryvalidate::ValidationContextPlease run
cargo clippy --all-targetsand perform a full test build across the workspace to ensure no other breaking changes remain.stdlib/Cargo.toml (1)
105-109: socket2 & dns-lookup API compatibility confirmedOur inspection of
stdlib/src/socket.rsshows only stable, unchanged APIs in both crates:
- socket2 usage is limited to:
Socket::new,SockAddr::unix,MsgHdr::new,- conversion via
AsRawFd/AsFd(no calls tofrom_raw_fdorSockAddr::from_libc).- dns-lookup usage remains on:
getaddrinfo,getnameinfo,AddrInfoHints,LookupErrorKind::System,All of these are unchanged in socket2 0.6 and dns-lookup 2.1. No breaking‐change–related refactors are required.
| Err(ReadlineError::Eof) => ReadlineResult::Eof, | ||
| Err(ReadlineError::Io(e)) => ReadlineResult::Io(e), | ||
| Err(ReadlineError::WindowResized) => continue, | ||
| Err(ReadlineError::Signal(_)) => continue, | ||
| Err(e) => ReadlineResult::Other(e.into()), | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Signal(_) swallows all signals – double-check intent
Switching from the narrow WindowResized variant to Signal(_) means the loop silently ignores every signal Rustyline forwards (e.g. SIGCONT, SIGTSTP).
If we ever want to surface some of these to Python, this will mask them.
Consider filtering only the signals we truly want to ignore (currently libc::SIGWINCH) and handle / propagate the rest:
-Err(ReadlineError::Signal(_)) => continue,
+Err(ReadlineError::Signal(sig)) if sig == libc::SIGWINCH => continue,
+Err(ReadlineError::Signal(sig)) => ReadlineResult::Other(
+ io::Error::new(io::ErrorKind::Interrupted, format!("received signal {sig}")).into()
+),📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| Err(ReadlineError::Eof) => ReadlineResult::Eof, | |
| Err(ReadlineError::Io(e)) => ReadlineResult::Io(e), | |
| Err(ReadlineError::WindowResized) => continue, | |
| Err(ReadlineError::Signal(_)) => continue, | |
| Err(e) => ReadlineResult::Other(e.into()), | |
| }; | |
| Err(ReadlineError::Eof) => ReadlineResult::Eof, | |
| Err(ReadlineError::Io(e)) => ReadlineResult::Io(e), | |
| Err(ReadlineError::Signal(sig)) if sig == libc::SIGWINCH => continue, | |
| Err(ReadlineError::Signal(sig)) => ReadlineResult::Other( | |
| io::Error::new(io::ErrorKind::Interrupted, format!("received signal {sig}")).into() | |
| ), | |
| Err(e) => ReadlineResult::Other(e.into()), |
🤖 Prompt for AI Agents
In vm/src/readline.rs around lines 118 to 122, the current code matches all
signals with Err(ReadlineError::Signal(_)) and silently continues, which
swallows every signal Rustyline forwards. To fix this, change the match to
specifically check for the SIGWINCH signal (window resize) and continue only for
that, while allowing other signals to be handled or propagated appropriately.
This ensures only the intended signals are ignored and others can be surfaced to
Python if needed.
Summary by CodeRabbit
Bug Fixes
Chores