From f8180c3c429794b87968932166e8c8333f682956 Mon Sep 17 00:00:00 2001 From: Ada Alakbarova Date: Sun, 7 Sep 2025 15:17:25 +0200 Subject: [PATCH 1/2] extract to a separate file --- clippy_lints/src/declared_lints.rs | 2 +- clippy_lints/src/lib.rs | 2 + clippy_lints/src/misc.rs | 118 +------------------------- clippy_lints/src/toplevel_ref_arg.rs | 120 +++++++++++++++++++++++++++ 4 files changed, 126 insertions(+), 116 deletions(-) create mode 100644 clippy_lints/src/toplevel_ref_arg.rs diff --git a/clippy_lints/src/declared_lints.rs b/clippy_lints/src/declared_lints.rs index d0c7443a4a4b..e610e655f158 100644 --- a/clippy_lints/src/declared_lints.rs +++ b/clippy_lints/src/declared_lints.rs @@ -503,7 +503,6 @@ pub static LINTS: &[&::declare_clippy_lint::LintInfo] = &[ crate::min_ident_chars::MIN_IDENT_CHARS_INFO, crate::minmax::MIN_MAX_INFO, crate::misc::SHORT_CIRCUIT_STATEMENT_INFO, - crate::misc::TOPLEVEL_REF_ARG_INFO, crate::misc::USED_UNDERSCORE_BINDING_INFO, crate::misc::USED_UNDERSCORE_ITEMS_INFO, crate::misc_early::BUILTIN_TYPE_SHADOW_INFO, @@ -706,6 +705,7 @@ pub static LINTS: &[&::declare_clippy_lint::LintInfo] = &[ crate::tests_outside_test_module::TESTS_OUTSIDE_TEST_MODULE_INFO, crate::to_digit_is_some::TO_DIGIT_IS_SOME_INFO, crate::to_string_trait_impl::TO_STRING_TRAIT_IMPL_INFO, + crate::toplevel_ref_arg::TOPLEVEL_REF_ARG_INFO, crate::trailing_empty_array::TRAILING_EMPTY_ARRAY_INFO, crate::trait_bounds::TRAIT_DUPLICATION_IN_BOUNDS_INFO, crate::trait_bounds::TYPE_REPETITION_IN_BOUNDS_INFO, diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index a89cf3fdc1ee..7590bec9e1e9 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -360,6 +360,7 @@ mod temporary_assignment; mod tests_outside_test_module; mod to_digit_is_some; mod to_string_trait_impl; +mod toplevel_ref_arg; mod trailing_empty_array; mod trait_bounds; mod transmute; @@ -831,5 +832,6 @@ pub fn register_lint_passes(store: &mut rustc_lint::LintStore, conf: &'static Co store.register_late_pass(|_| Box::new(cloned_ref_to_slice_refs::ClonedRefToSliceRefs::new(conf))); store.register_late_pass(|_| Box::new(infallible_try_from::InfallibleTryFrom)); store.register_late_pass(|_| Box::new(coerce_container_to_any::CoerceContainerToAny)); + store.register_late_pass(|_| Box::new(toplevel_ref_arg::ToplevelRefArg)); // add lints here, do not remove this comment, it's used in `new_lint` } diff --git a/clippy_lints/src/misc.rs b/clippy_lints/src/misc.rs index 09ee6f7037c6..19e9910dfe9d 100644 --- a/clippy_lints/src/misc.rs +++ b/clippy_lints/src/misc.rs @@ -1,57 +1,11 @@ -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::diagnostics::{span_lint_and_then, span_lint_hir_and_then}; use clippy_utils::sugg::Sugg; -use clippy_utils::{ - SpanlessEq, fulfill_or_allowed, get_parent_expr, in_automatically_derived, is_lint_allowed, iter_input_pats, - last_path_segment, -}; +use clippy_utils::{SpanlessEq, fulfill_or_allowed, get_parent_expr, in_automatically_derived, last_path_segment}; use rustc_errors::Applicability; use rustc_hir::def::Res; -use rustc_hir::intravisit::FnKind; -use rustc_hir::{ - BinOpKind, BindingMode, Body, ByRef, Expr, ExprKind, FnDecl, Mutability, PatKind, QPath, Stmt, StmtKind, -}; +use rustc_hir::{BinOpKind, Expr, ExprKind, QPath, Stmt, StmtKind}; use rustc_lint::{LateContext, LateLintPass, LintContext}; use rustc_session::declare_lint_pass; -use rustc_span::Span; -use rustc_span::def_id::LocalDefId; - -use crate::ref_patterns::REF_PATTERNS; - -declare_clippy_lint! { - /// ### What it does - /// Checks for function arguments and let bindings denoted as - /// `ref`. - /// - /// ### Why is this bad? - /// The `ref` declaration makes the function take an owned - /// value, but turns the argument into a reference (which means that the value - /// is destroyed when exiting the function). This adds not much value: either - /// take a reference type, or take an owned value and create references in the - /// body. - /// - /// For let bindings, `let x = &foo;` is preferred over `let ref x = foo`. The - /// type of `x` is more obvious with the former. - /// - /// ### Known problems - /// If the argument is dereferenced within the function, - /// removing the `ref` will lead to errors. This can be fixed by removing the - /// dereferences, e.g., changing `*x` to `x` within the function. - /// - /// ### Example - /// ```no_run - /// fn foo(ref _x: u8) {} - /// ``` - /// - /// Use instead: - /// ```no_run - /// fn foo(_x: &u8) {} - /// ``` - #[clippy::version = "pre 1.29.0"] - pub TOPLEVEL_REF_ARG, - style, - "an entire binding declared as `ref`, in a function argument or a `let` statement" -} declare_clippy_lint! { /// ### What it does @@ -140,79 +94,13 @@ declare_clippy_lint! { } declare_lint_pass!(LintPass => [ - TOPLEVEL_REF_ARG, USED_UNDERSCORE_BINDING, USED_UNDERSCORE_ITEMS, SHORT_CIRCUIT_STATEMENT, ]); impl<'tcx> LateLintPass<'tcx> for LintPass { - fn check_fn( - &mut self, - cx: &LateContext<'tcx>, - k: FnKind<'tcx>, - decl: &'tcx FnDecl<'_>, - body: &'tcx Body<'_>, - _: Span, - _: LocalDefId, - ) { - 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) - && !arg.span.in_external_macro(cx.tcx.sess.source_map()) - { - span_lint_hir( - cx, - TOPLEVEL_REF_ARG, - arg.hir_id, - arg.pat.span, - "`ref` directly on a function parameter does not prevent taking ownership of the passed argument. \ - Consider using a reference type instead", - ); - } - } - } - } - fn check_stmt(&mut self, cx: &LateContext<'tcx>, stmt: &'tcx Stmt<'_>) { - 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) - && !stmt.span.in_external_macro(cx.tcx.sess.source_map()) - { - let ctxt = local.span.ctxt(); - let mut app = Applicability::MachineApplicable; - let sugg_init = Sugg::hir_with_context(cx, init, ctxt, "..", &mut app); - let (mutopt, initref) = if mutabl == Mutability::Mut { - ("mut ", sugg_init.mut_addr()) - } else { - ("", sugg_init.addr()) - }; - let tyopt = if let Some(ty) = local.ty { - let ty_snip = snippet_with_context(cx, ty.span, ctxt, "_", &mut app).0; - format!(": &{mutopt}{ty_snip}") - } else { - String::new() - }; - span_lint_hir_and_then( - cx, - TOPLEVEL_REF_ARG, - init.hir_id, - local.pat.span, - "`ref` on an entire `let` pattern is discouraged, take a reference with `&` instead", - |diag| { - diag.span_suggestion( - stmt.span, - "try", - format!("let {name}{tyopt} = {initref};", name = snippet(cx, name.span, ".."),), - app, - ); - }, - ); - } if let StmtKind::Semi(expr) = stmt.kind && let ExprKind::Binary(binop, a, b) = &expr.kind && matches!(binop.node, BinOpKind::And | BinOpKind::Or) diff --git a/clippy_lints/src/toplevel_ref_arg.rs b/clippy_lints/src/toplevel_ref_arg.rs new file mode 100644 index 000000000000..04da5efce5da --- /dev/null +++ b/clippy_lints/src/toplevel_ref_arg.rs @@ -0,0 +1,120 @@ +use clippy_utils::diagnostics::{span_lint_hir, span_lint_hir_and_then}; +use clippy_utils::source::{snippet, snippet_with_context}; +use clippy_utils::sugg::Sugg; +use clippy_utils::{is_lint_allowed, iter_input_pats}; +use rustc_errors::Applicability; +use rustc_hir::intravisit::FnKind; +use rustc_hir::{BindingMode, Body, ByRef, FnDecl, Mutability, PatKind, Stmt, StmtKind}; +use rustc_lint::{LateContext, LateLintPass}; +use rustc_session::declare_lint_pass; +use rustc_span::Span; +use rustc_span::def_id::LocalDefId; + +use crate::ref_patterns::REF_PATTERNS; + +declare_clippy_lint! { + /// ### What it does + /// Checks for function arguments and let bindings denoted as + /// `ref`. + /// + /// ### Why is this bad? + /// The `ref` declaration makes the function take an owned + /// value, but turns the argument into a reference (which means that the value + /// is destroyed when exiting the function). This adds not much value: either + /// take a reference type, or take an owned value and create references in the + /// body. + /// + /// For let bindings, `let x = &foo;` is preferred over `let ref x = foo`. The + /// type of `x` is more obvious with the former. + /// + /// ### Known problems + /// If the argument is dereferenced within the function, + /// removing the `ref` will lead to errors. This can be fixed by removing the + /// dereferences, e.g., changing `*x` to `x` within the function. + /// + /// ### Example + /// ```no_run + /// fn foo(ref _x: u8) {} + /// ``` + /// + /// Use instead: + /// ```no_run + /// fn foo(_x: &u8) {} + /// ``` + #[clippy::version = "pre 1.29.0"] + pub TOPLEVEL_REF_ARG, + style, + "an entire binding declared as `ref`, in a function argument or a `let` statement" +} + +declare_lint_pass!(ToplevelRefArg => [TOPLEVEL_REF_ARG]); + +impl<'tcx> LateLintPass<'tcx> for ToplevelRefArg { + fn check_fn( + &mut self, + cx: &LateContext<'tcx>, + k: FnKind<'tcx>, + decl: &'tcx FnDecl<'_>, + body: &'tcx Body<'_>, + _: Span, + _: LocalDefId, + ) { + 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) + && !arg.span.in_external_macro(cx.tcx.sess.source_map()) + { + span_lint_hir( + cx, + TOPLEVEL_REF_ARG, + arg.hir_id, + arg.pat.span, + "`ref` directly on a function parameter does not prevent taking ownership of the passed argument. \ + Consider using a reference type instead", + ); + } + } + } + } + + fn check_stmt(&mut self, cx: &LateContext<'tcx>, stmt: &'tcx Stmt<'_>) { + 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) + && !stmt.span.in_external_macro(cx.tcx.sess.source_map()) + { + let ctxt = local.span.ctxt(); + let mut app = Applicability::MachineApplicable; + let sugg_init = Sugg::hir_with_context(cx, init, ctxt, "..", &mut app); + let (mutopt, initref) = if mutabl == Mutability::Mut { + ("mut ", sugg_init.mut_addr()) + } else { + ("", sugg_init.addr()) + }; + let tyopt = if let Some(ty) = local.ty { + let ty_snip = snippet_with_context(cx, ty.span, ctxt, "_", &mut app).0; + format!(": &{mutopt}{ty_snip}") + } else { + String::new() + }; + span_lint_hir_and_then( + cx, + TOPLEVEL_REF_ARG, + init.hir_id, + local.pat.span, + "`ref` on an entire `let` pattern is discouraged, take a reference with `&` instead", + |diag| { + diag.span_suggestion( + stmt.span, + "try", + format!("let {name}{tyopt} = {initref};", name = snippet(cx, name.span, ".."),), + app, + ); + }, + ); + } + } +} From 2c27b586183779d2437ac758055bb21011c6551c Mon Sep 17 00:00:00 2001 From: Ada Alakbarova Date: Sun, 7 Sep 2025 15:26:50 +0200 Subject: [PATCH 2/2] tiny clean-up --- clippy_lints/src/toplevel_ref_arg.rs | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/clippy_lints/src/toplevel_ref_arg.rs b/clippy_lints/src/toplevel_ref_arg.rs index 04da5efce5da..074b79263d37 100644 --- a/clippy_lints/src/toplevel_ref_arg.rs +++ b/clippy_lints/src/toplevel_ref_arg.rs @@ -89,10 +89,9 @@ impl<'tcx> LateLintPass<'tcx> for ToplevelRefArg { let ctxt = local.span.ctxt(); let mut app = Applicability::MachineApplicable; let sugg_init = Sugg::hir_with_context(cx, init, ctxt, "..", &mut app); - let (mutopt, initref) = if mutabl == Mutability::Mut { - ("mut ", sugg_init.mut_addr()) - } else { - ("", sugg_init.addr()) + let (mutopt, initref) = match mutabl { + Mutability::Mut => ("mut ", sugg_init.mut_addr()), + Mutability::Not => ("", sugg_init.addr()), }; let tyopt = if let Some(ty) = local.ty { let ty_snip = snippet_with_context(cx, ty.span, ctxt, "_", &mut app).0;