From df500e9b71187fe658da76adafdb3300a51de2ef Mon Sep 17 00:00:00 2001 From: "Tobin C. Harding" Date: Mon, 3 Mar 2025 15:06:44 +1100 Subject: [PATCH] primitives: Enable pedantic lints Enable all the pedantic lints and fix warnings. Notable items: - `enum_glob_used` import types with a single character alias - `doc_markdown`: add a whitelist that includes SegWit and OpenSSL --- clippy.toml | 1 + primitives/Cargo.toml | 128 ++++++++++++++++++++++++++++ primitives/src/block.rs | 26 +++--- primitives/src/lib.rs | 6 -- primitives/src/locktime/absolute.rs | 99 ++++++++++----------- primitives/src/locktime/relative.rs | 58 ++++++------- primitives/src/opcodes.rs | 14 +-- primitives/src/pow.rs | 10 +-- primitives/src/script/mod.rs | 18 ++-- primitives/src/sequence.rs | 42 ++++----- primitives/src/transaction.rs | 40 ++++----- primitives/src/witness.rs | 36 ++++---- 12 files changed, 301 insertions(+), 177 deletions(-) diff --git a/clippy.toml b/clippy.toml index 0d5416666..47eac63c9 100644 --- a/clippy.toml +++ b/clippy.toml @@ -1,3 +1,4 @@ msrv = "1.63.0" too-many-arguments-threshold = 13 avoid-breaking-exported-api = false +doc-valid-idents = ["SegWit", "OpenSSL"] diff --git a/primitives/Cargo.toml b/primitives/Cargo.toml index afb74fd00..3b7e3b916 100644 --- a/primitives/Cargo.toml +++ b/primitives/Cargo.toml @@ -40,3 +40,131 @@ rustdoc-args = ["--cfg", "docsrs"] [lints.rust] unexpected_cfgs = { level = "deny", check-cfg = ['cfg(mutate)'] } + +[lints.clippy] +# Exclude lints we don't think are valuable. +needless_question_mark = "allow" # https://github.com/rust-bitcoin/rust-bitcoin/pull/2134 +manual_range_contains = "allow" # More readable than clippy's format. +# Exhaustive list of pedantic clippy lints +assigning_clones = "warn" +bool_to_int_with_if = "warn" +borrow_as_ptr = "warn" +case_sensitive_file_extension_comparisons = "warn" +cast_lossless = "warn" +cast_possible_truncation = "allow" # All casts should include a code comment (except test code). +cast_possible_wrap = "allow" # Same as above re code comment. +cast_precision_loss = "warn" +cast_ptr_alignment = "warn" +cast_sign_loss = "allow" # All casts should include a code comment (except in test code). +checked_conversions = "warn" +cloned_instead_of_copied = "warn" +copy_iterator = "warn" +default_trait_access = "warn" +doc_link_with_quotes = "warn" +doc_markdown = "warn" +empty_enum = "warn" +enum_glob_use = "warn" +expl_impl_clone_on_copy = "warn" +explicit_deref_methods = "warn" +explicit_into_iter_loop = "warn" +explicit_iter_loop = "warn" +filter_map_next = "warn" +flat_map_option = "warn" +float_cmp = "allow" # Bitcoin floats are typically limited to 8 decimal places and we want them exact. +fn_params_excessive_bools = "warn" +from_iter_instead_of_collect = "warn" +if_not_else = "warn" +ignored_unit_patterns = "warn" +implicit_clone = "warn" +implicit_hasher = "warn" +inconsistent_struct_constructor = "warn" +index_refutable_slice = "warn" +inefficient_to_string = "warn" +inline_always = "warn" +into_iter_without_iter = "warn" +invalid_upcast_comparisons = "warn" +items_after_statements = "warn" +iter_filter_is_ok = "warn" +iter_filter_is_some = "warn" +iter_not_returning_iterator = "warn" +iter_without_into_iter = "warn" +large_digit_groups = "warn" +large_futures = "warn" +large_stack_arrays = "warn" +large_types_passed_by_value = "warn" +linkedlist = "warn" +macro_use_imports = "warn" +manual_assert = "warn" +manual_instant_elapsed = "warn" +manual_is_power_of_two = "warn" +manual_is_variant_and = "warn" +manual_let_else = "warn" +manual_ok_or = "warn" +manual_string_new = "warn" +many_single_char_names = "warn" +map_unwrap_or = "warn" +match_bool = "allow" # Adds extra indentation and LOC. +match_on_vec_items = "warn" +match_same_arms = "allow" # Collapses things that are conceptually unrelated to each other. +match_wild_err_arm = "warn" +match_wildcard_for_single_variants = "warn" +maybe_infinite_iter = "warn" +mismatching_type_param_order = "warn" +missing_errors_doc = "allow" # TODO: Write errors section in docs. +missing_fields_in_debug = "warn" +missing_panics_doc = "warn" +must_use_candidate = "allow" # Useful for audit but many false positives. +mut_mut = "warn" +naive_bytecount = "warn" +needless_bitwise_bool = "warn" +needless_continue = "warn" +needless_for_each = "warn" +needless_pass_by_value = "warn" +needless_raw_string_hashes = "warn" +no_effect_underscore_binding = "warn" +no_mangle_with_rust_abi = "warn" +option_as_ref_cloned = "warn" +option_option = "warn" +ptr_as_ptr = "warn" +ptr_cast_constness = "warn" +pub_underscore_fields = "warn" +range_minus_one = "warn" +range_plus_one = "warn" +redundant_closure_for_method_calls = "warn" +redundant_else = "warn" +ref_as_ptr = "warn" +ref_binding_to_reference = "warn" +ref_option = "warn" +ref_option_ref = "warn" +return_self_not_must_use = "warn" +same_functions_in_if_condition = "warn" +semicolon_if_nothing_returned = "warn" +should_panic_without_expect = "warn" +similar_names = "allow" # Too many (subjectively) false positives. +single_char_pattern = "warn" +single_match_else = "warn" +stable_sort_primitive = "warn" +str_split_at_newline = "warn" +string_add_assign = "warn" +struct_excessive_bools = "warn" +struct_field_names = "allow" # TODO: Triggers warning for `witness_elements`. +too_many_lines = "warn" +transmute_ptr_to_ptr = "warn" +trivially_copy_pass_by_ref = "warn" +unchecked_duration_subtraction = "warn" +unicode_not_nfc = "warn" +uninlined_format_args = "allow" # This is a subjective style choice. +unnecessary_box_returns = "warn" +unnecessary_join = "warn" +unnecessary_literal_bound = "warn" +unnecessary_wraps = "warn" +unnested_or_patterns = "allow" # TODO +unreadable_literal = "warn" +unsafe_derive_deserialize = "warn" +unused_async = "warn" +unused_self = "warn" +used_underscore_binding = "warn" +used_underscore_items = "warn" +verbose_bit_mask = "warn" +wildcard_imports = "warn" +zero_sized_map_values = "warn" diff --git a/primitives/src/block.rs b/primitives/src/block.rs index b819734e5..4afe23c32 100644 --- a/primitives/src/block.rs +++ b/primitives/src/block.rs @@ -281,7 +281,7 @@ impl Version { /// /// A block is signalling for a soft fork under BIP-9 if the first 3 bits are `001` and /// the version bit for the specific soft fork is toggled on. - pub fn is_signalling_soft_fork(&self, bit: u8) -> bool { + pub fn is_signalling_soft_fork(self, bit: u8) -> bool { // Only bits [0, 28] inclusive are used for signalling. if bit > 28 { return false; @@ -362,36 +362,36 @@ mod tests { #[test] fn version_is_not_signalling_with_invalid_bit() { - let arbitrary_version = Version::from_consensus(1234567890); + let arbitrary_version = Version::from_consensus(1_234_567_890); // The max bit number to signal is 28. - assert!(!Version::is_signalling_soft_fork(&arbitrary_version, 29)); + assert!(!Version::is_signalling_soft_fork(arbitrary_version, 29)); } #[test] fn version_is_not_signalling_when_use_version_bit_not_set() { - let version = Version::from_consensus(0b01000000000000000000000000000000); + let version = Version::from_consensus(0b0100_0000_0000_0000_0000_0000_0000_0000); // Top three bits must be 001 to signal. - assert!(!Version::is_signalling_soft_fork(&version, 1)); + assert!(!Version::is_signalling_soft_fork(version, 1)); } #[test] fn version_is_signalling() { - let version = Version::from_consensus(0b00100000000000000000000000000010); - assert!(Version::is_signalling_soft_fork(&version, 1)); - let version = Version::from_consensus(0b00110000000000000000000000000000); - assert!(Version::is_signalling_soft_fork(&version, 28)); + let version = Version::from_consensus(0b0010_0000_0000_0000_0000_0000_0000_0010); + assert!(Version::is_signalling_soft_fork(version, 1)); + let version = Version::from_consensus(0b0011_0000_0000_0000_0000_0000_0000_0000); + assert!(Version::is_signalling_soft_fork(version, 28)); } #[test] fn version_is_not_signalling() { - let version = Version::from_consensus(0b00100000000000000000000000000010); - assert!(!Version::is_signalling_soft_fork(&version, 0)); + let version = Version::from_consensus(0b0010_0000_0000_0000_0000_0000_0000_0010); + assert!(!Version::is_signalling_soft_fork(version, 0)); } #[test] fn version_to_consensus() { - let version = Version::from_consensus(1234567890); - assert_eq!(version.to_consensus(), 1234567890); + let version = Version::from_consensus(1_234_567_890); + assert_eq!(version.to_consensus(), 1_234_567_890); } // Check that the size of the header consensus serialization matches the const SIZE value diff --git a/primitives/src/lib.rs b/primitives/src/lib.rs index 77b74b889..b01717ddd 100644 --- a/primitives/src/lib.rs +++ b/primitives/src/lib.rs @@ -16,12 +16,6 @@ #![warn(missing_docs)] #![warn(deprecated_in_future)] #![doc(test(attr(warn(unused))))] -// Pedantic lints that we enforce. -// #![warn(clippy::must_use_candidate)] -#![warn(clippy::return_self_not_must_use)] -// Exclude lints we don't think are valuable. -#![allow(clippy::needless_question_mark)] // https://github.com/rust-bitcoin/rust-bitcoin/pull/2134 -#![allow(clippy::manual_range_contains)] // More readable than clippy's format. #[cfg(feature = "alloc")] extern crate alloc; diff --git a/primitives/src/locktime/absolute.rs b/primitives/src/locktime/absolute.rs index 639963aa7..8d90cf254 100644 --- a/primitives/src/locktime/absolute.rs +++ b/primitives/src/locktime/absolute.rs @@ -1,6 +1,6 @@ // SPDX-License-Identifier: CC0-1.0 -//! Provides type [`LockTime`] that implements the logic around nLockTime/OP_CHECKLOCKTIMEVERIFY. +//! Provides type [`LockTime`] that implements the logic around `nLockTime`/`OP_CHECKLOCKTIMEVERIFY`. //! //! There are two types of lock time: lock-by-blockheight and lock-by-blocktime, distinguished by //! whether `LockTime < LOCKTIME_THRESHOLD`. @@ -22,7 +22,7 @@ pub use units::locktime::absolute::{ConversionError, Height, ParseHeightError, P /// since epoch). /// /// Used for transaction lock time (`nLockTime` in Bitcoin Core and [`Transaction::lock_time`] -/// in this library) and also for the argument to opcode 'OP_CHECKLOCKTIMEVERIFY`. +/// in this library) and also for the argument to opcode `OP_CHECKLOCKTIMEVERIFY`. /// /// ### Note on ordering /// @@ -42,13 +42,13 @@ pub use units::locktime::absolute::{ConversionError, Height, ParseHeightError, P /// # Examples /// /// ``` -/// # use bitcoin_primitives::absolute::{self, LockTime::*}; +/// use bitcoin_primitives::absolute::{self, LockTime as L}; /// # let n = absolute::LockTime::from_consensus(741521); // n OP_CHECKLOCKTIMEVERIFY /// # let lock_time = absolute::LockTime::from_consensus(741521); // nLockTime /// // To compare absolute lock times there are various `is_satisfied_*` methods, you may also use: /// let _is_satisfied = match (n, lock_time) { -/// (Blocks(n), Blocks(lock_time)) => n <= lock_time, -/// (Seconds(n), Seconds(lock_time)) => n <= lock_time, +/// (L::Blocks(n), L::Blocks(lock_time)) => n <= lock_time, +/// (L::Seconds(n), L::Seconds(lock_time)) => n <= lock_time, /// _ => panic!("handle invalid comparison error"), /// }; /// ``` @@ -126,7 +126,7 @@ impl LockTime { Ok(Self::from_consensus(lock_time)) } - /// Constructs a new `LockTime` from an nLockTime value or the argument to OP_CHEKCLOCKTIMEVERIFY. + /// Constructs a new `LockTime` from an `nLockTime` value or the argument to `OP_CHEKCLOCKTIMEVERIFY`. /// /// # Examples /// @@ -138,6 +138,7 @@ impl LockTime { /// let lock_time = absolute::LockTime::from_consensus(n_lock_time); /// assert_eq!(lock_time.to_consensus_u32(), n_lock_time); #[inline] + #[allow(clippy::missing_panics_doc)] pub fn from_consensus(n: u32) -> Self { if units::locktime::absolute::is_block_height(n) { Self::Blocks(Height::from_consensus(n).expect("n is valid")) @@ -197,7 +198,7 @@ impl LockTime { /// Returns true if both lock times use the same unit i.e., both height based or both time based. #[inline] - pub const fn is_same_unit(&self, other: LockTime) -> bool { + pub const fn is_same_unit(self, other: LockTime) -> bool { matches!( (self, other), (LockTime::Blocks(_), LockTime::Blocks(_)) @@ -207,11 +208,11 @@ impl LockTime { /// Returns true if this lock time value is a block height. #[inline] - pub const fn is_block_height(&self) -> bool { matches!(*self, LockTime::Blocks(_)) } + pub const fn is_block_height(self) -> bool { matches!(self, LockTime::Blocks(_)) } /// Returns true if this lock time value is a block time (UNIX timestamp). #[inline] - pub const fn is_block_time(&self) -> bool { !self.is_block_height() } + pub const fn is_block_time(self) -> bool { !self.is_block_height() } /// Returns true if this timelock constraint is satisfied by the respective `height`/`time`. /// @@ -219,7 +220,7 @@ impl LockTime { /// blocktime based lock it is checked against `time`. /// /// A 'timelock constraint' refers to the `n` from `n OP_CHEKCLOCKTIMEVERIFY`, this constraint - /// is satisfied if a transaction with nLockTime ([`Transaction::lock_time`]) set to + /// is satisfied if a transaction with `nLockTime` ([`Transaction::lock_time`]) set to /// `height`/`time` is valid. /// /// # Examples @@ -236,12 +237,12 @@ impl LockTime { /// } /// ```` #[inline] - pub fn is_satisfied_by(&self, height: Height, time: Time) -> bool { - use LockTime::*; + pub fn is_satisfied_by(self, height: Height, time: Time) -> bool { + use LockTime as L; - match *self { - Blocks(n) => n <= height, - Seconds(n) => n <= time, + match self { + L::Blocks(n) => n <= height, + L::Seconds(n) => n <= time, } } @@ -265,18 +266,18 @@ impl LockTime { /// assert!(lock_time.is_implied_by(check)); /// ``` #[inline] - pub fn is_implied_by(&self, other: LockTime) -> bool { - use LockTime::*; + pub fn is_implied_by(self, other: LockTime) -> bool { + use LockTime as L; - match (*self, other) { - (Blocks(this), Blocks(other)) => this <= other, - (Seconds(this), Seconds(other)) => this <= other, + match (self, other) { + (L::Blocks(this), L::Blocks(other)) => this <= other, + (L::Seconds(this), L::Seconds(other)) => this <= other, _ => false, // Not the same units. } } /// Returns the inner `u32` value. This is the value used when creating this `LockTime` - /// i.e., `n OP_CHECKLOCKTIMEVERIFY` or nLockTime. + /// i.e., `n OP_CHECKLOCKTIMEVERIFY` or `nLockTime`. /// /// # Warning /// @@ -287,13 +288,13 @@ impl LockTime { /// # Examples /// /// ```rust - /// # use bitcoin_primitives::absolute::{self, LockTime::*}; + /// use bitcoin_primitives::absolute::{self, LockTime as L}; /// # let n = absolute::LockTime::from_consensus(741521); // n OP_CHECKLOCKTIMEVERIFY /// # let lock_time = absolute::LockTime::from_consensus(741521 + 1); // nLockTime /// /// let _is_satisfied = match (n, lock_time) { - /// (Blocks(n), Blocks(lock_time)) => n <= lock_time, - /// (Seconds(n), Seconds(lock_time)) => n <= lock_time, + /// (L::Blocks(n), L::Blocks(lock_time)) => n <= lock_time, + /// (L::Seconds(n), L::Seconds(lock_time)) => n <= lock_time, /// _ => panic!("invalid comparison"), /// }; /// @@ -324,28 +325,28 @@ impl From