Fix clippy warnings and enable clippy in CI (#1008)

gibbz00 · web-flow · commit 04be6049ea67 · 2024-04-09T15:05:33.000Z
* prost: clarify encoding test buffer copying:

fixes `noop_method_call` clippy lint warning.

* ci: add lint checking step:

* Makes that the entrire workspace is checked, but tests too.

* chore: fix `needless_borrow` lints

* chore: fix `needless_return` lints

* chore: remove noop clippy allows

* chore: fix `box_default` lints

* chore: fix `unnecessary_cast` lints

* chore: fix `slow_vector_initialization` lints

* chore: fix `assigning_clones` lints

* chore: fix `unused_import` lints

* chore: fix `single_match` lints
diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml
@@ -12,7 +12,6 @@ env:
   PROTOC_VERSION: 3.20.3
 
 jobs:
-
   rustfmt:
     runs-on: ubuntu-latest
     steps:
@@ -22,6 +21,21 @@ jobs:
           components: rustfmt
       - run: cargo fmt --all --check
 
+  clippy:
+    runs-on: ubuntu-latest
+    steps:
+      - uses: actions/checkout@v4
+      - name: install protoc
+        uses: taiki-e/install-action@v2
+        with:
+          tool: protoc@${{ env.PROTOC_VERSION }}
+      - name: install ninja
+        uses: ./.github/actions/setup-ninja
+      - uses: dtolnay/rust-toolchain@stable
+        with:
+          components: clippy
+      - run: cargo clippy --workspace --all-features --tests -- -D warnings
+
   machete:
     runs-on: ubuntu-latest
     steps:
diff --git a/conformance/src/main.rs b/conformance/src/main.rs
@@ -12,7 +12,7 @@ use tests::{roundtrip, RoundtripResult};
 
 fn main() -> io::Result<()> {
     env_logger::init();
-    let mut bytes = Vec::new();
+    let mut bytes = vec![0; 4];
 
     loop {
         bytes.resize(4, 0);
diff --git a/prost-build/src/code_generator.rs b/prost-build/src/code_generator.rs
@@ -205,9 +205,7 @@ impl<'a> CodeGenerator<'a> {
                 .as_ref()
                 .and_then(|type_name| map_types.get(type_name))
             {
-                Some(&(ref key, ref value)) => {
-                    self.append_map_field(&fq_message_name, field, key, value)
-                }
+                Some((key, value)) => self.append_map_field(&fq_message_name, field, key, value),
                 None => self.append_field(&fq_message_name, field),
             }
             self.path.pop();
@@ -273,7 +271,7 @@ impl<'a> CodeGenerator<'a> {
         self.buf.push_str(&format!(
             "impl {}::Name for {} {{\n",
             self.config.prost_path.as_deref().unwrap_or("::prost"),
-            to_upper_camel(&message_name)
+            to_upper_camel(message_name)
         ));
         self.depth += 1;
 
@@ -382,7 +380,7 @@ impl<'a> CodeGenerator<'a> {
             || (self
                 .config
                 .boxed
-                .get_first_field(&fq_message_name, field.name())
+                .get_first_field(fq_message_name, field.name())
                 .is_some());
 
         debug!(
@@ -564,10 +562,7 @@ impl<'a> CodeGenerator<'a> {
         self.buf.push_str(&format!(
             "#[prost(oneof=\"{}\", tags=\"{}\")]\n",
             name,
-            fields
-                .iter()
-                .map(|&(ref field, _)| field.number())
-                .join(", ")
+            fields.iter().map(|(field, _)| field.number()).join(", ")
         ));
         self.append_field_attributes(fq_message_name, oneof.name());
         self.push_indent();
@@ -601,7 +596,7 @@ impl<'a> CodeGenerator<'a> {
             "#[derive(Clone, PartialEq, {}::Oneof)]\n",
             prost_path(self.config)
         ));
-        self.append_skip_debug(&fq_message_name);
+        self.append_skip_debug(fq_message_name);
         self.push_indent();
         self.buf.push_str("pub enum ");
         self.buf.push_str(&to_upper_camel(oneof.name()));
diff --git a/prost-build/src/ident.rs b/prost-build/src/ident.rs
@@ -68,9 +68,6 @@ pub fn strip_enum_prefix(prefix: &str, name: &str) -> String {
 
 #[cfg(test)]
 mod tests {
-
-    #![allow(clippy::cognitive_complexity)]
-
     use super::*;
 
     #[test]
diff --git a/prost-derive/src/field/mod.rs b/prost-derive/src/field/mod.rs
@@ -269,9 +269,7 @@ fn bool_attr(key: &str, attr: &Meta) -> Result<Option<bool>, Error> {
     }
     match *attr {
         Meta::Path(..) => Ok(Some(true)),
-        Meta::List(ref meta_list) => {
-            return Ok(Some(meta_list.parse_args::<LitBool>()?.value()));
-        }
+        Meta::List(ref meta_list) => Ok(Some(meta_list.parse_args::<LitBool>()?.value())),
         Meta::NameValue(MetaNameValue {
             value:
                 Expr::Lit(ExprLit {
@@ -310,9 +308,7 @@ pub(super) fn tag_attr(attr: &Meta) -> Result<Option<u32>, Error> {
         return Ok(None);
     }
     match *attr {
-        Meta::List(ref meta_list) => {
-            return Ok(Some(meta_list.parse_args::<LitInt>()?.base10_parse()?));
-        }
+        Meta::List(ref meta_list) => Ok(Some(meta_list.parse_args::<LitInt>()?.base10_parse()?)),
         Meta::NameValue(MetaNameValue {
             value: Expr::Lit(ref expr),
             ..
diff --git a/prost-derive/src/field/scalar.rs b/prost-derive/src/field/scalar.rs
@@ -1,4 +1,3 @@
-use std::convert::TryFrom;
 use std::fmt;
 
 use anyhow::{anyhow, bail, Error};
@@ -272,7 +271,7 @@ impl Field {
     pub fn methods(&self, ident: &TokenStream) -> Option<TokenStream> {
         let mut ident_str = ident.to_string();
         if ident_str.starts_with("r#") {
-            ident_str = ident_str[2..].to_owned();
+            ident_str = ident_str.split_off(2);
         }
 
         // Prepend `get_` for getter methods of tuple structs.
diff --git a/prost-derive/src/lib.rs b/prost-derive/src/lib.rs
@@ -88,12 +88,12 @@ fn try_message(input: TokenStream) -> Result<TokenStream, Error> {
     // TODO: This encodes oneof fields in the position of their lowest tag,
     // regardless of the currently occupied variant, is that consequential?
     // See: https://developers.google.com/protocol-buffers/docs/encoding#order
-    fields.sort_by_key(|&(_, ref field)| field.tags().into_iter().min().unwrap());
+    fields.sort_by_key(|(_, field)| field.tags().into_iter().min().unwrap());
     let fields = fields;
 
     let mut tags = fields
         .iter()
-        .flat_map(|&(_, ref field)| field.tags())
+        .flat_map(|(_, field)| field.tags())
         .collect::<Vec<_>>();
     let num_tags = tags.len();
     tags.sort_unstable();
@@ -104,13 +104,13 @@ fn try_message(input: TokenStream) -> Result<TokenStream, Error> {
 
     let encoded_len = fields
         .iter()
-        .map(|&(ref field_ident, ref field)| field.encoded_len(quote!(self.#field_ident)));
+        .map(|(field_ident, field)| field.encoded_len(quote!(self.#field_ident)));
 
     let encode = fields
         .iter()
-        .map(|&(ref field_ident, ref field)| field.encode(quote!(self.#field_ident)));
+        .map(|(field_ident, field)| field.encode(quote!(self.#field_ident)));
 
-    let merge = fields.iter().map(|&(ref field_ident, ref field)| {
+    let merge = fields.iter().map(|(field_ident, field)| {
         let merge = field.merge(quote!(value));
         let tags = field.tags().into_iter().map(|tag| quote!(#tag));
         let tags = Itertools::intersperse(tags, quote!(|));
@@ -136,7 +136,7 @@ fn try_message(input: TokenStream) -> Result<TokenStream, Error> {
 
     let clear = fields
         .iter()
-        .map(|&(ref field_ident, ref field)| field.clear(quote!(self.#field_ident)));
+        .map(|(field_ident, field)| field.clear(quote!(self.#field_ident)));
 
     let default = if is_struct {
         let default = fields.iter().map(|(field_ident, field)| {
@@ -158,7 +158,7 @@ fn try_message(input: TokenStream) -> Result<TokenStream, Error> {
 
     let methods = fields
         .iter()
-        .flat_map(|&(ref field_ident, ref field)| field.methods(field_ident))
+        .flat_map(|(field_ident, field)| field.methods(field_ident))
         .collect::<Vec<_>>();
     let methods = if methods.is_empty() {
         quote!()
@@ -213,7 +213,7 @@ fn try_message(input: TokenStream) -> Result<TokenStream, Error> {
     let expanded = if skip_debug {
         expanded
     } else {
-        let debugs = unsorted_fields.iter().map(|&(ref field_ident, ref field)| {
+        let debugs = unsorted_fields.iter().map(|(field_ident, field)| {
             let wrapper = field.debug(quote!(self.#field_ident));
             let call = if is_struct {
                 quote!(builder.field(stringify!(#field_ident), &wrapper))
@@ -300,16 +300,14 @@ fn try_enumeration(input: TokenStream) -> Result<TokenStream, Error> {
 
     let default = variants[0].0.clone();
 
-    let is_valid = variants
+    let is_valid = variants.iter().map(|(_, value)| quote!(#value => true));
+    let from = variants
         .iter()
-        .map(|&(_, ref value)| quote!(#value => true));
-    let from = variants.iter().map(
-        |&(ref variant, ref value)| quote!(#value => ::core::option::Option::Some(#ident::#variant)),
-    );
+        .map(|(variant, value)| quote!(#value => ::core::option::Option::Some(#ident::#variant)));
 
-    let try_from = variants.iter().map(
-        |&(ref variant, ref value)| quote!(#value => ::core::result::Result::Ok(#ident::#variant)),
-    );
+    let try_from = variants
+        .iter()
+        .map(|(variant, value)| quote!(#value => ::core::result::Result::Ok(#ident::#variant)));
 
     let is_valid_doc = format!("Returns `true` if `value` is a variant of `{}`.", ident);
     let from_i32_doc = format!(
@@ -416,7 +414,7 @@ fn try_oneof(input: TokenStream) -> Result<TokenStream, Error> {
 
     let mut tags = fields
         .iter()
-        .flat_map(|&(ref variant_ident, ref field)| -> Result<u32, Error> {
+        .flat_map(|(variant_ident, field)| -> Result<u32, Error> {
             if field.tags().len() > 1 {
                 bail!(
                     "invalid oneof variant {}::{}: oneof variants may only have a single tag",
@@ -433,12 +431,12 @@ fn try_oneof(input: TokenStream) -> Result<TokenStream, Error> {
         panic!("invalid oneof {}: variants have duplicate tags", ident);
     }
 
-    let encode = fields.iter().map(|&(ref variant_ident, ref field)| {
+    let encode = fields.iter().map(|(variant_ident, field)| {
         let encode = field.encode(quote!(*value));
         quote!(#ident::#variant_ident(ref value) => { #encode })
     });
 
-    let merge = fields.iter().map(|&(ref variant_ident, ref field)| {
+    let merge = fields.iter().map(|(variant_ident, field)| {
         let tag = field.tags()[0];
         let merge = field.merge(quote!(value));
         quote! {
@@ -457,7 +455,7 @@ fn try_oneof(input: TokenStream) -> Result<TokenStream, Error> {
         }
     });
 
-    let encoded_len = fields.iter().map(|&(ref variant_ident, ref field)| {
+    let encoded_len = fields.iter().map(|(variant_ident, field)| {
         let encoded_len = field.encoded_len(quote!(*value));
         quote!(#ident::#variant_ident(ref value) => #encoded_len)
     });
@@ -499,7 +497,7 @@ fn try_oneof(input: TokenStream) -> Result<TokenStream, Error> {
     let expanded = if skip_debug {
         expanded
     } else {
-        let debug = fields.iter().map(|&(ref variant_ident, ref field)| {
+        let debug = fields.iter().map(|(variant_ident, field)| {
             let wrapper = field.debug(quote!(*value));
             quote!(#ident::#variant_ident(ref value) => {
                 let wrapper = #wrapper;
diff --git a/prost-types/src/any.rs b/prost-types/src/any.rs
@@ -20,16 +20,13 @@ impl Any {
     {
         let expected_type_url = M::type_url();
 
-        match (
+        if let (Some(expected), Some(actual)) = (
             TypeUrl::new(&expected_type_url),
             TypeUrl::new(&self.type_url),
         ) {
-            (Some(expected), Some(actual)) => {
-                if expected == actual {
-                    return Ok(M::decode(self.value.as_slice())?);
-                }
+            if expected == actual {
+                return M::decode(self.value.as_slice());
             }
-            _ => (),
         }
 
         let mut err = DecodeError::new(format!(
diff --git a/prost-types/src/datetime.rs b/prost-types/src/datetime.rs
@@ -567,10 +567,7 @@ pub(crate) fn parse_duration(s: &str) -> Option<Duration> {
         (seconds, nanos as i32)
     };
 
-    Some(Duration {
-        seconds,
-        nanos: nanos as i32,
-    })
+    Some(Duration { seconds, nanos })
 }
 
 impl From<DateTime> for Timestamp {
diff --git a/prost-types/src/timestamp.rs b/prost-types/src/timestamp.rs
@@ -122,7 +122,6 @@ impl Name for Timestamp {
 impl Eq for Timestamp {}
 
 #[cfg(feature = "std")]
-#[allow(clippy::derive_hash_xor_eq)] // Derived logic is correct: comparing the 2 fields for equality
 impl std::hash::Hash for Timestamp {
     fn hash<H: std::hash::Hasher>(&self, state: &mut H) {
         self.seconds.hash(state);
diff --git a/src/encoding.rs b/src/encoding.rs
@@ -9,7 +9,6 @@ use alloc::format;
 use alloc::string::String;
 use alloc::vec::Vec;
 use core::cmp::min;
-use core::convert::TryFrom;
 use core::mem;
 use core::str;
 use core::u32;
@@ -1423,15 +1422,16 @@ pub mod btree_map {
 
 #[cfg(test)]
 mod test {
+    #[cfg(not(feature = "std"))]
     use alloc::string::ToString;
     use core::borrow::Borrow;
     use core::fmt::Debug;
     use core::u64;
 
-    use ::bytes::{Bytes, BytesMut};
+    use ::bytes::BytesMut;
     use proptest::{prelude::*, test_runner::TestCaseResult};
 
-    use crate::encoding::*;
+    use super::*;
 
     pub fn check_type<T, B>(
         value: T,
@@ -1616,11 +1616,14 @@ mod test {
 
             assert_eq!(encoded_len_varint(value), encoded.len());
 
-            let roundtrip_value = decode_varint(&mut encoded.clone()).expect("decoding failed");
+            // See: https://github.com/tokio-rs/prost/pull/1008 for copying reasoning.
+            let mut encoded_copy = encoded;
+            let roundtrip_value = decode_varint(&mut encoded_copy).expect("decoding failed");
             assert_eq!(value, roundtrip_value);
 
+            let mut encoded_copy = encoded;
             let roundtrip_value =
-                decode_varint_slow(&mut encoded.clone()).expect("slow decoding failed");
+                decode_varint_slow(&mut encoded_copy).expect("slow decoding failed");
             assert_eq!(value, roundtrip_value);
         }
 
@@ -1679,13 +1682,18 @@ mod test {
         );
     }
 
+    const U64_MAX_PLUS_ONE: &[u8] = &[0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0x02];
+
     #[test]
     fn varint_overflow() {
-        let u64_max_plus_one: &[u8] = &[0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0x02];
+        let mut copy = U64_MAX_PLUS_ONE;
+        decode_varint(&mut copy).expect_err("decoding u64::MAX + 1 succeeded");
+    }
 
-        decode_varint(&mut u64_max_plus_one.clone()).expect_err("decoding u64::MAX + 1 succeeded");
-        decode_varint_slow(&mut u64_max_plus_one.clone())
-            .expect_err("slow decoding u64::MAX + 1 succeeded");
+    #[test]
+    fn variant_slow_overflow() {
+        let mut copy = U64_MAX_PLUS_ONE;
+        decode_varint_slow(&mut copy).expect_err("slow decoding u64::MAX + 1 succeeded");
     }
 
     /// This big bowl o' macro soup generates an encoding property test for each combination of map
diff --git a/src/error.rs b/src/error.rs
@@ -1,7 +1,9 @@
 //! Protobuf encoding and decoding errors.
 
 use alloc::borrow::Cow;
+#[cfg(not(feature = "std"))]
 use alloc::boxed::Box;
+#[cfg(not(feature = "std"))]
 use alloc::vec::Vec;
 
 use core::fmt;
diff --git a/src/message.rs b/src/message.rs
@@ -1,8 +1,9 @@
+#[cfg(not(feature = "std"))]
 use alloc::boxed::Box;
+#[cfg(not(feature = "std"))]
 use alloc::vec::Vec;
 
 use core::fmt::Debug;
-use core::usize;
 
 use bytes::{Buf, BufMut};
 
diff --git a/src/name.rs b/src/name.rs
@@ -1,6 +1,8 @@
 //! Support for associating type name information with a [`Message`].
 
 use crate::Message;
+
+#[cfg(not(feature = "std"))]
 use alloc::{format, string::String};
 
 /// Associate a type name with a [`Message`] type.
diff --git a/src/types.rs b/src/types.rs
diff --git a/tests/src/build.rs b/tests/src/build.rs
diff --git a/tests/src/debug.rs b/tests/src/debug.rs
diff --git a/tests/src/deprecated_field.rs b/tests/src/deprecated_field.rs
diff --git a/tests/src/lib.rs b/tests/src/lib.rs
diff --git a/tests/src/message_encoding.rs b/tests/src/message_encoding.rs
diff --git a/tests/src/skip_debug.rs b/tests/src/skip_debug.rs

Original file line number	Diff line number	Diff line change
`@@ -20,16 +20,13 @@ impl Any {`
`20`	`20`	`{`
`21`	`21`	`let expected_type_url = M::type_url();`
`22`	`22`
`23`		`- match (`
	`23`	`+ if let (Some(expected), Some(actual)) = (`
`24`	`24`	`TypeUrl::new(&expected_type_url),`
`25`	`25`	`TypeUrl::new(&self.type_url),`
`26`	`26`	`) {`
`27`		`- (Some(expected), Some(actual)) => {`
`28`		`- if expected == actual {`
`29`		`- return Ok(M::decode(self.value.as_slice())?);`
`30`		`- }`
	`27`	`+ if expected == actual {`
	`28`	`+ return M::decode(self.value.as_slice());`
`31`	`29`	`}`
`32`		`- _ => (),`
`33`	`30`	`}`
`34`	`31`
`35`	`32`	`let mut err = DecodeError::new(format!(`