Merge rust-bitcoin/rust-bitcoin#1061: Enabe clippy on CI

e2e4650 Add clippy to pre-commit githook (Tobin C. Harding)
820adc0 Add githooks directory (Tobin C. Harding)
668b37a Enable clippy on CI (Tobin C. Harding)
a2a54b3 Remove unnecessary ? operator (Tobin C. Harding)
67ed8f6 Remove unneeded clone (Tobin C. Harding)
eccd401 Remove unneeded reference (Tobin C. Harding)
fd4239f Use custom digit grouping (Tobin C. Harding)
acd551e Remove unnecessary 'static lifetime (Tobin C. Harding)
3102a48 Allow clippy::collapsible_else_if (Tobin C. Harding)
63f4ff6 Remove redundant field names (Tobin C. Harding)

Pull request description:

  We are almost ready to enable clippy on CI!

  First clear a few remaining warnings from feature gated code. Then add a CI job to run clippy using `--all-features` (note no `--all-targets`). Next add githooks directory (this is the patch from https://github.com/rust-bitcoin/rust-bitcoin/pull/1044/commits). Finally add `cargo clippy` to the pre-commit hook.

  EDIT: I just realized the running of clippy could have been done in `test.sh` instead of as a github action. Does that matter?

ACKs for top commit:
  apoelstra:
    ACK e2e4650
  Kixunil:
    ACK e2e4650

Tree-SHA512: c78cb78973d3935e5be7908eec7d6ffaa7989724b3e29551b9fa2cb35df1f39574e31e5cc93fdfb32b35039ac9dac5c080ae4287a1e6979dd076bab56e6eda1b

Author: apoelstra

.github/workflows/rust.yml

@@ -107,3 +107,18 @@ jobs:
           CARGO_TARGET_THUMBV7M_NONE_EABI_RUNNER: "qemu-system-arm -cpu cortex-m3 -machine mps2-an385 -nographic -semihosting-config enable=on,target=native -kernel"
         run: cd embedded && cargo run --target thumbv7m-none-eabi
 
+  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: --all-features -- -D warnings

README.md

@@ -125,6 +125,17 @@ In order to speed up the review process the CI pipeline can be run locally using
 skipped when using `act` due to caching being unsupported at this time. We do
 not *actively* support `act` but will merge PRs fixing `act` issues.
 
+### 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.
+
 ## Policy on Altcoins/Altchains
 
 Patches which add support for non-Bitcoin cryptocurrencies by adding constants

githooks/pre-commit

@@ -0,0 +1,49 @@
+#!/bin/sh
+#
+# 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 --all-features -- -D warnings || exit 1

src/blockdata/script.rs

@@ -1085,7 +1085,7 @@ impl serde::Serialize for Script {
         if serializer.is_human_readable() {
             serializer.serialize_str(&format!("{:x}", self))
         } else {
-            serializer.serialize_bytes(&self.as_bytes())
+            serializer.serialize_bytes(self.as_bytes())
         }
     }
 }

src/blockdata/transaction.rs

@@ -631,7 +631,7 @@ impl Transaction {
             if let Some(output) = spent(&input.previous_output) {
                 output.script_pubkey.verify_with_flags(idx, crate::Amount::from_sat(output.value), tx.as_slice(), flags)?;
             } else {
-                return Err(script::Error::UnknownSpentOutput(input.previous_output.clone()));
+                return Err(script::Error::UnknownSpentOutput(input.previous_output));
             }
         }
         Ok(())
@@ -1662,7 +1662,7 @@ mod benches {
     use crate::hashes::hex::FromHex;
     use test::{black_box, Bencher};
 
-    const SOME_TX: &'static str = "0100000001a15d57094aa7a21a28cb20b59aab8fc7d1149a3bdbcddba9c622e4f5f6a99ece010000006c493046022100f93bb0e7d8db7bd46e40132d1f8242026e045f03a0efe71bbb8e3f475e970d790221009337cd7f1f929f00cc6ff01f03729b069a7c21b59b1736ddfee5db5946c5da8c0121033b9b137ee87d5a812d6f506efdd37f0affa7ffc310711c06c7f3e097c9447c52ffffffff0100e1f505000000001976a9140389035a9225b3839e2bbf32d826a1e222031fd888ac00000000";
+    const SOME_TX: &str = "0100000001a15d57094aa7a21a28cb20b59aab8fc7d1149a3bdbcddba9c622e4f5f6a99ece010000006c493046022100f93bb0e7d8db7bd46e40132d1f8242026e045f03a0efe71bbb8e3f475e970d790221009337cd7f1f929f00cc6ff01f03729b069a7c21b59b1736ddfee5db5946c5da8c0121033b9b137ee87d5a812d6f506efdd37f0affa7ffc310711c06c7f3e097c9447c52ffffffff0100e1f505000000001976a9140389035a9225b3839e2bbf32d826a1e222031fd888ac00000000";
 
     #[bench]
     pub fn bench_transaction_size(bh: &mut Bencher) {

src/internal_macros.rs

@@ -276,7 +276,7 @@ macro_rules! serde_struct_human_string_impl {
                             )*
 
                             let ret = $name {
-                                $($fe: $fe),*
+                                $($fe),*
                             };
 
                             Ok(ret)
@@ -312,7 +312,7 @@ macro_rules! serde_struct_human_string_impl {
                             )*
 
                             let ret = $name {
-                                $($fe: $fe),*
+                                $($fe),*
                             };
 
                             Ok(ret)

src/util/amount.rs

@@ -1310,7 +1310,7 @@ pub mod serde {
         }
         fn des_btc<'d, D: Deserializer<'d>>(d: D) -> Result<Self, D::Error> {
             use serde::de::Error;
-            Ok(Amount::from_btc(f64::deserialize(d)?).map_err(D::Error::custom)?)
+            Amount::from_btc(f64::deserialize(d)?).map_err(D::Error::custom)
         }
     }
 
@@ -1338,7 +1338,7 @@ pub mod serde {
         }
         fn des_btc<'d, D: Deserializer<'d>>(d: D) -> Result<Self, D::Error> {
             use serde::de::Error;
-            Ok(SignedAmount::from_btc(f64::deserialize(d)?).map_err(D::Error::custom)?)
+            SignedAmount::from_btc(f64::deserialize(d)?).map_err(D::Error::custom)
         }
     }
 
@@ -1622,9 +1622,9 @@ mod tests {
         assert_eq!(p("1.1", btc), Ok(Amount::from_sat(1_100_000_00)));
         assert_eq!(p("100", sat), Ok(Amount::from_sat(100)));
         assert_eq!(p("55", sat), Ok(Amount::from_sat(55)));
-        assert_eq!(p("5500000000000000000", sat), Ok(Amount::from_sat(5_500_000_000_000_000_000)));
+        assert_eq!(p("5500000000000000000", sat), Ok(Amount::from_sat(55_000_000_000_000_000_00)));
         // Should this even pass?
-        assert_eq!(p("5500000000000000000.", sat), Ok(Amount::from_sat(5_500_000_000_000_000_000)));
+        assert_eq!(p("5500000000000000000.", sat), Ok(Amount::from_sat(55_000_000_000_000_000_00)));
         assert_eq!(
             p("12345678901.12345678", btc),
             Ok(Amount::from_sat(12_345_678_901__123_456_78))
@@ -2006,6 +2006,7 @@ mod tests {
 
     #[cfg(feature = "serde")]
     #[test]
+    #[allow(clippy::inconsistent_digit_grouping)] // Group to show 100,000,000 sats per bitcoin.
     fn serde_as_btc() {
         use serde_json;
 
@@ -2041,6 +2042,7 @@ mod tests {
 
     #[cfg(feature = "serde")]
     #[test]
+    #[allow(clippy::inconsistent_digit_grouping)] // Group to show 100,000,000 sats per bitcoin.
     fn serde_as_btc_opt() {
         use serde_json;
 
@@ -2054,8 +2056,8 @@ mod tests {
         }
 
         let with = T {
-            amt: Some(Amount::from_sat(2__500_000_00)),
-            samt: Some(SignedAmount::from_sat(-2__500_000_00)),
+            amt: Some(Amount::from_sat(2_500_000_00)),
+            samt: Some(SignedAmount::from_sat(-2_500_000_00)),
         };
         let without = T {
             amt: None,
@@ -2085,6 +2087,7 @@ mod tests {
 
     #[cfg(feature = "serde")]
     #[test]
+    #[allow(clippy::inconsistent_digit_grouping)] // Group to show 100,000,000 sats per bitcoin.
     fn serde_as_sat_opt() {
         use serde_json;
 
@@ -2098,8 +2101,8 @@ mod tests {
         }
 
         let with = T {
-            amt: Some(Amount::from_sat(2__500_000_00)),
-            samt: Some(SignedAmount::from_sat(-2__500_000_00)),
+            amt: Some(Amount::from_sat(2_500_000_00)),
+            samt: Some(SignedAmount::from_sat(-2_500_000_00)),
         };
         let without = T {
             amt: None,

src/util/key.rs

@@ -411,6 +411,7 @@ impl<'de> ::serde::Deserialize<'de> for PrivateKey {
 
 #[cfg(feature = "serde")]
 #[cfg_attr(docsrs, doc(cfg(feature = "serde")))]
+#[allow(clippy::collapsible_else_if)] // Aids readability.
 impl ::serde::Serialize for PublicKey {
     fn serialize<S: ::serde::Serializer>(&self, s: S) -> Result<S::Ok, S::Error> {
         if s.is_human_readable() {
@@ -549,9 +550,9 @@ mod tests {
     fn test_key_serde() {
         use serde_test::{Configure, Token, assert_tokens};
 
-        static KEY_WIF: &'static str = "cVt4o7BGAig1UXywgGSmARhxMdzP5qvQsxKkSsc1XEkw3tDTQFpy";
-        static PK_STR: &'static str = "039b6347398505f5ec93826dc61c19f47c66c0283ee9be980e29ce325a0f4679ef";
-        static PK_STR_U: &'static str = "\
+        static KEY_WIF: &str = "cVt4o7BGAig1UXywgGSmARhxMdzP5qvQsxKkSsc1XEkw3tDTQFpy";
+        static PK_STR: &str = "039b6347398505f5ec93826dc61c19f47c66c0283ee9be980e29ce325a0f4679ef";
+        static PK_STR_U: &str = "\
             04\
             9b6347398505f5ec93826dc61c19f47c66c0283ee9be980e29ce325a0f4679ef\
             87288ed73ce47fc4f5c79d19ebfa57da7cff3aff6e819e4ee971d86b5e61875d\

src/util/psbt/mod.rs

@@ -269,7 +269,7 @@ mod display_from_str {
 
         fn from_str(s: &str) -> Result<Self, Self::Err> {
             let data = ::base64::decode(s).map_err(PsbtParseError::Base64Encoding)?;
-            Ok(encode::deserialize(&data).map_err(PsbtParseError::PsbtEncoding)?)
+            encode::deserialize(&data).map_err(PsbtParseError::PsbtEncoding)
         }
     }
 }