clippy: dissallow some methods and types (#1411)

* Allow-list some words in our markdown docstrings

* Forbid a bunch of macros, functions and types

* name the threads

* Do not ignore errors

* Replace use of std::sync::Mutex with parking_lot::Mutex

* Warn on failure to spawn analytics threads instead of panicing

Author: emilk

Cargo.lock

@@ -1,5 +1,6 @@
 # https://github.com/ericseppanen/cargo-cranky
 # cargo install cargo-cranky && cargo cranky
+# See also clippy.toml
 
 deny = [
   "unsafe_code",
@@ -17,8 +18,11 @@ warn = [
   "clippy::dbg_macro",
   "clippy::debug_assert_with_mut_call",
   "clippy::derive_partial_eq_without_eq",
-  "clippy::disallowed_methods",
-  "clippy::disallowed_script_idents",
+  "clippy::disallowed_macros",                  # See clippy.toml
+  "clippy::disallowed_methods",                 # See clippy.toml
+  "clippy::disallowed_names",                   # See clippy.toml
+  "clippy::disallowed_script_idents",           # See clippy.toml
+  "clippy::disallowed_types",                   # See clippy.toml
   "clippy::doc_link_with_quotes",
   "clippy::doc_markdown",
   "clippy::empty_enum",

Cranky.toml

@@ -0,0 +1,56 @@
+msrv = "1.67"
+
+
+# https://rust-lang.github.io/rust-clippy/master/index.html#disallowed_macros
+disallowed-macros = [
+  'dbg',
+
+  # TODO(emilk): consider forbidding these to encourage the use of proper log stream, and then explicitly allow legitimate uses
+  # 'std::eprint',
+  # 'std::eprintln',
+  # 'std::print',
+  # 'std::println',
+
+  # 'std::unimplemented', # generated by ArrowDeserialize derive-macro :(
+]
+
+# https://rust-lang.github.io/rust-clippy/master/index.html#disallowed_methods
+disallowed-methods = [
+  "std::env::temp_dir", # Use the tempdir crate instead
+
+  # Disabled because of https://github.com/rust-lang/rust-clippy/issues/10406
+  #   "std::time::Instant::now",      # use `instant` crate instead for wasm/web compatability
+  "std::time::Duration::elapsed", # use `instant` crate instead for wasm/web compatability
+  "std::time::SystemTime::now",   # use `instant` or `time` crates instead for wasm/web compatability
+
+  "std::thread::spawn", # Use `std::thread::Builder` and name the thread
+
+  "sha1::Digest::new", # SHA1 is cryptographically broken
+]
+
+# https://rust-lang.github.io/rust-clippy/master/index.html#disallowed_names
+disallowed-names = []
+
+# https://rust-lang.github.io/rust-clippy/master/index.html#disallowed_types
+disallowed-types = [
+  # Use the faster & simpler non-poisonable primitives in `parking_lot` instead
+  "std::sync::Mutex",
+  "std::sync::RwLock",
+  "std::sync::Condvar",
+  # "std::sync::Once",  # enabled for now as the `log_once` macro uses it internally
+
+  "ring::digest::SHA1_FOR_LEGACY_USE_ONLY", # SHA1 is cryptographically broken
+]
+
+# Allow-list of words for markdown in dosctrings https://rust-lang.github.io/rust-clippy/master/index.html#doc_markdown
+doc-valid-idents = [
+  "GLB",
+  "GLTF",
+  "iOS",
+  "macOS",
+  "NaN",
+  "OBJ",
+  "sRGB",
+  "sRGBA",
+  "WebGL",
+]

clippy.toml

@@ -72,7 +72,7 @@ impl Pipeline {
         // The eventual part comes from the fact that this only runs as part of the Rerun viewer,
         // and as such there's no guarantee it will ever run again, even if there's pending data.
 
-        _ = std::thread::Builder::new()
+        if let Err(err) = std::thread::Builder::new()
             .name("pipeline_catchup".into())
             .spawn({
                 let config = config.clone();
@@ -85,9 +85,12 @@ impl Pipeline {
                     let res = flush_pending_events(&config, &sink);
                     trace!(%analytics_id, %session_id, ?res, "pipeline catchup thread shut down");
                 }
-            });
+            })
+        {
+            re_log::warn!("Failed to spawn analytics thread: {err}");
+        }
 
-        _ = std::thread::Builder::new().name("pipeline".into()).spawn({
+        if let Err(err) = std::thread::Builder::new().name("pipeline".into()).spawn({
             let config = config.clone();
             let event_tx = event_tx.clone();
             move || {
@@ -99,7 +102,9 @@ impl Pipeline {
                     realtime_pipeline(&config, &sink, session_file, tick, &event_tx, &event_rx);
                 trace!(%analytics_id, %session_id, ?res, "pipeline thread shut down");
             }
-        });
+        }) {
+            re_log::warn!("Failed to spawn analytics thread: {err}");
+        }
 
         Ok(Some(Self { event_tx }))
     }
@@ -266,7 +271,7 @@ fn append_event(
         // corrupt row in the analytics file, that we'll simply discard later on.
         // We'll try to write a linefeed one more time, just in case, to avoid potentially
         // impacting other events.
-        _ = session_file.write_all(b"\n");
+        session_file.write_all(b"\n").ok();
         warn!(%err, %analytics_id, %session_id, "couldn't write to analytics data file");
         return Err(event);
     }

crates/re_analytics/src/pipeline_native.rs

@@ -326,7 +326,7 @@ impl ArrowDeserialize for EncodedMesh3D {
 
 // ----------------------------------------------------------------------------
 
-/// The format of a binary mesh file, e.g. `GLTF`, `GLB`, `OBJ`
+/// The format of a binary mesh file, e.g. GLTF, GLB, OBJ
 #[derive(Clone, Copy, Debug, PartialEq, Eq, Hash, ArrowField, ArrowSerialize, ArrowDeserialize)]
 #[arrow_field(type = "dense")]
 #[cfg_attr(feature = "serde", derive(serde::Deserialize, serde::Serialize))]

crates/re_log_types/src/component_types/mesh3d.rs

@@ -54,14 +54,14 @@ re_sdk_comms = { workspace = true, features = ["client"] }
 re_smart_channel.workspace = true
 re_string_interner.workspace = true
 
-
 anyhow.workspace = true
 arrow2.workspace = true
 crossbeam = "0.8"
 document-features = "0.2"
 lazy_static.workspace = true
 nohash-hasher = "0.2"
 once_cell = "1.12"
+parking_lot = "0.12"
 thiserror.workspace = true
 
 # Optional dependencies:

crates/re_sdk/Cargo.toml

@@ -4,7 +4,7 @@ use crate::session::Session;
 ///
 /// By default, logging is enabled. To disable logging, call `set_enabled(false)` on the global `Session`, or
 /// set the `RERUN` environment variable to `false`.
-pub fn global_session() -> std::sync::MutexGuard<'static, Session> {
+pub fn global_session() -> parking_lot::MutexGuard<'static, Session> {
     let default_enabled = true;
     global_session_with_default_enabled(default_enabled)
 }
@@ -15,11 +15,11 @@ pub fn global_session() -> std::sync::MutexGuard<'static, Session> {
 /// It can be overridden with the `RERUN` environment variable.
 pub fn global_session_with_default_enabled(
     default_enabled: bool,
-) -> std::sync::MutexGuard<'static, Session> {
+) -> parking_lot::MutexGuard<'static, Session> {
     use once_cell::sync::OnceCell;
-    use std::sync::Mutex;
+    use parking_lot::Mutex;
     static INSTANCE: OnceCell<Mutex<Session>> = OnceCell::new();
 
     let mutex = INSTANCE.get_or_init(|| Mutex::new(Session::with_default_enabled(default_enabled)));
-    mutex.lock().unwrap()
+    mutex.lock()
 }

crates/re_sdk/src/global.rs

@@ -405,7 +405,10 @@ impl Session {
         let app_env = self.app_env();
 
         // NOTE: Forget the handle on purpose, leave that thread be.
-        _ = std::thread::spawn(move || run(self));
+        std::thread::Builder::new()
+            .name("spawned".into())
+            .spawn(move || run(self))
+            .expect("Failed to spawn thread");
 
         // NOTE: Some platforms still mandate that the UI must run on the main thread, so make sure
         // to spawn the viewer in place and migrate the user callback to a new thread.

crates/re_sdk/src/session.rs

@@ -99,9 +99,12 @@ fn main() {
     let host = host.as_deref().unwrap_or("localhost");
     let port = port.as_deref().unwrap_or("8000");
 
-    std::thread::spawn(|| {
-        cargo_run_wasm::run_wasm_with_css(CSS);
-    });
+    std::thread::Builder::new()
+        .name("cargo_run_wasm".into())
+        .spawn(|| {
+            cargo_run_wasm::run_wasm_with_css(CSS);
+        })
+        .expect("Failed to spawn thread");
 
     // Wait for the server to be up before opening a browser tab.
     let addr = format!("{host}:{port}")