Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #51 +/- ##
==========================================
+ Coverage 98.27% 98.64% +0.37%
==========================================
Files 11 11
Lines 753 887 +134
==========================================
+ Hits 740 875 +135
+ Misses 13 12 -1
... and 1 file with indirect coverage changes Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
al8n
changed the title
There was a problem hiding this comment.
Pull request overview
This PR prepares the fs4 1.1.0 release by unifying the sync/async extension traits at the crate root, sealing them to allow future API evolution, and adding an object-safe async extension trait.
Changes:
- Consolidates per-backend
FileExt/AsyncFileExttraits into crate-rootfs4::FileExtandfs4::AsyncFileExt, with backend modules re-exporting the unified traits. - Seals the extension traits and adds reference blanket impls plus a new object-safe
DynAsyncFileExtbacked by boxed futures. - Updates tests/imports and rustfmt configuration; bumps version and changelog for 1.1.0.
Reviewed changes
Copilot reviewed 17 out of 17 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| src/lib.rs | Introduces sealed/unified FileExt/AsyncFileExt, adds DynAsyncFileExt and boxed future alias, updates backend re-exports. |
| src/file_ext/sync_impl.rs | Refactors macro to implement crate-root FileExt and sealing; adjusts test imports. |
| src/file_ext/async_impl.rs | Refactors macro to implement crate-root AsyncFileExt and adds DynAsyncFileExt impl; adjusts test imports. |
| src/file_ext/async_impl/tokio_impl.rs | Removes now-unneeded backend trait import in tests. |
| src/file_ext/async_impl/async_std_impl.rs | Removes now-unneeded backend trait import in tests. |
| src/file_ext/async_impl/smol_impl.rs | Removes now-unneeded backend trait import in tests. |
| src/file_ext/async_impl/fs_err2_tokio_impl.rs | Removes now-unneeded backend trait import in tests. |
| src/file_ext/async_impl/fs_err3_tokio_impl.rs | Removes now-unneeded backend trait import in tests. |
| src/unix/async_impl.rs | Updates test macro imports to use crate-root AsyncFileExt. |
| src/unix/async_impl/tokio_impl.rs | Removes now-unneeded backend trait import in tests. |
| src/unix/async_impl/async_std_impl.rs | Removes now-unneeded backend trait import in tests. |
| src/unix/async_impl/smol_impl.rs | Removes now-unneeded backend trait import in tests. |
| src/unix/async_impl/fs_err2_tokio_impl.rs | Removes now-unneeded backend trait import in tests. |
| src/unix/async_impl/fs_err3_tokio_impl.rs | Removes now-unneeded backend trait import in tests. |
| rustfmt.toml | Configures rustfmt to format imports at crate granularity. |
| Cargo.toml | Bumps crate version to 1.1.0. |
| CHANGELOG.md | Adds 1.1.0 release notes describing the trait consolidation, sealing, and new dyn trait. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| /// This is the blocking counterpart of [`FileExt::try_lock`]. It mirrors | ||
| /// [`std::fs::File::lock`]. | ||
| fn lock(&self) -> Result<()>; | ||
|
|
||
| /// Attempts to acquire a shared lock on the file, without blocking. | ||
| /// | ||
| /// Returns `Ok(())` if the lock was acquired, or | ||
| /// `Err(`[`TryLockError::WouldBlock`](crate::TryLockError::WouldBlock)`)` | ||
| /// if the file is currently locked. Mirrors | ||
| /// [`std::fs::File::try_lock_shared`]. | ||
| fn try_lock_shared(&self) -> std::result::Result<(), TryLockError>; | ||
|
|
||
| /// Attempts to acquire an exclusive lock on the file, without blocking. | ||
| /// | ||
| /// Returns `Ok(())` if the lock was acquired, or | ||
| /// `Err(`[`TryLockError::WouldBlock`](crate::TryLockError::WouldBlock)`)` | ||
| /// if the file is currently locked. Mirrors [`std::fs::File::try_lock`]. |
There was a problem hiding this comment.
These docs reference std::fs::File::try_lock_shared / std::fs::File::try_lock, which do not exist in std. Update the links/text to point to the corresponding fs4::FileExt methods (or remove the std::fs::File references) so the docs don't contain dead/incorrect links.
| /// This is the blocking counterpart of [`FileExt::try_lock`]. It mirrors | |
| /// [`std::fs::File::lock`]. | |
| fn lock(&self) -> Result<()>; | |
| /// Attempts to acquire a shared lock on the file, without blocking. | |
| /// | |
| /// Returns `Ok(())` if the lock was acquired, or | |
| /// `Err(`[`TryLockError::WouldBlock`](crate::TryLockError::WouldBlock)`)` | |
| /// if the file is currently locked. Mirrors | |
| /// [`std::fs::File::try_lock_shared`]. | |
| fn try_lock_shared(&self) -> std::result::Result<(), TryLockError>; | |
| /// Attempts to acquire an exclusive lock on the file, without blocking. | |
| /// | |
| /// Returns `Ok(())` if the lock was acquired, or | |
| /// `Err(`[`TryLockError::WouldBlock`](crate::TryLockError::WouldBlock)`)` | |
| /// if the file is currently locked. Mirrors [`std::fs::File::try_lock`]. | |
| /// This is the blocking counterpart of [`FileExt::try_lock`]. | |
| fn lock(&self) -> Result<()>; | |
| /// Attempts to acquire a shared lock on the file, without blocking. | |
| /// | |
| /// Returns `Ok(())` if the lock was acquired, or | |
| /// `Err(`[`TryLockError::WouldBlock`](crate::TryLockError::WouldBlock)`)` | |
| /// if the file is currently locked. This is the non-blocking counterpart | |
| /// of [`FileExt::lock_shared`]. | |
| fn try_lock_shared(&self) -> std::result::Result<(), TryLockError>; | |
| /// Attempts to acquire an exclusive lock on the file, without blocking. | |
| /// | |
| /// Returns `Ok(())` if the lock was acquired, or | |
| /// `Err(`[`TryLockError::WouldBlock`](crate::TryLockError::WouldBlock)`)` | |
| /// if the file is currently locked. This is the non-blocking counterpart | |
| /// of [`FileExt::lock`]. |
| fn lock_shared(&self) -> Result<()>; | ||
|
|
||
| /// Acquires an exclusive lock on the file, blocking until the lock can be | ||
| /// acquired. Mirrors [`std::fs::File::lock`]. |
There was a problem hiding this comment.
This doc comment says the method mirrors std::fs::File::lock, but std::fs::File has no such inherent method. Consider referencing fs4::FileExt::lock/fs4::AsyncFileExt::lock instead, or describe the semantics without pointing at std APIs that don't exist.
| /// acquired. Mirrors [`std::fs::File::lock`]. | |
| /// acquired using the platform's file-locking primitive. |
| impl<F: AsyncFileExt + ?Sized> AsyncFileExt for &F { | ||
| #[cfg_attr(not(tarpaulin), inline(always))] | ||
| async fn allocated_size(&self) -> Result<u64> { | ||
| (*self).allocated_size().await | ||
| } |
There was a problem hiding this comment.
The blanket impl impl<F: AsyncFileExt + ?Sized> AsyncFileExt for &F is recursively calling itself (e.g. (*self).allocated_size().await resolves to the same AsyncFileExt for &F impl again). This will cause infinite recursion when using AsyncFileExt methods on references. Prefer fully-qualified calls to the underlying F implementation (e.g. <F as AsyncFileExt>::allocated_size(*self).await, and similarly for allocate/unlock_async).
| /// This is the blocking counterpart of [`FileExt::try_lock`]. It mirrors | ||
| /// [`std::fs::File::lock`]. | ||
| fn lock(&self) -> Result<()>; | ||
|
|
||
| /// Attempts to acquire a shared lock on the file, without blocking. | ||
| /// | ||
| /// Returns `Ok(())` if the lock was acquired, or | ||
| /// `Err(`[`TryLockError::WouldBlock`](crate::TryLockError::WouldBlock)`)` | ||
| /// if the file is currently locked. Mirrors | ||
| /// [`std::fs::File::try_lock_shared`]. | ||
| fn try_lock_shared(&self) -> std::result::Result<(), TryLockError>; | ||
|
|
||
| /// Attempts to acquire an exclusive lock on the file, without blocking. | ||
| /// | ||
| /// Returns `Ok(())` if the lock was acquired, or | ||
| /// `Err(`[`TryLockError::WouldBlock`](crate::TryLockError::WouldBlock)`)` | ||
| /// if the file is currently locked. Mirrors [`std::fs::File::try_lock`]. |
There was a problem hiding this comment.
These docs reference std::fs::File::lock, but the standard library File type does not provide inherent lock/try_lock* methods. Consider updating the wording/link to refer to fs4::FileExt::lock/try_lock (or describe the behavior without pointing at std).
| /// This is the blocking counterpart of [`FileExt::try_lock`]. It mirrors | |
| /// [`std::fs::File::lock`]. | |
| fn lock(&self) -> Result<()>; | |
| /// Attempts to acquire a shared lock on the file, without blocking. | |
| /// | |
| /// Returns `Ok(())` if the lock was acquired, or | |
| /// `Err(`[`TryLockError::WouldBlock`](crate::TryLockError::WouldBlock)`)` | |
| /// if the file is currently locked. Mirrors | |
| /// [`std::fs::File::try_lock_shared`]. | |
| fn try_lock_shared(&self) -> std::result::Result<(), TryLockError>; | |
| /// Attempts to acquire an exclusive lock on the file, without blocking. | |
| /// | |
| /// Returns `Ok(())` if the lock was acquired, or | |
| /// `Err(`[`TryLockError::WouldBlock`](crate::TryLockError::WouldBlock)`)` | |
| /// if the file is currently locked. Mirrors [`std::fs::File::try_lock`]. | |
| /// This is the blocking counterpart of [`FileExt::try_lock`]. | |
| fn lock(&self) -> Result<()>; | |
| /// Attempts to acquire a shared lock on the file, without blocking. | |
| /// | |
| /// Returns `Ok(())` if the lock was acquired, or | |
| /// `Err(`[`TryLockError::WouldBlock`](crate::TryLockError::WouldBlock)`)` | |
| /// if the file is currently locked. | |
| fn try_lock_shared(&self) -> std::result::Result<(), TryLockError>; | |
| /// Attempts to acquire an exclusive lock on the file, without blocking. | |
| /// | |
| /// Returns `Ok(())` if the lock was acquired, or | |
| /// `Err(`[`TryLockError::WouldBlock`](crate::TryLockError::WouldBlock)`)` | |
| /// if the file is currently locked. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 17 out of 17 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| fn lock_shared(&self) -> Result<()>; | ||
|
|
||
| /// Acquires an exclusive lock on the file, blocking until the lock can be | ||
| /// acquired. Mirrors [`std::fs::File::lock`]. |
There was a problem hiding this comment.
This doc comment references std::fs::File::lock, which doesn’t exist, so the intra-doc link will be broken and the statement is misleading. Consider rewording to describe the behavior without referencing a non-existent std method.
| /// acquired. Mirrors [`std::fs::File::lock`]. | |
| /// acquired. |
| mod sealed { | ||
| pub trait Sealed {} | ||
|
|
||
| impl<F: Sealed + ?Sized> Sealed for &F {} | ||
| } |
There was a problem hiding this comment.
FileExt/AsyncFileExt/DynAsyncFileExt (and the sealed module / BoxFuture) are currently defined unconditionally, even though earlier in this file and in the PR description they’re described as only being available/meaningful on unix/windows targets (and feature-gated backends). This changes the public API surface on targets like wasm32-wasi*/--no-default-features (e.g. use fs4::*; will now pull in these traits) and makes the “degrades to just FsStats + TryLockError” statement inaccurate. Consider gating these definitions with #[cfg(any(unix, windows))] (and cfg_sync!/cfg_async! as appropriate), or update the docs/description to match the new always-present API.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 17 out of 17 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| mod sealed { | ||
| pub trait Sealed {} | ||
|
|
||
| impl<F: Sealed + ?Sized> Sealed for &F {} | ||
| } | ||
|
|
||
| /// Extension trait for file which provides allocation and locking methods. | ||
| /// | ||
| /// This trait is sealed and cannot be implemented for types outside of `fs4`. | ||
| /// | ||
| /// ## Notes on File Locks | ||
| /// | ||
| /// This library provides whole-file locks in both shared (read) and exclusive | ||
| /// (read-write) varieties. | ||
| /// | ||
| /// File locks are a cross-platform hazard since the file lock APIs exposed by | ||
| /// operating system kernels vary in subtle and not-so-subtle ways. | ||
| /// | ||
| /// The API exposed by this library can be safely used across platforms as long | ||
| /// as the following rules are followed: | ||
| /// | ||
| /// * Multiple locks should not be created on an individual `File` instance | ||
| /// concurrently. | ||
| /// * Duplicated files should not be locked without great care. | ||
| /// * Files to be locked should be opened with at least read or write | ||
| /// permissions. | ||
| /// * File locks may only be relied upon to be advisory. | ||
| /// | ||
| /// File locks are released automatically when the file handle is closed (for | ||
| /// example when the owning `File` is dropped), so calling [`FileExt::unlock`] | ||
| /// explicitly is optional. | ||
| /// | ||
| /// File locks are implemented with | ||
| /// [`flock(2)`](http://man7.org/linux/man-pages/man2/flock.2.html) on Unix and | ||
| /// [`LockFileEx`](https://learn.microsoft.com/en-us/windows/win32/api/fileapi/nf-fileapi-lockfileex) | ||
| /// on Windows. | ||
| pub trait FileExt: sealed::Sealed { | ||
| /// Returns the amount of physical space allocated for a file. |
There was a problem hiding this comment.
FileExt is now defined without #[cfg(any(unix, windows))]/feature gating, so it becomes part of the public API even on targets where the crate previously compiled down to only FsStats + TryLockError (e.g. wasm32-wasi*) or with --no-default-features. If the intent is to preserve the previous platform/feature-specific API surface (as described in the PR), gate sealed/FileExt (and similarly AsyncFileExt/DynAsyncFileExt/BoxFuture) behind the same cfg conditions used for the backends, or update the docs/PR description to match the new always-present traits.
| /// | ||
| /// Returns `Ok(())` if the lock was acquired, or | ||
| /// `Err(`[`TryLockError::WouldBlock`](crate::TryLockError::WouldBlock)`)` | ||
| /// if the file is currently locked. Mirrors [`std::fs::File::try_lock`]. |
There was a problem hiding this comment.
[std::fs::File::try_lock] is not a valid rustdoc link/method (no such API in std), so this documentation is currently broken/misleading. Update the reference to a real item or rephrase to describe semantics without linking to std methods.
| /// if the file is currently locked. Mirrors [`std::fs::File::try_lock`]. | |
| /// if the file is currently locked. This is the non-blocking counterpart | |
| /// of [`FileExt::lock`]. |
Summary
Unify the per-backend
FileExt/AsyncFileExttraits into a single crate-root trait, seal both, and add an object-safeDynAsyncFileExtmirror so callers can hold fs4 file handles behinddyn(e.g.Box<dyn DynAsyncFileExt>).file_ext!/async_file_ext!macros generated a freshpub trait FileExt/AsyncFileExtper backend module, sofs4::tokio::AsyncFileExtandfs4::async_std::AsyncFileExtwere distinct types. Now there is onefs4::FileExtand onefs4::AsyncFileExtdefined directly inlib.rs; the per-backend modules (fs4::tokio,fs4::async_std,fs4::smol,fs4::fs_err2,fs4::fs_err3,fs4::fs_err2_tokio,fs4::fs_err3_tokio) re-export the unified trait. Method-call sites that imported the trait viausecontinue to compile unchanged.impl<F: FileExt + ?Sized> FileExt for &Fandimpl<F: AsyncFileExt + ?Sized> AsyncFileExt for &F, so the extension methods are now callable through shared references.FileExt,AsyncFileExt, andDynAsyncFileExtnow carry a privatesealed::Sealedsupertrait; the implementing set is closed to the concrete file types fs4 supports plus their&references.DynAsyncFileExt.AsyncFileExtuses RPITIT (-> impl Future<Output = ...>) and is therefore not object-safe. The newDynAsyncFileExtmirrors the same operations behindBoxFuture<'_, T>(apub typealias forPin<Box<dyn Future<Output = T> + Send + 'a>>) so it can be used as a trait object (Box<dyn DynAsyncFileExt>,&dyn DynAsyncFileExt). The impl is generated per-backend in theasync_file_ext!macro so each concrete file type'sSend-ness is checked at build time. Includes its own&Fblanket impl. Sealed. Re-exported alongsideAsyncFileExtfrom each async backend submodule.#[inline(always)]on the thin delegating methods (excluded undertarpaulin).imports_granularity = \"Crate\".FileExt(sync) is already object-safe — all methods return concreteResult<T>— so noDynFileExtis needed;dyn FileExtworks directly. The crate continues to compile onwasm32-wasi*(which has neithercfg(unix)norcfg(windows)); on those targets it degrades to justFsStats+TryLockErroras before.Semver
Minor bump (1.1.0). The new blanket impls and
DynAsyncFileExtare additions. Trait-identity unification (the per-backend traits are now the same trait) and the newSealedsupertrait can in principle break downstream code that relied on the distinctness of the per-backend traits or implementedFileExt/AsyncFileExton its own wrapper type — both patterns are uncommon in practice. The mainstream consumer pattern (calling methods throughuse fs4::<backend>::{FileExt,AsyncFileExt}) is preserved.Full details in CHANGELOG.md.
Test plan