Merge pull request #78 from corewa-rs/ian/pedantic-clippy

ian/pedantic-clippy
corewa-rs · Sep 25, 2021 · d94fc67 · d94fc67
2 parents bfa059f + 5df673c
commit d94fc67
Show file tree

Hide file tree

Showing 25 changed files with 300 additions and 247 deletions.
diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml
@@ -27,8 +27,12 @@ jobs:
       - name: Run clippy linting
         run: cargo clippy --color=always --workspace --verbose --all-targets
         env:
+          # TODO: fix cast warnings
           RUSTFLAGS:
-            "-D warnings"
+            --deny=warnings
+            --deny=clippy::pedantic
+            --allow=clippy::cast_possible_wrap
+            --allow=clippy::cast_possible_truncation
 
   validate:
     strategy:

diff --git a/corewars-core/src/lib.rs b/corewars-core/src/lib.rs
@@ -1,3 +1,6 @@
+// TODO(#43): include these
+#![allow(clippy::missing_panics_doc)]
+
 // Macro-exporting modules
 #[macro_use]
 mod util;

diff --git a/corewars-core/src/load_file.rs b/corewars-core/src/load_file.rs
@@ -1,4 +1,4 @@
-use std::fmt;
+use std::{convert::TryInto, fmt};
 
 use lazy_static::lazy_static;
 use maplit::hashmap;
@@ -41,12 +41,14 @@ impl fmt::Display for Warrior {
 }
 
 impl Warrior {
-    /// The number of instrcutions defined in this Warrior's code
-    pub fn len(&self) -> u32 {
-        self.program.instructions.len() as u32
+    /// The number of instructions defined in this Warrior's code
+    #[must_use]
+    pub fn len(&self) -> usize {
+        self.program.instructions.len()
     }
 
     /// Whether the warrior's program is empty (i.e. 0 instructions)
+    #[must_use]
     pub fn is_empty(&self) -> bool {
         self.program.instructions.is_empty()
     }
@@ -69,37 +71,46 @@ impl fmt::Display for Field {
 }
 
 impl Field {
+    #[must_use]
     pub fn direct(value: i32) -> Self {
         Self {
             address_mode: AddressMode::Direct,
             value: Value::Literal(value),
         }
     }
 
-    pub fn direct_label<S: ToString>(label: S) -> Self {
+    pub fn direct_label<S: ToString>(label: &S) -> Self {
         Self {
             address_mode: AddressMode::Direct,
             value: Value::Label(label.to_string()),
         }
     }
 
+    #[must_use]
     pub fn immediate(value: i32) -> Self {
         Self {
             address_mode: AddressMode::Immediate,
             value: Value::Literal(value),
         }
     }
 
+    #[must_use]
     pub fn unwrap_value(&self) -> i32 {
         self.value.unwrap()
     }
 
+    #[must_use]
     pub fn as_offset(&self, core_size: u32) -> Offset {
         Offset::new(self.unwrap_value(), core_size)
     }
 
     pub fn set_value(&mut self, offset: Offset) {
-        self.value = Value::Literal(offset.value() as i32)
+        self.value = Value::Literal(
+            offset
+                .value()
+                .try_into()
+                .expect("Offset should always be convertible to i32"),
+        );
     }
 }
 
@@ -112,6 +123,7 @@ pub struct Instruction {
 }
 
 impl Instruction {
+    #[must_use]
     pub fn new(opcode: Opcode, a_field: Field, b_field: Field) -> Self {
         let modifier =
             Modifier::default_88_to_94(opcode, a_field.address_mode, b_field.address_mode);
@@ -158,6 +170,6 @@ mod test {
             },
         };
 
-        assert_eq!(Instruction::default(), expected_instruction)
+        assert_eq!(Instruction::default(), expected_instruction);
     }
 }
diff --git a/corewars-core/src/load_file/metadata.rs b/corewars-core/src/load_file/metadata.rs
@@ -34,7 +34,7 @@ impl Metadata {
     /// Parse warrior metadata out of a line. Any comments will be removed and
     /// the resulting string returned, with whitespace trimmed.
     pub fn parse_line(&mut self, line: &str) -> String {
-        let split_line: Vec<&str> = line.splitn(2, ';').map(|p| p.trim()).collect();
+        let split_line: Vec<&str> = line.splitn(2, ';').map(str::trim).collect();
 
         if split_line.len() > 1 {
             let split_comment: Vec<&str> = split_line[1].splitn(2, char::is_whitespace).collect();

diff --git a/corewars-core/src/load_file/offset.rs b/corewars-core/src/load_file/offset.rs
@@ -1,4 +1,7 @@
-use std::ops::{Add, AddAssign, Div, DivAssign, Mul, MulAssign, Rem, RemAssign, Sub, SubAssign};
+use std::{
+    convert::TryInto,
+    ops::{Add, AddAssign, Div, DivAssign, Mul, MulAssign, Rem, RemAssign, Sub, SubAssign},
+};
 
 /// A non-negative offset from the beginning of a core.
 #[derive(Copy, Clone, Debug, Eq, PartialEq)]
@@ -10,16 +13,17 @@ pub struct Offset {
 impl Offset {
     /// Create a new Offset. The value will be adjusted to be within bounds of the core.
     ///
-    /// Panics if `core_size` is invalid. Both 0 and `u32::MAX` are disallowed.
+    /// # Panics
+    /// If `core_size` is invalid. Both 0 and `u32::MAX` are disallowed.
+    #[must_use]
     pub fn new(value: i32, core_size: u32) -> Self {
         // TODO: should there be a minimum allowed core size?
-        let core_isize = core_size as i32;
-        if !core_isize.is_positive() {
+        TryInto::<i32>::try_into(core_size).unwrap_or_else(|_| {
             panic!(
                 "Attempt to create offset with invalid core_size {}",
-                core_isize
+                core_size
             )
-        }
+        });
 
         let mut result = Self {
             value: 0,
@@ -30,29 +34,29 @@ impl Offset {
     }
 
     /// Get the value of the offset. This will always be less than the core size.
+    #[must_use]
     pub fn value(&self) -> u32 {
         self.value
     }
 
     /// Set the value of the offset. The value will be adjusted to be within
     /// bounds of the core size.
     pub fn set_value(&mut self, value: i32) {
-        let core_isize = self.core_size as i32;
-        let new_value = value.rem_euclid(core_isize);
-        self.value = if new_value.is_negative() {
-            new_value + core_isize
-        } else {
-            new_value
-        } as u32;
+        let core_isize = self
+            .core_size
+            .try_into()
+            .expect("Core size should never be > i32::MAX");
+
+        self.value = value.rem_euclid(core_isize) as u32;
     }
 
     /// Verify another offset has the same core size. Panics otherwise
-    fn check_core_size(&self, other: &Self) {
+    fn check_core_size(self, other: Self) {
         if self.core_size != other.core_size {
             panic!(
                 "attempt to add mismatching core sizes: {} != {}",
                 self.core_size, other.core_size
-            )
+            );
         }
     }
 }
@@ -75,7 +79,7 @@ macro_rules! impl_offset_op {
             /// Panics if the  right-hand side has a different `core_size`
             /// than the left-hand side.
             fn $op(self, rhs: Self) -> Self {
-                self.check_core_size(&rhs);
+                self.check_core_size(rhs);
                 let mut result = Self::new(0, self.core_size);
                 result.set_value((self.value as i32).$op(rhs.value as i32));
                 result
@@ -150,17 +154,17 @@ mod tests {
     fn add_offset() {
         let mut offset = Offset::new(0, 12);
 
-        assert_eq!(offset + 17i32, Offset::new(5, 12));
-        assert_eq!(offset + -17i32, Offset::new(7, 12));
+        assert_eq!(offset + 17_i32, Offset::new(5, 12));
+        assert_eq!(offset + -17_i32, Offset::new(7, 12));
         assert_eq!(offset + Offset::new(17, 12), Offset::new(5, 12));
         assert_eq!(offset + Offset::new(-17, 12), Offset::new(7, 12));
-        assert_eq!(offset + 17u32, Offset::new(5, 12));
+        assert_eq!(offset + 17_u32, Offset::new(5, 12));
 
-        offset += 17i32;
+        offset += 17_i32;
         assert_eq!(offset, Offset::new(5, 12));
         offset = Offset::new(0, 12);
 
-        offset += -17i32;
+        offset += -17_i32;
         assert_eq!(offset, Offset::new(7, 12));
         offset = Offset::new(0, 12);
 
@@ -172,25 +176,25 @@ mod tests {
         assert_eq!(offset, Offset::new(7, 12));
         offset = Offset::new(0, 12);
 
-        offset += 17u32;
+        offset += 17_u32;
         assert_eq!(offset, Offset::new(5, 12));
     }
 
     #[test]
     fn sub_offset() {
         let mut offset = Offset::new(0, 12);
 
-        assert_eq!(offset - 17i32, Offset::new(7, 12));
-        assert_eq!(offset - -17i32, Offset::new(5, 12));
+        assert_eq!(offset - 17_i32, Offset::new(7, 12));
+        assert_eq!(offset - -17_i32, Offset::new(5, 12));
         assert_eq!(offset - Offset::new(17, 12), Offset::new(7, 12));
         assert_eq!(offset - Offset::new(-17, 12), Offset::new(5, 12));
-        assert_eq!(offset - 17u32, Offset::new(7, 12));
+        assert_eq!(offset - 17_u32, Offset::new(7, 12));
 
-        offset -= 17i32;
+        offset -= 17_i32;
         assert_eq!(offset, Offset::new(7, 12));
 
         offset = Offset::new(0, 12);
-        offset -= -17i32;
+        offset -= -17_i32;
         assert_eq!(offset, Offset::new(5, 12));
 
         offset = Offset::new(0, 12);
@@ -202,25 +206,25 @@ mod tests {
         assert_eq!(offset, Offset::new(5, 12));
 
         offset = Offset::new(0, 12);
-        offset -= 17u32;
+        offset -= 17_u32;
         assert_eq!(offset, Offset::new(7, 12));
     }
 
     #[test]
     fn mul_offset() {
         let mut offset = Offset::new(2, 12);
 
-        assert_eq!(offset * 5i32, Offset::new(10, 12));
-        assert_eq!(offset * -5i32, Offset::new(2, 12));
+        assert_eq!(offset * 5_i32, Offset::new(10, 12));
+        assert_eq!(offset * -5_i32, Offset::new(2, 12));
         assert_eq!(offset * Offset::new(5, 12), Offset::new(10, 12));
         assert_eq!(offset * Offset::new(-5, 12), Offset::new(2, 12));
-        assert_eq!(offset * 5u32, Offset::new(10, 12));
+        assert_eq!(offset * 5_u32, Offset::new(10, 12));
 
-        offset *= 5i32;
+        offset *= 5_i32;
         assert_eq!(offset, Offset::new(10, 12));
 
         offset = Offset::new(2, 12);
-        offset *= -5i32;
+        offset *= -5_i32;
         assert_eq!(offset, Offset::new(2, 12));
 
         offset = Offset::new(2, 12);
@@ -232,25 +236,25 @@ mod tests {
         assert_eq!(offset, Offset::new(2, 12));
 
         offset = Offset::new(2, 12);
-        offset *= 5u32;
+        offset *= 5_u32;
         assert_eq!(offset, Offset::new(10, 12));
     }
 
     #[test]
     fn div_offset() {
         let mut offset = Offset::new(10, 12);
 
-        assert_eq!(offset / 5i32, Offset::new(2, 12));
-        assert_eq!(offset / -5i32, Offset::new(1, 12));
+        assert_eq!(offset / 5_i32, Offset::new(2, 12));
+        assert_eq!(offset / -5_i32, Offset::new(1, 12));
         assert_eq!(offset / Offset::new(5, 12), Offset::new(2, 12));
         assert_eq!(offset / Offset::new(-5, 12), Offset::new(1, 12));
-        assert_eq!(offset / 5u32, Offset::new(2, 12));
+        assert_eq!(offset / 5_u32, Offset::new(2, 12));
 
-        offset /= 5i32;
+        offset /= 5_i32;
         assert_eq!(offset, Offset::new(2, 12));
 
         offset = Offset::new(10, 12);
-        offset /= -5i32;
+        offset /= -5_i32;
         assert_eq!(offset, Offset::new(1, 12));
 
         offset = Offset::new(10, 12);
@@ -262,25 +266,25 @@ mod tests {
         assert_eq!(offset, Offset::new(1, 12));
 
         offset = Offset::new(10, 12);
-        offset /= 5u32;
+        offset /= 5_u32;
         assert_eq!(offset, Offset::new(2, 12));
     }
 
     #[test]
     fn rem_offset() {
         let mut offset = Offset::new(8, 12);
 
-        assert_eq!(offset % 5i32, Offset::new(3, 12));
-        assert_eq!(offset % -5i32, Offset::new(1, 12));
+        assert_eq!(offset % 5_i32, Offset::new(3, 12));
+        assert_eq!(offset % -5_i32, Offset::new(1, 12));
         assert_eq!(offset % Offset::new(5, 12), Offset::new(3, 12));
         assert_eq!(offset % Offset::new(-5, 12), Offset::new(1, 12));
-        assert_eq!(offset % 5u32, Offset::new(3, 12));
+        assert_eq!(offset % 5_u32, Offset::new(3, 12));
 
-        offset %= 5i32;
+        offset %= 5_i32;
         assert_eq!(offset, Offset::new(3, 12));
 
         offset = Offset::new(8, 12);
-        offset %= -5i32;
+        offset %= -5_i32;
         assert_eq!(offset, Offset::new(1, 12));
 
         offset = Offset::new(8, 12);
@@ -292,7 +296,7 @@ mod tests {
         assert_eq!(offset, Offset::new(1, 12));
 
         offset = Offset::new(8, 12);
-        offset %= 5u32;
+        offset %= 5_u32;
         assert_eq!(offset, Offset::new(3, 12));
     }
 }
diff --git a/corewars-core/src/load_file/program.rs b/corewars-core/src/load_file/program.rs
@@ -20,6 +20,7 @@ pub struct Program {
 }
 
 impl Program {
+    #[must_use]
     pub fn get(&self, index: usize) -> Option<Instruction> {
         self.instructions.get(index).cloned()
     }
@@ -41,7 +42,7 @@ impl fmt::Debug for Program {
         let lines = self
             .instructions
             .iter()
-            .map(|s| s.to_string())
+            .map(std::string::ToString::to_string)
             .collect::<Vec<_>>();
 
         write!(formatter, "lines: {:#?},", lines)?;
@@ -60,7 +61,7 @@ impl fmt::Display for Program {
             self.origin.unwrap_or_default()
         ));
 
-        for instruction in self.instructions.iter() {
+        for instruction in &self.instructions {
             lines.push(instruction.to_string());
         }