Fix clippy warnings and enforce -Dwarnings (#224)

jamesmunns · web-flow · commit ba471c8d448f · 2023-08-11T18:25:15.000-05:00
This PR fixes existing clippy warnings and adds `-Dwarnings` to the clippy invocations of the justfile. I used the CLI args rather than RUSTFLAGS as there can be some issues with overriding other rustflags used. See rust-lang/cargo#8424 for more context re: rustflags issues
diff --git a/justfile b/justfile
@@ -15,7 +15,17 @@ _rustflags := env_var_or_default("RUSTFLAGS", "")
 
 # If we're running in Github Actions and cargo-action-fmt is installed, then add
 # a command suffix that formats errors.
-_fmt := if env_var_or_default("GITHUB_ACTIONS", "") != "true" { "" } else {
+#
+# Clippy version also gets -Dwarnings.
+_fmt_clippy := if env_var_or_default("GITHUB_ACTIONS", "") != "true" { "-- -Dwarnings" } else {
+    ```
+    if command -v cargo-action-fmt >/dev/null 2>&1; then
+        echo "--message-format=json -- -Dwarnings | cargo-action-fmt"
+    fi
+    ```
+}
+
+_fmt_check_doc := if env_var_or_default("GITHUB_ACTIONS", "") != "true" { "" } else {
     ```
     if command -v cargo-action-fmt >/dev/null 2>&1; then
         echo "--message-format=json | cargo-action-fmt"
@@ -47,27 +57,28 @@ default:
 check: && (check-crate _d1_pkg) (check-crate _espbuddy_pkg) (check-crate _x86_bootloader_pkg)
     {{ _cargo }} check \
         --lib --bins --examples --tests --benches \
-        {{ _fmt }}
+        {{ _fmt_check_doc }}
 
 # check a crate.
 check-crate crate:
     {{ _cargo }} check \
         --lib --bins --examples --tests --benches --all-features \
         --package {{ crate }} \
-        {{ _fmt }}
+        {{ _fmt_check_doc }}
 
 # run Clippy checks for all crates, across workspaces.
 clippy: && (clippy-crate _d1_pkg) (clippy-crate _espbuddy_pkg) (clippy-crate _x86_bootloader_pkg)
     {{ _cargo }} clippy \
         --lib --bins --examples --tests --benches --all-features \
-        {{ _fmt }}
+        {{ _fmt_clippy }}
 
 # run clippy checks for a crate.
+# NOTE: -Dwarnings is added by _fmt because reasons
 clippy-crate crate:
     {{ _cargo }} clippy \
         --lib --bins --examples --tests --benches \
         --package {{ crate }} \
-        {{ _fmt }}
+        {{ _fmt_clippy }}
 
 # test all packages, across workspaces
 test: (_get-cargo-command "nextest" "cargo-nextest" no-nextest)
@@ -138,7 +149,7 @@ docs *FLAGS:
         {{ _cargo }} doc \
         --all-features \
         {{ FLAGS }} \
-        {{ _fmt }}
+        {{ _fmt_check_doc }}
 
 _get-cargo-command name pkg skip='':
     #!/usr/bin/env bash
@@ -158,4 +169,4 @@ _get-cargo-command name pkg skip='':
     err "missing cargo-{{ name }} executable"
     if confirm "       install it?"; then
         cargo install {{ pkg }}
-    fi
+    fi
diff --git a/platforms/allwinner-d1/boards/build.rs b/platforms/allwinner-d1/boards/build.rs
@@ -6,7 +6,7 @@ use std::path::Path;
 fn main() {
     let out_dir = env::var("OUT_DIR").expect("No out dir");
     let dest_path = Path::new(&out_dir);
-    let mut f = File::create(&dest_path.join("memory.x")).expect("Could not create file");
+    let mut f = File::create(dest_path.join("memory.x")).expect("Could not create file");
 
     f.write_all(include_bytes!("memory.x"))
         .expect("Could not write file");
diff --git a/platforms/allwinner-d1/boards/src/bin/mq-pro.rs b/platforms/allwinner-d1/boards/src/bin/mq-pro.rs
@@ -128,6 +128,8 @@ fn main() -> ! {
     d1.run()
 }
 
+// Note: pass by ref mut to enforce exclusive access
+#[allow(clippy::needless_pass_by_ref_mut)]
 fn init_i2c_puppet_irq(gpio: &mut d1_pac::GPIO, plic: &mut Plic) -> &'static WaitCell {
     use d1_pac::Interrupt;
     use mnemos_d1_core::plic::Priority;
diff --git a/platforms/allwinner-d1/core/src/clint.rs b/platforms/allwinner-d1/core/src/clint.rs
@@ -21,6 +21,12 @@ impl Clint {
         self.clint
     }
 
+    /// Summon the clint peripheral
+    ///
+    /// # Safety
+    ///
+    /// This is intended for use in interrupt context. Care should be taken not to have
+    /// multiple instances live at the same time that may race or cause other UB issues
     #[must_use]
     pub unsafe fn summon() -> Self {
         Self {
diff --git a/platforms/allwinner-d1/core/src/drivers/spim.rs b/platforms/allwinner-d1/core/src/drivers/spim.rs
@@ -1,4 +1,7 @@
-// Spi Sender
+// Note: We sometimes force a pass by ref mut to enforce exclusive access
+#![allow(clippy::needless_pass_by_ref_mut)]
+
+//! Spi Sender
 
 use core::ptr::NonNull;
 
diff --git a/platforms/allwinner-d1/core/src/drivers/twi.rs b/platforms/allwinner-d1/core/src/drivers/twi.rs
@@ -1,3 +1,6 @@
+// Note: We sometimes force a pass by ref mut to enforce exclusive access
+#![allow(clippy::needless_pass_by_ref_mut)]
+
 //! Drivers for the Allwinner D1's I²C/TWI peripherals.
 //!
 //! This module contains an implementation of a driver for controlling the
diff --git a/platforms/allwinner-d1/core/src/drivers/uart.rs b/platforms/allwinner-d1/core/src/drivers/uart.rs
@@ -1,3 +1,6 @@
+// Note: We sometimes force a pass by ref mut to enforce exclusive access
+#![allow(clippy::needless_pass_by_ref_mut)]
+
 use d1_pac::{GPIO, UART0};
 
 use core::{
diff --git a/platforms/beepy/src/i2c_puppet.rs b/platforms/beepy/src/i2c_puppet.rs
@@ -241,7 +241,7 @@ pub enum RegistrationError {
 // https://github.com/solderparty/i2c_puppet#protocol
 const ADDR: u8 = 0x1f;
 
-//// i2c_puppet I2C registers
+/// i2c_puppet I2C registers
 mod reg {
     /// To write with a register, we must OR the register number with this mask:
     /// <https://github.com/solderparty/i2c_puppet#protocol>
diff --git a/platforms/esp32c3-buddy/src/bin/qtpy.rs b/platforms/esp32c3-buddy/src/bin/qtpy.rs
@@ -40,7 +40,7 @@ fn main() -> ! {
 
     let k = mnemos_esp32c3_buddy::init();
     mnemos_esp32c3_buddy::spawn_serial(
-        &k,
+        k,
         peripherals.USB_DEVICE,
         &mut system.peripheral_clock_control,
     );
@@ -51,5 +51,5 @@ fn main() -> ! {
     // Alarm 1 will be used to generate "sleep until" interrupts.
     let alarm1 = syst.alarm1;
 
-    mnemos_esp32c3_buddy::run(&k, alarm1)
+    mnemos_esp32c3_buddy::run(k, alarm1)
 }
diff --git a/platforms/esp32c3-buddy/src/bin/xiao.rs b/platforms/esp32c3-buddy/src/bin/xiao.rs
@@ -41,7 +41,7 @@ fn main() -> ! {
 
     mnemos_esp32c3_buddy::spawn_daemons(k);
     mnemos_esp32c3_buddy::spawn_serial(
-        &k,
+        k,
         peripherals.USB_DEVICE,
         &mut system.peripheral_clock_control,
     );
@@ -51,5 +51,5 @@ fn main() -> ! {
     // Alarm 1 will be used to generate "sleep until" interrupts.
     let alarm1 = syst.alarm1;
 
-    mnemos_esp32c3_buddy::run(&k, alarm1)
+    mnemos_esp32c3_buddy::run(k, alarm1)
 }
diff --git a/platforms/esp32c3-buddy/src/heap.rs b/platforms/esp32c3-buddy/src/heap.rs
@@ -38,6 +38,13 @@ impl UnderlyingAllocator for UnderlyingEspHeap {
     ///
     /// May or may not require a call to [UnderlyingAllocator::init()] before the allocator
     /// is actually ready for use.
+    //
+    // clippy note: <https://rust-lang.github.io/rust-clippy/master/index.html#/declare_interior_mutable_const>
+    //
+    // > A “non-constant” const item is a legacy way to supply an initialized value to
+    // > downstream static items (e.g., the std::sync::ONCE_INIT constant). In this
+    // > case the use of const is legit, and this lint should be suppressed.
+    #[allow(clippy::declare_interior_mutable_const)]
     const INIT: Self = UnderlyingEspHeap(EspHeap::empty());
 
     /// Initialize the allocator, if it is necessary to populate with a region
diff --git a/platforms/esp32c3-buddy/src/lib.rs b/platforms/esp32c3-buddy/src/lib.rs
@@ -101,7 +101,7 @@ pub fn run(k: &'static Kernel, alarm1: Alarm<Target, 1>) -> ! {
         // Timer is downcounting
         let elapsed = SystemTimer::now() - start;
 
-        let turn = k.timer().force_advance_ticks(elapsed as u64 / 2u64);
+        let turn = k.timer().force_advance_ticks(elapsed / 2u64);
 
         // If there is nothing else scheduled, and we didn't just wake something up,
         // sleep for some amount of time
@@ -138,7 +138,7 @@ pub fn run(k: &'static Kernel, alarm1: Alarm<Target, 1>) -> ! {
             // Account for time slept
             let elapsed = SystemTimer::now() - wfi_start;
 
-            let _turn = k.timer().force_advance_ticks(elapsed as u64 / 2u64);
+            let _turn = k.timer().force_advance_ticks(elapsed / 2u64);
         }
     }
 }
diff --git a/platforms/melpomene/src/sim_drivers/tcp_serial.rs b/platforms/melpomene/src/sim_drivers/tcp_serial.rs
@@ -64,11 +64,11 @@ impl TcpSerial {
 
         let _hdl = tokio::spawn(
             async move {
-                let mut handle = a_ring;
+                let handle = a_ring;
                 loop {
                     match listener.accept().await {
                         Ok((stream, addr)) => {
-                            process_stream(&mut handle, stream, irq.clone())
+                            process_stream(&handle, stream, irq.clone())
                                 .instrument(info_span!("process_stream", client.addr = %addr))
                                 .await
                         }
@@ -88,7 +88,7 @@ impl TcpSerial {
     }
 }
 
-async fn process_stream(handle: &mut BidiHandle, mut stream: TcpStream, irq: Arc<Notify>) {
+async fn process_stream(handle: &BidiHandle, mut stream: TcpStream, irq: Arc<Notify>) {
     loop {
         // Wait until either the socket has data to read, or the other end of
         // the BBQueue has data to write.
@@ -111,7 +111,7 @@ async fn process_stream(handle: &mut BidiHandle, mut stream: TcpStream, irq: Arc
                 // Try to read data, this may still fail with `WouldBlock`
                 // if the readiness event is a false positive.
                 match stream.try_read(&mut in_grant) {
-                    Ok(used) if used == 0 => {
+                    Ok(0) => {
                         warn!("Empty read, socket probably closed.");
                         return;
                     },
diff --git a/platforms/x86_64/core/src/bin/bootloader/framebuf.rs b/platforms/x86_64/core/src/bin/bootloader/framebuf.rs
@@ -35,6 +35,7 @@ pub(super) unsafe fn mk_framebuf() -> FramebufWriter {
 /// This forcibly unlocks a potentially-locked mutex, violating mutual
 /// exclusion! This should only be called in conditions where no other CPU core
 /// will *ever* attempt to access the framebuffer again (such as while oopsing).
+#[allow(dead_code)]
 pub(super) unsafe fn force_unlock() {
     if let Some((_, fb)) = FRAMEBUFFER.try_get() {
         fb.force_unlock();
diff --git a/platforms/x86_64/core/src/bin/bootloader/main.rs b/platforms/x86_64/core/src/bin/bootloader/main.rs
@@ -64,6 +64,7 @@ pub fn kernel_start(info: &'static mut bootloader_api::BootInfo) -> ! {
 
 #[cold]
 #[cfg_attr(target_os = "none", panic_handler)]
+#[allow(dead_code)]
 fn panic(panic: &core::panic::PanicInfo<'_>) -> ! {
     use core::fmt::Write;
     use embedded_graphics::{
diff --git a/platforms/x86_64/core/src/drivers/framebuf.rs b/platforms/x86_64/core/src/drivers/framebuf.rs
@@ -73,7 +73,7 @@ where
         // if we have reached the bottom of the screen, we'll need to scroll
         // previous framebuffer contents up to make room for new line(s) of
         // text.
-        self.point.y = self.point.y + self.style.font.character_size.height as i32;
+        self.point.y += self.style.font.character_size.height as i32;
         self.point.x = self.start_x;
     }
 }
@@ -133,7 +133,7 @@ where
                 // line, wrap the line.
                 let rem = self.px_to_len(self.width_px - (self.point.x as u32));
                 if line.len() > rem {
-                    let (curr, next) = line.split_at(rem as usize);
+                    let (curr, next) = line.split_at(rem);
                     line = next;
                     chunk = curr;
                     has_newline = true;
diff --git a/platforms/x86_64/core/src/interrupt.rs b/platforms/x86_64/core/src/interrupt.rs
@@ -111,7 +111,7 @@ impl hal_core::interrupt::Handlers<Registers> for InterruptHandlers {
         C: interrupt::Context<Registers = Registers> + interrupt::ctx::CodeFault,
     {
         // TODO: add a nice fault handler
-        let _fault = match cx.details() {
+        match cx.details() {
             Some(deets) => panic!("code fault {}: \n{deets}", cx.fault_kind()),
             None => panic!("code fault {}!", cx.fault_kind()),
         };
diff --git a/platforms/x86_64/core/src/trace.rs b/platforms/x86_64/core/src/trace.rs
@@ -127,7 +127,7 @@ where
                     }
                 }) as &mut dyn tracing::field::Visit,
             );
-            writeln!(&mut writer, "").unwrap();
+            writeln!(&mut writer).unwrap();
 
             self.point
                 .store(pack_point(writer.next_point()), Ordering::Release);

Original file line number	Diff line number	Diff line change
`@@ -128,6 +128,8 @@ fn main() -> ! {`
`128`	`128`	`d1.run()`
`129`	`129`	`}`
`130`	`130`
	`131`	`+// Note: pass by ref mut to enforce exclusive access`
	`132`	`+#[allow(clippy::needless_pass_by_ref_mut)]`
`131`	`133`	`fn init_i2c_puppet_irq(gpio: &mut d1_pac::GPIO, plic: &mut Plic) -> &'static WaitCell {`
`132`	`134`	`use d1_pac::Interrupt;`
`133`	`135`	`use mnemos_d1_core::plic::Priority;`