Merge #448: Add clippy to CI

65186e7 Add githooks (Tobin C. Harding)
6d76bd4 Add clippy to CI (Tobin C. Harding)
9f1ebb9 Allow nonminimal_bool in unit test (Tobin C. Harding)
685444c Use "a".repeats() instead of manual implementation (Tobin C. Harding)
42de876 Allow let_and_return for feature guarded code (Tobin C. Harding)
d64132c Allow missing_safety_doc (Tobin C. Harding)
2cb687f Use to_le_bytes instead of mem::transmute (Tobin C. Harding)
c15b9d2 Remove unneeded explicit reference (Tobin C. Harding)
35d59e7 Remove explicit 'static lifetime (Tobin C. Harding)
1a582db Remove redundant import (Tobin C. Harding)

Pull request description:

  The first 8 patches clear clippy warnings. Next we add a CI job to run clippy. Finally we add a `githooks` directory that includes running clippy, also adds a section to the README on how to use the githooks. This is identical to the text in the [open PR](rust-bitcoin/rust-bitcoin#1044) on `rust-bitcoin` that adds githooks _without_ yet adding clippy.

  **Note**: The new clippy CI job runs and is green :)

ACKs for top commit:
  Kixunil:
    ACK 65186e7
  apoelstra:
    ACK 65186e7

Tree-SHA512: f70a157896ce2a83af8cfc10f2fbacc8f68256ac96ef7dec4d190aa72324b568d2267418eb4fe99099aeda5486957c31070943d7c209973859b7b9290676ccd7

Author: apoelstra

.github/workflows/rust.yml

@@ -73,3 +73,19 @@ jobs:
         env:
           DO_WASM: true
         run: ./contrib/test.sh
+
+  Clippy:
+    name: Clippy
+    runs-on: ubuntu-latest
+    steps:
+      - uses: actions/checkout@v2
+      - uses: actions-rs/toolchain@v1
+        with:
+          profile: minimal
+          toolchain: stable
+          override: true
+      - run: rustup component add clippy
+      - uses: actions-rs/cargo@v1
+        with:
+          command: clippy
+          args: --features=rand-std,recovery,lowmemory,global-context --all-targets -- -D warnings

README.md

@@ -22,6 +22,17 @@ Contributions to this library are welcome. A few guidelines:
 * No crypto should be implemented in Rust, with the possible exception of hash functions. Cryptographic contributions should be directed upstream to libsecp256k1.
 * This library should always compile with any combination of features on **Rust 1.41.1**.
 
+### Githooks
+
+To assist devs in catching errors _before_ running CI we provide some githooks. If you do not
+already have locally configured githooks you can use the ones in this repository by running, in the
+root directory of the repository:
+```
+git config --local core.hooksPath githooks/
+```
+
+Alternatively add symlinks in your `.git/hooks` directory to any of the githooks we provide.
+
 ## Fuzzing
 
 If you want to fuzz this library, or any library which depends on it, you will

githooks/pre-commit

@@ -0,0 +1,50 @@
+#!/bin/sh
+#
+# A hook script to verify what is about to be committed.
+# Called by "git commit" with no arguments.  The hook should
+# exit with non-zero status after issuing an appropriate message if
+# it wants to stop the commit.
+
+if git rev-parse --verify HEAD >/dev/null 2>&1
+then
+	against=HEAD
+else
+	# Initial commit: diff against an empty tree object
+	against=$(git hash-object -t tree /dev/null)
+fi
+
+# If you want to allow non-ASCII filenames set this variable to true.
+allownonascii=$(git config --bool hooks.allownonascii)
+
+# Redirect output to stderr.
+exec 1>&2
+
+# Cross platform projects tend to avoid non-ASCII filenames; prevent
+# them from being added to the repository. We exploit the fact that the
+# printable range starts at the space character and ends with tilde.
+if [ "$allownonascii" != "true" ] &&
+	# Note that the use of brackets around a tr range is ok here, (it's
+	# even required, for portability to Solaris 10's /usr/bin/tr), since
+	# the square bracket bytes happen to fall in the designated range.
+	test $(git diff --cached --name-only --diff-filter=A -z $against |
+	  LC_ALL=C tr -d '[ -~]\0' | wc -c) != 0
+then
+	cat <<\EOF
+Error: Attempt to add a non-ASCII file name.
+
+This can cause problems if you want to work with people on other platforms.
+
+To be portable it is advisable to rename the file.
+
+If you know what you are doing you can disable this check using:
+
+  git config hooks.allownonascii true
+EOF
+	exit 1
+fi
+
+# If there are whitespace errors, print the offending file names and fail.
+git diff-index --check --cached $against -- || exit 1
+
+# Check that code lints cleanly.
+cargo clippy --features=rand-std,recovery,lowmemory,global-context --all-targets -- -D warnings || exit 1

secp256k1-sys/src/lib.rs

@@ -19,6 +19,8 @@
 // Coding conventions
 #![deny(non_upper_case_globals, non_camel_case_types, non_snake_case, unused_mut)]
 
+#![allow(clippy::missing_safety_doc)]
+
 #![cfg_attr(all(not(test), not(feature = "std")), no_std)]
 #![cfg_attr(docsrs, feature(doc_cfg))]
 

src/context.rs

@@ -114,9 +114,6 @@ mod alloc_only {
     use crate::ffi::{self, types::{c_uint, c_void}};
     use crate::{Secp256k1, Signing, Verification, Context, AlignedType};
 
-    #[cfg(feature = "rand-std")]
-    use rand;
-
     impl private::Sealed for SignOnly {}
     impl private::Sealed for All {}
     impl private::Sealed for VerifyOnly {}
@@ -191,7 +188,7 @@ mod alloc_only {
         /// ctx.seeded_randomize(&seed);
         /// # }
         /// ```
-        #[allow(unused_mut)]    // Unused when `rand-std` is not enabled.
+        #[cfg_attr(not(feature = "rand-std"), allow(clippy::let_and_return, unused_mut))]
         pub fn gen_new() -> Secp256k1<C> {
             #[cfg(target_arch = "wasm32")]
             ffi::types::sanity_checks_for_wasm();

src/ecdh.rs

@@ -176,7 +176,7 @@ impl ::serde::Serialize for SharedSecret {
             let mut buf = [0u8; SHARED_SECRET_SIZE * 2];
             s.serialize_str(crate::to_hex(&self.0, &mut buf).expect("fixed-size hex serialization"))
         } else {
-            s.serialize_bytes(&self.as_ref()[..])
+            s.serialize_bytes(self.as_ref())
         }
     }
 }
@@ -272,9 +272,7 @@ mod tests {
             0xff, 0xff, 0, 0, 0xff, 0xff, 0, 0,
             99, 99, 99, 99, 99, 99, 99, 99
         ];
-        static STR: &'static str = "\
-            01010101010101010001020304050607ffff0000ffff00006363636363636363\
-        ";
+        static STR: &str = "01010101010101010001020304050607ffff0000ffff00006363636363636363";
 
         let secret = SharedSecret::from_slice(&BYTES).unwrap();
 

src/ecdsa/mod.rs

@@ -1,6 +1,6 @@
 //! Structs and functionality related to the ECDSA signature algorithm.
 
-use core::{fmt, str, ops, ptr, mem};
+use core::{fmt, str, ops, ptr};
 
 use crate::{Signing, Verification, Message, PublicKey, Secp256k1, SecretKey, from_hex, Error, ffi};
 use crate::ffi::CPtr;
@@ -398,14 +398,8 @@ impl<C: Signing> Secp256k1<C> {
                     }
 
                     counter += 1;
-                    // From 1.32 can use `to_le_bytes` instead
-                    let le_counter = counter.to_le();
-                    let le_counter_bytes : [u8; 4] = mem::transmute(le_counter);
-                    for (i, b) in le_counter_bytes.iter().enumerate() {
-                        extra_entropy[i] = *b;
-                    }
-
-                    entropy_p = extra_entropy.as_ptr() as *const ffi::types::c_void;
+                    extra_entropy[..4].copy_from_slice(&counter.to_le_bytes());
+                    entropy_p = extra_entropy.as_ptr().cast::<ffi::types::c_void>();
 
                     // When fuzzing, these checks will usually spinloop forever, so just short-circuit them.
                     #[cfg(fuzzing)]

src/key.rs

@@ -431,7 +431,7 @@ impl PublicKey {
     #[cfg(feature = "global-context")]
     #[cfg_attr(docsrs, doc(cfg(feature = "global-context")))]
     pub fn from_secret_key_global(sk: &SecretKey) -> PublicKey {
-        PublicKey::from_secret_key(&SECP256K1, sk)
+        PublicKey::from_secret_key(SECP256K1, sk)
     }
 
     /// Creates a public key directly from a slice.
@@ -1621,7 +1621,7 @@ pub mod serde_keypair {
         let secret_key = SecretKey::deserialize(deserializer)?;
 
         Ok(KeyPair::from_secret_key(
-            &crate::SECP256K1,
+            crate::SECP256K1,
             &secret_key,
         ))
     }
@@ -1875,7 +1875,7 @@ mod test {
         assert!(PublicKey::from_str("0218845781f631c48f1c9709e23092067d06837f30aa0cd0544ac887fe91ddd1").is_err());
         assert!(PublicKey::from_str("xx0218845781f631c48f1c9709e23092067d06837f30aa0cd0544ac887fe91ddd1").is_err());
 
-        let long_str: String = core::iter::repeat('a').take(1024 * 1024).collect();
+        let long_str = "a".repeat(1024 * 1024);
         assert!(SecretKey::from_str(&long_str).is_err());
         assert!(PublicKey::from_str(&long_str).is_err());
     }
@@ -2077,6 +2077,7 @@ mod test {
 
     #[cfg(not(fuzzing))]
     #[test]
+    #[allow(clippy::nonminimal_bool)]
     fn pubkey_equal() {
         let pk1 = PublicKey::from_slice(
             &hex!("0241cc121c419921942add6db6482fb36243faf83317c866d2a28d8c6d7089f7ba"),
@@ -2108,9 +2109,8 @@ mod test {
             0xff, 0xff, 0, 0, 0xff, 0xff, 0, 0,
             99, 99, 99, 99, 99, 99, 99, 99
         ];
-        static SK_STR: &'static str = "\
-            01010101010101010001020304050607ffff0000ffff00006363636363636363\
-        ";
+        static SK_STR: &str = "01010101010101010001020304050607ffff0000ffff00006363636363636363";
+
         #[cfg(fuzzing)]
         static PK_BYTES: [u8; 33] = [
             0x02,
@@ -2119,9 +2119,7 @@ mod test {
             0x06, 0x83, 0x7f, 0x30, 0xaa, 0x0c, 0xd0, 0x54,
             0x4a, 0xc8, 0x87, 0xfe, 0x91, 0xdd, 0xd1, 0x66,
         ];
-        static PK_STR: &'static str = "\
-            0218845781f631c48f1c9709e23092067d06837f30aa0cd0544ac887fe91ddd166\
-        ";
+        static PK_STR: &str = "0218845781f631c48f1c9709e23092067d06837f30aa0cd0544ac887fe91ddd166";
 
         #[cfg(not(fuzzing))]
         let s = Secp256k1::new();
@@ -2236,9 +2234,7 @@ mod test {
             0xff, 0xff, 0, 0, 0xff, 0xff, 0, 0,
             99, 99, 99, 99, 99, 99, 99, 99
         ];
-        static SK_STR: &'static str = "\
-            01010101010101010001020304050607ffff0000ffff00006363636363636363\
-        ";
+        static SK_STR: &str = "01010101010101010001020304050607ffff0000ffff00006363636363636363";
 
         let sk = KeyPairWrapper(KeyPair::from_seckey_slice(&crate::SECP256K1, &SK_BYTES).unwrap());
         assert_tokens(&sk.compact(), &[

src/lib.rs

@@ -152,6 +152,8 @@
 #![deny(non_upper_case_globals, non_camel_case_types, non_snake_case)]
 #![warn(missing_docs, missing_copy_implementations, missing_debug_implementations)]
 
+#![allow(clippy::missing_safety_doc)]
+
 #![cfg_attr(all(not(test), not(feature = "std")), no_std)]
 #![cfg_attr(all(test, feature = "unstable"), feature(test))]
 #![cfg_attr(docsrs, feature(doc_cfg))]
@@ -729,10 +731,10 @@ mod tests {
 
         assert_eq!(
             ecdsa::Signature::from_der(&byte_str).expect("byte str decode"),
-            ecdsa::Signature::from_str(&hex_str).expect("byte str decode")
+            ecdsa::Signature::from_str(hex_str).expect("byte str decode")
         );
 
-        let sig = ecdsa::Signature::from_str(&hex_str).expect("byte str decode");
+        let sig = ecdsa::Signature::from_str(hex_str).expect("byte str decode");
         assert_eq!(&sig.to_string(), hex_str);
         assert_eq!(&format!("{:?}", sig), hex_str);
 
@@ -759,7 +761,7 @@ mod tests {
 
         // 71 byte signature
         let hex_str = "30450221009d0bad576719d32ae76bedb34c774866673cbde3f4e12951555c9408e6ce774b02202876e7102f204f6bfee26c967c3926ce702cf97d4b010062e193f763190f6776";
-        let sig = ecdsa::Signature::from_str(&hex_str).expect("byte str decode");
+        let sig = ecdsa::Signature::from_str(hex_str).expect("byte str decode");
         assert_eq!(&format!("{}", sig), hex_str);
     }
 
@@ -1001,7 +1003,7 @@ mod tests {
             226, 108, 150, 124, 57, 38, 206, 112, 44, 249, 125, 75, 1, 0, 98, 225,
             147, 247, 99, 25, 15, 103, 118
         ];
-        static SIG_STR: &'static str = "\
+        static SIG_STR: &str = "\
             30450221009d0bad576719d32ae76bedb34c774866673cbde3f4e12951555c9408e6ce77\
             4b02202876e7102f204f6bfee26c967c3926ce702cf97d4b010062e193f763190f6776\
         ";
@@ -1026,7 +1028,7 @@ mod tests {
         let msg = Message::from_slice(&msg_data).unwrap();
 
         // Check usage as explicit parameter
-        let pk = PublicKey::from_secret_key(&SECP256K1, &sk);
+        let pk = PublicKey::from_secret_key(SECP256K1, &sk);
 
         // Check usage as self
         let sig = SECP256K1.sign_ecdsa(&msg, &sk);

src/schnorr.rs

@@ -505,7 +505,7 @@ mod tests {
         )
         .is_err());
 
-        let long_str: String = core::iter::repeat('a').take(1024 * 1024).collect();
+        let long_str: String = "a".repeat(1024 * 1024);
         assert!(XOnlyPublicKey::from_str(&long_str).is_err());
     }
 
@@ -548,17 +548,15 @@ mod tests {
             0x9e, 0x4d, 0x4e, 0xf5, 0x67, 0x8a, 0xd0, 0xd9, 0xd5, 0x32, 0xc0, 0xdf, 0xa9, 0x07,
             0xb5, 0x68, 0x72, 0x2d, 0x0b, 0x01, 0x19, 0xba,
         ];
-        static SIG_STR: &'static str = "\
+        static SIG_STR: &str = "\
             14d0bf1a8953506fb460f58be141af767fd112535fb3922ef217308e2c26706f1eeb432b3dba9a01082f9e4d4ef5678ad0d9d532c0dfa907b568722d0b0119ba\
         ";
 
         static PK_BYTES: [u8; 32] = [
             24, 132, 87, 129, 246, 49, 196, 143, 28, 151, 9, 226, 48, 146, 6, 125, 6, 131, 127,
             48, 170, 12, 208, 84, 74, 200, 135, 254, 145, 221, 209, 102
         ];
-        static PK_STR: &'static str = "\
-            18845781f631c48f1c9709e23092067d06837f30aa0cd0544ac887fe91ddd166\
-        ";
+        static PK_STR: &str = "18845781f631c48f1c9709e23092067d06837f30aa0cd0544ac887fe91ddd166";
         let pk = XOnlyPublicKey::from_slice(&PK_BYTES).unwrap();
 
         assert_tokens(&sig.compact(), &[Token::BorrowedBytes(&SIG_BYTES[..])]);