| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
Sorry, something went wrong.
|
Important Review skippedReview was skipped due to path filters ⛔ Files ignored due to path filters (1)
CodeRabbit blocks several paths by default. You can override this behavior by explicitly including those paths in the path filters. For example, including **/dist/** will override the default block on the dist directory, by removing the pattern from both the lists. ⚙️ Run configurationConfiguration used: Path: .coderabbit.yml Review profile: CHILL Plan: Pro Run ID: 18682e81-819b-45be-97ce-ef57cf1e5c76 You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file. Use the checkbox below for a quick retry:
WalkthroughAdds target-specific host_env dependencies, many platform-gated libc re-exports and helpers (fs::which, socket::hostname/mac_address, fileutils::CFile/fclose, readline/system_configuration), and rewires stdlib/vm to consume rustpython_host_env symbols while migrating FFI integer types to core::ffi equivalents. ChangesLibc abstraction and modernization
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
Poem🚥 Pre-merge checks | ✅ 3 | ❌ 2 ❌ Failed checks (1 warning, 1 inconclusive)
✏️ 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. ❤️ ShareComment @coderabbitai help to get the list of available commands. |
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agentsVerify 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 `@crates/host_env/src/lib.rs`: - Line 33: The module export `pub mod readline;` is currently unconditional but its crate dependency is only available for non-wasm32 targets; wrap the export with a target cfg to match Cargo.toml (e.g. change to `#[cfg(not(target_arch = "wasm32"))] pub mod readline;`) so the module is only compiled on supported targets; locate the `pub mod readline` line in lib.rs and apply the cfg attribute (or an equivalent cfg_if) to avoid the cfg/dependency mismatch.
Fix all unresolved CodeRabbit comments on this PR:
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 17e7dc80-94cd-446a-8624-c6b9bff09c11
📥 CommitsReviewing files that changed from the base of the PR and between ae3804f and 1c27152.
⛔ Files ignored due to path filters (1)
Sorry, something went wrong.
| pub mod fs; | ||
| #[cfg(any(unix, windows))] | ||
| pub mod locale; | ||
| pub mod readline; |
There was a problem hiding this comment.
⚠️ Potential issue | 🟠 Major | ⚡ Quick win
Gate readline export by target to avoid cfg/dependency mismatch.
Line 33 exports readline on all targets, but its dependency surface is target-gated in Cargo.toml (non-wasm32). This can break unsupported targets at compile time.
🔧 Proposed fix- pub mod readline;
+ #[cfg(not(target_arch = "wasm32"))]
+ pub mod readline;‼️ 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.
| pub mod readline; | |
| #[cfg(not(target_arch = "wasm32"))] | |
| pub mod readline; |
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/host_env/src/lib.rs` at line 33, The module export `pub mod readline;` is currently unconditional but its crate dependency is only available for non-wasm32 targets; wrap the export with a target cfg to match Cargo.toml (e.g. change to `#[cfg(not(target_arch = "wasm32"))] pub mod readline;`) so the module is only compiled on supported targets; locate the `pub mod readline` line in lib.rs and apply the cfg attribute (or an equivalent cfg_if) to avoid the cfg/dependency mismatch.
Sorry, something went wrong.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)crates/stdlib/src/socket.rs (3)1187-1242: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win
Do not accept an AF_CAN payload you never serialize.
This branch allows (interface, addr) but only writes can_family and can_ifindex; tuple[1] is ignored. That means bind(), connect(), and sendto() can silently target the wrong CAN endpoint instead of rejecting unsupported tuple shapes.
🤖 Prompt for AI AgentsVerify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/stdlib/src/socket.rs` around lines 1187 - 1242, The AF_CAN branch currently accepts a two-element tuple (interface, addr) but ignores tuple[1]; update this logic to either fully serialize the second element into sockaddr_can or reject two-element tuples: simplest fix is to disallow tuple.len() == 2 — change the tuple length validation in the AF_CAN branch (the code that builds `tuple: PyTupleRef`) to only accept a single-element tuple `(interface,)` and return a TypeError if a second element is provided; reference the AF_CAN branch, `tuple`, `can_addr`/`sockaddr_can`, and `caller` when making the change so callers get a clear error instead of silently ignoring the second element.
2279-2289: ⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift
AF_CAN addresses are truncated on the read path.
get_addr_tuple() always returns (ifname,) for AF_CAN, so recvfrom(), getsockname(), and getpeername() lose the CAN-specific address payload. That breaks address round-tripping for protocol-specific CAN sockets.
🤖 Prompt for AI AgentsVerify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/stdlib/src/socket.rs` around lines 2279 - 2289, get_addr_tuple() currently collapses AF_CAN sockaddr_can into only (ifname,) losing the can_id and breaking round-trips for recvfrom()/getsockname()/getpeername(); fix by reading both can_ifindex and can_id from the sockaddr_can (use unsafe { &*(addr.as_ptr() as *const c::sockaddr_can) }), convert ifindex to ifname as before, and return a tuple containing both fields when can_id != 0 (e.g. (ifname, can_id)) or include a zero/null can_id explicitly so vm.ctx.new_tuple(...) produces the full CAN address instead of only vm.ctx.new_str(ifname).into().
1343-1348: ⚠️ Potential issue | 🔴 Critical | ⚡ Quick win
Add missing EISCONN constant to the platform abstraction layer.
Line 1347 references c::EISCONN, but this constant is not exported from rustpython_host_env::socket. The import list at lines 60-85 in crates/stdlib/src/socket.rs does not include it, and it's missing from both:
- The Windows WinSock imports (where WSAEISCONN must be added and mapped)
- The Unix libc exports (where EISCONN should be re-exported)
This is a compile blocker on all platforms. Add EISCONN to crates/host_env/src/socket.rs by either directly importing it from libc (Unix) or mapping WSAEISCONN as a platform-specific constant (Windows), then update the mod c re-export list to include it.
🤖 Prompt for AI AgentsVerify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/stdlib/src/socket.rs` around lines 1343 - 1348, Add the missing EISCONN constant to the platform abstraction so c::EISCONN used in socket.rs compiles: in crates/host_env/src/socket.rs import/re-export EISCONN for Unix (pull from libc::EISCONN) and on Windows define/map a constant from winsock (WSAEISCONN) to the same name, then add EISCONN to the mod c re-export list; this will satisfy the use of c::EISCONN in methods like sock_op / SelectKind::Connect and ensure platform-specific mapping for WSAEISCONN on Windows.
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@crates/stdlib/src/socket.rs`:
- Around line 1187-1242: The AF_CAN branch currently accepts a two-element tuple
(interface, addr) but ignores tuple[1]; update this logic to either fully
serialize the second element into sockaddr_can or reject two-element tuples:
simplest fix is to disallow tuple.len() == 2 — change the tuple length
validation in the AF_CAN branch (the code that builds `tuple: PyTupleRef`) to
only accept a single-element tuple `(interface,)` and return a TypeError if a
second element is provided; reference the AF_CAN branch, `tuple`,
`can_addr`/`sockaddr_can`, and `caller` when making the change so callers get a
clear error instead of silently ignoring the second element.
- Around line 2279-2289: get_addr_tuple() currently collapses AF_CAN
sockaddr_can into only (ifname,) losing the can_id and breaking round-trips for
recvfrom()/getsockname()/getpeername(); fix by reading both can_ifindex and
can_id from the sockaddr_can (use unsafe { &*(addr.as_ptr() as *const
c::sockaddr_can) }), convert ifindex to ifname as before, and return a tuple
containing both fields when can_id != 0 (e.g. (ifname, can_id)) or include a
zero/null can_id explicitly so vm.ctx.new_tuple(...) produces the full CAN
address instead of only vm.ctx.new_str(ifname).into().
- Around line 1343-1348: Add the missing EISCONN constant to the platform
abstraction so c::EISCONN used in socket.rs compiles: in
crates/host_env/src/socket.rs import/re-export EISCONN for Unix (pull from
libc::EISCONN) and on Windows define/map a constant from winsock (WSAEISCONN) to
the same name, then add EISCONN to the mod c re-export list; this will satisfy
the use of c::EISCONN in methods like sock_op / SelectKind::Connect and ensure
platform-specific mapping for WSAEISCONN on Windows.
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 2130668c-941c-4855-a192-2b4380901f71
📥 CommitsReviewing files that changed from the base of the PR and between 1c27152 and c2e08c9.
⛔ Files ignored due to path filters (1)
Sorry, something went wrong.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)crates/stdlib/src/socket.rs (1)2319-2323: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win
Avoid panic on non-UTF8 hostnames in gethostname.
Line 2322 calls err.into_string().unwrap() on an OsString error, which panics for non-UTF8 hostnames. Use vm.fsdecode() instead, which is the idiomatic RustPython pattern for handling OS strings with potential non-UTF8 content and is already used elsewhere in this file (line 2279).
Proposed fix🤖 Prompt for AI Agents#[pyfunction] fn gethostname(vm: &VirtualMachine) -> PyResult<PyStrRef> { - rustpython_host_env::socket::hostname() - .into_string() - .map(|hostname| vm.ctx.new_str(hostname)) - .map_err(|err| vm.new_os_error(err.into_string().unwrap())) + let hostname = rustpython_host_env::socket::hostname(); + Ok(vm.fsdecode(hostname.as_os_str())) }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/stdlib/src/socket.rs` around lines 2319 - 2323, The map_err closure in the gethostname path currently calls err.into_string().unwrap(), which can panic on non-UTF8 hostnames; replace that unwrap with the RustPython fsdecode helper so the OS string is safely converted into a Python string for the error. Locate the closure on the result of rustpython_host_env::socket::hostname() (the map_err that constructs vm.new_os_error) and change it to call vm.fsdecode on the OsString/OsStr error and pass that decoded string into vm.new_os_error (use vm.fsdecode instead of into_string().unwrap()).
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. Outside diff comments: In `@crates/stdlib/src/socket.rs`: - Around line 2319-2323: The map_err closure in the gethostname path currently calls err.into_string().unwrap(), which can panic on non-UTF8 hostnames; replace that unwrap with the RustPython fsdecode helper so the OS string is safely converted into a Python string for the error. Locate the closure on the result of rustpython_host_env::socket::hostname() (the map_err that constructs vm.new_os_error) and change it to call vm.fsdecode on the OsString/OsStr error and pass that decoded string into vm.new_os_error (use vm.fsdecode instead of into_string().unwrap()).
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 707278ab-88aa-4c7b-ab07-01dde84f5278
📥 CommitsReviewing files that changed from the base of the PR and between c2e08c9 and 8d89a7d.
⛔ Files ignored due to path filters (1)
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agentsVerify 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 `@crates/stdlib/src/socket.rs`: - Around line 2319-2322: The current error branch calls err.into_string().unwrap(), which can panic on non‑UTF8 hostnames; update the conversion in the chain starting at rustpython_host_env::socket::hostname() so it never unwraps: when mapping the Ok case produce vm.ctx.new_str(hostname) as before, and for the Err path pass a safe String into vm.new_os_error by using to_string_lossy()/to_owned() (or otherwise fall back to lossy conversion) instead of unwrap; ensure the map_err closure returns vm.new_os_error(err_lossy) without panicking.
Fix all unresolved CodeRabbit comments on this PR:
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: aee56f42-d640-42e6-b230-76949aee47c0
📥 CommitsReviewing files that changed from the base of the PR and between 8d89a7d and c601816.
⛔ Files ignored due to path filters (1)
Sorry, something went wrong.
| rustpython_host_env::socket::hostname() | ||
| .into_string() | ||
| .map(|hostname| vm.ctx.new_str(hostname)) | ||
| .map_err(|err| vm.new_os_error(err.into_string().unwrap())) |
There was a problem hiding this comment.
⚠️ Potential issue | 🔴 Critical | ⚡ Quick win
🧩 Analysis chain🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n --type rust -C3 '\bfn\s+hostname\b' crates/host_env/src/socket.rs
rg -n --type rust -C2 'into_string\(\)|to_string_lossy\(' crates/stdlib/src/socket.rsRepository: RustPython/RustPython
Length of output: 905
Avoid panic when converting hostname to UTF-8
rustpython_host_env::socket::hostname() returns OsString; the current error path uses err.into_string().unwrap() at crates/stdlib/src/socket.rs:2322, which will panic on non‑UTF8 hostnames.
💡 Suggested fix fn gethostname(vm: &VirtualMachine) -> PyResult<PyStrRef> {
rustpython_host_env::socket::hostname()
.into_string()
.map(|hostname| vm.ctx.new_str(hostname))
- .map_err(|err| vm.new_os_error(err.into_string().unwrap()))
+ .map_err(|err| vm.new_os_error(err.to_string_lossy().into_owned()))
}‼️ 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.
| rustpython_host_env::socket::hostname() | |
| .into_string() | |
| .map(|hostname| vm.ctx.new_str(hostname)) | |
| .map_err(|err| vm.new_os_error(err.into_string().unwrap())) | |
| rustpython_host_env::socket::hostname() | |
| .into_string() | |
| .map(|hostname| vm.ctx.new_str(hostname)) | |
| .map_err(|err| vm.new_os_error(err.to_string_lossy().into_owned())) |
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/stdlib/src/socket.rs` around lines 2319 - 2322, The current error branch calls err.into_string().unwrap(), which can panic on non‑UTF8 hostnames; update the conversion in the chain starting at rustpython_host_env::socket::hostname() so it never unwraps: when mapping the Ok case produce vm.ctx.new_str(hostname) as before, and for the Err path pass a safe String into vm.new_os_error by using to_string_lossy()/to_owned() (or otherwise fall back to lossy conversion) instead of unwrap; ensure the map_err closure returns vm.new_os_error(err_lossy) without panicking.
Sorry, something went wrong.
Add host_env::fs::which wrapper and update getpath.rs call site.
Move crates/vm/src/readline.rs to crates/host_env/src/readline.rs and re-export it from rustpython_vm::readline. Move rustyline dependency from vm to host_env. Drop the host_env feature gates on history I/O inside the moved module.
Add host_env::socket::hostname and host_env::socket::mac_address wrappers. Update _socket.gethostname and _uuid.get_node_id call sites.
Move direct libc constant and type references in stdlib (faulthandler, fcntl, multiprocessing, posixshmem, select, syslog) to use host_env re-exports. Replace libc::c_int and libc::c_uint in stdlib with core::ffi equivalents.
Move direct libc MADV_*, MAP_*, PROT_*, EOVERFLOW references in stdlib::mmap to host_env::mmap re-exports. Replace libc::c_int with core::ffi::c_int.
Move direct libc LC_*, ABDAY_*, ABMON_*, TIOC*, FIO* and related constants in stdlib::locale and stdlib::termios to host_env re-exports. Replace libc::c_char in stdlib::locale and stdlib::openssl::cert with core::ffi::c_char.
Replace libc::c_int, c_long, c_uchar, c_uint, c_ulong, c_void,
c_char in stdlib::openssl with core::ffi equivalents. Replace
libc::ENOENT with host_env::errno::errors::ENOENT. Add
host_env::fileutils::{CFile, fclose} and route the load_dh_params
fclose call and PEM_read_DHparams FILE pointer type through them.
host_mmap::EOVERFLOW and host_faulthandler::{SIGSEGV, SIGFPE}
re-exports were gated to cfg(unix) but stdlib uses them from Windows
and platform-agnostic code paths.
Move direct libc references in stdlib (resource, posixsubprocess,
grp, socket) to host_env re-exports. Replace libc::c_long,
libc::c_longlong, libc::c_char in stdlib::socket with core::ffi
equivalents. Add resource::{RLIMIT_*, RUSAGE_*, c_long, rlim_t,
rlimit, timeval}, posix::{c_char, pid_t}, grp::gid_t, and
socket::{AF_*, SOCK_STREAM, sa_family_t, socklen_t, sockaddr_*}
re-exports.
Re-export socket2 as host_env::socket::raw and dns-lookup as host_env::socket::dns. Update _socket call sites to route through host_env aliases.
Re-export the macOS system-configuration crate as host_env::system_configuration and update _scproxy call sites.
libc on Android (bionic) does not define sockaddr_can, so importing it for target_os = "android" fails to compile. Keep AF_ALG/AF_CAN for both linux and android, and gate sockaddr_alg/sockaddr_can to linux only. Assisted-by: Claude
| Back | FazBrowse Home | New Git URL |
Summary by CodeRabbit
New Features
Bug Fixes
Refactor