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

@@ -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 }}

cspell.config.yaml

@@ -27,6 +27,7 @@ words:
   - cbindgen
   - cdylib
   - classpath
+  - clippy
   - comde
   - compatch
   - Condvar

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);
 
 /**

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;
 

library/src/c_api/mod.rs

@@ -220,15 +220,15 @@ fn to_update_result(status: anyhow::Result<UpdateStatus>) -> 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<UpdateStatus>) -> 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.

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(())

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,

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);
 

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)]

library/src/updater.rs

@@ -261,10 +261,7 @@ pub fn check_for_downloadable_update(channel: Option<&str>) -> anyhow::Result<bo
     let (request, url, request_fn) = with_config(|config| {
         let mut config = config.clone();
 
-        match channel {
-            Some(channel) => 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