Improve clippy, and add more docs (#3306)

* Silence a few clippy warnings

* Use named threads

* Remove some deprecated functions

* Document Context and Ui fully

* Use `parking_lot::Mutex` in `eframe`

* Expand clippy.toml files

* build fix

Cargo.lock

@@ -1152,6 +1152,7 @@ dependencies = [
  "js-sys",
  "log",
  "objc",
+ "parking_lot",
  "percent-encoding",
  "pollster",
  "puffin",

clippy.toml

@@ -1,7 +1,68 @@
 # There is also a scripts/clippy_wasm/clippy.toml which forbids some mthods that are not available in wasm.
 
+# -----------------------------------------------------------------------------
+# Section identical to scripts/clippy_wasm/clippy.toml:
+
 msrv = "1.67"
 
+allow-unwrap-in-tests = true
+
+# https://doc.rust-lang.org/nightly/clippy/lint_configuration.html#avoid-breaking-exported-api
+# We want suggestions, even if it changes public API.
+avoid-breaking-exported-api = false
+
+max-fn-params-bools = 2 # TODO(emilk): decrease this to 1
+
+# https://rust-lang.github.io/rust-clippy/master/index.html#/large_include_file
+max-include-file-size = 100000
+
+too-many-lines-threshold = 100
+
+# -----------------------------------------------------------------------------
+
+# https://rust-lang.github.io/rust-clippy/master/index.html#disallowed_macros
+disallowed-macros = [
+    'dbg',
+    'std::unimplemented',
+
+    # 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',
+]
+
+# https://rust-lang.github.io/rust-clippy/master/index.html#disallowed_methods
+disallowed-methods = [
+    "std::env::temp_dir", # Use the tempdir crate instead
+
+    # There are many things that aren't allowed on wasm,
+    # but we cannot disable them all here (because of e.g. https://github.com/rust-lang/rust-clippy/issues/10406)
+    # so we do that in `clipppy_wasm.toml` instead.
+
+    "std::thread::spawn", # Use `std::thread::Builder` and name the thread
+
+    "sha1::Digest::new", # SHA1 is cryptographically broken
+
+    "std::panic::catch_unwind", # We compile with `panic = "abort"`
+]
+
+# 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 = [
     # You must also update the same list in the root `clippy.toml`!

crates/eframe/Cargo.toml

@@ -96,6 +96,8 @@ egui = { version = "0.22.0", path = "../egui", default-features = false, feature
   "log",
 ] }
 log = { version = "0.4", features = ["std"] }
+parking_lot = "0.12"
+static_assertions = "1.1.0"
 thiserror.workspace = true
 
 #! ### Optional dependencies
@@ -106,7 +108,6 @@ egui_glow = { version = "0.22.0", path = "../egui_glow", optional = true, defaul
 glow = { version = "0.12", optional = true }
 ron = { version = "0.8", optional = true, features = ["integer128"] }
 serde = { version = "1", optional = true, features = ["derive"] }
-static_assertions = "1.1.0"
 
 # -------------------------------------------
 # native:

crates/eframe/src/native/run.rs

@@ -1092,6 +1092,8 @@ pub use glow_integration::run_glow;
 mod wgpu_integration {
     use std::sync::Arc;
 
+    use parking_lot::Mutex;
+
     use super::*;
 
     /// State that is initialized when the application is first starts running via
@@ -1104,7 +1106,7 @@ mod wgpu_integration {
     }
 
     struct WgpuWinitApp {
-        repaint_proxy: Arc<std::sync::Mutex<EventLoopProxy<UserEvent>>>,
+        repaint_proxy: Arc<Mutex<EventLoopProxy<UserEvent>>>,
         app_name: String,
         native_options: epi::NativeOptions,
         app_creator: Option<epi::AppCreator>,
@@ -1130,7 +1132,7 @@ mod wgpu_integration {
             );
 
             Self {
-                repaint_proxy: Arc::new(std::sync::Mutex::new(event_loop.create_proxy())),
+                repaint_proxy: Arc::new(Mutex::new(event_loop.create_proxy())),
                 app_name: app_name.to_owned(),
                 native_options,
                 running: None,
@@ -1215,7 +1217,7 @@ mod wgpu_integration {
             );
             #[cfg(feature = "accesskit")]
             {
-                integration.init_accesskit(&window, self.repaint_proxy.lock().unwrap().clone());
+                integration.init_accesskit(&window, self.repaint_proxy.lock().clone());
             }
             let theme = system_theme.unwrap_or(self.native_options.default_theme);
             integration.egui_ctx.set_visuals(theme.egui_visuals());
@@ -1232,7 +1234,6 @@ mod wgpu_integration {
                         let frame_nr = info.current_frame_nr;
                         event_loop_proxy
                             .lock()
-                            .unwrap()
                             .send_event(UserEvent::RequestRepaint { when, frame_nr })
                             .ok();
                     });

crates/egui/src/context.rs

@@ -1,4 +1,5 @@
-// #![warn(missing_docs)]
+#![warn(missing_docs)] // Let's keep `Context` well-documented.
+
 use std::sync::Arc;
 
 use crate::{
@@ -1552,6 +1553,7 @@ impl Context {
 }
 
 impl Context {
+    /// Show a ui for settings (style and tessellation options).
     pub fn settings_ui(&self, ui: &mut Ui) {
         use crate::containers::*;
 
@@ -1574,6 +1576,7 @@ impl Context {
             });
     }
 
+    /// Show the state of egui, including its input and output.
     pub fn inspection_ui(&self, ui: &mut Ui) {
         use crate::containers::*;
         crate::trace!(ui);
@@ -1703,6 +1706,7 @@ impl Context {
         });
     }
 
+    /// Shows the contents of [`Self::memory`].
     pub fn memory_ui(&self, ui: &mut crate::Ui) {
         if ui
             .button("Reset all")
@@ -1803,6 +1807,7 @@ impl Context {
 }
 
 impl Context {
+    /// Edit the active [`Style`].
     pub fn style_ui(&self, ui: &mut Ui) {
         let mut style: Style = (*self.style()).clone();
         style.ui(ui);
@@ -1818,6 +1823,8 @@ impl Context {
     /// the function is still called, but with no other effect.
     ///
     /// No locks are held while the given closure is called.
+    #[allow(clippy::unused_self)]
+    #[inline]
     pub fn with_accessibility_parent(&self, _id: Id, f: impl FnOnce()) {
         // TODO(emilk): this isn't thread-safe - another thread can call this function between the push/pop calls
         #[cfg(feature = "accesskit")]

crates/egui/src/os.rs

@@ -1,3 +1,5 @@
+/// An `enum` of common operating systems.
+#[allow(clippy::upper_case_acronyms)] // `Ios` looks too ugly
 #[derive(Clone, Copy, Debug, PartialEq, Eq, Hash)]
 pub enum OperatingSystem {
     /// Unknown OS - could be wasm
@@ -26,6 +28,7 @@ impl Default for OperatingSystem {
 }
 
 impl OperatingSystem {
+    /// Uses the compile-time `target_arch` to identify the OS.
     pub const fn from_target_os() -> Self {
         if cfg!(target_arch = "wasm32") {
             Self::Unknown

crates/egui/src/ui.rs

@@ -1,4 +1,4 @@
-// #![warn(missing_docs)]
+#![warn(missing_docs)] // Let's keep `Ui` well-documented.
 
 use std::hash::Hash;
 use std::sync::Arc;
@@ -249,11 +249,6 @@ impl Ui {
         self.painter.is_visible()
     }
 
-    #[deprecated = "Renamed is_visible"]
-    pub fn visible(&self) -> bool {
-        self.painter.is_visible()
-    }
-
     /// Calling `set_visible(false)` will cause all further widgets to be invisible,
     /// yet still allocate space.
     ///
@@ -281,6 +276,7 @@ impl Ui {
         }
     }
 
+    /// Read the [`Layout`].
     #[inline]
     pub fn layout(&self) -> &Layout {
         self.placer.layout()
@@ -611,6 +607,7 @@ impl Ui {
         Id::new(self.next_auto_id_source)
     }
 
+    /// Same as `ui.next_auto_id().with(id_source)`
     pub fn auto_id_with<IdSource>(&self, id_source: IdSource) -> Id
     where
         IdSource: Hash,
@@ -618,6 +615,7 @@ impl Ui {
         Id::new(self.next_auto_id_source).with(id_source)
     }
 
+    /// Pretend like `count` widgets have been allocated.
     pub fn skip_ahead_auto_ids(&mut self, count: usize) {
         self.next_auto_id_source = self.next_auto_id_source.wrapping_add(count as u64);
     }
@@ -2038,11 +2036,6 @@ impl Ui {
         InnerResponse::new(inner, self.interact(rect, child_ui.id, Sense::hover()))
     }
 
-    #[deprecated = "Use ui.vertical_centered or ui.centered_and_justified"]
-    pub fn centered<R>(&mut self, add_contents: impl FnOnce(&mut Self) -> R) -> InnerResponse<R> {
-        self.vertical_centered(add_contents)
-    }
-
     /// This will make the next added widget centered and justified in the available space.
     ///
     /// Only one widget may be added to the inner `Ui`!

crates/egui_demo_app/src/backend_panel.rs

@@ -451,10 +451,13 @@ impl EguiWindows {
 
 #[cfg(not(target_arch = "wasm32"))]
 fn call_after_delay(delay: std::time::Duration, f: impl FnOnce() + Send + 'static) {
-    std::thread::spawn(move || {
+    std::thread::Builder::new()
+        .name("call_after_delay".to_owned())
+        .spawn(move || {
             std::thread::sleep(delay);
             f();
-    });
+        })
+        .unwrap();
 }
 
 #[cfg(target_arch = "wasm32")]

crates/epaint/src/bezier.rs

@@ -1,4 +1,6 @@
 #![allow(clippy::many_single_char_names)]
+#![allow(clippy::wrong_self_convention)] // False positives
+
 use std::ops::Range;
 
 use crate::{shape::Shape, Color32, PathShape, Stroke};

crates/epaint/src/mutex.rs

@@ -374,6 +374,8 @@ where
 
 #[cfg(test)]
 mod tests {
+    #![allow(clippy::disallowed_methods)] // Ok for tests
+
     use crate::mutex::Mutex;
     use std::time::Duration;
 
@@ -414,6 +416,8 @@ mod tests {
 #[cfg(feature = "deadlock_detection")]
 #[cfg(test)]
 mod tests_rwlock {
+    #![allow(clippy::disallowed_methods)] // Ok for tests
+
     use crate::mutex::RwLock;
     use std::time::Duration;
 

crates/epaint/src/text/text_layout_types.rs

@@ -1,4 +1,5 @@
 #![allow(clippy::derive_hash_xor_eq)] // We need to impl Hash for f32, but we don't implement Eq, which is fine
+#![allow(clippy::wrong_self_convention)] // We use `from_` to indicate conversion direction. It's non-diomatic, but makes sense in this context.
 
 use std::ops::Range;
 use std::sync::Arc;

scripts/clippy_wasm/clippy.toml

@@ -3,8 +3,26 @@
 # We cannot forbid all these methods in the main `clippy.toml` because of
 # https://github.com/rust-lang/rust-clippy/issues/10406
 
+# -----------------------------------------------------------------------------
+# Section identical to the root clippy.toml:
+
 msrv = "1.67"
 
+allow-unwrap-in-tests = true
+
+# https://doc.rust-lang.org/nightly/clippy/lint_configuration.html#avoid-breaking-exported-api
+# We want suggestions, even if it changes public API.
+avoid-breaking-exported-api = false
+
+max-fn-params-bools = 2 # TODO(emilk): decrease this to 1
+
+# https://rust-lang.github.io/rust-clippy/master/index.html#/large_include_file
+max-include-file-size = 100000
+
+too-many-lines-threshold = 100
+
+# -----------------------------------------------------------------------------
+
 # https://rust-lang.github.io/rust-clippy/master/index.html#disallowed_methods
 disallowed-methods = [
   "std::time::Instant::now",      # use `instant` crate instead for wasm/web compatibility
@@ -17,8 +35,9 @@ disallowed-methods = [
 
 # https://rust-lang.github.io/rust-clippy/master/index.html#disallowed_types
 disallowed-types = [
-  # Cannot spawn threads on wasm:
+  { path = "instant::SystemTime", reason = "Known bugs. Use web-time." },
-  "std::thread::Builder",
+  { path = "std::thread::Builder", reason = "Cannot spawn threads on wasm" },
+  # { path = "std::path::PathBuf", reason = "Can't read/write files on web" }, // TODO(emilk): consider banning Path on wasm
 ]
 
 # Allow-list of words for markdown in dosctrings https://rust-lang.github.io/rust-clippy/master/index.html#doc_markdown