From 50954da68a607c5a3f262288534b9d645e182e5f Mon Sep 17 00:00:00 2001 From: Eric Seidel Date: Wed, 1 Apr 2026 19:02:05 -0700 Subject: [PATCH] style: fix clippy warnings and add clippy check to CI (#321) * style: fix clippy warnings and add clippy check to CI Runs `cargo clippy --all-targets -- -D warnings` in the rust_crate CI action to catch lint issues before they land. Fixes: redundant field names, needless returns, unnecessary closures, io_other_error, needless borrows, single_match, unnecessary_unwrap, and adds missing safety docs. * chore: add 'clippy' to cspell dictionary --- .github/actions/rust_crate/action.yaml | 5 +++++ cspell.config.yaml | 1 + library/include/updater.h | 8 ++++++++ library/src/c_api/c_file.rs | 4 ++-- library/src/c_api/mod.rs | 16 +++++++++++----- library/src/cache/updater_state.rs | 6 +++--- library/src/events.rs | 1 + library/src/file_errors.rs | 5 +++-- library/src/network.rs | 1 + library/src/updater.rs | 9 +++------ 10 files changed, 38 insertions(+), 18 deletions(-) diff --git a/.github/actions/rust_crate/action.yaml b/.github/actions/rust_crate/action.yaml index a02ec4f..92fce3b 100644 --- a/.github/actions/rust_crate/action.yaml +++ b/.github/actions/rust_crate/action.yaml @@ -13,6 +13,11 @@ inputs: runs: using: "composite" steps: + - name: Clippy + working-directory: ${{ inputs.working_directory }} + shell: ${{ inputs.shell }} + run: cargo clippy --all-targets -- -D warnings + - name: Build working-directory: ${{ inputs.working_directory }} shell: ${{ inputs.shell }} diff --git a/cspell.config.yaml b/cspell.config.yaml index 24df0f5..5e47dec 100644 --- a/cspell.config.yaml +++ b/cspell.config.yaml @@ -27,6 +27,7 @@ words: - cbindgen - cdylib - classpath + - clippy - comde - compatch - Condvar diff --git a/library/include/updater.h b/library/include/updater.h index 6578ce5..df1785c 100644 --- a/library/include/updater.h +++ b/library/include/updater.h @@ -156,6 +156,14 @@ SHOREBIRD_EXPORT char *shorebird_next_boot_patch_path(void); */ SHOREBIRD_EXPORT void shorebird_free_string(const char *c_string); +/** + * Frees an `UpdateResult` previously returned by `shorebird_check_for_update`. + * + * # Safety + * + * `result` must be a valid pointer returned by `shorebird_check_for_update`, + * or null (in which case this is a no-op). + */ SHOREBIRD_EXPORT void shorebird_free_update_result(struct UpdateResult *result); /** diff --git a/library/src/c_api/c_file.rs b/library/src/c_api/c_file.rs index 8446343..c827a91 100644 --- a/library/src/c_api/c_file.rs +++ b/library/src/c_api/c_file.rs @@ -55,8 +55,7 @@ impl Seek for CFile { }; let result = (self.file_callbacks.seek)(self.handle, offset, whence); if result < 0 { - Err(std::io::Error::new( - std::io::ErrorKind::Other, + Err(std::io::Error::other( format!("CFile seek failed with error code: {}", result), )) } else { @@ -66,6 +65,7 @@ impl Seek for CFile { } #[cfg(test)] +#[allow(static_mut_refs)] // Test-only statics guarded by #[serial]; will migrate to SyncUnsafeCell when stabilized. mod test { use serial_test::serial; diff --git a/library/src/c_api/mod.rs b/library/src/c_api/mod.rs index dc385c4..97f78de 100644 --- a/library/src/c_api/mod.rs +++ b/library/src/c_api/mod.rs @@ -220,15 +220,15 @@ fn to_update_result(status: anyhow::Result) -> UpdateResult { return UpdateResult { status: status as i32, message: allocate_c_string(message.as_str()) - .unwrap_or_else(|_| std::ptr::null_mut()), + .unwrap_or(std::ptr::null_mut()), }; } Err(err) => UpdateResult { status: SHOREBIRD_UPDATE_ERROR, - message: allocate_c_string(&err.to_string()).unwrap_or_else(|_| std::ptr::null_mut()), + message: allocate_c_string(&err.to_string()).unwrap_or(std::ptr::null_mut()), }, }; - return result; + result } /// Performs integrity checks on the next boot patch. If the patch fails these checks, the patch @@ -237,7 +237,7 @@ fn to_update_result(status: anyhow::Result) -> UpdateResult { #[no_mangle] pub extern "C" fn shorebird_validate_next_boot_patch() { log_on_error( - || updater::validate_next_boot_patch(), + updater::validate_next_boot_patch, "validating next_boot_patch", (), ); @@ -272,6 +272,12 @@ pub unsafe extern "C" fn shorebird_free_string(c_string: *const c_char) { } } +/// Frees an `UpdateResult` previously returned by `shorebird_check_for_update`. +/// +/// # Safety +/// +/// `result` must be a valid pointer returned by `shorebird_check_for_update`, +/// or null (in which case this is a no-op). #[no_mangle] pub unsafe extern "C" fn shorebird_free_update_result(result: *mut UpdateResult) { if result.is_null() { @@ -337,7 +343,7 @@ pub extern "C" fn shorebird_update_with_result(c_channel: *const c_char) -> *con Ok(channel) => to_update_result(updater::update(channel.as_deref())), Err(err) => to_update_result(Err(err)), }; - return Box::into_raw(Box::new(result)); + Box::into_raw(Box::new(result)) } /// Start a thread to download an update if one is available. diff --git a/library/src/cache/updater_state.rs b/library/src/cache/updater_state.rs index 285ff29..7c4241b 100644 --- a/library/src/cache/updater_state.rs +++ b/library/src/cache/updater_state.rs @@ -99,7 +99,7 @@ impl UpdaterState { verification_mode, )), serialized_state: SerializedState { - client_id: client_id, + client_id, release_version, queued_events: Vec::new(), }, @@ -557,7 +557,7 @@ mod tests { let tmp_dir = TempDir::new()?; // Create a new state, add a patch, and save it. - let mut state = UpdaterState::load_or_new_on_error(&tmp_dir.path(), "1.0.0+1", None, PatchVerificationMode::default()); + let mut state = UpdaterState::load_or_new_on_error(tmp_dir.path(), "1.0.0+1", None, PatchVerificationMode::default()); let patch = fake_patch(&tmp_dir, 1); state.install_patch(&patch, "hash", None)?; state.save()?; @@ -568,7 +568,7 @@ mod tests { std::fs::write(&state_file, "corrupt json")?; // Ensure that, by corrupting the file, we've reset the patches state. - let mut state = UpdaterState::load_or_new_on_error(&tmp_dir.path(), "1.0.0+2", None, PatchVerificationMode::default()); + let mut state = UpdaterState::load_or_new_on_error(tmp_dir.path(), "1.0.0+2", None, PatchVerificationMode::default()); assert!(state.next_boot_patch().is_none()); Ok(()) diff --git a/library/src/events.rs b/library/src/events.rs index 60bb7e8..b197c54 100644 --- a/library/src/events.rs +++ b/library/src/events.rs @@ -9,6 +9,7 @@ use crate::{ }; #[derive(Debug, Clone, PartialEq)] +#[allow(clippy::enum_variant_names)] // Prefix matches the domain concept, not a naming mistake. pub enum EventType { PatchInstallSuccess, PatchInstallFailure, diff --git a/library/src/file_errors.rs b/library/src/file_errors.rs index 803c05c..149c920 100644 --- a/library/src/file_errors.rs +++ b/library/src/file_errors.rs @@ -11,6 +11,7 @@ pub enum FileOperation { CreateFile, WriteFile, ReadFile, + #[allow(dead_code)] // Included for completeness; not yet used outside tests. DeleteFile, DeleteDir, RenameFile, @@ -168,7 +169,7 @@ mod tests { #[test] fn test_enhance_io_error_includes_operation_path_and_error() { - let error = Error::new(ErrorKind::Other, "some error"); + let error = Error::other("some error"); let path = Path::new("/some/path/file.txt"); let message = enhance_io_error(&error, FileOperation::ReadFile, path); @@ -179,7 +180,7 @@ mod tests { #[test] fn test_enhance_io_error_no_hint_for_unknown_error() { - let error = Error::new(ErrorKind::Other, "unknown error"); + let error = Error::other("unknown error"); let path = Path::new("/path/file.txt"); let message = enhance_io_error(&error, FileOperation::ReadFile, path); diff --git a/library/src/network.rs b/library/src/network.rs index d974157..879eb58 100644 --- a/library/src/network.rs +++ b/library/src/network.rs @@ -178,6 +178,7 @@ pub const UNEXPECTED_DOWNLOAD: DownloadToPathFn = |_, _, _| panic!("unexpected d #[cfg(test)] /// Panicking placeholder for tests that should never reach the report step. +#[allow(dead_code)] pub const UNEXPECTED_REPORT: ReportEventFn = |_, _| panic!("unexpected report event call"); #[cfg(test)] diff --git a/library/src/updater.rs b/library/src/updater.rs index 5dd7995..42a4f85 100644 --- a/library/src/updater.rs +++ b/library/src/updater.rs @@ -261,10 +261,7 @@ pub fn check_for_downloadable_update(channel: Option<&str>) -> anyhow::Result config.channel = channel.to_string(), - None => {} - } + if let Some(channel) = channel { config.channel = channel.to_string() } Ok(( PatchCheckRequest::new(&config, &client_id), @@ -371,8 +368,8 @@ fn update_internal(_: &UpdaterLockState, channel: Option<&str>) -> anyhow::Resul // Saves state to disk (holds Config lock while writing). let mut config = copy_update_config()?; - if channel.is_some() { - config.channel = channel.unwrap().to_string(); + if let Some(channel) = channel { + config.channel = channel.to_string(); } // We discard any events if we have more than 3 queued to make sure