Auto merge of rust-lang#13072 - Jarcho:misc_small5, r=xFrednet

Misc refactorings part 5

`toplevel_ref_arg` gets a small fix so it can be allowed on function arguments. Otherwise just some rearrangements.

changelog: none

Author: bors

clippy_lints/src/misc.rs

@@ -1,4 +1,4 @@
-use clippy_utils::diagnostics::{span_lint, span_lint_and_then, span_lint_hir_and_then};
+use clippy_utils::diagnostics::{span_lint_and_then, span_lint_hir, span_lint_hir_and_then};
 use clippy_utils::source::{snippet, snippet_with_context};
 use clippy_utils::sugg::Sugg;
 use clippy_utils::{
@@ -114,40 +114,35 @@ impl<'tcx> LateLintPass<'tcx> for LintPass {
         k: FnKind<'tcx>,
         decl: &'tcx FnDecl<'_>,
         body: &'tcx Body<'_>,
-        span: Span,
+        _: Span,
         _: LocalDefId,
     ) {
-        if let FnKind::Closure = k {
-            // Does not apply to closures
-            return;
-        }
-        if in_external_macro(cx.tcx.sess, span) {
-            return;
-        }
-        for arg in iter_input_pats(decl, body) {
-            // Do not emit if clippy::ref_patterns is not allowed to avoid having two lints for the same issue.
-            if !is_lint_allowed(cx, REF_PATTERNS, arg.pat.hir_id) {
-                return;
-            }
-            if let PatKind::Binding(BindingMode(ByRef::Yes(_), _), ..) = arg.pat.kind {
-                span_lint(
-                    cx,
-                    TOPLEVEL_REF_ARG,
-                    arg.pat.span,
-                    "`ref` directly on a function argument is ignored. \
-                    Consider using a reference type instead",
-                );
+        if !matches!(k, FnKind::Closure) {
+            for arg in iter_input_pats(decl, body) {
+                if let PatKind::Binding(BindingMode(ByRef::Yes(_), _), ..) = arg.pat.kind
+                    && is_lint_allowed(cx, REF_PATTERNS, arg.pat.hir_id)
+                    && !in_external_macro(cx.tcx.sess, arg.span)
+                {
+                    span_lint_hir(
+                        cx,
+                        TOPLEVEL_REF_ARG,
+                        arg.hir_id,
+                        arg.pat.span,
+                        "`ref` directly on a function argument is ignored. \
+                        Consider using a reference type instead",
+                    );
+                }
             }
         }
     }
 
     fn check_stmt(&mut self, cx: &LateContext<'tcx>, stmt: &'tcx Stmt<'_>) {
-        if !in_external_macro(cx.tcx.sess, stmt.span)
-            && let StmtKind::Let(local) = stmt.kind
+        if let StmtKind::Let(local) = stmt.kind
             && let PatKind::Binding(BindingMode(ByRef::Yes(mutabl), _), .., name, None) = local.pat.kind
             && let Some(init) = local.init
             // Do not emit if clippy::ref_patterns is not allowed to avoid having two lints for the same issue.
             && is_lint_allowed(cx, REF_PATTERNS, local.pat.hir_id)
+            && !in_external_macro(cx.tcx.sess, stmt.span)
         {
             let ctxt = local.span.ctxt();
             let mut app = Applicability::MachineApplicable;

clippy_lints/src/mismatching_type_param_order.rs

@@ -49,12 +49,12 @@ declare_lint_pass!(TypeParamMismatch => [MISMATCHING_TYPE_PARAM_ORDER]);
 
 impl<'tcx> LateLintPass<'tcx> for TypeParamMismatch {
     fn check_item(&mut self, cx: &LateContext<'tcx>, item: &'tcx Item<'tcx>) {
-        if !item.span.from_expansion()
-            && let ItemKind::Impl(imp) = &item.kind
+        if let ItemKind::Impl(imp) = &item.kind
             && let TyKind::Path(QPath::Resolved(_, path)) = &imp.self_ty.kind
-            && let Some(segment) = path.segments.iter().next()
+            && let [segment, ..] = path.segments
             && let Some(generic_args) = segment.args
             && !generic_args.args.is_empty()
+            && !item.span.from_expansion()
         {
             // get the name and span of the generic parameters in the Impl
             let mut impl_params = Vec::new();

clippy_lints/src/mut_mut.rs

@@ -35,33 +35,18 @@ impl<'tcx> LateLintPass<'tcx> for MutMut {
     }
 
     fn check_ty(&mut self, cx: &LateContext<'tcx>, ty: &'tcx hir::Ty<'_>) {
-        if in_external_macro(cx.sess(), ty.span) {
-            return;
-        }
-
-        if let hir::TyKind::Ref(
-            _,
-            hir::MutTy {
-                ty: pty,
-                mutbl: hir::Mutability::Mut,
-            },
-        ) = ty.kind
+        if let hir::TyKind::Ref(_, mty) = ty.kind
+            && mty.mutbl == hir::Mutability::Mut
+            && let hir::TyKind::Ref(_, mty) = mty.ty.kind
+            && mty.mutbl == hir::Mutability::Mut
+            && !in_external_macro(cx.sess(), ty.span)
         {
-            if let hir::TyKind::Ref(
-                _,
-                hir::MutTy {
-                    mutbl: hir::Mutability::Mut,
-                    ..
-                },
-            ) = pty.kind
-            {
-                span_lint(
-                    cx,
-                    MUT_MUT,
-                    ty.span,
-                    "generally you want to avoid `&mut &mut _` if possible",
-                );
-            }
+            span_lint(
+                cx,
+                MUT_MUT,
+                ty.span,
+                "generally you want to avoid `&mut &mut _` if possible",
+            );
         }
     }
 }

clippy_lints/src/needless_borrowed_ref.rs

@@ -37,83 +37,75 @@ declare_lint_pass!(NeedlessBorrowedRef => [NEEDLESS_BORROWED_REFERENCE]);
 
 impl<'tcx> LateLintPass<'tcx> for NeedlessBorrowedRef {
     fn check_pat(&mut self, cx: &LateContext<'tcx>, ref_pat: &'tcx Pat<'_>) {
-        if ref_pat.span.from_expansion() {
-            // OK, simple enough, lints doesn't check in macro.
-            return;
-        }
-
-        // Do not lint patterns that are part of an OR `|` pattern, the binding mode must match in all arms
-        for (_, node) in cx.tcx.hir().parent_iter(ref_pat.hir_id) {
-            let Node::Pat(pat) = node else { break };
-
-            if matches!(pat.kind, PatKind::Or(_)) {
-                return;
+        if let PatKind::Ref(pat, Mutability::Not) = ref_pat.kind
+            && !ref_pat.span.from_expansion()
+            && cx
+                .tcx
+                .hir()
+                .parent_iter(ref_pat.hir_id)
+                .map_while(|(_, parent)| if let Node::Pat(pat) = parent { Some(pat) } else { None })
+                // Do not lint patterns that are part of an OR `|` pattern, the binding mode must match in all arms
+                .all(|pat| !matches!(pat.kind, PatKind::Or(_)))
+        {
+            match pat.kind {
+                // Check sub_pat got a `ref` keyword (excluding `ref mut`).
+                PatKind::Binding(BindingMode::REF, _, ident, None) => {
+                    span_lint_and_then(
+                        cx,
+                        NEEDLESS_BORROWED_REFERENCE,
+                        ref_pat.span,
+                        "this pattern takes a reference on something that is being dereferenced",
+                        |diag| {
+                            // `&ref ident`
+                            //  ^^^^^
+                            let span = ref_pat.span.until(ident.span);
+                            diag.span_suggestion_verbose(
+                                span,
+                                "try removing the `&ref` part",
+                                String::new(),
+                                Applicability::MachineApplicable,
+                            );
+                        },
+                    );
+                },
+                // Slices where each element is `ref`: `&[ref a, ref b, ..., ref z]`
+                PatKind::Slice(
+                    before,
+                    None
+                    | Some(Pat {
+                        kind: PatKind::Wild, ..
+                    }),
+                    after,
+                ) => {
+                    check_subpatterns(
+                        cx,
+                        "dereferencing a slice pattern where every element takes a reference",
+                        ref_pat,
+                        pat,
+                        itertools::chain(before, after),
+                    );
+                },
+                PatKind::Tuple(subpatterns, _) | PatKind::TupleStruct(_, subpatterns, _) => {
+                    check_subpatterns(
+                        cx,
+                        "dereferencing a tuple pattern where every element takes a reference",
+                        ref_pat,
+                        pat,
+                        subpatterns,
+                    );
+                },
+                PatKind::Struct(_, fields, _) => {
+                    check_subpatterns(
+                        cx,
+                        "dereferencing a struct pattern where every field's pattern takes a reference",
+                        ref_pat,
+                        pat,
+                        fields.iter().map(|field| field.pat),
+                    );
+                },
+                _ => {},
             }
         }
-
-        // Only lint immutable refs, because `&mut ref T` may be useful.
-        let PatKind::Ref(pat, Mutability::Not) = ref_pat.kind else {
-            return;
-        };
-
-        match pat.kind {
-            // Check sub_pat got a `ref` keyword (excluding `ref mut`).
-            PatKind::Binding(BindingMode::REF, _, ident, None) => {
-                span_lint_and_then(
-                    cx,
-                    NEEDLESS_BORROWED_REFERENCE,
-                    ref_pat.span,
-                    "this pattern takes a reference on something that is being dereferenced",
-                    |diag| {
-                        // `&ref ident`
-                        //  ^^^^^
-                        let span = ref_pat.span.until(ident.span);
-                        diag.span_suggestion_verbose(
-                            span,
-                            "try removing the `&ref` part",
-                            String::new(),
-                            Applicability::MachineApplicable,
-                        );
-                    },
-                );
-            },
-            // Slices where each element is `ref`: `&[ref a, ref b, ..., ref z]`
-            PatKind::Slice(
-                before,
-                None
-                | Some(Pat {
-                    kind: PatKind::Wild, ..
-                }),
-                after,
-            ) => {
-                check_subpatterns(
-                    cx,
-                    "dereferencing a slice pattern where every element takes a reference",
-                    ref_pat,
-                    pat,
-                    itertools::chain(before, after),
-                );
-            },
-            PatKind::Tuple(subpatterns, _) | PatKind::TupleStruct(_, subpatterns, _) => {
-                check_subpatterns(
-                    cx,
-                    "dereferencing a tuple pattern where every element takes a reference",
-                    ref_pat,
-                    pat,
-                    subpatterns,
-                );
-            },
-            PatKind::Struct(_, fields, _) => {
-                check_subpatterns(
-                    cx,
-                    "dereferencing a struct pattern where every field's pattern takes a reference",
-                    ref_pat,
-                    pat,
-                    fields.iter().map(|field| field.pat),
-                );
-            },
-            _ => {},
-        }
     }
 }
 

clippy_lints/src/needless_for_each.rs

@@ -3,7 +3,7 @@ use rustc_hir::intravisit::{walk_expr, Visitor};
 use rustc_hir::{Block, BlockCheckMode, Closure, Expr, ExprKind, Stmt, StmtKind};
 use rustc_lint::{LateContext, LateLintPass};
 use rustc_session::declare_lint_pass;
-use rustc_span::{sym, Span, Symbol};
+use rustc_span::{sym, Span};
 
 use clippy_utils::diagnostics::span_lint_and_then;
 use clippy_utils::is_trait_method;
@@ -55,23 +55,17 @@ declare_lint_pass!(NeedlessForEach => [NEEDLESS_FOR_EACH]);
 
 impl<'tcx> LateLintPass<'tcx> for NeedlessForEach {
     fn check_stmt(&mut self, cx: &LateContext<'tcx>, stmt: &'tcx Stmt<'_>) {
-        let (StmtKind::Expr(expr) | StmtKind::Semi(expr)) = stmt.kind else {
-            return;
-        };
-
-        if let ExprKind::MethodCall(method_name, for_each_recv, [for_each_arg], _) = expr.kind
-            // Check the method name is `for_each`.
-            && method_name.ident.name == Symbol::intern("for_each")
-            // Check `for_each` is an associated function of `Iterator`.
-            && is_trait_method(cx, expr, sym::Iterator)
-            // Checks the receiver of `for_each` is also a method call.
+        if let StmtKind::Expr(expr) | StmtKind::Semi(expr) = stmt.kind
+            && let ExprKind::MethodCall(method_name, for_each_recv, [for_each_arg], _) = expr.kind
             && let ExprKind::MethodCall(_, iter_recv, [], _) = for_each_recv.kind
             // Skip the lint if the call chain is too long. e.g. `v.field.iter().for_each()` or
             // `v.foo().iter().for_each()` must be skipped.
             && matches!(
                 iter_recv.kind,
                 ExprKind::Array(..) | ExprKind::Call(..) | ExprKind::Path(..)
             )
+            && method_name.ident.name.as_str() == "for_each"
+            && is_trait_method(cx, expr, sym::Iterator)
             // Checks the type of the `iter` method receiver is NOT a user defined type.
             && has_iter_method(cx, cx.typeck_results().expr_ty(iter_recv)).is_some()
             // Skip the lint if the body is not block because this is simpler than `for` loop.

clippy_lints/src/neg_cmp_op_on_partial_ord.rs

@@ -45,10 +45,10 @@ declare_lint_pass!(NoNegCompOpForPartialOrd => [NEG_CMP_OP_ON_PARTIAL_ORD]);
 
 impl<'tcx> LateLintPass<'tcx> for NoNegCompOpForPartialOrd {
     fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) {
-        if !in_external_macro(cx.sess(), expr.span)
-            && let ExprKind::Unary(UnOp::Not, inner) = expr.kind
+        if let ExprKind::Unary(UnOp::Not, inner) = expr.kind
             && let ExprKind::Binary(ref op, left, _) = inner.kind
             && let BinOpKind::Le | BinOpKind::Ge | BinOpKind::Lt | BinOpKind::Gt = op.node
+            && !in_external_macro(cx.sess(), expr.span)
         {
             let ty = cx.typeck_results().expr_ty(left);
 

clippy_lints/src/option_env_unwrap.rs

@@ -3,7 +3,7 @@ use clippy_utils::is_direct_expn_of;
 use rustc_ast::ast::{Expr, ExprKind, MethodCall};
 use rustc_lint::{EarlyContext, EarlyLintPass};
 use rustc_session::declare_lint_pass;
-use rustc_span::{sym, Span};
+use rustc_span::sym;
 
 declare_clippy_lint! {
     /// ### What it does
@@ -35,30 +35,18 @@ declare_lint_pass!(OptionEnvUnwrap => [OPTION_ENV_UNWRAP]);
 
 impl EarlyLintPass for OptionEnvUnwrap {
     fn check_expr(&mut self, cx: &EarlyContext<'_>, expr: &Expr) {
-        fn lint(cx: &EarlyContext<'_>, span: Span) {
+        if let ExprKind::MethodCall(box MethodCall { seg, receiver, .. }) = &expr.kind
+            && matches!(seg.ident.name, sym::expect | sym::unwrap)
+            && is_direct_expn_of(receiver.span, "option_env").is_some()
+        {
             span_lint_and_help(
                 cx,
                 OPTION_ENV_UNWRAP,
-                span,
+                expr.span,
                 "this will panic at run-time if the environment variable doesn't exist at compile-time",
                 None,
                 "consider using the `env!` macro instead",
             );
         }
-
-        if let ExprKind::MethodCall(box MethodCall { seg, receiver, .. }) = &expr.kind
-            && matches!(seg.ident.name, sym::expect | sym::unwrap)
-        {
-            if let ExprKind::Call(caller, _) = &receiver.kind &&
-            // If it exists, it will be ::core::option::Option::Some("<env var>").unwrap() (A method call in the HIR)
-            is_direct_expn_of(caller.span, "option_env").is_some()
-            {
-                lint(cx, expr.span);
-            } else if let ExprKind::Path(_, caller) = &receiver.kind && // If it doesn't exist, it will be ::core::option::Option::None::<&'static str>.unwrap() (A path in the HIR)
-            is_direct_expn_of(caller.span, "option_env").is_some()
-            {
-                lint(cx, expr.span);
-            }
-        }
     }
 }

clippy_lints/src/permissions_set_readonly_false.rs

@@ -31,10 +31,10 @@ declare_lint_pass!(PermissionsSetReadonlyFalse => [PERMISSIONS_SET_READONLY_FALS
 impl<'tcx> LateLintPass<'tcx> for PermissionsSetReadonlyFalse {
     fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'tcx>) {
         if let ExprKind::MethodCall(path, receiver, [arg], _) = &expr.kind
-            && is_type_diagnostic_item(cx, cx.typeck_results().expr_ty(receiver), sym::FsPermissions)
-            && path.ident.name == sym!(set_readonly)
             && let ExprKind::Lit(lit) = &arg.kind
             && LitKind::Bool(false) == lit.node
+            && path.ident.name.as_str() == "set_readonly"
+            && is_type_diagnostic_item(cx, cx.typeck_results().expr_ty(receiver), sym::FsPermissions)
         {
             span_lint_and_then(
                 cx,