From 1c2fa51bcc7259b1db4d9c1b0c356ff14f26ce9a Mon Sep 17 00:00:00 2001 From: Andrew Poelstra Date: Tue, 29 Apr 2025 17:13:10 +0000 Subject: [PATCH 1/8] clippy: whitelist lints that fire on current nightly --- Cargo.toml | 4 ++++ clippy.toml | 2 ++ 2 files changed, 6 insertions(+) diff --git a/Cargo.toml b/Cargo.toml index 53c0abc..3b5ec2c 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -38,3 +38,7 @@ getrandom = { version = "0.2", features = ["js"] } [workspace] members = ["codegen"] exclude = ["fuzz", "bitcoind-tests"] + +[lints.clippy] +# Exclude lints we don't think are valuable. +large_enum_variant = "allow" # docs say "measure before paying attention to this"; why is it on by default?? diff --git a/clippy.toml b/clippy.toml index a93e948..8d622bc 100644 --- a/clippy.toml +++ b/clippy.toml @@ -1 +1,3 @@ msrv = "1.78.0" +# We have an error type, `RichError`, of size 144. This is pushing it but probably fine. +large-error-threshold = 145 From 7a7d16bf939efe74478bcb68669b924d2fe14426 Mon Sep 17 00:00:00 2001 From: Andrew Poelstra Date: Tue, 29 Apr 2025 17:15:33 +0000 Subject: [PATCH 2/8] clippy: copy giant list of lints from bitcoin-units, with all "allow"ed We will set these all to "warn" in the following commits. --- Cargo.toml | 125 +++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 125 insertions(+) diff --git a/Cargo.toml b/Cargo.toml index 3b5ec2c..f2c3328 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -42,3 +42,128 @@ exclude = ["fuzz", "bitcoind-tests"] [lints.clippy] # Exclude lints we don't think are valuable. large_enum_variant = "allow" # docs say "measure before paying attention to this"; why is it on by default?? +similar_names = "allow" # Too many (subjectively) false positives. +uninlined_format_args = "allow" # This is a subjective style choice. +match_bool = "allow" # Adds extra indentation and LOC. +match_same_arms = "allow" # Collapses things that are conceptually unrelated to each other. +must_use_candidate = "allow" # Useful for audit but many false positives. +# Whitelist the cast lints because sometimes casts are unavoidable. But +# every cast should contain a code comment! +cast_possible_truncation = "allow" +cast_possible_wrap = "allow" +cast_sign_loss = "allow" +# Exhaustive list of pedantic clippy lints +assigning_clones = "allow" +bool_to_int_with_if = "allow" +borrow_as_ptr = "allow" +case_sensitive_file_extension_comparisons = "allow" +cast_lossless = "allow" +cast_precision_loss = "allow" +cast_ptr_alignment = "allow" +checked_conversions = "allow" +cloned_instead_of_copied = "allow" +copy_iterator = "allow" +default_trait_access = "allow" +doc_link_with_quotes = "allow" +doc_markdown = "allow" +empty_enum = "allow" +enum_glob_use = "allow" +expl_impl_clone_on_copy = "allow" +explicit_deref_methods = "allow" +explicit_into_iter_loop = "allow" +explicit_iter_loop = "allow" +filter_map_next = "allow" +flat_map_option = "allow" +float_cmp = "allow" +fn_params_excessive_bools = "allow" +from_iter_instead_of_collect = "allow" +if_not_else = "allow" +ignored_unit_patterns = "allow" +implicit_clone = "allow" +implicit_hasher = "allow" +inconsistent_struct_constructor = "allow" +index_refutable_slice = "allow" +inefficient_to_string = "allow" +inline_always = "allow" +into_iter_without_iter = "allow" +invalid_upcast_comparisons = "allow" +items_after_statements = "allow" +iter_filter_is_ok = "allow" +iter_filter_is_some = "allow" +iter_not_returning_iterator = "allow" +iter_without_into_iter = "allow" +large_digit_groups = "allow" +large_futures = "allow" +large_stack_arrays = "allow" +large_types_passed_by_value = "allow" +linkedlist = "allow" +macro_use_imports = "allow" +manual_assert = "allow" +manual_instant_elapsed = "allow" +manual_is_power_of_two = "allow" +manual_is_variant_and = "allow" +manual_let_else = "allow" +manual_ok_or = "allow" +manual_string_new = "allow" +many_single_char_names = "allow" +map_unwrap_or = "allow" +match_on_vec_items = "allow" +match_wild_err_arm = "allow" +match_wildcard_for_single_variants = "allow" +maybe_infinite_iter = "allow" +mismatching_type_param_order = "allow" +missing_errors_doc = "allow" +missing_fields_in_debug = "allow" +missing_panics_doc = "allow" +mut_mut = "allow" +naive_bytecount = "allow" +needless_bitwise_bool = "allow" +needless_continue = "allow" +needless_for_each = "allow" +needless_pass_by_value = "allow" +needless_raw_string_hashes = "allow" +no_effect_underscore_binding = "allow" +no_mangle_with_rust_abi = "allow" +option_as_ref_cloned = "allow" +option_option = "allow" +ptr_as_ptr = "allow" +ptr_cast_constness = "allow" +pub_underscore_fields = "allow" +range_minus_one = "allow" +range_plus_one = "allow" +redundant_closure_for_method_calls = "allow" +redundant_else = "allow" +ref_as_ptr = "allow" +ref_binding_to_reference = "allow" +ref_option = "allow" +ref_option_ref = "allow" +return_self_not_must_use = "allow" +same_functions_in_if_condition = "allow" +semicolon_if_nothing_returned = "allow" +should_panic_without_expect = "allow" +single_char_pattern = "allow" +single_match_else = "allow" +stable_sort_primitive = "allow" +str_split_at_newline = "allow" +string_add_assign = "allow" +struct_excessive_bools = "allow" +struct_field_names = "allow" +too_many_lines = "allow" +transmute_ptr_to_ptr = "allow" +trivially_copy_pass_by_ref = "allow" +unchecked_duration_subtraction = "allow" +unicode_not_nfc = "allow" +unnecessary_box_returns = "allow" +unnecessary_join = "allow" +unnecessary_literal_bound = "allow" +unnecessary_wraps = "allow" +unnested_or_patterns = "allow" +unreadable_literal = "allow" +unsafe_derive_deserialize = "allow" +unused_async = "allow" +unused_self = "allow" +used_underscore_binding = "allow" +used_underscore_items = "allow" +verbose_bit_mask = "allow" +wildcard_imports = "allow" +zero_sized_map_values = "allow" From 51304417f4f4dd154294f612fa2d86b2825ce286 Mon Sep 17 00:00:00 2001 From: Andrew Poelstra Date: Tue, 29 Apr 2025 21:36:34 +0000 Subject: [PATCH 3/8] clippy: turn on all lints that don't trigger anywhere These can be immediately turned on without code changes. --- Cargo.toml | 172 ++++++++++++++++++++++++++--------------------------- 1 file changed, 86 insertions(+), 86 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index f2c3328..9ccdf63 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -53,117 +53,117 @@ cast_possible_truncation = "allow" cast_possible_wrap = "allow" cast_sign_loss = "allow" # Exhaustive list of pedantic clippy lints -assigning_clones = "allow" +assigning_clones = "warn" bool_to_int_with_if = "allow" -borrow_as_ptr = "allow" -case_sensitive_file_extension_comparisons = "allow" +borrow_as_ptr = "warn" +case_sensitive_file_extension_comparisons = "warn" cast_lossless = "allow" -cast_precision_loss = "allow" -cast_ptr_alignment = "allow" -checked_conversions = "allow" -cloned_instead_of_copied = "allow" -copy_iterator = "allow" -default_trait_access = "allow" -doc_link_with_quotes = "allow" +cast_precision_loss = "warn" +cast_ptr_alignment = "warn" +checked_conversions = "warn" +cloned_instead_of_copied = "warn" +copy_iterator = "warn" +default_trait_access = "warn" +doc_link_with_quotes = "warn" doc_markdown = "allow" -empty_enum = "allow" +empty_enum = "warn" enum_glob_use = "allow" -expl_impl_clone_on_copy = "allow" -explicit_deref_methods = "allow" -explicit_into_iter_loop = "allow" +expl_impl_clone_on_copy = "warn" +explicit_deref_methods = "warn" +explicit_into_iter_loop = "warn" explicit_iter_loop = "allow" -filter_map_next = "allow" +filter_map_next = "warn" flat_map_option = "allow" -float_cmp = "allow" -fn_params_excessive_bools = "allow" -from_iter_instead_of_collect = "allow" +float_cmp = "warn" +fn_params_excessive_bools = "warn" +from_iter_instead_of_collect = "warn" if_not_else = "allow" ignored_unit_patterns = "allow" -implicit_clone = "allow" -implicit_hasher = "allow" -inconsistent_struct_constructor = "allow" -index_refutable_slice = "allow" +implicit_clone = "warn" +implicit_hasher = "warn" +inconsistent_struct_constructor = "warn" +index_refutable_slice = "warn" inefficient_to_string = "allow" -inline_always = "allow" -into_iter_without_iter = "allow" -invalid_upcast_comparisons = "allow" +inline_always = "warn" +into_iter_without_iter = "warn" +invalid_upcast_comparisons = "warn" items_after_statements = "allow" -iter_filter_is_ok = "allow" -iter_filter_is_some = "allow" -iter_not_returning_iterator = "allow" -iter_without_into_iter = "allow" -large_digit_groups = "allow" -large_futures = "allow" -large_stack_arrays = "allow" -large_types_passed_by_value = "allow" -linkedlist = "allow" -macro_use_imports = "allow" +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 = "allow" -manual_instant_elapsed = "allow" -manual_is_power_of_two = "allow" -manual_is_variant_and = "allow" +manual_instant_elapsed = "warn" +manual_is_power_of_two = "warn" +manual_is_variant_and = "warn" manual_let_else = "allow" -manual_ok_or = "allow" -manual_string_new = "allow" -many_single_char_names = "allow" +manual_ok_or = "warn" +manual_string_new = "warn" +many_single_char_names = "warn" map_unwrap_or = "allow" -match_on_vec_items = "allow" -match_wild_err_arm = "allow" +match_on_vec_items = "warn" +match_wild_err_arm = "warn" match_wildcard_for_single_variants = "allow" -maybe_infinite_iter = "allow" -mismatching_type_param_order = "allow" +maybe_infinite_iter = "warn" +mismatching_type_param_order = "warn" missing_errors_doc = "allow" -missing_fields_in_debug = "allow" +missing_fields_in_debug = "warn" missing_panics_doc = "allow" -mut_mut = "allow" -naive_bytecount = "allow" -needless_bitwise_bool = "allow" +mut_mut = "warn" +naive_bytecount = "warn" +needless_bitwise_bool = "warn" needless_continue = "allow" -needless_for_each = "allow" +needless_for_each = "warn" needless_pass_by_value = "allow" needless_raw_string_hashes = "allow" -no_effect_underscore_binding = "allow" -no_mangle_with_rust_abi = "allow" -option_as_ref_cloned = "allow" +no_effect_underscore_binding = "warn" +no_mangle_with_rust_abi = "warn" +option_as_ref_cloned = "warn" option_option = "allow" -ptr_as_ptr = "allow" -ptr_cast_constness = "allow" -pub_underscore_fields = "allow" -range_minus_one = "allow" -range_plus_one = "allow" +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 = "allow" -redundant_else = "allow" -ref_as_ptr = "allow" -ref_binding_to_reference = "allow" -ref_option = "allow" -ref_option_ref = "allow" +redundant_else = "warn" +ref_as_ptr = "warn" +ref_binding_to_reference = "warn" +ref_option = "warn" +ref_option_ref = "warn" return_self_not_must_use = "allow" -same_functions_in_if_condition = "allow" +same_functions_in_if_condition = "warn" semicolon_if_nothing_returned = "allow" -should_panic_without_expect = "allow" -single_char_pattern = "allow" +should_panic_without_expect = "warn" +single_char_pattern = "warn" single_match_else = "allow" -stable_sort_primitive = "allow" -str_split_at_newline = "allow" -string_add_assign = "allow" -struct_excessive_bools = "allow" -struct_field_names = "allow" +stable_sort_primitive = "warn" +str_split_at_newline = "warn" +string_add_assign = "warn" +struct_excessive_bools = "warn" +struct_field_names = "warn" too_many_lines = "allow" -transmute_ptr_to_ptr = "allow" -trivially_copy_pass_by_ref = "allow" -unchecked_duration_subtraction = "allow" -unicode_not_nfc = "allow" -unnecessary_box_returns = "allow" -unnecessary_join = "allow" -unnecessary_literal_bound = "allow" -unnecessary_wraps = "allow" +transmute_ptr_to_ptr = "warn" +trivially_copy_pass_by_ref = "warn" +unchecked_duration_subtraction = "warn" +unicode_not_nfc = "warn" +unnecessary_box_returns = "warn" +unnecessary_join = "warn" +unnecessary_literal_bound = "warn" +unnecessary_wraps = "warn" unnested_or_patterns = "allow" unreadable_literal = "allow" -unsafe_derive_deserialize = "allow" -unused_async = "allow" -unused_self = "allow" -used_underscore_binding = "allow" -used_underscore_items = "allow" -verbose_bit_mask = "allow" +unsafe_derive_deserialize = "warn" +unused_async = "warn" +unused_self = "warn" +used_underscore_binding = "warn" +used_underscore_items = "warn" +verbose_bit_mask = "warn" wildcard_imports = "allow" -zero_sized_map_values = "allow" +zero_sized_map_values = "warn" From 53f5610245acbb348534b2706d463d96227b4998 Mon Sep 17 00:00:00 2001 From: Andrew Poelstra Date: Tue, 29 Apr 2025 21:37:17 +0000 Subject: [PATCH 4/8] clippy: use built-in conversion from bool to numeric This is really just a stylistic thing, but it makes the code shorter so I think we should use it. --- Cargo.toml | 2 +- src/value.rs | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index 9ccdf63..9e7589d 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -54,7 +54,7 @@ cast_possible_wrap = "allow" cast_sign_loss = "allow" # Exhaustive list of pedantic clippy lints assigning_clones = "warn" -bool_to_int_with_if = "allow" +bool_to_int_with_if = "warn" borrow_as_ptr = "warn" case_sensitive_file_extension_comparisons = "warn" cast_lossless = "allow" diff --git a/src/value.rs b/src/value.rs index 6de7d07..9459506 100644 --- a/src/value.rs +++ b/src/value.rs @@ -155,7 +155,7 @@ impl UIntValue { let mut byte = 0u8; for _ in 0..8 { let bit = padded_bits.next().unwrap(); - byte = (byte << 1) | if bit == '1' { 1 } else { 0 }; + byte = (byte << 1) | u8::from(bit == '1'); } bytes.push(byte); } From 9d5a79620b3601c96aa01aee6d9bbf82545a5cab Mon Sep 17 00:00:00 2001 From: Andrew Poelstra Date: Tue, 29 Apr 2025 21:38:41 +0000 Subject: [PATCH 5/8] clippy: don't use lossless casts When casting between numeric types, if the conversion is infallible then we can use `from`. This means that if the types change to make the conversion lossy, we'll get a compiler error rather than the logic silently becoming wrong. --- Cargo.toml | 2 +- src/num.rs | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index 9e7589d..faa192a 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -57,7 +57,7 @@ assigning_clones = "warn" bool_to_int_with_if = "warn" borrow_as_ptr = "warn" case_sensitive_file_extension_comparisons = "warn" -cast_lossless = "allow" +cast_lossless = "warn" cast_precision_loss = "warn" cast_ptr_alignment = "warn" checked_conversions = "warn" diff --git a/src/num.rs b/src/num.rs index 350eeae..5795c87 100644 --- a/src/num.rs +++ b/src/num.rs @@ -299,7 +299,7 @@ impl fmt::Display for U256 { // Divide by 10, starting at the most significant bytes for byte in bytes.iter_mut() { - let value = carry * 256 + *byte as u32; + let value = carry * 256 + u32::from(*byte); *byte = (value / 10) as u8; carry = value % 10; @@ -334,7 +334,7 @@ impl FromStr for U256 { // Add to the least significant bytes first for byte in bytes.iter_mut().rev() { - let value = *byte as u32 * 10 + carry; + let value = u32::from(*byte) * 10 + carry; *byte = (value % 256) as u8; carry = value / 256; } From 2c7b1fbc099fe5181acfb9e4ea0be1dc37cbb2c9 Mon Sep 17 00:00:00 2001 From: Andrew Poelstra Date: Tue, 29 Apr 2025 21:42:51 +0000 Subject: [PATCH 6/8] clippy: don't use .iter and .iter_mut where not necessary Also a style thing, but makes the code more idiomatic. --- Cargo.toml | 2 +- src/num.rs | 2 +- src/serde.rs | 2 +- src/value.rs | 4 ++-- 4 files changed, 5 insertions(+), 5 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index faa192a..c95b356 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -71,7 +71,7 @@ enum_glob_use = "allow" expl_impl_clone_on_copy = "warn" explicit_deref_methods = "warn" explicit_into_iter_loop = "warn" -explicit_iter_loop = "allow" +explicit_iter_loop = "warn" filter_map_next = "warn" flat_map_option = "allow" float_cmp = "warn" diff --git a/src/num.rs b/src/num.rs index 5795c87..7df5790 100644 --- a/src/num.rs +++ b/src/num.rs @@ -298,7 +298,7 @@ impl fmt::Display for U256 { is_zero = true; // Divide by 10, starting at the most significant bytes - for byte in bytes.iter_mut() { + for byte in &mut bytes { let value = carry * 256 + u32::from(*byte); *byte = (value / 10) as u8; carry = value % 10; diff --git a/src/serde.rs b/src/serde.rs index 7c4ed14..2a12550 100644 --- a/src/serde.rs +++ b/src/serde.rs @@ -144,7 +144,7 @@ impl<'a> Serialize for WitnessMapSerializer<'a> { S: Serializer, { let mut map = serializer.serialize_map(Some(self.0.len()))?; - for (name, value) in self.0.iter() { + for (name, value) in self.0 { map.serialize_entry(name.as_inner(), &ValueMapSerializer(value))?; } map.end() diff --git a/src/value.rs b/src/value.rs index 9459506..ccedc8a 100644 --- a/src/value.rs +++ b/src/value.rs @@ -934,7 +934,7 @@ impl ValueConstructible for StructuralValue { fn array>(elements: I, ty: Self::Type) -> Self { let elements: Vec = elements.into_iter().collect(); - for element in elements.iter() { + for element in &elements { assert!( element.is_of_type(&ty), "Element {element} is not of expected type {ty}" @@ -954,7 +954,7 @@ impl ValueConstructible for StructuralValue { elements.len() < bound.get(), "There must be fewer list elements than the bound {bound}" ); - for element in elements.iter() { + for element in &elements { assert!( element.is_of_type(&ty), "Element {element} is not of expected type {ty}" From 3fdacb5673f8aff5d2e3d06402c18f803f57c32e Mon Sep 17 00:00:00 2001 From: Andrew Poelstra Date: Tue, 29 Apr 2025 21:47:57 +0000 Subject: [PATCH 7/8] clippy: replace flat_map with filter_map BasePattern::as_identifier returns an Option; when iterating we can filter out the Nones with `filter_map`. We were instead using `flat_map`, which is much more general: it turns the Options into iterators (which will yield 0 or 1 items) and then yields everything from the iterators. This is potentially slower and more complicated to read. --- Cargo.toml | 2 +- src/pattern.rs | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index c95b356..09438c1 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -73,7 +73,7 @@ explicit_deref_methods = "warn" explicit_into_iter_loop = "warn" explicit_iter_loop = "warn" filter_map_next = "warn" -flat_map_option = "allow" +flat_map_option = "warn" float_cmp = "warn" fn_params_excessive_bools = "warn" from_iter_instead_of_collect = "warn" diff --git a/src/pattern.rs b/src/pattern.rs index 0f80da9..d88e4df 100644 --- a/src/pattern.rs +++ b/src/pattern.rs @@ -259,7 +259,7 @@ impl BasePattern { /// Get an iterator over all identifiers inside the pattern. pub fn identifiers(&self) -> impl Iterator { - self.pre_order_iter().flat_map(BasePattern::as_identifier) + self.pre_order_iter().filter_map(BasePattern::as_identifier) } /// Check if all `identifiers` are contained inside the pattern. From 8e15fd3e032978584b5839100a29cba568eece73 Mon Sep 17 00:00:00 2001 From: Andrew Poelstra Date: Tue, 29 Apr 2025 22:36:30 +0000 Subject: [PATCH 8/8] clippy: invert != in if statements This is a stylistic lint which you could argue either way, but I like having it on for consistency. --- Cargo.toml | 2 +- src/ast.rs | 12 ++++++------ 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index 09438c1..e1b0054 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -77,7 +77,7 @@ flat_map_option = "warn" float_cmp = "warn" fn_params_excessive_bools = "warn" from_iter_instead_of_collect = "warn" -if_not_else = "allow" +if_not_else = "warn" ignored_unit_patterns = "allow" implicit_clone = "warn" implicit_hasher = "warn" diff --git a/src/ast.rs b/src/ast.rs index dfc2615..8c08722 100644 --- a/src/ast.rs +++ b/src/ast.rs @@ -1039,13 +1039,13 @@ impl AbstractSyntaxTree for Call { parse_args: &[parse::Expression], expected_tys: &[ResolvedType], ) -> Result<(), Error> { - if parse_args.len() != expected_tys.len() { + if parse_args.len() == expected_tys.len() { + Ok(()) + } else { Err(Error::InvalidNumberOfArguments( expected_tys.len(), parse_args.len(), )) - } else { - Ok(()) } } @@ -1053,13 +1053,13 @@ impl AbstractSyntaxTree for Call { observed_ty: &ResolvedType, expected_ty: &ResolvedType, ) -> Result<(), Error> { - if observed_ty != expected_ty { + if observed_ty == expected_ty { + Ok(()) + } else { Err(Error::ExpressionTypeMismatch( expected_ty.clone(), observed_ty.clone(), )) - } else { - Ok(()) } }