Auto merge of #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

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 @@ fn check_fn(
         k: FnKind<'tcx>,
         decl: &'tcx FnDecl<'_>,
         body: &'tcx Body<'_>,
-        span: Span,
+        _: Span,
         _: LocalDefId,
     ) {
-        if let FnKind::Closure = k {
+        if !matches!(k, FnKind::Closure) {
-            // Does not apply to closures
+            for arg in iter_input_pats(decl, body) {
-            return;
+                if let PatKind::Binding(BindingMode(ByRef::Yes(_), _), ..) = arg.pat.kind
-        }
+                    && is_lint_allowed(cx, REF_PATTERNS, arg.pat.hir_id)
-        if in_external_macro(cx.tcx.sess, span) {
+                    && !in_external_macro(cx.tcx.sess, arg.span)
-            return;
+                {
-        }
+                    span_lint_hir(
-        for arg in iter_input_pats(decl, body) {
+                        cx,
-            // Do not emit if clippy::ref_patterns is not allowed to avoid having two lints for the same issue.
+                        TOPLEVEL_REF_ARG,
-            if !is_lint_allowed(cx, REF_PATTERNS, arg.pat.hir_id) {
+                        arg.hir_id,
-                return;
+                        arg.pat.span,
-            }
+                        "`ref` directly on a function argument is ignored. \
-            if let PatKind::Binding(BindingMode(ByRef::Yes(_), _), ..) = arg.pat.kind {
+                        Consider using a reference type instead",
-                span_lint(
+                    );
-                    cx,
+                }
-                    TOPLEVEL_REF_ARG,
-                    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)
+        if let StmtKind::Let(local) = stmt.kind
-            && 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 @@
 
 impl<'tcx> LateLintPass<'tcx> for TypeParamMismatch {
     fn check_item(&mut self, cx: &LateContext<'tcx>, item: &'tcx Item<'tcx>) {
-        if !item.span.from_expansion()
+        if let ItemKind::Impl(imp) = &item.kind
-            && 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 @@ fn check_block(&mut self, cx: &LateContext<'tcx>, block: &'tcx hir::Block<'_>) {
     }
 
     fn check_ty(&mut self, cx: &LateContext<'tcx>, ty: &'tcx hir::Ty<'_>) {
-        if in_external_macro(cx.sess(), ty.span) {
+        if let hir::TyKind::Ref(_, mty) = ty.kind
-            return;
+            && mty.mutbl == hir::Mutability::Mut
-        }
+            && let hir::TyKind::Ref(_, mty) = mty.ty.kind
-
+            && mty.mutbl == hir::Mutability::Mut
-        if let hir::TyKind::Ref(
+            && !in_external_macro(cx.sess(), ty.span)
-            _,
-            hir::MutTy {
-                ty: pty,
-                mutbl: hir::Mutability::Mut,
-            },
-        ) = ty.kind
         {
-            if let hir::TyKind::Ref(
+            span_lint(
-                _,
+                cx,
-                hir::MutTy {
+                MUT_MUT,
-                    mutbl: hir::Mutability::Mut,
+                ty.span,
-                    ..
+                "generally you want to avoid `&mut &mut _` if possible",
-                },
+            );
-            ) = pty.kind
-            {
-                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 @@
 
 impl<'tcx> LateLintPass<'tcx> for NeedlessBorrowedRef {
     fn check_pat(&mut self, cx: &LateContext<'tcx>, ref_pat: &'tcx Pat<'_>) {
-        if ref_pat.span.from_expansion() {
+        if let PatKind::Ref(pat, Mutability::Not) = ref_pat.kind
-            // OK, simple enough, lints doesn't check in macro.
+            && !ref_pat.span.from_expansion()
-            return;
+            && cx
-        }
+                .tcx
-
+                .hir()
-        // Do not lint patterns that are part of an OR `|` pattern, the binding mode must match in all arms
+                .parent_iter(ref_pat.hir_id)
-        for (_, node) in cx.tcx.hir().parent_iter(ref_pat.hir_id) {
+                .map_while(|(_, parent)| if let Node::Pat(pat) = parent { Some(pat) } else { None })
-            let Node::Pat(pat) = node else { break };
+                // 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(_)))
-            if matches!(pat.kind, PatKind::Or(_)) {
+        {
-                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),
+                    );
+                },
+                _ => {},
             }
         }
-
-        // 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::{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,16 +55,8 @@
 
 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 {
+        if let StmtKind::Expr(expr) | StmtKind::Semi(expr) = stmt.kind
-            return;
+            && let ExprKind::MethodCall(method_name, for_each_recv, [for_each_arg], _) = expr.kind
-        };
-
-        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.
             && 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.
@@ -72,6 +64,8 @@ fn check_stmt(&mut self, cx: &LateContext<'tcx>, stmt: &'tcx Stmt<'_>) {
                 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 @@
 
 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)
+        if let ExprKind::Unary(UnOp::Not, inner) = expr.kind
-            && 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 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 @@
 
 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 @@
 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,

clippy_lints/src/raw_strings.rs

@@ -1,6 +1,6 @@
 use clippy_config::Conf;
 use clippy_utils::diagnostics::span_lint_and_then;
-use clippy_utils::source::snippet;
+use clippy_utils::source::SpanRangeExt;
 use rustc_ast::ast::{Expr, ExprKind};
 use rustc_ast::token::LitKind;
 use rustc_errors::Applicability;
@@ -71,20 +71,17 @@ pub fn new(conf: &'static Conf) -> Self {
 
 impl EarlyLintPass for RawStrings {
     fn check_expr(&mut self, cx: &EarlyContext<'_>, expr: &Expr) {
-        if !in_external_macro(cx.sess(), expr.span)
+        if let ExprKind::Lit(lit) = expr.kind
-            && let ExprKind::Lit(lit) = expr.kind
+            && let (prefix, max) = match lit.kind {
-            && let LitKind::StrRaw(max) | LitKind::ByteStrRaw(max) | LitKind::CStrRaw(max) = lit.kind
+                LitKind::StrRaw(max) => ("r", max),
+                LitKind::ByteStrRaw(max) => ("br", max),
+                LitKind::CStrRaw(max) => ("cr", max),
+                _ => return,
+            }
+            && !in_external_macro(cx.sess(), expr.span)
+            && expr.span.check_source_text(cx, |src| src.starts_with(prefix))
         {
             let str = lit.symbol.as_str();
-            let prefix = match lit.kind {
-                LitKind::StrRaw(..) => "r",
-                LitKind::ByteStrRaw(..) => "br",
-                LitKind::CStrRaw(..) => "cr",
-                _ => unreachable!(),
-            };
-            if !snippet(cx, expr.span, prefix).trim().starts_with(prefix) {
-                return;
-            }
             let descr = lit.kind.descr();
 
             if !str.contains(['\\', '"']) {
@@ -94,7 +91,7 @@ fn check_expr(&mut self, cx: &EarlyContext<'_>, expr: &Expr) {
                     expr.span,
                     "unnecessary raw string literal",
                     |diag| {
-                        let (start, end) = hash_spans(expr.span, prefix, 0, max);
+                        let (start, end) = hash_spans(expr.span, prefix.len(), 0, max);
 
                         // BytePos: skip over the `b` in `br`, we checked the prefix appears in the source text
                         let r_pos = expr.span.lo() + BytePos::from_usize(prefix.len() - 1);
@@ -156,7 +153,7 @@ fn check_expr(&mut self, cx: &EarlyContext<'_>, expr: &Expr) {
                     expr.span,
                     "unnecessary hashes around raw string literal",
                     |diag| {
-                        let (start, end) = hash_spans(expr.span, prefix, req, max);
+                        let (start, end) = hash_spans(expr.span, prefix.len(), req, max);
 
                         let message = match max - req {
                             _ if req == 0 => format!("remove all the hashes around the {descr} literal"),
@@ -182,11 +179,11 @@ fn check_expr(&mut self, cx: &EarlyContext<'_>, expr: &Expr) {
 /// r###".."###
 ///   ^^    ^^
 /// ```
-fn hash_spans(literal_span: Span, prefix: &str, req: u8, max: u8) -> (Span, Span) {
+fn hash_spans(literal_span: Span, prefix_len: usize, req: u8, max: u8) -> (Span, Span) {
     let literal_span = literal_span.data();
 
     // BytePos: we checked prefix appears literally in the source text
-    let hash_start = literal_span.lo + BytePos::from_usize(prefix.len());
+    let hash_start = literal_span.lo + BytePos::from_usize(prefix_len);
     let hash_end = literal_span.hi;
 
     // BytePos: req/max are counts of the ASCII character #

tests/ui/toplevel_ref_arg.fixed

@@ -36,4 +36,6 @@ fn main() {
 
     // do not lint in external macro
     external!(let ref _y = 42;);
+
+    fn f(#[allow(clippy::toplevel_ref_arg)] ref x: i32) {}
 }

tests/ui/toplevel_ref_arg.rs

@@ -36,4 +36,6 @@ fn main() {
 
     // do not lint in external macro
     external!(let ref _y = 42;);
+
+    fn f(#[allow(clippy::toplevel_ref_arg)] ref x: i32) {}
 }