Cleanup vm/src/stdlib/stat.rs
#6018
Conversation
WalkthroughThis change refactors the platform-specific constant definitions in the Changes
Sequence Diagram(s)No sequence diagram generated as the changes are limited to constant definitions and do not affect control flow or introduce new features. Estimated code review effort3 (~45 minutes) Possibly related PRs
Poem
✨ Finishing Touches
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. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
vm/src/stdlib/stat.rs
Outdated
| #[pyattr] | ||
| pub const S_IFREG: Mode = 0o100000; | ||
| pub const S_IFDIR: Mode = unix_libc_get!(S_IFDIR, 0o040000); |
There was a problem hiding this comment.
This is a good idea! Looks a lot better than before.
I have a suggestion for the macro interface. How about this?
| pub const S_IFDIR: Mode = unix_libc_get!(S_IFDIR, 0o040000); | |
| pub const S_IFDIR: Mode = libc_const!(#[cfg(unix)] S_IFDIR, 0o040000); |
There was a problem hiding this comment.
I'm on the fence about this one, as it is clear. but will require to put it for every const definition, I tried to make it less verbose overall as there's no reason to do it imo.
That's why I defined two macros that are the same except the condition inside the cfg_if::cfg_if macro.
I tried to do this cleanup using couple of ways such as:
#[cfg(unix)]
mod maybe_unix {
pub use libc::{A, B, C};
}
#[cfg(not(unix))]
mod maybe_unix {
pub const A:Mode = 1;
...
}
mod stat {
#[pyattr]
pub use maybe_unix::{A, B, C};
}But I decided to drop it as it would require to sync between 3 places (2 modules, and the stat module).
I also tried to have the cfg condition to be passed as an optional argument to the macro, but then I encountered some weird issues with #[pyattr].
At the end this is the best compromise that I found, unless you have a better idea
There was a problem hiding this comment.
I have tried to implement the macro based on the suggested interface. I'm unsure how this would work. can you help me out here how do you envision the macro to look like?
There was a problem hiding this comment.
Does current version make sense?
I also tried libc_const!(@unix S_IFDIR, 0o040000); and libc_const!(@macos UF_NODUMP, 0x00000001);, to make a balance of concise forms, but not confident about readability.
There was a problem hiding this comment.
The current version looks way better than mine, clearly you have more experience 😆
vm/src/stdlib/stat.rs
Outdated
| #[pyattr] | ||
| pub const S_IEXEC: Mode = 0o0100; | ||
| pub const UF_NODUMP: u32 = macos_libc_get!(UF_NODUMP, 0x00000001); |
There was a problem hiding this comment.
Then this one also will have the same pattern
| pub const UF_NODUMP: u32 = macos_libc_get!(UF_NODUMP, 0x00000001); | |
| pub const UF_NODUMP: u32 = libc_const!(#[cfg(target_os = "macos")] UF_NODUMP, 0x00000001); |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
vm/src/stdlib/stat.rs (1)
13-46: Consider consolidating the two macros to reduce duplication.While this refactoring improves upon the previous approach, both
unix_libc_get!andmacos_libc_get!have identical implementations. I understand from the past discussion that you encountered issues with#[pyattr]when trying to pass the cfg condition as a parameter.As an alternative to the current approach or the inline cfg attribute suggestion, consider using a single macro with a naming convention:
- // unix_libc_get - cfg_if::cfg_if! { - if #[cfg(unix)] { - macro_rules! unix_libc_get { - ($name:ident, $val:expr) => { - libc::$name - }; - } - } else { - macro_rules! unix_libc_get { - ($name:ident, $val:expr) => { - $val - }; - } - } - - } - - // macos_libc_get - cfg_if::cfg_if! { - if #[cfg(target_os = "macos")] { - macro_rules! macos_libc_get { - ($name:ident, $val:expr) => { - libc::$name - }; - } - } else { - macro_rules! macos_libc_get { - ($name:ident, $val:expr) => { - $val - }; - } - } - } + macro_rules! platform_const { + (unix $name:ident, $val:expr) => { + { + #[cfg(unix)] + let value = libc::$name; + #[cfg(not(unix))] + let value = $val; + value + } + }; + (macos $name:ident, $val:expr) => { + { + #[cfg(target_os = "macos")] + let value = libc::$name; + #[cfg(not(target_os = "macos"))] + let value = $val; + value + } + }; + }This would allow usage like:
pub const S_IFDIR: Mode = platform_const!(unix S_IFDIR, 0o040000);However, if this approach also has issues with
#[pyattr], the current solution is acceptable.
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
vm/src/stdlib/stat.rs(1 hunks)
📓 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/stdlib/stat.rs
🧠 Learnings (2)
📓 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
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.
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.
vm/src/stdlib/stat.rs (5)
Learnt from: moreal
PR: #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.
Learnt from: moreal
PR: #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.
Learnt from: arihant2math
PR: #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.
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
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
🧰 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/stdlib/stat.rs
🧠 Learnings (2)
📓 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
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.
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.
vm/src/stdlib/stat.rs (5)
Learnt from: moreal
PR: #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.
Learnt from: moreal
PR: #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.
Learnt from: arihant2math
PR: #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.
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
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
🔇 Additional comments (4)
vm/src/stdlib/stat.rs (4)
48-137: Clean consolidation of Unix mode constants!The refactoring successfully eliminates the duplicate
#[cfg(unix)]and#[cfg(not(unix))]declarations while maintaining the correct constant values. The approach is consistent and improves maintainability.
139-158: Appropriate use of inline cfg_if! for complex platform conditions.The inline
cfg_if!blocks are the right choice here since these constants require more specific platform checks (excluding Android and Redox) that don't fit the simpleunix_libc_get!pattern.
162-171: Excellent simplification of Windows constants!Re-exporting the constants directly from
windows_sysis a much cleaner approach than individually declaring each constant. This ensures the values stay in sync with the Windows API definitions.
175-232: Well-structured platform-specific constant definitions.The implementation correctly:
- Uses
macos_libc_get!for macOS-preferring constants- Applies platform-specific values for
SF_SETTABLE- Follows the learning about conditionally declaring
SF_SUPPORTEDandSF_SYNTHETIConly for macOS- Defines cross-platform constants directly when they have fixed values
This approach aligns with CPython's platform-specific constant handling.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
vm/src/stdlib/stat.rs (2)
86-90: Consider usinglibc_const!macro for consistency.For consistency with other constants, consider using the
libc_const!macro pattern:-pub const S_IFWHT: Mode = if cfg!(target_os = "macos") { - 0o160000 -} else { - 0 -}; +pub const S_IFWHT: Mode = libc_const!( + #[cfg(target_os = "macos")] + S_IFWHT, + 0 +);
306-310: Consider usinglibc_const!macro for consistency.Similar to S_IFWHT, SF_SETTABLE could use the macro pattern for consistency:
-pub const SF_SETTABLE: u32 = if cfg!(target_os = "macos") { - 0x3fff0000 -} else { - 0xffff0000 -}; +pub const SF_SETTABLE: u32 = libc_const!( + #[cfg(target_os = "macos")] + SF_SETTABLE, + 0xffff0000 +);
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
vm/src/stdlib/stat.rs(1 hunks)
📓 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/stdlib/stat.rs
🧠 Learnings (2)
📓 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
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.
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.
vm/src/stdlib/stat.rs (5)
Learnt from: moreal
PR: #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.
Learnt from: moreal
PR: #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.
Learnt from: arihant2math
PR: #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.
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
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
🧰 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/stdlib/stat.rs
🧠 Learnings (2)
📓 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
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.
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.
vm/src/stdlib/stat.rs (5)
Learnt from: moreal
PR: #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.
Learnt from: moreal
PR: #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.
Learnt from: arihant2math
PR: #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.
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
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
🔇 Additional comments (3)
vm/src/stdlib/stat.rs (3)
14-25: LGTM!The
libc_const!macro implementation is clean and follows the pattern discussed in previous reviews. It properly handles conditional compilation with appropriate fallbacks.
229-238: Excellent refactoring of Windows constants!Re-exporting Windows file attributes collectively is much cleaner than individual declarations. This reduces code duplication and improves maintainability.
329-335: Correct implementation of platform-specific constants!The macOS-specific constants
SF_SUPPORTEDandSF_SYNTHETICare properly conditionally declared only for macOS, following CPython's approach as mentioned in the retrieved learnings. This is the correct pattern for platform-specific constants.
Summary by CodeRabbit