chore: fix 30 of the 54 warnings from clippy pedantic. (#81)

I just ran `cargo clippy -- -W clippy::pedantic` and fixed things.

These are more invasive that the default set and the remaining
warnings are mostly about our (abysmal) public docs missing
Error and Panic sections to explain errors and panicks.

library/Cargo.toml

@@ -70,6 +70,6 @@ simple-logging = "2.0.2"
 serial_test = "2.0.0"
 tempdir = "0.3.7"
 
-# https://github.com/eqrion/cbindgen/blob/master/docs.md#buildrs
+# <https://github.com/eqrion/cbindgen/blob/master/docs.md#buildrs>
 [build-dependencies]
 cbindgen = "0.24.0"

library/build.rs

@@ -3,9 +3,9 @@ extern crate cbindgen;
 use std::env;
 
 // See:
-// https://github.com/eqrion/cbindgen/blob/master/docs.md#buildrs
+// <https://github.com/eqrion/cbindgen/blob/master/docs.md#buildrs>
-// https://doc.rust-lang.org/cargo/reference/build-scripts.html
+// <https://doc.rust-lang.org/cargo/reference/build-scripts.html>
-// https://doc.rust-lang.org/cargo/reference/build-script-examples.html
+// <https://doc.rust-lang.org/cargo/reference/build-script-examples.html>
 fn main() {
     let crate_dir = env::var("CARGO_MANIFEST_DIR").unwrap();
 
@@ -16,7 +16,7 @@ fn main() {
             contents.write_to_file("include/updater.h");
         }
         Err(e) => {
-            println!("cargo:warning=Error generating bindings: {}", e);
+            println!("cargo:warning=Error generating bindings: {e}");
             // If we were to exit 1 here we would stop local rust
             // analysis from working. So we just print the error
             // and continue.

library/include/updater.h

@@ -104,11 +104,11 @@ SHOREBIRD_EXPORT void shorebird_start_update_thread(void);
 
 /**
  * Tell the updater that we're launching from what it told us was the
- * next patch to boot from. This will copy the next_boot patch to be the
+ * next patch to boot from. This will copy the next boot patch to be the
- * current_boot patch.
+ * `current_boot` patch.
  *
  * It is required to call this function before calling
- * shorebird_report_launch_success or shorebird_report_launch_failure.
+ * `shorebird_report_launch_success` or `shorebird_report_launch_failure`.
  */
 SHOREBIRD_EXPORT void shorebird_report_launch_start(void);
 

library/src/android.rs

@@ -3,7 +3,7 @@ use std::fs;
 use std::io::{Cursor, Read};
 use std::path::{Path, PathBuf};
 
-// https://stackoverflow.com/questions/67087597/is-it-possible-to-use-rusts-log-info-for-tests
+// <https://stackoverflow.com/questions/67087597/is-it-possible-to-use-rusts-log-info-for-tests>
 #[cfg(test)]
 use std::println as debug; // Workaround to use println! for logs.
 

library/src/c_api.rs

@@ -2,21 +2,21 @@
 
 // Currently manually prefixing all functions with "shorebird_" to avoid
 // name collisions with other libraries.
-// cbindgen:prefix-with-name could do this for us.
+// `cbindgen:prefix-with-name` could do this for us.
 
 /// This file contains the C API for the updater library.
 /// It is intended to be used by language bindings, and is not intended to be
 /// used directly by Rust code.
 /// The C API is not stable and may change at any time.
 /// You can see usage of this API in Shorebird's Flutter engine:
-/// https://github.com/shorebirdtech/engine/blob/shorebird/dev/shell/common/shorebird.cc
+/// <https://github.com/shorebirdtech/engine/blob/shorebird/dev/shell/common/shorebird.cc>
 use std::ffi::{CStr, CString};
 use std::os::raw::c_char;
 use std::path::PathBuf;
 
 use crate::updater;
 
-// https://stackoverflow.com/questions/67087597/is-it-possible-to-use-rusts-log-info-for-tests
+// <https://stackoverflow.com/questions/67087597/is-it-possible-to-use-rusts-log-info-for-tests>
 #[cfg(test)]
 use std::{println as info, println as error}; // Workaround to use println! for logs.
 
@@ -130,11 +130,7 @@ pub extern "C" fn shorebird_should_auto_update() -> bool {
 #[no_mangle]
 pub extern "C" fn shorebird_current_boot_patch_number() -> usize {
     log_on_error(
-        || {
+        || Ok(updater::current_boot_patch()?.map_or(0, |p| p.number)),
-            Ok(updater::current_boot_patch()?
-                .map(|p| p.number)
-                .unwrap_or(0))
-        },
         "fetching next_boot_patch_number",
         0,
     )
@@ -145,7 +141,7 @@ pub extern "C" fn shorebird_current_boot_patch_number() -> usize {
 #[no_mangle]
 pub extern "C" fn shorebird_next_boot_patch_number() -> usize {
     log_on_error(
-        || Ok(updater::next_boot_patch()?.map(|p| p.number).unwrap_or(0)),
+        || Ok(updater::next_boot_patch()?.map_or(0, |p| p.number)),
         "fetching next_boot_patch_number",
         0,
     )
@@ -210,11 +206,11 @@ pub extern "C" fn shorebird_start_update_thread() {
 }
 
 /// Tell the updater that we're launching from what it told us was the
-/// next patch to boot from. This will copy the next_boot patch to be the
+/// next patch to boot from. This will copy the next boot patch to be the
-/// current_boot patch.
+/// `current_boot` patch.
 ///
 /// It is required to call this function before calling
-/// shorebird_report_launch_success or shorebird_report_launch_failure.
+/// `shorebird_report_launch_success` or `shorebird_report_launch_failure`.
 #[no_mangle]
 pub extern "C" fn shorebird_report_launch_start() {
     log_on_error(updater::report_launch_start, "reporting launch start", ());

library/src/cache.rs

@@ -81,7 +81,7 @@ fn generate_client_id() -> String {
 }
 
 impl UpdaterState {
-    /// Creates a new UpdaterState. If client_id is None, a new one will be generated.
+    /// Creates a new `UpdaterState`. If `client_id` is None, a new one will be generated.
     fn new(cache_dir: PathBuf, release_version: String, client_id: Option<String>) -> Self {
         Self {
             cache_dir,
@@ -97,7 +97,7 @@ impl UpdaterState {
     }
 
     pub fn client_id_or_default(&self) -> String {
-        self.client_id.clone().unwrap_or("".to_string())
+        self.client_id.clone().unwrap_or(String::new())
     }
 
     pub fn is_known_good_patch(&self, patch_number: usize) -> bool {
@@ -349,7 +349,7 @@ impl UpdaterState {
             self.slots.resize(index + 1, Slot::default());
         }
         // Set the given slot to the given version.
-        self.slots[index] = slot
+        self.slots[index] = slot;
     }
 
     fn patch_path_for_index(&self, index: usize) -> PathBuf {
@@ -357,10 +357,10 @@ impl UpdaterState {
     }
 
     fn slot_dir_for_index(&self, index: usize) -> PathBuf {
-        Path::new(&self.cache_dir).join(format!("slot_{}", index))
+        Path::new(&self.cache_dir).join(format!("slot_{index}"))
     }
 
-    pub fn install_patch(&mut self, patch: PatchInfo) -> anyhow::Result<()> {
+    pub fn install_patch(&mut self, patch: &PatchInfo) -> anyhow::Result<()> {
         let slot_index = self.available_slot();
         let slot_dir_string = self.slot_dir_for_index(slot_index);
         let slot_dir = PathBuf::from(&slot_dir_string);
@@ -373,7 +373,7 @@ impl UpdaterState {
         if self.is_known_bad_patch(patch.number) {
             return Err(UpdateError::InvalidArgument(
                 "patch".to_owned(),
-                format!("Refusing to install known bad patch: {:?}", patch),
+                format!("Refusing to install known bad patch: {patch:?}"),
             )
             .into());
         }
@@ -394,27 +394,27 @@ impl UpdaterState {
         if let Some(latest) = self.latest_patch_number() {
             if patch.number < latest {
                 warn!(
-                    "Installed patch {} but latest downloaded patch is {:?}",
+                    "Installed patch {} but latest downloaded patch is {latest:?}",
-                    patch.number, latest
+                    patch.number
                 );
             }
         }
         self.save()?;
 
         let path = self.patch_path_for_index(slot_index);
-        if !path.exists() {
+        if path.exists() {
+            debug!("Patch {} installed to {:?}", patch.number, path);
+        } else {
             warn!(
                 "Patch {} installed but does not exist {:?}",
                 patch.number, path
             );
-        } else {
-            debug!("Patch {} installed to {:?}", patch.number, path);
         }
 
         Ok(())
     }
 
-    /// Sets the current_boot slot to the next_boot slot.
+    /// Sets the `current_boot` slot to the `next_boot` slot.
     pub fn activate_current_patch(&mut self) -> Result<(), UpdateError> {
         if self.next_boot_slot_index.is_none() {
             return Err(UpdateError::InvalidState(
@@ -501,11 +501,11 @@ mod tests {
         let tmp_dir = TempDir::new("example").unwrap();
         let mut state = test_state(&tmp_dir);
         assert_eq!(state.latest_patch_number(), None);
-        state.install_patch(fake_patch(&tmp_dir, 1)).unwrap();
+        state.install_patch(&fake_patch(&tmp_dir, 1)).unwrap();
         assert_eq!(state.latest_patch_number(), Some(1));
-        state.install_patch(fake_patch(&tmp_dir, 2)).unwrap();
+        state.install_patch(&fake_patch(&tmp_dir, 2)).unwrap();
         assert_eq!(state.latest_patch_number(), Some(2));
-        state.install_patch(fake_patch(&tmp_dir, 1)).unwrap();
+        state.install_patch(&fake_patch(&tmp_dir, 1)).unwrap();
         // This probably should be Some(2) assuming we didn't write
         // over the top of patch 2 when re-installing patch 1.
         // I expect if we support rollbacks we might be more explicit
@@ -520,7 +520,7 @@ mod tests {
         let bad_patch = fake_patch(&tmp_dir, 1);
         state.mark_patch_as_bad(bad_patch.number).unwrap();
         let number = bad_patch.number;
-        assert!(state.install_patch(bad_patch).is_err());
+        assert!(state.install_patch(&bad_patch).is_err());
 
         // Calling a second time should not error.
         state.mark_patch_as_bad(number).unwrap();
@@ -567,8 +567,7 @@ mod tests {
         let tmp_dir = TempDir::new("example").unwrap();
         let state = UpdaterState::load_or_new_on_error(tmp_dir.path(), "1.0.0+1");
         assert!(state.client_id.is_some());
-        let saved_state =
+        let saved_state = UpdaterState::load_or_new_on_error(tmp_dir.path(), "1.0.0+1");
-            UpdaterState::load_or_new_on_error(tmp_dir.path(), "1.0.0+1");
         assert_eq!(state.client_id, saved_state.client_id);
     }
 

library/src/config.rs

@@ -85,7 +85,7 @@ pub struct UpdateConfig {
 pub fn set_config(
     app_config: AppConfig,
     libapp_path: PathBuf,
-    yaml: YamlConfig,
+    yaml: &YamlConfig,
     network_hooks: NetworkHooks,
 ) -> anyhow::Result<()> {
     with_config_mut(|config| {

library/src/events.rs

@@ -30,16 +30,13 @@ impl<'de> Deserialize<'de> for EventType {
         match s.as_str() {
             "__patch_install__" => Ok(EventType::PatchInstallSuccess),
             "__patch_install_failure__" => Ok(EventType::PatchInstallFailure),
-            _ => Err(serde::de::Error::custom(format!(
+            _ => Err(serde::de::Error::custom(format!("Unknown event type: {s}"))),
-                "Unknown event type: {}",
-                s
-            ))),
         }
     }
 }
 /// Any edits to this struct should be made carefully and in accordance
 /// with our privacy policy:
-/// https://docs.shorebird.dev/privacy
+/// <https://docs.shorebird.dev/privacy>
 /// An event that is sent to the server when a patch is successfully installed.
 #[derive(Debug, Serialize, Deserialize, Clone)]
 pub struct PatchEvent {

library/src/network.rs

@@ -17,11 +17,11 @@ use crate::events::PatchEvent;
 use std::{println as info, println as debug}; // Workaround to use println! for logs.
 
 fn patches_check_url(base_url: &str) -> String {
-    format!("{}/api/v1/patches/check", base_url)
+    format!("{base_url}/api/v1/patches/check")
 }
 
 fn patches_events_url(base_url: &str) -> String {
-    format!("{}/api/v1/patches/events", base_url)
+    format!("{base_url}/api/v1/patches/events")
 }
 
 pub type PatchCheckRequestFn = fn(&str, PatchCheckRequest) -> anyhow::Result<PatchCheckResponse>;
@@ -186,7 +186,7 @@ pub struct Patch {
 
 /// Any edits to this struct should be made carefully and in accordance
 /// with our privacy policy:
-/// https://docs.shorebird.dev/privacy
+/// <https://docs.shorebird.dev/privacy>
 /// The request body for the patch check endpoint.
 #[derive(Debug, Serialize)]
 pub struct PatchCheckRequest {
@@ -194,7 +194,7 @@ pub struct PatchCheckRequest {
     /// app_ids are unique to each app and are used to identify the app
     /// within Shorebird's system (similar to a bundle identifier).  They
     /// are not secret and are safe to share publicly.
-    /// https://docs.shorebird.dev/concepts
+    /// <https://docs.shorebird.dev/concepts>
     pub app_id: String,
     /// The Shorebird channel built into the shorebird.yaml in the app.
     /// This is not currently used, but intended for future use to allow
@@ -218,7 +218,7 @@ pub struct PatchCheckRequest {
 /// The request body for the create patch install event endpoint.
 ///
 /// We may want to consider making this more generic if/when we add more events
-/// using something like https://github.com/dtolnay/typetag.
+/// using something like <https://github.com/dtolnay/typetag>.
 #[derive(Debug, Serialize)]
 pub struct CreatePatchEventRequest {
     event: PatchEvent,

library/src/updater.rs

@@ -61,9 +61,9 @@ impl Display for UpdateError {
     fn fmt(&self, f: &mut Formatter) -> std::fmt::Result {
         match self {
             UpdateError::InvalidArgument(name, value) => {
-                write!(f, "Invalid Argument: {} -> {}", name, value)
+                write!(f, "Invalid Argument: {name} -> {value}")
             }
-            UpdateError::InvalidState(msg) => write!(f, "Invalid State: {}", msg),
+            UpdateError::InvalidState(msg) => write!(f, "Invalid State: {msg}"),
             UpdateError::FailedToSaveState => write!(f, "Failed to save state"),
             UpdateError::BadServerResponse => write!(f, "Bad server response"),
             UpdateError::ConfigNotInitialized => write!(f, "Config not initialized"),
@@ -74,9 +74,9 @@ impl Display for UpdateError {
     }
 }
 
-// AppConfig is the rust API.  ResolvedConfig is the internal storage.
+// `AppConfig` is the rust API.  `ResolvedConfig` is the internal storage.
-// However rusty api would probably used &str instead of String,
+// However rusty api would probably used `&str` instead of `String`,
-// but making &str from CStr* is a bit of a pain.
+// but making `&str` from `CStr*` is a bit of a pain.
 pub struct AppConfig {
     pub cache_dir: String,
     pub release_version: String,
@@ -98,9 +98,9 @@ fn libapp_path_from_settings(original_libapp_paths: &[String]) -> Result<PathBuf
 }
 
 /// Initialize the updater library.
-/// Takes a AppConfig struct and a yaml string.
+/// Takes a `AppConfig` struct and a yaml string.
-/// The yaml string is the contents of the shorebird.yaml file.
+/// The yaml string is the contents of the `shorebird.yaml` file.
-/// The AppConfig struct is information about the running app and where
+/// The `AppConfig` struct is information about the running app and where
 /// the updater should keep its cache.
 pub fn init(app_config: AppConfig, yaml: &str) -> Result<(), UpdateError> {
     #[cfg(any(target_os = "android", test))]
@@ -112,7 +112,7 @@ pub fn init(app_config: AppConfig, yaml: &str) -> Result<(), UpdateError> {
 
     let libapp_path = libapp_path_from_settings(&app_config.original_libapp_paths)?;
     debug!("libapp_path: {:?}", libapp_path);
-    set_config(app_config, libapp_path, config, NetworkHooks::default())
+    set_config(app_config, libapp_path, &config, NetworkHooks::default())
         .map_err(|err| UpdateError::InvalidState(err.to_string()))
 }
 
@@ -135,12 +135,12 @@ pub fn check_for_update() -> anyhow::Result<bool> {
 }
 
 fn check_hash(path: &Path, expected_string: &str) -> anyhow::Result<()> {
+    use sha2::{Digest, Sha256}; // `Digest` is needed for `Sha256::new()`;
+
     let expected = hex::decode(expected_string).context("Invalid hash string from server.")?;
 
-    use sha2::{Digest, Sha256}; // Digest is needed for Sha256::new();
-
     // Based on guidance from:
-    // https://github.com/RustCrypto/hashes#hashing-readable-objects
+    // <https://github.com/RustCrypto/hashes#hashing-readable-objects>
 
     let mut file = fs::File::open(path)?;
     let mut hasher = Sha256::new();
@@ -149,7 +149,7 @@ fn check_hash(path: &Path, expected_string: &str) -> anyhow::Result<()> {
     let hash = hasher.finalize();
     let hash_matches = hash.as_slice() == expected;
     // This is a common error for developers.  We could avoid it entirely
-    // by sending the hash of libapp.so to the server and having the
+    // by sending the hash of `libapp.so` to the server and having the
     // server only send updates when the hash matches.
     // https://github.com/shorebirdtech/updater/issues/56
     if !hash_matches {
@@ -162,9 +162,8 @@ fn check_hash(path: &Path, expected_string: &str) -> anyhow::Result<()> {
             expected_string,
             hex::encode(hash)
         );
-    } else {
-        debug!("Hash match: {:?}", path);
     }
+    debug!("Hash match: {:?}", path);
     Ok(())
 }
 
@@ -176,9 +175,9 @@ fn prepare_for_install(
     download_path: &Path,
     output_path: &Path,
 ) -> anyhow::Result<()> {
-    // We abuse libapp_path to actually be the path to the data dir for now.
+    // We abuse `libapp_path` to actually be the path to the data dir for now.
-    // This is an abuse because the variable name is libapp_path, but
+    // This is an abuse because the variable name is `libapp_path`, but
-    // we're making it point to a the app_data directory instead.
+    // we're making it point to a the `app_data` directory instead.
     let app_dir = &config.libapp_path;
     debug!("app_dir: {:?}", app_dir);
     let base_r = crate::android::open_base_lib(app_dir, "libapp.so")?;
@@ -285,7 +284,7 @@ fn update_internal(_: &UpdaterLockState) -> anyhow::Result<UpdateStatus> {
         let mut state =
             UpdaterState::load_or_new_on_error(&config.cache_dir, &config.release_version);
         // Move/state update should be "atomic" (it isn't today).
-        state.install_patch(patch_info)?;
+        state.install_patch(&patch_info)?;
         info!("Patch {} successfully installed.", patch.number);
         // Should set some state to say the status is "update required" and that
         // we now have a different "next" version of the app from the current
@@ -348,7 +347,7 @@ where
 
 /// The patch which will be run on next boot (which may still be the same
 /// as the current boot).
-/// This may be changed any time update() or start_update_thread() are called.
+/// This may be changed any time `update()` or `start_update_thread()` are called.
 pub fn next_boot_patch() -> anyhow::Result<Option<PatchInfo>> {
     with_config(|config| {
         let state = UpdaterState::load_or_new_on_error(&config.cache_dir, &config.release_version);
@@ -356,9 +355,9 @@ pub fn next_boot_patch() -> anyhow::Result<Option<PatchInfo>> {
     })
 }
 
-/// The patch which is currently booted.  This is None until
+/// The patch which is currently booted.  This is `None` until
-/// report_launch_start() is called at which point it is copied from
+/// `report_launch_start()` is called at which point it is copied from
-/// next_boot_patch.
+/// `next_boot_patch`.
 pub fn current_boot_patch() -> anyhow::Result<Option<PatchInfo>> {
     with_config(|config| {
         let state = UpdaterState::load_or_new_on_error(&config.cache_dir, &config.release_version);
@@ -514,7 +513,7 @@ mod tests {
             let mut state =
                 UpdaterState::load_or_new_on_error(&config.cache_dir, &config.release_version);
             state
-                .install_patch(PatchInfo {
+                .install_patch(&PatchInfo {
                     path: artifact_path,
                     number: 1,
                 })
@@ -627,7 +626,7 @@ mod tests {
             let mut state =
                 UpdaterState::load_or_new_on_error(&config.cache_dir, &config.release_version);
             state
-                .install_patch(PatchInfo {
+                .install_patch(&PatchInfo {
                     path: artifact_path,
                     number: 1,
                 })
@@ -678,7 +677,7 @@ mod tests {
             let mut state =
                 UpdaterState::load_or_new_on_error(&config.cache_dir, &config.release_version);
             state
-                .install_patch(PatchInfo {
+                .install_patch(&PatchInfo {
                     path: artifact_path,
                     number: 1,
                 })

library/src/updater_lock.rs

@@ -39,7 +39,9 @@ where
         // This should never happen. Poisoning only happens if a thread panics
         // while holding the lock, and we never allow the updater thread to
         // panic.
-        Err(std::sync::TryLockError::Poisoned(e)) => panic!("Updater lock poisoned: {:?}", e),
+        Err(std::sync::TryLockError::Poisoned(e)) => {
+            panic!("Updater lock poisoned: {e:?}")
+        }
     }
 }
 

patch/src/bin/string_patch.rs

@@ -1,6 +1,8 @@
 // This might combine with patch/main.rs. Just starting with a copy for ease.
 
 fn main() {
+    use sha2::{Digest, Sha256}; // Digest is needed for Sha256::new();
+
     let mut args = std::env::args();
     args.next(); // skip program name
     let older = args.next().expect("base string");
@@ -14,13 +16,12 @@ fn main() {
 
     let patch = patch.into_inner();
 
-    use sha2::{Digest, Sha256}; // Digest is needed for Sha256::new();
     let mut hasher = Sha256::new();
     hasher.update(&newer);
     let hash = hasher.finalize();
 
-    println!("Base: {}", older);
+    println!("Base: {older}");
-    println!("New: {}", newer);
+    println!("New: {newer}");
-    println!("Patch: {:?}", patch);
+    println!("Patch: {patch:?}");
     println!("Hash (new): {}", hex::encode(hash));
 }