Skip to content

Commit 368b235

Browse files
move toplevel_ref_args out of misc (#15627)
For #6680 I wanted to move the lint into `functions` because it has a `check_fn` part, but it also has a `check_stmt`, which I wouldn't know what to do with. changelog: none
2 parents 847ca73 + 2c27b58 commit 368b235

File tree

4 files changed

+125
-116
lines changed

4 files changed

+125
-116
lines changed

clippy_lints/src/declared_lints.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -503,7 +503,6 @@ pub static LINTS: &[&::declare_clippy_lint::LintInfo] = &[
503503
crate::min_ident_chars::MIN_IDENT_CHARS_INFO,
504504
crate::minmax::MIN_MAX_INFO,
505505
crate::misc::SHORT_CIRCUIT_STATEMENT_INFO,
506-
crate::misc::TOPLEVEL_REF_ARG_INFO,
507506
crate::misc::USED_UNDERSCORE_BINDING_INFO,
508507
crate::misc::USED_UNDERSCORE_ITEMS_INFO,
509508
crate::misc_early::BUILTIN_TYPE_SHADOW_INFO,
@@ -706,6 +705,7 @@ pub static LINTS: &[&::declare_clippy_lint::LintInfo] = &[
706705
crate::tests_outside_test_module::TESTS_OUTSIDE_TEST_MODULE_INFO,
707706
crate::to_digit_is_some::TO_DIGIT_IS_SOME_INFO,
708707
crate::to_string_trait_impl::TO_STRING_TRAIT_IMPL_INFO,
708+
crate::toplevel_ref_arg::TOPLEVEL_REF_ARG_INFO,
709709
crate::trailing_empty_array::TRAILING_EMPTY_ARRAY_INFO,
710710
crate::trait_bounds::TRAIT_DUPLICATION_IN_BOUNDS_INFO,
711711
crate::trait_bounds::TYPE_REPETITION_IN_BOUNDS_INFO,

clippy_lints/src/lib.rs

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -360,6 +360,7 @@ mod temporary_assignment;
360360
mod tests_outside_test_module;
361361
mod to_digit_is_some;
362362
mod to_string_trait_impl;
363+
mod toplevel_ref_arg;
363364
mod trailing_empty_array;
364365
mod trait_bounds;
365366
mod transmute;
@@ -831,5 +832,6 @@ pub fn register_lint_passes(store: &mut rustc_lint::LintStore, conf: &'static Co
831832
store.register_late_pass(|_| Box::new(cloned_ref_to_slice_refs::ClonedRefToSliceRefs::new(conf)));
832833
store.register_late_pass(|_| Box::new(infallible_try_from::InfallibleTryFrom));
833834
store.register_late_pass(|_| Box::new(coerce_container_to_any::CoerceContainerToAny));
835+
store.register_late_pass(|_| Box::new(toplevel_ref_arg::ToplevelRefArg));
834836
// add lints here, do not remove this comment, it's used in `new_lint`
835837
}

clippy_lints/src/misc.rs

Lines changed: 3 additions & 115 deletions
Original file line numberDiff line numberDiff line change
@@ -1,57 +1,11 @@
1-
use clippy_utils::diagnostics::{span_lint_and_then, span_lint_hir, span_lint_hir_and_then};
2-
use clippy_utils::source::{snippet, snippet_with_context};
1+
use clippy_utils::diagnostics::{span_lint_and_then, span_lint_hir_and_then};
32
use clippy_utils::sugg::Sugg;
4-
use clippy_utils::{
5-
SpanlessEq, fulfill_or_allowed, get_parent_expr, in_automatically_derived, is_lint_allowed, iter_input_pats,
6-
last_path_segment,
7-
};
3+
use clippy_utils::{SpanlessEq, fulfill_or_allowed, get_parent_expr, in_automatically_derived, last_path_segment};
84
use rustc_errors::Applicability;
95
use rustc_hir::def::Res;
10-
use rustc_hir::intravisit::FnKind;
11-
use rustc_hir::{
12-
BinOpKind, BindingMode, Body, ByRef, Expr, ExprKind, FnDecl, Mutability, PatKind, QPath, Stmt, StmtKind,
13-
};
6+
use rustc_hir::{BinOpKind, Expr, ExprKind, QPath, Stmt, StmtKind};
147
use rustc_lint::{LateContext, LateLintPass, LintContext};
158
use rustc_session::declare_lint_pass;
16-
use rustc_span::Span;
17-
use rustc_span::def_id::LocalDefId;
18-
19-
use crate::ref_patterns::REF_PATTERNS;
20-
21-
declare_clippy_lint! {
22-
/// ### What it does
23-
/// Checks for function arguments and let bindings denoted as
24-
/// `ref`.
25-
///
26-
/// ### Why is this bad?
27-
/// The `ref` declaration makes the function take an owned
28-
/// value, but turns the argument into a reference (which means that the value
29-
/// is destroyed when exiting the function). This adds not much value: either
30-
/// take a reference type, or take an owned value and create references in the
31-
/// body.
32-
///
33-
/// For let bindings, `let x = &foo;` is preferred over `let ref x = foo`. The
34-
/// type of `x` is more obvious with the former.
35-
///
36-
/// ### Known problems
37-
/// If the argument is dereferenced within the function,
38-
/// removing the `ref` will lead to errors. This can be fixed by removing the
39-
/// dereferences, e.g., changing `*x` to `x` within the function.
40-
///
41-
/// ### Example
42-
/// ```no_run
43-
/// fn foo(ref _x: u8) {}
44-
/// ```
45-
///
46-
/// Use instead:
47-
/// ```no_run
48-
/// fn foo(_x: &u8) {}
49-
/// ```
50-
#[clippy::version = "pre 1.29.0"]
51-
pub TOPLEVEL_REF_ARG,
52-
style,
53-
"an entire binding declared as `ref`, in a function argument or a `let` statement"
54-
}
559

5610
declare_clippy_lint! {
5711
/// ### What it does
@@ -140,79 +94,13 @@ declare_clippy_lint! {
14094
}
14195

14296
declare_lint_pass!(LintPass => [
143-
TOPLEVEL_REF_ARG,
14497
USED_UNDERSCORE_BINDING,
14598
USED_UNDERSCORE_ITEMS,
14699
SHORT_CIRCUIT_STATEMENT,
147100
]);
148101

149102
impl<'tcx> LateLintPass<'tcx> for LintPass {
150-
fn check_fn(
151-
&mut self,
152-
cx: &LateContext<'tcx>,
153-
k: FnKind<'tcx>,
154-
decl: &'tcx FnDecl<'_>,
155-
body: &'tcx Body<'_>,
156-
_: Span,
157-
_: LocalDefId,
158-
) {
159-
if !matches!(k, FnKind::Closure) {
160-
for arg in iter_input_pats(decl, body) {
161-
if let PatKind::Binding(BindingMode(ByRef::Yes(_), _), ..) = arg.pat.kind
162-
&& is_lint_allowed(cx, REF_PATTERNS, arg.pat.hir_id)
163-
&& !arg.span.in_external_macro(cx.tcx.sess.source_map())
164-
{
165-
span_lint_hir(
166-
cx,
167-
TOPLEVEL_REF_ARG,
168-
arg.hir_id,
169-
arg.pat.span,
170-
"`ref` directly on a function parameter does not prevent taking ownership of the passed argument. \
171-
Consider using a reference type instead",
172-
);
173-
}
174-
}
175-
}
176-
}
177-
178103
fn check_stmt(&mut self, cx: &LateContext<'tcx>, stmt: &'tcx Stmt<'_>) {
179-
if let StmtKind::Let(local) = stmt.kind
180-
&& let PatKind::Binding(BindingMode(ByRef::Yes(mutabl), _), .., name, None) = local.pat.kind
181-
&& let Some(init) = local.init
182-
// Do not emit if clippy::ref_patterns is not allowed to avoid having two lints for the same issue.
183-
&& is_lint_allowed(cx, REF_PATTERNS, local.pat.hir_id)
184-
&& !stmt.span.in_external_macro(cx.tcx.sess.source_map())
185-
{
186-
let ctxt = local.span.ctxt();
187-
let mut app = Applicability::MachineApplicable;
188-
let sugg_init = Sugg::hir_with_context(cx, init, ctxt, "..", &mut app);
189-
let (mutopt, initref) = if mutabl == Mutability::Mut {
190-
("mut ", sugg_init.mut_addr())
191-
} else {
192-
("", sugg_init.addr())
193-
};
194-
let tyopt = if let Some(ty) = local.ty {
195-
let ty_snip = snippet_with_context(cx, ty.span, ctxt, "_", &mut app).0;
196-
format!(": &{mutopt}{ty_snip}")
197-
} else {
198-
String::new()
199-
};
200-
span_lint_hir_and_then(
201-
cx,
202-
TOPLEVEL_REF_ARG,
203-
init.hir_id,
204-
local.pat.span,
205-
"`ref` on an entire `let` pattern is discouraged, take a reference with `&` instead",
206-
|diag| {
207-
diag.span_suggestion(
208-
stmt.span,
209-
"try",
210-
format!("let {name}{tyopt} = {initref};", name = snippet(cx, name.span, ".."),),
211-
app,
212-
);
213-
},
214-
);
215-
}
216104
if let StmtKind::Semi(expr) = stmt.kind
217105
&& let ExprKind::Binary(binop, a, b) = &expr.kind
218106
&& matches!(binop.node, BinOpKind::And | BinOpKind::Or)
Lines changed: 119 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,119 @@
1+
use clippy_utils::diagnostics::{span_lint_hir, span_lint_hir_and_then};
2+
use clippy_utils::source::{snippet, snippet_with_context};
3+
use clippy_utils::sugg::Sugg;
4+
use clippy_utils::{is_lint_allowed, iter_input_pats};
5+
use rustc_errors::Applicability;
6+
use rustc_hir::intravisit::FnKind;
7+
use rustc_hir::{BindingMode, Body, ByRef, FnDecl, Mutability, PatKind, Stmt, StmtKind};
8+
use rustc_lint::{LateContext, LateLintPass};
9+
use rustc_session::declare_lint_pass;
10+
use rustc_span::Span;
11+
use rustc_span::def_id::LocalDefId;
12+
13+
use crate::ref_patterns::REF_PATTERNS;
14+
15+
declare_clippy_lint! {
16+
/// ### What it does
17+
/// Checks for function arguments and let bindings denoted as
18+
/// `ref`.
19+
///
20+
/// ### Why is this bad?
21+
/// The `ref` declaration makes the function take an owned
22+
/// value, but turns the argument into a reference (which means that the value
23+
/// is destroyed when exiting the function). This adds not much value: either
24+
/// take a reference type, or take an owned value and create references in the
25+
/// body.
26+
///
27+
/// For let bindings, `let x = &foo;` is preferred over `let ref x = foo`. The
28+
/// type of `x` is more obvious with the former.
29+
///
30+
/// ### Known problems
31+
/// If the argument is dereferenced within the function,
32+
/// removing the `ref` will lead to errors. This can be fixed by removing the
33+
/// dereferences, e.g., changing `*x` to `x` within the function.
34+
///
35+
/// ### Example
36+
/// ```no_run
37+
/// fn foo(ref _x: u8) {}
38+
/// ```
39+
///
40+
/// Use instead:
41+
/// ```no_run
42+
/// fn foo(_x: &u8) {}
43+
/// ```
44+
#[clippy::version = "pre 1.29.0"]
45+
pub TOPLEVEL_REF_ARG,
46+
style,
47+
"an entire binding declared as `ref`, in a function argument or a `let` statement"
48+
}
49+
50+
declare_lint_pass!(ToplevelRefArg => [TOPLEVEL_REF_ARG]);
51+
52+
impl<'tcx> LateLintPass<'tcx> for ToplevelRefArg {
53+
fn check_fn(
54+
&mut self,
55+
cx: &LateContext<'tcx>,
56+
k: FnKind<'tcx>,
57+
decl: &'tcx FnDecl<'_>,
58+
body: &'tcx Body<'_>,
59+
_: Span,
60+
_: LocalDefId,
61+
) {
62+
if !matches!(k, FnKind::Closure) {
63+
for arg in iter_input_pats(decl, body) {
64+
if let PatKind::Binding(BindingMode(ByRef::Yes(_), _), ..) = arg.pat.kind
65+
&& is_lint_allowed(cx, REF_PATTERNS, arg.pat.hir_id)
66+
&& !arg.span.in_external_macro(cx.tcx.sess.source_map())
67+
{
68+
span_lint_hir(
69+
cx,
70+
TOPLEVEL_REF_ARG,
71+
arg.hir_id,
72+
arg.pat.span,
73+
"`ref` directly on a function parameter does not prevent taking ownership of the passed argument. \
74+
Consider using a reference type instead",
75+
);
76+
}
77+
}
78+
}
79+
}
80+
81+
fn check_stmt(&mut self, cx: &LateContext<'tcx>, stmt: &'tcx Stmt<'_>) {
82+
if let StmtKind::Let(local) = stmt.kind
83+
&& let PatKind::Binding(BindingMode(ByRef::Yes(mutabl), _), .., name, None) = local.pat.kind
84+
&& let Some(init) = local.init
85+
// Do not emit if clippy::ref_patterns is not allowed to avoid having two lints for the same issue.
86+
&& is_lint_allowed(cx, REF_PATTERNS, local.pat.hir_id)
87+
&& !stmt.span.in_external_macro(cx.tcx.sess.source_map())
88+
{
89+
let ctxt = local.span.ctxt();
90+
let mut app = Applicability::MachineApplicable;
91+
let sugg_init = Sugg::hir_with_context(cx, init, ctxt, "..", &mut app);
92+
let (mutopt, initref) = match mutabl {
93+
Mutability::Mut => ("mut ", sugg_init.mut_addr()),
94+
Mutability::Not => ("", sugg_init.addr()),
95+
};
96+
let tyopt = if let Some(ty) = local.ty {
97+
let ty_snip = snippet_with_context(cx, ty.span, ctxt, "_", &mut app).0;
98+
format!(": &{mutopt}{ty_snip}")
99+
} else {
100+
String::new()
101+
};
102+
span_lint_hir_and_then(
103+
cx,
104+
TOPLEVEL_REF_ARG,
105+
init.hir_id,
106+
local.pat.span,
107+
"`ref` on an entire `let` pattern is discouraged, take a reference with `&` instead",
108+
|diag| {
109+
diag.span_suggestion(
110+
stmt.span,
111+
"try",
112+
format!("let {name}{tyopt} = {initref};", name = snippet(cx, name.span, ".."),),
113+
app,
114+
);
115+
},
116+
);
117+
}
118+
}
119+
}

0 commit comments

Comments
 (0)