st_file_attributes by youknowone · Pull Request #6353 · RustPython/RustPython

youknowone

📜 Review details

Configuration used: Path: .coderabbit.yml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between a3d638a and 5a5944b.

⛔ Files ignored due to path filters (2)

Lib/test/test_os.py is excluded by !Lib/**
Lib/test/test_site.py is excluded by !Lib/**

📒 Files selected for processing (1)

crates/vm/src/stdlib/os.rs (3 hunks)

🧰 Additional context used

📓 Path-based instructions (1)

**/*.rs

📄 CodeRabbit inference engine (.github/copilot-instructions.md)

**/*.rs: Follow the default rustfmt code style by running cargo fmt to format Rust code
Always run clippy to lint Rust code (cargo clippy) before completing tasks and fix any warnings or lints introduced by 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:

crates/vm/src/stdlib/os.rs

🧠 Learnings (3)

📓 Common learnings

Learnt from: moreal
Repo: RustPython/RustPython 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
Repo: RustPython/RustPython 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.

📚 Learning: 2025-06-27T14:47:28.810Z

Learnt from: moreal
Repo: RustPython/RustPython 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.

Applied to files:

crates/vm/src/stdlib/os.rs

📚 Learning: 2025-06-27T14:47:28.810Z

Learnt from: moreal
Repo: RustPython/RustPython 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.

Applied to files:

crates/vm/src/stdlib/os.rs

🧬 Code graph analysis (1)

crates/vm/src/stdlib/os.rs (2)

crates/derive-impl/src/lib.rs (1)

pystruct_sequence (61-63)

crates/derive/src/lib.rs (1)

pystruct_sequence (235-239)

🔇 Additional comments (2)

crates/vm/src/stdlib/os.rs (2)

781-783: StatResultData gains st_file_attributes with appropriate typing and defaults

Adding pub st_file_attributes: u32 with #[pyarg(any, default)] and #[pystruct_sequence(skip)] is consistent with how st_reparse_tag is modeled, and the u32 type matches the underlying Win32 file-attribute representation. I don’t see issues with this field definition.

841-842: Struct initialization correctly threads through st_file_attributes

Including st_file_attributes in the Self { ... } initializer ensures the new field is always populated from the cfg-gated local variable. This keeps StatResultData::from_stat self-contained and in sync with the struct definition.