Auto merge of #3715 - jtgeibel:clippy-fixes, r=Turbo87

Address clippy findings

This addresses warnings identified by clippy. Some of these warnings are currently generated on stable, but a missing environment variable caused those warnings to be logged but not to fail the build. The next to last commit fixes this.

This final commit pins to 1.52.1 for linting, until a compiler ICE in 1.53.0 is addressed. I've just [reported] this ICE against the pre-release, but it may be too late to address in 1.53.0.

r? `@Turbo87`

[reported]: https://internals.rust-lang.org/t/1-53-0-prerelease-testing/14884/2?u=jtgeibel

Author: bors

.github/workflows/ci.yml

@@ -68,6 +68,10 @@ jobs:
     name: Backend (linting)
     runs-on: ubuntu-20.04
 
+    env:
+      CARGO_INCREMENTAL: 0
+      RUSTFLAGS: "-D warnings"
+
     steps:
       - uses: actions/checkout@v2
 
@@ -98,8 +102,9 @@ jobs:
       - name: Install Rust
         run: |
           rustup set profile minimal
-          rustup update stable
-          rustup default stable
+          # Pin to older version until a clippy regression is fixed
+          rustup update 1.52.1
+          rustup default 1.52.1
 
       - run: rustup component add rustfmt
       - run: rustup component add clippy

src/admin/migrate.rs

@@ -1,6 +1,6 @@
 use anyhow::Error;
 
-static CATEGORIES_TOML: &'static str = include_str!("../boot/categories.toml");
+static CATEGORIES_TOML: &str = include_str!("../boot/categories.toml");
 diesel_migrations::embed_migrations!("./migrations");
 
 #[derive(clap::Clap, Debug, Copy, Clone)]

src/app.rs

@@ -141,7 +141,7 @@ impl App {
                     .connection_customizer(Box::new(replica_db_connection_config))
                     .thread_pool(thread_pool);
 
-                Some(DieselPool::new(&url, replica_db_config).unwrap())
+                Some(DieselPool::new(url, replica_db_config).unwrap())
             }
         } else {
             None

src/bin/crates-admin.rs

@@ -29,7 +29,7 @@ enum SubCommand {
 fn main() -> anyhow::Result<()> {
     let opts: Opts = Opts::parse();
 
-    Ok(match opts.command {
+    match opts.command {
         SubCommand::DeleteCrate(opts) => delete_crate::run(opts),
         SubCommand::DeleteVersion(opts) => delete_version::run(opts),
         SubCommand::Populate(opts) => populate::run(opts),
@@ -38,5 +38,7 @@ fn main() -> anyhow::Result<()> {
         SubCommand::TransferCrates(opts) => transfer_crates::run(opts),
         SubCommand::VerifyToken(opts) => verify_token::run(opts).unwrap(),
         SubCommand::Migrate(opts) => migrate::run(opts)?,
-    })
+    }
+
+    Ok(())
 }

src/controllers/crate_owner_invitation.rs

@@ -61,7 +61,7 @@ pub fn list(req: &mut dyn RequestExt) -> EndpointResult {
                 .cloned()
                 .unwrap_or_else(|| String::from("(unknown crate name)"));
 
-            let expires_at = invitation.expires_at(&config);
+            let expires_at = invitation.expires_at(config);
             EncodableCrateOwnerInvitation::from(invitation, inviter_name, crate_name, expires_at)
         })
         .collect();
@@ -101,11 +101,11 @@ pub fn handle_invite(req: &mut dyn RequestExt) -> EndpointResult {
     let conn = &*req.db_conn()?;
     let config = &req.app().config;
 
-    let invitation = CrateOwnerInvitation::find_by_id(user_id, crate_invite.crate_id, &conn)?;
+    let invitation = CrateOwnerInvitation::find_by_id(user_id, crate_invite.crate_id, conn)?;
     if crate_invite.accepted {
-        invitation.accept(&conn, config)?;
+        invitation.accept(conn, config)?;
     } else {
-        invitation.decline(&conn)?;
+        invitation.decline(conn)?;
     }
 
     #[derive(Serialize)]

src/controllers/krate/search.rs

@@ -73,7 +73,7 @@ pub fn search(req: &mut dyn RequestExt) -> EndpointResult {
             query = query.filter(
                 q.clone()
                     .matches(crates::textsearchable_index_col)
-                    .or(Crate::loosly_matches_name(&q_string)),
+                    .or(Crate::loosly_matches_name(q_string)),
             );
 
             query = query.select((

src/controllers/user/session.rs

@@ -103,12 +103,7 @@ pub fn authorize(req: &mut dyn RequestExt) -> EndpointResult {
 
     // Fetch the user info from GitHub using the access token we just got and create a user record
     let ghuser = req.app().github.current_user(token)?;
-    let user = save_user_to_database(
-        &ghuser,
-        &token.secret(),
-        &req.app().emails,
-        &*req.db_conn()?,
-    )?;
+    let user = save_user_to_database(&ghuser, token.secret(), &req.app().emails, &*req.db_conn()?)?;
 
     // Log in by setting a cookie and the middleware authentication
     req.session_mut()

src/controllers/util.rs

@@ -127,7 +127,7 @@ impl<'a> UserAuthenticationExt for dyn RequestExt + 'a {
             };
             if still_locked {
                 return Err(account_locked(
-                    &reason,
+                    reason,
                     authenticated_user.user.account_lock_until,
                 ));
             }

src/controllers/version.rs

@@ -13,7 +13,7 @@ fn version_and_crate(
     semver: &str,
 ) -> AppResult<(Version, Crate)> {
     let krate: Crate = Crate::by_name(crate_name).first(conn)?;
-    let version = krate.find_version(&conn, semver)?;
+    let version = krate.find_version(conn, semver)?;
 
     Ok((version, krate))
 }

src/downloads_counter.rs

@@ -78,7 +78,7 @@ impl DownloadsCounter {
         let mut stats = PersistStats::default();
         for shard in self.inner.shards() {
             let shard = std::mem::take(&mut *shard.write());
-            stats = stats.merge(self.persist_shard(&conn, shard)?);
+            stats = stats.merge(self.persist_shard(conn, shard)?);
         }
 
         Ok(stats)
@@ -92,7 +92,7 @@ impl DownloadsCounter {
         let idx = self.shard_idx.fetch_add(1, Ordering::SeqCst) % shards.len();
         let shard = std::mem::take(&mut *shards[idx].write());
 
-        let mut stats = self.persist_shard(&conn, shard)?;
+        let mut stats = self.persist_shard(conn, shard)?;
         stats.shard = Some(idx);
         Ok(stats)
     }

src/email.rs

@@ -109,7 +109,7 @@ or go to https://{domain}/me/pending-invites to manage all of your crate ownersh
                 login,
                 password,
             } => {
-                SmtpTransport::relay(&server)?
+                SmtpTransport::relay(server)?
                     .credentials(Credentials::new(login.clone(), password.clone()))
                     .authentication(vec![Mechanism::Plain])
                     .build()

src/git.rs

@@ -203,7 +203,7 @@ impl Repository {
         let parent = self.repository.find_commit(head)?;
         let sig = self.repository.signature()?;
         self.repository
-            .commit(Some("HEAD"), &sig, &sig, &msg, &tree, &[&parent])?;
+            .commit(Some("HEAD"), &sig, &sig, msg, &tree, &[&parent])?;
 
         self.push()
     }

src/metrics/log_encoder.rs

@@ -27,6 +27,12 @@ const CHUNKS_MAX_SIZE_BYTES: usize = 5000;
 #[derive(Debug, Clone, Copy)]
 pub struct LogEncoder(());
 
+impl Default for LogEncoder {
+    fn default() -> Self {
+        Self::new()
+    }
+}
+
 impl LogEncoder {
     pub fn new() -> Self {
         Self(())
@@ -132,6 +138,7 @@ fn serialize_and_split_list<'a, S: Serialize + 'a>(
         let mut serializer = Serializer::new(EncoderWriter::new(&mut writer, base64::STANDARD));
 
         let mut seq = serializer.serialize_seq(None)?;
+        #[allow(clippy::while_let_on_iterator)]
         while let Some(next) = items.next() {
             seq.serialize_element(next)?;
             if written_count.get() >= max_size_hint {
@@ -363,7 +370,7 @@ mod tests {
     fn test_encoding() -> Result<(), Error> {
         let gauge = IntGauge::with_opts(Opts::new("sample_gauge", "sample_gauge help message"))?;
         let registry = Registry::new();
-        registry.register(Box::new(gauge.clone()))?;
+        registry.register(Box::new(gauge))?;
 
         let mut output = Vec::new();
         LogEncoder::new().encode(&registry.gather(), &mut output)?;

src/models/krate.rs

@@ -336,7 +336,7 @@ impl Crate {
                 let config = &app.config;
                 match CrateOwnerInvitation::create(user.id, req_user.id, self.id, conn, config)? {
                     NewCrateOwnerInvitationOutcome::InviteCreated { plaintext_token } => {
-                        if let Ok(Some(email)) = user.verified_email(&conn) {
+                        if let Ok(Some(email)) = user.verified_email(conn) {
                             // Swallow any error. Whether or not the email is sent, the invitation
                             // entry will be created in the database and the user will see the
                             // invitation when they visit https://crates.io/me/pending-invites/.

src/models/version.rs

@@ -72,13 +72,13 @@ impl TopVersions {
             .collect();
 
         let newest = pairs.iter().max().map(|(_, v)| v.clone());
-        let highest = pairs.iter().map(|(_, v)| v).max().map(|v| v.clone());
+        let highest = pairs.iter().map(|(_, v)| v).max().cloned();
         let highest_stable = pairs
             .iter()
             .map(|(_, v)| v)
             .filter(|v| v.pre.is_empty())
             .max()
-            .map(|v| v.clone());
+            .cloned();
 
         Self {
             highest,

src/render.rs

@@ -276,7 +276,7 @@ pub fn render_and_upload_readme(
     let rendered = readme_to_html(&text, &file_name, base_url.as_deref());
 
     conn.transaction(|| {
-        Version::record_readme_rendering(version_id, &conn)?;
+        Version::record_readme_rendering(version_id, conn)?;
         let (crate_name, vers): (String, String) = versions::table
             .find(version_id)
             .inner_join(crates::table)

src/s3/lib.rs

@@ -46,7 +46,7 @@ impl Bucket {
         content_type: &str,
         extra_headers: header::HeaderMap,
     ) -> Result<Response, Error> {
-        let path = path.strip_prefix("/").unwrap_or(path);
+        let path = path.strip_prefix('/').unwrap_or(path);
         let date = Utc::now().to_rfc2822();
         let auth = self.auth("PUT", &date, path, "", content_type);
         let url = self.url(path);
@@ -66,7 +66,7 @@ impl Bucket {
     }
 
     pub fn delete(&self, client: &Client, path: &str) -> Result<Response, Error> {
-        let path = path.strip_prefix("/").unwrap_or(path);
+        let path = path.strip_prefix('/').unwrap_or(path);
         let date = Utc::now().to_rfc2822();
         let auth = self.auth("DELETE", &date, path, "", "");
         let url = self.url(path);

src/tasks/update_downloads.rs

@@ -8,7 +8,7 @@ use swirl::PerformError;
 
 #[swirl::background_job]
 pub fn update_downloads(conn: &PgConnection) -> Result<(), PerformError> {
-    update(&conn)?;
+    update(conn)?;
     Ok(())
 }
 

src/tests/category.rs

@@ -75,7 +75,7 @@ fn update_crate() {
         assert_ok!(
             new_category("Category 2", "category-2", "Category 2 crates").create_or_update(conn)
         );
-        let krate = CrateBuilder::new("foo_crate", user.id).expect_build(&conn);
+        let krate = CrateBuilder::new("foo_crate", user.id).expect_build(conn);
 
         // Updating with no categories has no effect
         Category::update_crate(conn, &krate, &[]).unwrap();
@@ -127,7 +127,7 @@ fn update_crate() {
 
         // Add a category and its subcategory
         assert_ok!(new_category("cat1::bar", "cat1::bar", "bar crates").create_or_update(conn));
-        Category::update_crate(&conn, &krate, &["cat1", "cat1::bar"]).unwrap();
+        Category::update_crate(conn, &krate, &["cat1", "cat1::bar"]).unwrap();
 
         assert_eq!(count(&anon, "cat1"), 1);
         assert_eq!(count(&anon, "cat1::bar"), 1);

src/tests/krate/publish.rs

@@ -640,7 +640,7 @@ fn publish_records_an_audit_action() {
 
     let (app, anon, _, token) = TestApp::full().with_token();
 
-    app.db(|conn| assert!(VersionOwnerAction::all(&conn).unwrap().is_empty()));
+    app.db(|conn| assert!(VersionOwnerAction::all(conn).unwrap().is_empty()));
 
     // Upload a new crate, putting it in the git index
     let crate_to_publish = PublishBuilder::new("fyk");

src/tests/krate/yanking.rs

@@ -40,6 +40,7 @@ impl crate::util::MockCookieUser {
 }
 
 #[test]
+#[allow(unknown_lints, clippy::bool_assert_comparison)] // for claim::assert_some_eq! with bool
 fn yank_works_as_intended() {
     let (app, anon, cookie, token) = TestApp::full().with_token();
 

src/tests/owners.rs

@@ -215,7 +215,7 @@ fn modify_multiple_owners() {
         response.json(),
         json!({ "errors": [{ "detail": "cannot remove all individual owners of a crate. Team member don't have permission to modify owners, so at least one individual owner is required." }] })
     );
-    assert_eq!(app.db(|conn| krate.owners(&conn).unwrap()).len(), 3);
+    assert_eq!(app.db(|conn| krate.owners(conn).unwrap()).len(), 3);
 
     // Deleting two owners at once is allowed.
     let response = token.remove_named_owners("owners_multiple", &["user2", "user3"]);
@@ -224,7 +224,7 @@ fn modify_multiple_owners() {
         response.json(),
         json!({ "msg": "owners successfully removed", "ok": true })
     );
-    assert_eq!(app.db(|conn| krate.owners(&conn).unwrap()).len(), 1);
+    assert_eq!(app.db(|conn| krate.owners(conn).unwrap()).len(), 1);
 
     // Adding multiple users fails if one of them already is an owner.
     let response = token.add_named_owners("owners_multiple", &["user2", username]);
@@ -233,7 +233,7 @@ fn modify_multiple_owners() {
         response.json(),
         json!({ "errors": [{ "detail": "`foo` is already an owner" }] })
     );
-    assert_eq!(app.db(|conn| krate.owners(&conn).unwrap()).len(), 1);
+    assert_eq!(app.db(|conn| krate.owners(conn).unwrap()).len(), 1);
 
     // Adding multiple users at once succeeds.
     let response = token.add_named_owners("owners_multiple", &["user2", "user3"]);
@@ -249,7 +249,7 @@ fn modify_multiple_owners() {
     user2.accept_ownership_invitation(&krate.name, krate.id);
     user3.accept_ownership_invitation(&krate.name, krate.id);
 
-    assert_eq!(app.db(|conn| krate.owners(&conn).unwrap()).len(), 3);
+    assert_eq!(app.db(|conn| krate.owners(conn).unwrap()).len(), 3);
 }
 
 #[test]
@@ -449,7 +449,7 @@ fn invitations_list() {
         InvitationListResponse {
             crate_owner_invitations: vec![EncodableCrateOwnerInvitation {
                 crate_id: krate.id,
-                crate_name: krate.name.clone(),
+                crate_name: krate.name,
                 invited_by_username: owner.gh_login.clone(),
                 invitee_id: user.as_model().id,
                 inviter_id: owner.id,
@@ -484,7 +484,7 @@ fn invitations_list_does_not_include_expired_invites() {
         InvitationListResponse {
             crate_owner_invitations: vec![EncodableCrateOwnerInvitation {
                 crate_id: krate2.id,
-                crate_name: krate2.name.clone(),
+                crate_name: krate2.name,
                 invited_by_username: owner.gh_login.clone(),
                 invitee_id: user.as_model().id,
                 inviter_id: owner.id,

src/tests/server_binary.rs

@@ -92,10 +92,7 @@ impl ServerBin {
         env.remove("DB_MIN_SIZE");
         // Other configuration variables needed for the application to boot.
         env.insert("WEB_ALLOWED_ORIGINS".into(), "http://localhost:8888".into());
-        env.insert(
-            "SESSION_KEY".into(),
-            std::iter::repeat('a').take(32).collect(),
-        );
+        env.insert("SESSION_KEY".into(), "a".repeat(32));
         env.insert("GH_CLIENT_ID".into(), String::new());
         env.insert("GH_CLIENT_SECRET".into(), String::new());
 

src/tests/token.rs

@@ -97,11 +97,10 @@ fn list_tokens_exclude_revoked() {
     // Check that we now have one less token being listed.
     let json: ListResponse = user.get(URL).good();
     assert_eq!(json.api_tokens.len(), tokens.len() - 1);
-    assert!(json
+    assert!(!json
         .api_tokens
         .iter()
-        .find(|token| token.name == tokens[0].model.name)
-        .is_none());
+        .any(|token| token.name == tokens[0].model.name));
 }
 
 #[test]
@@ -175,7 +174,7 @@ fn create_token_success() {
         app.db(|conn| assert_ok!(ApiToken::belonging_to(user.as_model()).load(conn)));
     assert_eq!(tokens.len(), 1);
     assert_eq!(tokens[0].name, "bar");
-    assert_eq!(tokens[0].revoked, false);
+    assert!(!tokens[0].revoked);
     assert_eq!(tokens[0].last_used_at, None);
 }