Skip to content

Turn on a bunch of cippy ints #127

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 9 commits into from
May 1, 2025
129 changes: 129 additions & 0 deletions Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -38,3 +38,132 @@ 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??
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 = "warn"
bool_to_int_with_if = "warn"
borrow_as_ptr = "warn"
case_sensitive_file_extension_comparisons = "warn"
cast_lossless = "warn"
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 = "warn"
enum_glob_use = "allow"
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 = "warn"
fn_params_excessive_bools = "warn"
from_iter_instead_of_collect = "warn"
if_not_else = "warn"
ignored_unit_patterns = "allow"
implicit_clone = "warn"
implicit_hasher = "warn"
inconsistent_struct_constructor = "warn"
index_refutable_slice = "warn"
inefficient_to_string = "allow"
inline_always = "warn"
into_iter_without_iter = "warn"
invalid_upcast_comparisons = "warn"
items_after_statements = "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 = "warn"
manual_is_power_of_two = "warn"
manual_is_variant_and = "warn"
manual_let_else = "allow"
manual_ok_or = "warn"
manual_string_new = "warn"
many_single_char_names = "warn"
map_unwrap_or = "allow"
match_on_vec_items = "warn"
match_wild_err_arm = "warn"
match_wildcard_for_single_variants = "allow"
maybe_infinite_iter = "warn"
mismatching_type_param_order = "warn"
missing_errors_doc = "allow"
missing_fields_in_debug = "warn"
missing_panics_doc = "allow"
mut_mut = "warn"
naive_bytecount = "warn"
needless_bitwise_bool = "warn"
needless_continue = "allow"
needless_for_each = "warn"
needless_pass_by_value = "allow"
needless_raw_string_hashes = "allow"
no_effect_underscore_binding = "warn"
no_mangle_with_rust_abi = "warn"
option_as_ref_cloned = "warn"
option_option = "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 = "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 = "warn"
semicolon_if_nothing_returned = "allow"
should_panic_without_expect = "warn"
single_char_pattern = "warn"
single_match_else = "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 = "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 = "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 = "warn"
2 changes: 2 additions & 0 deletions clippy.toml
Original file line number Diff line number Diff line change
@@ -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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just boxing the Error in RichError would cut down the size a fair bit. I was gonna do a PR to replace the String errors with anyhow::Error in a few places, so I'll include it in that.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That sounds good. If you can remove this clippy.toml line I'd appreciate it!

Also, because this is both a library and a CLI tool, it may be a judgement call where to use anyhow ... I'd prefer we not use it on any "library errors" and only on "application errors".

Historically we've manually done all our errors, which is a bit tedious. I wouldn't mind if you wanted to introduce thiserror here. In rust-simplicity probably not, because we're much more agressive about pruning dependencies there.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah anyhow is kind of a lazy hack. Just less so than using String. I agree that libraries should use proper errors that you can actually match on. I'll fix up the PR I just opened.

12 changes: 6 additions & 6 deletions src/ast.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1039,27 +1039,27 @@ 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(())
}
}

fn check_output_type(
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(())
}
}

Expand Down
25 changes: 21 additions & 4 deletions src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ mod witness;

use std::sync::Arc;

use simplicity::jet::elements::ElementsEnv;
use simplicity::{jet::Elements, CommitNode, RedeemNode};

pub extern crate either;
Expand Down Expand Up @@ -142,15 +143,31 @@ impl CompiledProgram {
/// - Witness values have a different type than declared in the Simfony program.
/// - There are missing witness values.
pub fn satisfy(&self, witness_values: WitnessValues) -> Result<SatisfiedProgram, String> {
self.satisfy_with_env(witness_values, None)
}

/// Satisfy the Simfony program with the given `witness_values`.
/// If `env` is `None`, the program is not pruned, otherwise it is pruned with the given environment.
///
/// ## Errors
///
/// - Witness values have a different type than declared in the Simfony program.
/// - There are missing witness values.
pub fn satisfy_with_env(
&self,
witness_values: WitnessValues,
env: Option<&ElementsEnv<Arc<elements::Transaction>>>,
) -> Result<SatisfiedProgram, String> {
witness_values
.is_consistent(&self.witness_types)
.map_err(|e| e.to_string())?;
let simplicity_witness = named::to_witness_node(&self.simplicity, witness_values);
let simplicity_redeem = simplicity_witness
.finalize_unpruned()
.map_err(|e| e.to_string())?;
let simplicity_redeem = match env {
Some(env) => simplicity_witness.finalize_pruned(env),
None => simplicity_witness.finalize_unpruned(),
};
Ok(SatisfiedProgram {
simplicity: simplicity_redeem,
simplicity: simplicity_redeem.map_err(|e| e.to_string())?,
debug_symbols: self.debug_symbols.clone(),
})
}
Expand Down
6 changes: 3 additions & 3 deletions src/num.rs
Original file line number Diff line number Diff line change
Expand Up @@ -298,8 +298,8 @@ impl fmt::Display for U256 {
is_zero = true;

// Divide by 10, starting at the most significant bytes
for byte in bytes.iter_mut() {
let value = carry * 256 + *byte as u32;
for byte in &mut bytes {
let value = carry * 256 + u32::from(*byte);
*byte = (value / 10) as u8;
carry = value % 10;

Expand Down Expand Up @@ -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;
}
Expand Down
2 changes: 1 addition & 1 deletion src/pattern.rs
Original file line number Diff line number Diff line change
Expand Up @@ -259,7 +259,7 @@ impl BasePattern {

/// Get an iterator over all identifiers inside the pattern.
pub fn identifiers(&self) -> impl Iterator<Item = &Identifier> {
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.
Expand Down
2 changes: 1 addition & 1 deletion src/serde.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Expand Down
6 changes: 3 additions & 3 deletions src/value.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
Expand Down Expand Up @@ -934,7 +934,7 @@ impl ValueConstructible for StructuralValue {

fn array<I: IntoIterator<Item = Self>>(elements: I, ty: Self::Type) -> Self {
let elements: Vec<Self> = 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}"
Expand All @@ -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}"
Expand Down
Loading