From 1a3cc54275d4ab11d4ec16ec475cd46f05083016 Mon Sep 17 00:00:00 2001 From: Andrew Poelstra Date: Fri, 27 Jun 2025 20:20:45 +0000 Subject: [PATCH 1/3] ci: improve compiler versioning Reads MSRV from clippy.toml rather than hardcoding it; uses nightly compiler for clippy runs; adds cronjob to update nightly compiler pin. --- .github/workflows/main.yml | 16 ++++++++++++++-- 1 file changed, 14 insertions(+), 2 deletions(-) diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml index dc91fffb..f35e42b6 100644 --- a/.github/workflows/main.yml +++ b/.github/workflows/main.yml @@ -7,12 +7,22 @@ jobs: runs-on: ubuntu-24.04 outputs: nightly_version: ${{ steps.read_toolchain.outputs.nightly_version }} + msrv_version: ${{ steps.read_msrv.outputs.msrv_version }} steps: - name: "Checkout repo" uses: actions/checkout@v4 - name: "Read nightly version" id: read_toolchain - run: echo "nightly_version=$(cat nightly-version)" >> $GITHUB_OUTPUT + run: | + set -euo pipefail + version=$(cat nightly-version) + echo "nightly_version=$version" >> $GITHUB_OUTPUT + - name: Read MSRV from clippy.toml + id: read_msrv + run: | + set -euo pipefail + msrv=$(grep '^msrv *= *"' clippy.toml | sed -E 's/.*"([^"]+)".*/\1/') + echo "msrv_version=$msrv" >> "$GITHUB_OUTPUT" fmt: name: Rustfmt @@ -66,7 +76,7 @@ jobs: - stable - beta - ${{ needs.Prepare.outputs.nightly_version }} - - 1.78.0 + - ${{ needs.Prepare.outputs.msrv_version }} steps: - name: Checkout Crate uses: actions/checkout@v4 @@ -100,6 +110,7 @@ jobs: clippy: name: Clippy + needs: Prepare runs-on: ${{ matrix.os }} strategy: matrix: @@ -110,6 +121,7 @@ jobs: - name: Checkout Toolchain uses: dtolnay/rust-toolchain@stable with: + toolchain: ${{ needs.Prepare.outputs.nightly_version }} components: clippy - name: Running cargo clippy run: cargo clippy --workspace --all-targets -- --deny warnings From 1b43322d47af2c0fb6a43f034ceafeacd4efd91d Mon Sep 17 00:00:00 2001 From: Andrew Poelstra Date: Fri, 27 Jun 2025 20:25:40 +0000 Subject: [PATCH 2/3] clippy: address all current lints Mostly they are complaining about the new "format specifiers can directly hold the variable, if that variable is a single token, and if the format specifier is in println or format but not write or assert, so you should convert 40% of your instances to this different format, because it makes a big diff and leaves your code in an unreadable state" lint, which personally I don't feel is a net benefit. Copy the whole list of whitelisted clippy lints from rust-elements. --- Cargo.toml | 11 +++++++++++ simplicity-sys/Cargo.toml | 11 +++++++++++ src/node/mod.rs | 2 +- src/policy/satisfy.rs | 2 +- src/types/context.rs | 2 +- src/value.rs | 14 +++++++------- 6 files changed, 32 insertions(+), 10 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index e9c93249..f0bccb60 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -47,3 +47,14 @@ exclude = ["jets-bench"] [lints.rust] unexpected_cfgs = { level = "deny", check-cfg = ['cfg(bench)'] } + +[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. +uninlined_format_args = "allow" # This is a subjective style choice. +float_cmp = "allow" # Bitcoin floats are typically limited to 8 decimal places and we want them exact. +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. +similar_names = "allow" # Too many (subjectively) false positives. diff --git a/simplicity-sys/Cargo.toml b/simplicity-sys/Cargo.toml index 98a3cbe4..9c673f13 100644 --- a/simplicity-sys/Cargo.toml +++ b/simplicity-sys/Cargo.toml @@ -20,3 +20,14 @@ test-utils = [] [lints.rust] unexpected_cfgs = { level = "deny", check-cfg = ['cfg(fuzzing)'] } + +[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. +uninlined_format_args = "allow" # This is a subjective style choice. +float_cmp = "allow" # Bitcoin floats are typically limited to 8 decimal places and we want them exact. +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. +similar_names = "allow" # Too many (subjectively) false positives. diff --git a/src/node/mod.rs b/src/node/mod.rs index babcec0e..076ac390 100644 --- a/src/node/mod.rs +++ b/src/node/mod.rs @@ -703,7 +703,7 @@ impl Node { /// /// The linear string has no sharing and may be **exponentially larger** /// than the originally shared expression! - pub fn display_expr(&self) -> DisplayExpr { + pub fn display_expr(&self) -> DisplayExpr<'_, N> { DisplayExpr::from(self) } } diff --git a/src/policy/satisfy.rs b/src/policy/satisfy.rs index d7f2eda7..d15d25b2 100644 --- a/src/policy/satisfy.rs +++ b/src/policy/satisfy.rs @@ -312,7 +312,7 @@ mod tests { fn get_satisfier( env: &ElementsEnv>, - ) -> PolicySatisfier { + ) -> PolicySatisfier<'_, XOnlyPublicKey> { let mut preimages = HashMap::new(); for i in 0..3 { diff --git a/src/types/context.rs b/src/types/context.rs index 3cf9b364..a72aabba 100644 --- a/src/types/context.rs +++ b/src/types/context.rs @@ -231,7 +231,7 @@ impl Context { } /// Locks the underlying slab mutex. - fn lock(&self) -> LockedContext { + fn lock(&self) -> LockedContext<'_> { LockedContext { context: Arc::as_ptr(&self.slab), slab: self.slab.lock().unwrap(), diff --git a/src/value.rs b/src/value.rs index 36010c63..f3fdca11 100644 --- a/src/value.rs +++ b/src/value.rs @@ -435,7 +435,7 @@ impl Value { } /// A reference to this value, which can be recursed over. - pub fn as_ref(&self) -> ValueRef { + pub fn as_ref(&self) -> ValueRef<'_> { ValueRef { inner: &self.inner, bit_offset: self.bit_offset, @@ -444,17 +444,17 @@ impl Value { } /// Access the inner value of a left sum value. - pub fn as_left(&self) -> Option { + pub fn as_left(&self) -> Option> { self.as_ref().as_left() } /// Access the inner value of a right sum value. - pub fn as_right(&self) -> Option { + pub fn as_right(&self) -> Option> { self.as_ref().as_right() } /// Access the inner values of a product value. - pub fn as_product(&self) -> Option<(ValueRef, ValueRef)> { + pub fn as_product(&self) -> Option<(ValueRef<'_>, ValueRef<'_>)> { self.as_ref().as_product() } @@ -590,7 +590,7 @@ impl Value { /// The returned bytes match the padded bit-encoding of the value. You /// may wish to call [`Self::iter_padded`] instead to obtain the bits, /// but this method is more efficient in some contexts. - pub fn raw_byte_iter(&self) -> RawByteIter { + pub fn raw_byte_iter(&self) -> RawByteIter<'_> { RawByteIter { value: self, yielded_bytes: 0, @@ -600,14 +600,14 @@ impl Value { /// Return an iterator over the compact bit encoding of the value. /// /// This encoding is used for writing witness data and for computing IHRs. - pub fn iter_compact(&self) -> CompactBitsIter { + pub fn iter_compact(&self) -> CompactBitsIter<'_> { CompactBitsIter::new(self.as_ref()) } /// Return an iterator over the padded bit encoding of the value. /// /// This encoding is used to represent the value in the Bit Machine. - pub fn iter_padded(&self) -> PreOrderIter { + pub fn iter_padded(&self) -> PreOrderIter<'_> { PreOrderIter { inner: BitIter::new(self.raw_byte_iter()).take(self.ty.bit_width()), } From 48634a8d9d70e76eb62248bfe9502b7f3b3f1d82 Mon Sep 17 00:00:00 2001 From: Andrew Poelstra Date: Fri, 27 Jun 2025 20:33:47 +0000 Subject: [PATCH 3/3] clippy: enable lossless-cast lint There is a huge list of lints I'd like to enable (currently in rust-elements and a few other crates). As a start, enable the cast_lossless one, which I think is very important: it complaints if you use a cast where usize::from() would suffice. The benefit of the from() syntax is that it will compile iff the cast cannot truncate. So the reader of the code can tell immediately that there is no danger of changing the value, and if the lint is on, conversely the reader can tell that anything that *is* a cast is something that needs further scrutiny. --- Cargo.toml | 7 +++++++ src/policy/satisfy.rs | 6 +++--- 2 files changed, 10 insertions(+), 3 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index f0bccb60..09da5905 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -58,3 +58,10 @@ 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. similar_names = "allow" # Too many (subjectively) false positives. +# Cast-related lints +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). diff --git a/src/policy/satisfy.rs b/src/policy/satisfy.rs index d15d25b2..cd8bee2f 100644 --- a/src/policy/satisfy.rs +++ b/src/policy/satisfy.rs @@ -555,14 +555,14 @@ mod tests { let witness = to_witness(&program); assert_eq!(2, witness.len()); - assert_eq!(Value::u1(bit as u8), *witness[0]); + assert_eq!(Value::u1(u8::from(bit)), *witness[0]); let preimage_bytes = witness[1] .iter_padded() .try_collect_bytes() .expect("to bytes"); let witness_preimage = Preimage32::try_from(preimage_bytes.as_slice()).expect("to array"); - assert_eq!(preimages[bit as usize], witness_preimage); + assert_eq!(preimages[usize::from(bit)], witness_preimage); execute_successful(program, &env); }; @@ -663,7 +663,7 @@ mod tests { ], ); - match bit0 as u8 + bit1 as u8 + bit2 as u8 { + match u8::from(bit0) + u8::from(bit1) + u8::from(bit2) { 3 => assert_branches(&policy, &[bit0, bit1, false]), 2 => assert_branches(&policy, &[bit0, bit1, bit2]), _ => assert!(policy.satisfy(&satisfier, &env).is_err()),