Merge #10440

10440: Fix Clippy warnings and replace some `if let`s with `match` r=Veykril a=arzg

I decided to try fixing a bunch of Clippy warnings. I am aware of this project’s opinion of Clippy (I have read both [rust-lang/clippy#5537](rust-lang/rust-clippy#5537) and [rust-analyzer/rowan#57 (comment)](rust-analyzer/rowan#57 (comment))), so I totally understand if part of or the entirety of this PR is rejected. In particular, I can see how the semicolons and `if let` vs `match` commits provide comparatively little benefit when compared to the ensuing churn.

I tried to separate each kind of change into its own commit to make it easier to discard certain changes. I also only applied Clippy suggestions where I thought they provided a definite improvement to the code (apart from semicolons, which is IMO more of a formatting/consistency question than a linting question). In the end I accumulated a list of 28 Clippy lints I ignored entirely.

Sidenote: I should really have asked about this on Zulip before going through all 1,555 `if let`s in the codebase to decide which ones definitely look better as `match` :P

Co-authored-by: Aramis Razzaghipour <[email protected]>

crates/flycheck/src/lib.rs

@@ -179,7 +179,7 @@ impl FlycheckActor {
                         tracing::error!(
                             "Flycheck failed to run the following command: {:?}",
                             self.check_command()
-                        )
+                        );
                     }
                     self.progress(Progress::DidFinish(res));
                 }
@@ -253,14 +253,14 @@ impl FlycheckActor {
     }
 
     fn send(&self, check_task: Message) {
-        (self.sender)(check_task)
+        (self.sender)(check_task);
     }
 }
 
 struct CargoHandle {
     child: JodChild,
     #[allow(unused)]
-    thread: jod_thread::JoinHandle<io::Result<bool>>,
+    thread: jod_thread::JoinHandle<bool>,
     receiver: Receiver<CargoMessage>,
 }
 
@@ -279,7 +279,7 @@ impl CargoHandle {
         // It is okay to ignore the result, as it only errors if the process is already dead
         let _ = self.child.kill();
         let exit_status = self.child.wait()?;
-        let read_at_least_one_message = self.thread.join()?;
+        let read_at_least_one_message = self.thread.join();
         if !exit_status.success() && !read_at_least_one_message {
             // FIXME: Read the stderr to display the reason, see `read2()` reference in PR comment:
             // https://github.com/rust-analyzer/rust-analyzer/pull/3632#discussion_r395605298
@@ -304,7 +304,7 @@ impl CargoActor {
     fn new(child_stdout: process::ChildStdout, sender: Sender<CargoMessage>) -> CargoActor {
         CargoActor { child_stdout, sender }
     }
-    fn run(self) -> io::Result<bool> {
+    fn run(self) -> bool {
         // We manually read a line at a time, instead of using serde's
         // stream deserializers, because the deserializer cannot recover
         // from an error, resulting in it getting stuck, because we try to
@@ -334,20 +334,20 @@ impl CargoActor {
                     // Skip certain kinds of messages to only spend time on what's useful
                     JsonMessage::Cargo(message) => match message {
                         cargo_metadata::Message::CompilerArtifact(artifact) if !artifact.fresh => {
-                            self.sender.send(CargoMessage::CompilerArtifact(artifact)).unwrap()
+                            self.sender.send(CargoMessage::CompilerArtifact(artifact)).unwrap();
                         }
                         cargo_metadata::Message::CompilerMessage(msg) => {
-                            self.sender.send(CargoMessage::Diagnostic(msg.message)).unwrap()
+                            self.sender.send(CargoMessage::Diagnostic(msg.message)).unwrap();
                         }
                         _ => (),
                     },
                     JsonMessage::Rustc(message) => {
-                        self.sender.send(CargoMessage::Diagnostic(message)).unwrap()
+                        self.sender.send(CargoMessage::Diagnostic(message)).unwrap();
                     }
                 }
             }
         }
-        Ok(read_at_least_one_message)
+        read_at_least_one_message
     }
 }
 

crates/hir/src/lib.rs

@@ -1119,7 +1119,7 @@ impl DefWithBody {
                                 if let ast::Expr::RecordExpr(record_expr) =
                                     &source_ptr.value.to_node(&root)
                                 {
-                                    if let Some(_) = record_expr.record_expr_field_list() {
+                                    if record_expr.record_expr_field_list().is_some() {
                                         acc.push(
                                             MissingFields {
                                                 file: source_ptr.file_id,
@@ -1143,7 +1143,7 @@ impl DefWithBody {
                                 if let Some(expr) = source_ptr.value.as_ref().left() {
                                     let root = source_ptr.file_syntax(db.upcast());
                                     if let ast::Pat::RecordPat(record_pat) = expr.to_node(&root) {
-                                        if let Some(_) = record_pat.record_pat_field_list() {
+                                        if record_pat.record_pat_field_list().is_some() {
                                             acc.push(
                                                 MissingFields {
                                                     file: source_ptr.file_id,
@@ -2119,10 +2119,9 @@ impl Impl {
         };
 
         let fp = TyFingerprint::for_inherent_impl(&ty);
-        let fp = if let Some(fp) = fp {
-            fp
-        } else {
-            return Vec::new();
+        let fp = match fp {
+            Some(fp) => fp,
+            None => return Vec::new(),
         };
 
         let mut all = Vec::new();

crates/hir_def/src/body/lower.rs

@@ -474,10 +474,9 @@ impl ExprCollector<'_> {
             }
             ast::Expr::PrefixExpr(e) => {
                 let expr = self.collect_expr_opt(e.expr());
-                if let Some(op) = e.op_kind() {
-                    self.alloc_expr(Expr::UnaryOp { expr, op }, syntax_ptr)
-                } else {
-                    self.alloc_expr(Expr::Missing, syntax_ptr)
+                match e.op_kind() {
+                    Some(op) => self.alloc_expr(Expr::UnaryOp { expr, op }, syntax_ptr),
+                    None => self.alloc_expr(Expr::Missing, syntax_ptr),
                 }
             }
             ast::Expr::ClosureExpr(e) => {
@@ -624,10 +623,9 @@ impl ExprCollector<'_> {
     }
 
     fn collect_expr_opt(&mut self, expr: Option<ast::Expr>) -> ExprId {
-        if let Some(expr) = expr {
-            self.collect_expr(expr)
-        } else {
-            self.missing_expr()
+        match expr {
+            Some(expr) => self.collect_expr(expr),
+            None => self.missing_expr(),
         }
     }
 
@@ -724,10 +722,9 @@ impl ExprCollector<'_> {
     }
 
     fn collect_block_opt(&mut self, expr: Option<ast::BlockExpr>) -> ExprId {
-        if let Some(block) = expr {
-            self.collect_block(block)
-        } else {
-            self.missing_expr()
+        match expr {
+            Some(block) => self.collect_block(block),
+            None => self.missing_expr(),
         }
     }
 
@@ -890,10 +887,9 @@ impl ExprCollector<'_> {
     }
 
     fn collect_pat_opt(&mut self, pat: Option<ast::Pat>) -> PatId {
-        if let Some(pat) = pat {
-            self.collect_pat(pat)
-        } else {
-            self.missing_pat()
+        match pat {
+            Some(pat) => self.collect_pat(pat),
+            None => self.missing_pat(),
         }
     }
 

crates/hir_def/src/find_path.rs

@@ -209,10 +209,9 @@ fn find_path_inner(
             ) {
                 path.push_segment(name);
 
-                let new_path = if let Some(best_path) = best_path {
-                    select_best_path(best_path, path, prefer_no_std)
-                } else {
-                    path
+                let new_path = match best_path {
+                    Some(best_path) => select_best_path(best_path, path, prefer_no_std),
+                    None => path,
                 };
                 best_path_len = new_path.len();
                 best_path = Some(new_path);
@@ -243,10 +242,9 @@ fn find_path_inner(
         });
 
         for path in extern_paths {
-            let new_path = if let Some(best_path) = best_path {
-                select_best_path(best_path, path, prefer_no_std)
-            } else {
-                path
+            let new_path = match best_path {
+                Some(best_path) => select_best_path(best_path, path, prefer_no_std),
+                None => path,
             };
             best_path = Some(new_path);
         }
@@ -261,12 +259,11 @@ fn find_path_inner(
         }
     }
 
-    if let Some(prefix) = prefixed.map(PrefixKind::prefix) {
-        best_path.or_else(|| {
+    match prefixed.map(PrefixKind::prefix) {
+        Some(prefix) => best_path.or_else(|| {
             scope_name.map(|scope_name| ModPath::from_segments(prefix, vec![scope_name]))
-        })
-    } else {
-        best_path
+        }),
+        None => best_path,
     }
 }
 
@@ -346,15 +343,13 @@ fn find_local_import_locations(
 
         if let Some((name, vis)) = data.scope.name_of(item) {
             if vis.is_visible_from(db, from) {
-                let is_private = if let Visibility::Module(private_to) = vis {
-                    private_to.local_id == module.local_id
-                } else {
-                    false
+                let is_private = match vis {
+                    Visibility::Module(private_to) => private_to.local_id == module.local_id,
+                    Visibility::Public => false,
                 };
-                let is_original_def = if let Some(module_def_id) = item.as_module_def_id() {
-                    data.scope.declarations().any(|it| it == module_def_id)
-                } else {
-                    false
+                let is_original_def = match item.as_module_def_id() {
+                    Some(module_def_id) => data.scope.declarations().any(|it| it == module_def_id),
+                    None => false,
                 };
 
                 // Ignore private imports. these could be used if we are

crates/hir_def/src/item_tree.rs

@@ -475,10 +475,9 @@ macro_rules! mod_items {
                 }
 
                 fn id_from_mod_item(mod_item: ModItem) -> Option<FileItemTreeId<Self>> {
-                    if let ModItem::$typ(id) = mod_item {
-                        Some(id)
-                    } else {
-                        None
+                    match mod_item {
+                        ModItem::$typ(id) => Some(id),
+                        _ => None,
                     }
                 }
 

crates/hir_def/src/nameres/path_resolution.rs

@@ -400,13 +400,10 @@ impl DefMap {
         };
         let from_scope_or_builtin = match shadow {
             BuiltinShadowMode::Module => from_scope.or(from_builtin),
-            BuiltinShadowMode::Other => {
-                if let Some(ModuleDefId::ModuleId(_)) = from_scope.take_types() {
-                    from_builtin.or(from_scope)
-                } else {
-                    from_scope.or(from_builtin)
-                }
-            }
+            BuiltinShadowMode::Other => match from_scope.take_types() {
+                Some(ModuleDefId::ModuleId(_)) => from_builtin.or(from_scope),
+                Some(_) | None => from_scope.or(from_builtin),
+            },
         };
         let from_extern_prelude = self
             .extern_prelude

crates/hir_def/src/path/lower/lower_use.rs

@@ -18,10 +18,9 @@ pub(crate) fn convert_path(
     path: ast::Path,
     hygiene: &Hygiene,
 ) -> Option<ModPath> {
-    let prefix = if let Some(qual) = path.qualifier() {
-        Some(convert_path(db, prefix, qual, hygiene)?)
-    } else {
-        prefix
+    let prefix = match path.qualifier() {
+        Some(qual) => Some(convert_path(db, prefix, qual, hygiene)?),
+        None => prefix,
     };
 
     let segment = path.segment()?;

crates/hir_def/src/type_ref.rs

@@ -214,10 +214,9 @@ impl TypeRef {
     }
 
     pub(crate) fn from_ast_opt(ctx: &LowerCtx, node: Option<ast::Type>) -> Self {
-        if let Some(node) = node {
-            TypeRef::from_ast(ctx, node)
-        } else {
-            TypeRef::Error
+        match node {
+            Some(node) => TypeRef::from_ast(ctx, node),
+            None => TypeRef::Error,
         }
     }
 

crates/hir_expand/src/name.rs

@@ -48,10 +48,9 @@ impl Name {
 
     /// Resolve a name from the text of token.
     fn resolve(raw_text: &str) -> Name {
-        if let Some(text) = raw_text.strip_prefix("r#") {
-            Name::new_text(SmolStr::new(text))
-        } else {
-            Name::new_text(raw_text.into())
+        match raw_text.strip_prefix("r#") {
+            Some(text) => Name::new_text(SmolStr::new(text)),
+            None => Name::new_text(raw_text.into()),
         }
     }
 

crates/hir_ty/src/autoderef.rs

@@ -109,10 +109,9 @@ pub(crate) fn deref(
     ty: InEnvironment<&Canonical<Ty>>,
 ) -> Option<Canonical<Ty>> {
     let _p = profile::span("deref");
-    if let Some(derefed) = builtin_deref(&ty.goal.value) {
-        Some(Canonical { value: derefed, binders: ty.goal.binders.clone() })
-    } else {
-        deref_by_trait(db, krate, ty)
+    match builtin_deref(&ty.goal.value) {
+        Some(derefed) => Some(Canonical { value: derefed, binders: ty.goal.binders.clone() }),
+        None => deref_by_trait(db, krate, ty),
     }
 }
 

crates/hir_ty/src/chalk_ext.rs

@@ -104,10 +104,9 @@ impl TyExt for Ty {
     }
 
     fn as_fn_def(&self, db: &dyn HirDatabase) -> Option<FunctionId> {
-        if let Some(CallableDefId::FunctionId(func)) = self.callable_def(db) {
-            Some(func)
-        } else {
-            None
+        match self.callable_def(db) {
+            Some(CallableDefId::FunctionId(func)) => Some(func),
+            Some(CallableDefId::StructId(_) | CallableDefId::EnumVariantId(_)) | None => None,
         }
     }
     fn as_reference(&self) -> Option<(&Ty, Lifetime, Mutability)> {

crates/hir_ty/src/diagnostics/match_check/deconstruct_pat.rs

@@ -105,10 +105,9 @@ impl IntRange {
 
     #[inline]
     fn from_range(lo: u128, hi: u128, scalar_ty: Scalar) -> IntRange {
-        if let Scalar::Bool = scalar_ty {
-            IntRange { range: lo..=hi }
-        } else {
-            unimplemented!()
+        match scalar_ty {
+            Scalar::Bool => IntRange { range: lo..=hi },
+            _ => unimplemented!(),
         }
     }
 

crates/hir_ty/src/display.rs

@@ -167,10 +167,9 @@ impl<'a> HirFormatter<'a> {
     }
 
     pub fn should_truncate(&self) -> bool {
-        if let Some(max_size) = self.max_size {
-            self.curr_size >= max_size
-        } else {
-            false
+        match self.max_size {
+            Some(max_size) => self.curr_size >= max_size,
+            None => false,
         }
     }
 

crates/hir_ty/src/infer/expr.rs

@@ -264,10 +264,9 @@ impl<'a> InferenceContext<'a> {
 
                 // collect explicitly written argument types
                 for arg_type in arg_types.iter() {
-                    let arg_ty = if let Some(type_ref) = arg_type {
-                        self.make_ty(type_ref)
-                    } else {
-                        self.table.new_type_var()
+                    let arg_ty = match arg_type {
+                        Some(type_ref) => self.make_ty(type_ref),
+                        None => self.table.new_type_var(),
                     };
                     sig_tys.push(arg_ty);
                 }

crates/hir_ty/src/infer/pat.rs

@@ -204,10 +204,9 @@ impl<'a> InferenceContext<'a> {
                 } else {
                     BindingMode::convert(*mode)
                 };
-                let inner_ty = if let Some(subpat) = subpat {
-                    self.infer_pat(*subpat, &expected, default_bm)
-                } else {
-                    expected
+                let inner_ty = match subpat {
+                    Some(subpat) => self.infer_pat(*subpat, &expected, default_bm),
+                    None => expected,
                 };
                 let inner_ty = self.insert_type_vars_shallow(inner_ty);
 

crates/hir_ty/src/infer/unify.rs

@@ -324,10 +324,9 @@ impl<'a> InferenceTable<'a> {
 
     /// Unify two types and register new trait goals that arise from that.
     pub(crate) fn unify(&mut self, ty1: &Ty, ty2: &Ty) -> bool {
-        let result = if let Ok(r) = self.try_unify(ty1, ty2) {
-            r
-        } else {
-            return false;
+        let result = match self.try_unify(ty1, ty2) {
+            Ok(r) => r,
+            Err(_) => return false,
         };
         self.register_infer_ok(result);
         true