lint: add extra integer lints, and partially fix some code (#3409)

* lint: enable more clippy checks for bug-prone code

* fix(lint): stop denying lints, to avoid being excluded from Crater

Also categorise lints.

* lint: add some lints to the TODO list

* refactor(arithmetic): partial fixes for some integer arithmetic lints

* Document some weird lint behaviour

.cargo/config.toml

@@ -2,17 +2,65 @@
 [target.'cfg(all())']
 rustflags = [
     # Zebra standard lints for Rust 1.58+
-    "-Dunsafe_code",
-    "-Drust_2021_compatibility",
-    "-Dclippy::await_holding_lock",
 
-    "-Wmissing_docs",
+    # High-risk code
+    "-Dunsafe_code",
+
+    # Potential bugs
+    #
+    # If we deny these lints, we could be excluded from Crater builds:
+    # https://www.reddit.com/r/rust/comments/f5xpib/psa_denywarnings_is_actively_harmful/
+
+    # Compatibility
+    "-Wrust_2021_compatibility",
     "-Wnonstandard_style",
     "-Wfuture_incompatible",
 
+    # Async code
+    "-Wclippy::await_holding_lock",
+    "-Wclippy::await_holding_refcell_ref",
+
+    # Pointers
+    "-Wclippy::cast_ptr_alignment",
+    "-Wclippy::fn_to_numeric_cast_any",
+
+    # Integers
+    "-Wclippy::checked_conversions",
+    "-Wclippy::implicit_saturating_sub",
+    "-Wclippy::invalid_upcast_comparisons",
+    "-Wclippy::range_minus_one",
+    "-Wclippy::range_plus_one",
+    "-Wclippy::unnecessary_cast",
+
+    # Incomplete code
+    "-Wmissing_docs",
+    "-Wclippy::dbg_macro",
+    "-Wclippy::todo",
+
+    # Code styles we want to accept
     "-Aclippy::try_err",
 
     # TODOs:
+    # `cargo fix` might help do these fixes,
+    # or add a config.toml to sub-directories which should allow these lints,
+    # or try allowing the lint in the specific module (lib.rs doesn't seem to work in some cases)
+    #
+    # lint configs that don't work:
+    # - allowing these lints in lib.rs (command-line warn overrides allow in lib.rs?)
+    # - adding a [target.'cfg(not(test))'] rustflags config (it runs on test code anyway)
+
+    # fix code that triggers these lints,
+    # or disable the lint for that code (or for all test code)
+    #
+    #"-Wclippy::cast_lossless", # 30 non-test warnings, a few test warnings
+    #"-Wclippy::cast_possible_truncation", # 40 non-test warnings, 20 test warnings
+    #"-Wclippy::cast_possible_wrap", # 13 test warnings (fixed outside tests)
+    #"-Wclippy::cast_precision_loss", # 25 non-test warnings, 10 test warnings
+    #"-Wclippy::cast_sign_loss", # 6 non-test warnings, 15 test warnings
+
+    # disable these lints using a zebra-test config.toml, but warn for other crates
+    #"-Wclippy::print_stdout",
+    #"-Wclippy::print_stderr",
 
     # fix hidden lifetime parameters
     #"-Wrust_2018_idioms",

zebra-chain/src/block/arbitrary.rs

@@ -59,6 +59,16 @@ pub const PREVOUTS_CHAIN_HEIGHT: usize = 4;
 pub const MAX_PARTIAL_CHAIN_BLOCKS: usize =
     MIN_TRANSPARENT_COINBASE_MATURITY as usize + PREVOUTS_CHAIN_HEIGHT;
 
+impl Arbitrary for Height {
+    type Parameters = ();
+
+    fn arbitrary_with(_args: ()) -> Self::Strategy {
+        (Height::MIN.0..=Height::MAX.0).prop_map(Height).boxed()
+    }
+
+    type Strategy = BoxedStrategy<Self>;
+}
+
 #[derive(Debug, Clone, Copy)]
 #[non_exhaustive]
 /// The configuration data for proptest when generating arbitrary chains

zebra-chain/src/block/height.rs

@@ -125,19 +125,6 @@ impl Sub<i32> for Height {
     }
 }
 
-#[cfg(any(test, feature = "proptest-impl"))]
-use proptest::prelude::*;
-#[cfg(any(test, feature = "proptest-impl"))]
-impl Arbitrary for Height {
-    type Parameters = ();
-
-    fn arbitrary_with(_args: ()) -> Self::Strategy {
-        (Height::MIN.0..=Height::MAX.0).prop_map(Height).boxed()
-    }
-
-    type Strategy = BoxedStrategy<Self>;
-}
-
 #[test]
 fn operator_tests() {
     zebra_test::init();

zebra-chain/src/parameters/network_upgrade.rs

@@ -312,7 +312,7 @@ impl NetworkUpgrade {
     ///
     /// `AveragingWindowTimespan` from the Zcash specification.
     pub fn averaging_window_timespan(&self) -> Duration {
-        self.target_spacing() * (POW_AVERAGING_WINDOW as _)
+        self.target_spacing() * POW_AVERAGING_WINDOW.try_into().expect("fits in i32")
     }
 
     /// Returns the averaging window timespan for `network` and `height`.

zebra-chain/src/work/difficulty.rs

@@ -201,8 +201,9 @@ impl CompactDifficulty {
 
         // The exponent for the multiplier in the floating-point number
         // 256^(floor(x/(2^24)) - 3)
+        //
         // The i32 conversion is safe, because we've just divided self by 2^24.
-        let exponent = ((self.0 >> PRECISION) as i32) - OFFSET;
+        let exponent = i32::try_from(self.0 >> PRECISION).expect("fits in i32") - OFFSET;
 
         // Normalise the mantissa and exponent before multiplying.
         //

zebra-chain/src/work/u256.rs

@@ -2,6 +2,7 @@
 // This is a separate module to make it easier to disable clippy because
 // it raises a lot of issues in the macro.
 #![allow(clippy::all)]
+#![allow(clippy::range_plus_one)]
 
 use uint::construct_uint;
 

zebra-state/src/service.rs

@@ -460,17 +460,17 @@ impl StateService {
                 .expect("the intersection hash must be in the best chain")
         });
         let max_len_height = if let Some(intersection_height) = intersection_height {
+            let max_len = i32::try_from(max_len).expect("max_len fits in i32");
+
             // start after the intersection_height, and return max_len hashes
-            (intersection_height + (max_len as i32))
+            (intersection_height + max_len)
                 .expect("the Find response height does not exceed Height::MAX")
         } else {
+            let max_len = u32::try_from(max_len).expect("max_len fits in u32");
+            let max_height = block::Height(max_len);
+
             // start at genesis, and return max_len hashes
-            let max_height = block::Height(
-                max_len
-                    .try_into()
-                    .expect("max_len does not exceed Height::MAX"),
-            );
-            (max_height - 1).expect("max_len is at least 1")
+            (max_height - 1).expect("max_len is at least 1, and does not exceed Height::MAX + 1")
         };
 
         let stop_height = stop.and_then(|hash| self.best_height_by_hash(hash));