Use cfg_select! macro instead of cfg_if!#7636
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yml Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (1)
💤 Files with no reviewable changes (1)
📝 WalkthroughWalkthroughRemoved Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
67f5ebc to
8db3293
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/vm/src/stdlib/posix.rs (1)
1805-1820:⚠️ Potential issue | 🔴 CriticalRemove the stray
iffrom thecfg_select!arm.Line 1807 uses
if any(...) =>, but Rust 1.95cfg_select!arms use the cfg predicate directly without theifkeyword (any(...) =>). This is a compile-blocking syntax error. All othercfg_select!usages in the codebase follow the correct syntax withoutif.🐛 Proposed fix
// Note: POSIX_SPAWN_SETSID may not be available on all platforms cfg_select! { - if any( + any( target_os = "linux", target_os = "haiku", target_os = "solaris", target_os = "illumos", target_os = "hurd", ) => { flags.insert(nix::spawn::PosixSpawnFlags::from_bits_retain(libc::POSIX_SPAWN_SETSID));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/vm/src/stdlib/posix.rs` around lines 1805 - 1820, The cfg_select! arm in the flags handling block incorrectly uses the `if` keyword ("if any(...) =>") which is invalid syntax; edit the cfg_select! invocation (the block that inserts nix::spawn::PosixSpawnFlags::from_bits_retain(libc::POSIX_SPAWN_SETSID) and returns a new_not_implemented_error) to remove the stray `if` so the arm uses the predicate directly (e.g., `any(...) =>`), leaving the flags.insert(...) and the `_ => { return Err(vm.new_not_implemented_error(...)); }` arm unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/stdlib/src/openssl.rs`:
- Around line 35-42: The PROBE static initializer's cfg_select! branches end
with trailing semicolons making each branch evaluate to () instead of returning
a LazyLock<ProbeResult>; remove the semicolons after
LazyLock::new(openssl_probe::probe) and LazyLock::new(|| ProbeResult {
cert_file: None, cert_dir: vec![] }) so each branch yields a
LazyLock<ProbeResult> (affecting the PROBE static, the cfg_select! macro,
LazyLock::new invocations, and the ProbeResult construction).
In `@crates/vm/src/stdlib/_thread.rs`:
- Around line 34-39: The cfg_select! arms for PYTHREAD_NAME currently list the
broader unix => Some("pthread") arm before the more specific any(target_os =
"solaris", target_os = "illumos") arm, so Solaris/illumos targets never reach
their intended match; fix by reordering the cfg_select! arms in the
PYTHREAD_NAME constant so the any(target_os = "solaris", target_os = "illumos")
arm appears before the unix arm, preserving the other arms (windows and default)
as-is.
In `@crates/vm/src/stdlib/sys.rs`:
- Around line 248-255: The PLATFORM constant in sys.rs currently maps target_os
"macos" and Windows to non-CPython values; update the cfg_select! mapping in the
pub const PLATFORM definition so that target_os = "macos" => "darwin" and
windows => "win32" (leave the other arms unchanged) to match CPython's
sys.platform values; modify the PLATFORM const in crates/vm/src/stdlib/sys.rs
accordingly and run the build/tests to verify.
---
Outside diff comments:
In `@crates/vm/src/stdlib/posix.rs`:
- Around line 1805-1820: The cfg_select! arm in the flags handling block
incorrectly uses the `if` keyword ("if any(...) =>") which is invalid syntax;
edit the cfg_select! invocation (the block that inserts
nix::spawn::PosixSpawnFlags::from_bits_retain(libc::POSIX_SPAWN_SETSID) and
returns a new_not_implemented_error) to remove the stray `if` so the arm uses
the predicate directly (e.g., `any(...) =>`), leaving the flags.insert(...) and
the `_ => { return Err(vm.new_not_implemented_error(...)); }` arm unchanged.
🪄 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: 3c4d5dab-4eac-460c-b598-031fda07a667
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (19)
Cargo.tomlcrates/common/Cargo.tomlcrates/common/src/lock.rscrates/stdlib/Cargo.tomlcrates/stdlib/src/openssl.rscrates/stdlib/src/resource.rscrates/stdlib/src/socket.rscrates/vm/Cargo.tomlcrates/vm/src/builtins/type.rscrates/vm/src/macros.rscrates/vm/src/object/core.rscrates/vm/src/object/ext.rscrates/vm/src/object/payload.rscrates/vm/src/stdlib/_io.rscrates/vm/src/stdlib/_signal.rscrates/vm/src/stdlib/_thread.rscrates/vm/src/stdlib/os.rscrates/vm/src/stdlib/posix.rscrates/vm/src/stdlib/sys.rs
💤 Files with no reviewable changes (3)
- crates/common/Cargo.toml
- crates/vm/Cargo.toml
- crates/stdlib/Cargo.toml
| pub const PYTHREAD_NAME: Option<&str> = cfg_select! { | ||
| windows => Some("nt"), | ||
| unix => Some("pthread"), | ||
| any(target_os = "solaris", target_os = "illumos") => Some("solaris"), | ||
| _ => None, | ||
| }; |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Rust cfg_select first matching arm target_os solaris illumos cfg unix target_family
💡 Result:
In Rust, both Solaris (e.g., x86_64-pc-solaris) and illumos (e.g., x86_64-unknown-illumos, aarch64-unknown-illumos) targets have target_os = "solaris" for LLVM but are recognized distinctly in Rust's cfg system. Illumos uses base::illumos::opts, analogous to base::solaris::opts which explicitly sets target_family = ["unix"]. Custom illumos targets also specify "target-family": ["unix"]. Both match cfg(target_os = "solaris"), cfg(target_family = "unix"), and cfg(unix). The cfg_select! macro (stable since Rust 1.95) selects the first matching arm based on cfg predicates evaluated left-to-right. For a cfg_select prioritizing illumos over generic solaris/unix: cfg_select! { target_os = "solaris", target_os = "illumos" => { /* illumos-specific / }, target_os = "solaris" => { / Solaris-specific (excludes illumos) / }, target_family = "unix" => { / other unix / }, _ => { / fallback */ } } However, since illumos sets target_os = "illumos" in Rust (distinct from Solaris's "solaris"), target_os = "illumos" matches only illumos, while target_os = "solaris" matches only Solaris. If using target_os = "solaris" first, it matches neither, falling through. To match illumos first: target_os = "illumos" arm before target_os = "solaris". Both are under target_family = "unix".
Citations:
- 1: https://doc.rust-lang.org/rustc/platform-support/illumos.html
- 2: https://doc.rust-lang.org/beta/src/std/sys/fs/unix.rs.html
- 3: https://doc.rust-lang.org/nightly/nightly-rustc/src/rustc_target/spec/targets/x86_64_unknown_illumos.rs.html
- 4: https://stackoverflow.com/questions/76480789/rust-is-there-a-list-of-what-target-is-a-member-of-what-target-family
- 5: https://doc.rust-lang.org/std/macro.cfg_select.html
- 6: https://doc.rust-lang.org/beta/rustc/platform-support/illumos.html
- 7: https://doc.rust-lang.org/beta/nightly-rustc/src/rustc_target/spec/base/solaris.rs.html
- 8: https://doc.rust-lang.org/nightly/nightly-rustc/src/rustc_target/spec/targets/aarch64_unknown_illumos.rs.html
- 9: https://blog.openflowlabs.com/~/personal/Setting%20up%20a%20new%20Rust%20Target%20with%20a%20cross%20compiler/
🏁 Script executed:
cat -n crates/vm/src/stdlib/_thread.rs | sed -n '30,45p'Repository: RustPython/RustPython
Length of output: 751
Move the Solaris/illumos arm before the unix arm.
cfg_select! evaluates arms left-to-right and stops at the first match. Since both Solaris and illumos targets have target_family = "unix", they currently match the unix => Some("pthread") arm before reaching the more specific any(target_os = "solaris", target_os = "illumos") arm. Reorder to check the narrower condition first.
🔧 Proposed fix
pub const PYTHREAD_NAME: Option<&str> = cfg_select! {
windows => Some("nt"),
+ any(target_os = "solaris", target_os = "illumos") => Some("solaris"),
unix => Some("pthread"),
- any(target_os = "solaris", target_os = "illumos") => Some("solaris"),
_ => None,
};📝 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.
| pub const PYTHREAD_NAME: Option<&str> = cfg_select! { | |
| windows => Some("nt"), | |
| unix => Some("pthread"), | |
| any(target_os = "solaris", target_os = "illumos") => Some("solaris"), | |
| _ => None, | |
| }; | |
| pub const PYTHREAD_NAME: Option<&str> = cfg_select! { | |
| windows => Some("nt"), | |
| any(target_os = "solaris", target_os = "illumos") => Some("solaris"), | |
| unix => Some("pthread"), | |
| _ => None, | |
| }; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/vm/src/stdlib/_thread.rs` around lines 34 - 39, The cfg_select! arms
for PYTHREAD_NAME currently list the broader unix => Some("pthread") arm before
the more specific any(target_os = "solaris", target_os = "illumos") arm, so
Solaris/illumos targets never reach their intended match; fix by reordering the
cfg_select! arms in the PYTHREAD_NAME constant so the any(target_os = "solaris",
target_os = "illumos") arm appears before the unix arm, preserving the other
arms (windows and default) as-is.
8169811 to
7dce1bb
Compare
7dce1bb to
f446b28
Compare
Since 1.95
cfg_select!macro is stabilizedhttps://doc.rust-lang.org/stable/std/macro.cfg_select.html
Summary by CodeRabbit
New Requirements
Chores
Clarification