8000 Fix os.access not to raise exception when path doesn't exist by youknowone · Pull Request #6477 · RustPython/RustPython · GitHub 8000
[go: up one dir, main page]
Skip to content

Fix os.access not to raise exception when path doesn't exist #6477

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
youknowone merged 2 commits into from
Dec 24, 2025
Merged

Fix os.access not to raise exception when path doesn't exist #6477

youknowone merged 2 commits into from
Dec 24, 2025
+16 −5

Conversation

@youknowone
Copy link
Member
@youknowone youknowone commented Dec 24, 2025 •
edited by coderabbitai bot
Loading

fix #6334

Summary by CodeRabbit

Bug Fixes & Tests

  • Bug Fixes

    • Improved handling of file access checks for non-existent files, now returning false gracefully instead of propagating errors.

  • Tests

    • Added comprehensive test coverage for file access functionality, including validation of various permission modes and edge cases.

✏️ Tip: You can customize this high-level summary in your review settings.

@coderabbitai
Copy link
Contributor
coderabbitai bot commented Dec 24, 2025 •
edited
Loading
📝 Walkthrough

Walkthrough

The os.access function is refactored to return False for non-existent files instead of raising a FileNotFoundError, matching CPython behavior. Metadata retrieval failures now gracefully short-circuit the access check. Comprehensive tests are added to validate the corrected behavior across various access modes.

Changes

Cohort / File(s) Change Summary
Bug Fix: File Access Error Handling
crates/vm/src/stdlib/posix.rs		 Modified access function to gracefully handle missing files. Replaced unconditional fs::metadata call with a match statement that returns Ok(false) when metadata retrieval fails. Removed error-mapping code that propagated metadata errors as OSErrorBuilder exceptions. File-existence checks now short-circuit early with false for all access modes when the file is absent.
Test Coverage: Access Mode Validation
extra_tests/snippets/stdlib_os.py		 Added comprehensive test suite for os.access behavior, including edge cases: empty paths, non-existent files, and all access mode flags (F_OK, R_OK, W_OK, X_OK). Tests verify expected results against existing files (README.md) and missing files.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Poem

🐰 A file that doesn't exist, we once did fear,
Now softly returns false, crystal clear,
No error raised where CPython wouldn't dare,
The POSIX path hops forward with care! ✨

Pre-merge checks and finishing touches

✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title directly and clearly describes the main change: fixing os.access to not raise exceptions for non-existent paths.
Linked Issues check ✅ Passed The code changes in posix.rs implement the fix to return False for non-existent files instead of raising exceptions, matching CPython behavior as required by issue #6334.
Out of Scope Changes check ✅ Passed All changes are directly related to fixing os.access behavior for non-existent paths as specified in issue #6334; test additions validate the corrected behavior.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
📜 Recent 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 215c5c6 and 79c6ff5.

📒 Files selected for processing (2)
  • crates/vm/src/stdlib/posix.rs
  • extra_tests/snippets/stdlib_os.py
🧰 Additional context used
📓 Path-based instructions (2)
**/*.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/posix.rs
**/*.py

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

**/*.py: In most cases, Python code should not be edited; bug fixes should be made through Rust code modifications only
Follow PEP 8 style for custom Python code
Use ruff for linting Python code

Files:

  • extra_tests/snippets/stdlib_os.py
⏰ 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). (11)
  • GitHub Check: Run snippets and cpython tests (windows-latest)
  • GitHub Check: Check Rust code with clippy
  • GitHub Check: Run rust tests (ubuntu-latest)
  • GitHub Check: Run rust tests (windows-latest)
  • GitHub Check: Run rust tests (macos-latest)
  • GitHub Check: Check the WASM package and demo
  • GitHub Check: Run snippets and cpython tests on wasm-wasi
  • GitHub Check: Run snippets and cpython tests (ubuntu-latest)
  • GitHub Check: Run snippets and cpython tests (macos-latest)
  • GitHub Check: Run tests under miri
  • GitHub Check: Ensure compilation on various targets
🔇 Additional comments (4)
extra_tests/snippets/stdlib_os.py (1)

521-530: LGTM! Comprehensive test coverage for the fix.

The test cases properly validate the corrected behavior where os.access returns False for non-existent and empty paths instead of raising exceptions. The tests cover multiple access modes (F_OK, R_OK, W_OK, X_OK) and include positive assertions for existing files.

crates/vm/src/stdlib/posix.rs (3)

415-417: Logic looks correct for F_OK check.

Once metadata is successfully retrieved, returning True for F_OK (file exists) is the correct behavior.

419-430: Permission checking logic preserved correctly.

The permission evaluation logic using get_right_permission remains unchanged and continues to properly check R_OK, W_OK, and X_OK flags against the file's actual permissions.

408-412: The current implementation correctly matches CPython's behavior—no changes needed.

CPython's os.access() returns False for all syscall failures (ENOENT, EACCES, ENOTDIR, ENAMETOOLONG, etc.), not just file-not-found errors. It does not raise OSError for permission denied, invalid paths, or other I/O errors. Catching all errors with Err(_) => return Ok(false) is the correct approach and aligns with CPython's semantics. The suggested alternative would incorrectly raise exceptions for permission denied and other errors, breaking compatibility.

Likely an incorrect or invalid review comment.

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@youknowone youknowone marked this pull request as ready for review December 24, 2025 04:49
@youknowone youknowone merged commit 014622a into RustPython:main Dec 24, 2025
13 checks passed
@youknowone youknowone deleted the os-access branch December 24, 2025 05:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

os.access raises where CPython would return False

1 participant

@youknowone
0