Skip to content

Commit 2fc74a3

Browse files
borsbors
committed
Auto merge of #13108 - tesuji:fix_redundant_closure, r=xFrednet
Fix `redundant_closure` false positive with closures has return type contains `'static` Fix #13073 . Please enable "ignore white-space change" settings in github UI for easy reviewing. HACK: The third commit contains a hack to check if a type `T: 'static` when `fn() -> U where U: 'static`. I don't have a clean way to check for it. changelog: [`redundant_closure`] Fix false positive with closures has return type contains `'static`
2 parents 5542309 + 0bc9f00 commit 2fc74a3

File tree

4 files changed

+250
-185
lines changed

4 files changed

+250
-185
lines changed

‎clippy_lints/src/eta_reduction.rs

Lines changed: 176 additions & 151 deletions
Original file line numberDiff line numberDiff line change
@@ -9,8 +9,7 @@ use rustc_hir::{BindingMode, Expr, ExprKind, FnRetTy, Param, PatKind, QPath, Saf
99
use rustc_infer::infer::TyCtxtInferExt;
1010
use rustc_lint::{LateContext, LateLintPass};
1111
use rustc_middle::ty::{
12-
self, Binder, ClosureArgs, ClosureKind, FnSig, GenericArg, GenericArgKind, List, Region, RegionKind, Ty, TyCtxt,
13-
TypeVisitableExt, TypeckResults,
12+
self, Binder, ClosureKind, FnSig, GenericArg, GenericArgKind, List, Region, Ty, TypeVisitableExt, TypeckResults,
1413
};
1514
use rustc_session::declare_lint_pass;
1615
use rustc_span::symbol::sym;
@@ -74,159 +73,184 @@ declare_clippy_lint! {
7473
declare_lint_pass!(EtaReduction => [REDUNDANT_CLOSURE, REDUNDANT_CLOSURE_FOR_METHOD_CALLS]);
7574

7675
impl<'tcx> LateLintPass<'tcx> for EtaReduction {
77-
#[allow(clippy::too_many_lines)]
78-
fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) {
79-
let body = if let ExprKind::Closure(c) = expr.kind
80-
&& c.fn_decl.inputs.iter().all(|ty| matches!(ty.kind, TyKind::Infer))
81-
&& matches!(c.fn_decl.output, FnRetTy::DefaultReturn(_))
82-
&& !expr.span.from_expansion()
83-
{
84-
cx.tcx.hir().body(c.body)
85-
} else {
86-
return;
87-
};
88-
89-
if body.value.span.from_expansion() {
90-
if body.params.is_empty() {
91-
if let Some(VecArgs::Vec(&[])) = VecArgs::hir(cx, body.value) {
92-
// replace `|| vec![]` with `Vec::new`
93-
span_lint_and_sugg(
94-
cx,
95-
REDUNDANT_CLOSURE,
96-
expr.span,
97-
"redundant closure",
98-
"replace the closure with `Vec::new`",
99-
"std::vec::Vec::new".into(),
100-
Applicability::MachineApplicable,
101-
);
102-
}
76+
fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &Expr<'tcx>) {
77+
if let ExprKind::MethodCall(_method, receiver, args, _) = expr.kind {
78+
for arg in args {
79+
check_clousure(cx, Some(receiver), arg);
10380
}
104-
// skip `foo(|| macro!())`
105-
return;
10681
}
82+
if let ExprKind::Call(func, args) = expr.kind {
83+
check_clousure(cx, None, func);
84+
for arg in args {
85+
check_clousure(cx, None, arg);
86+
}
87+
}
88+
}
89+
}
10790

108-
let typeck = cx.typeck_results();
109-
let closure = if let ty::Closure(_, closure_subs) = typeck.expr_ty(expr).kind() {
110-
closure_subs.as_closure()
111-
} else {
112-
return;
113-
};
91+
#[allow(clippy::too_many_lines)]
92+
fn check_clousure<'tcx>(cx: &LateContext<'tcx>, outer_receiver: Option<&Expr<'tcx>>, expr: &Expr<'tcx>) {
93+
let body = if let ExprKind::Closure(c) = expr.kind
94+
&& c.fn_decl.inputs.iter().all(|ty| matches!(ty.kind, TyKind::Infer))
95+
&& matches!(c.fn_decl.output, FnRetTy::DefaultReturn(_))
96+
&& !expr.span.from_expansion()
97+
{
98+
cx.tcx.hir().body(c.body)
99+
} else {
100+
return;
101+
};
114102

115-
if is_adjusted(cx, body.value) {
116-
return;
103+
if body.value.span.from_expansion() {
104+
if body.params.is_empty() {
105+
if let Some(VecArgs::Vec(&[])) = VecArgs::hir(cx, body.value) {
106+
// replace `|| vec![]` with `Vec::new`
107+
span_lint_and_sugg(
108+
cx,
109+
REDUNDANT_CLOSURE,
110+
expr.span,
111+
"redundant closure",
112+
"replace the closure with `Vec::new`",
113+
"std::vec::Vec::new".into(),
114+
Applicability::MachineApplicable,
115+
);
116+
}
117117
}
118+
// skip `foo(|| macro!())`
119+
return;
120+
}
118121

119-
match body.value.kind {
120-
ExprKind::Call(callee, args)
121-
if matches!(
122-
callee.kind,
123-
ExprKind::Path(QPath::Resolved(..) | QPath::TypeRelative(..))
124-
) =>
122+
if is_adjusted(cx, body.value) {
123+
return;
124+
}
125+
126+
let typeck = cx.typeck_results();
127+
let closure = if let ty::Closure(_, closure_subs) = typeck.expr_ty(expr).kind() {
128+
closure_subs.as_closure()
129+
} else {
130+
return;
131+
};
132+
let closure_sig = cx.tcx.signature_unclosure(closure.sig(), Safety::Safe).skip_binder();
133+
match body.value.kind {
134+
ExprKind::Call(callee, args)
135+
if matches!(
136+
callee.kind,
137+
ExprKind::Path(QPath::Resolved(..) | QPath::TypeRelative(..))
138+
) =>
139+
{
140+
let callee_ty_raw = typeck.expr_ty(callee);
141+
let callee_ty = callee_ty_raw.peel_refs();
142+
if matches!(type_diagnostic_name(cx, callee_ty), Some(sym::Arc | sym::Rc))
143+
|| !check_inputs(typeck, body.params, None, args)
125144
{
126-
let callee_ty_raw = typeck.expr_ty(callee);
127-
let callee_ty = callee_ty_raw.peel_refs();
128-
if matches!(type_diagnostic_name(cx, callee_ty), Some(sym::Arc | sym::Rc))
129-
|| !check_inputs(typeck, body.params, None, args)
130-
{
131-
return;
132-
}
133-
let callee_ty_adjusted = typeck
134-
.expr_adjustments(callee)
135-
.last()
136-
.map_or(callee_ty, |a| a.target.peel_refs());
145+
return;
146+
}
147+
let callee_ty_adjusted = typeck
148+
.expr_adjustments(callee)
149+
.last()
150+
.map_or(callee_ty, |a| a.target.peel_refs());
137151

138-
let sig = match callee_ty_adjusted.kind() {
139-
ty::FnDef(def, _) => {
140-
// Rewriting `x(|| f())` to `x(f)` where f is marked `#[track_caller]` moves the `Location`
141-
if cx.tcx.has_attr(*def, sym::track_caller) {
142-
return;
143-
}
152+
let sig = match callee_ty_adjusted.kind() {
153+
ty::FnDef(def, _) => {
154+
// Rewriting `x(|| f())` to `x(f)` where f is marked `#[track_caller]` moves the `Location`
155+
if cx.tcx.has_attr(*def, sym::track_caller) {
156+
return;
157+
}
144158

145-
cx.tcx.fn_sig(def).skip_binder().skip_binder()
146-
},
147-
ty::FnPtr(sig) => sig.skip_binder(),
148-
ty::Closure(_, subs) => cx
149-
.tcx
150-
.signature_unclosure(subs.as_closure().sig(), Safety::Safe)
151-
.skip_binder(),
152-
_ => {
153-
if typeck.type_dependent_def_id(body.value.hir_id).is_some()
154-
&& let subs = typeck.node_args(body.value.hir_id)
155-
&& let output = typeck.expr_ty(body.value)
156-
&& let ty::Tuple(tys) = *subs.type_at(1).kind()
157-
{
158-
cx.tcx.mk_fn_sig(tys, output, false, Safety::Safe, Abi::Rust)
159-
} else {
160-
return;
161-
}
162-
},
163-
};
164-
if check_sig(cx, closure, sig)
165-
&& let generic_args = typeck.node_args(callee.hir_id)
166-
// Given some trait fn `fn f() -> ()` and some type `T: Trait`, `T::f` is not
167-
// `'static` unless `T: 'static`. The cast `T::f as fn()` will, however, result
168-
// in a type which is `'static`.
169-
// For now ignore all callee types which reference a type parameter.
170-
&& !generic_args.types().any(|t| matches!(t.kind(), ty::Param(_)))
171-
{
172-
span_lint_and_then(cx, REDUNDANT_CLOSURE, expr.span, "redundant closure", |diag| {
173-
if let Some(mut snippet) = snippet_opt(cx, callee.span) {
174-
if path_to_local(callee).map_or(false, |l| {
175-
// FIXME: Do we really need this `local_used_in` check?
176-
// Isn't it checking something like... `callee(callee)`?
177-
// If somehow this check is needed, add some test for it,
178-
// 'cuz currently nothing changes after deleting this check.
179-
local_used_in(cx, l, args) || local_used_after_expr(cx, l, expr)
180-
}) {
181-
match cx.tcx.infer_ctxt().build().err_ctxt().type_implements_fn_trait(
182-
cx.param_env,
183-
Binder::bind_with_vars(callee_ty_adjusted, List::empty()),
184-
ty::PredicatePolarity::Positive,
185-
) {
186-
// Mutable closure is used after current expr; we cannot consume it.
187-
Ok((ClosureKind::FnMut, _)) => snippet = format!("&mut {snippet}"),
188-
Ok((ClosureKind::Fn, _)) if !callee_ty_raw.is_ref() => {
189-
snippet = format!("&{snippet}");
190-
},
191-
_ => (),
192-
}
159+
cx.tcx.fn_sig(def).skip_binder().skip_binder()
160+
},
161+
ty::FnPtr(sig) => sig.skip_binder(),
162+
ty::Closure(_, subs) => cx
163+
.tcx
164+
.signature_unclosure(subs.as_closure().sig(), Safety::Safe)
165+
.skip_binder(),
166+
_ => {
167+
if typeck.type_dependent_def_id(body.value.hir_id).is_some()
168+
&& let subs = typeck.node_args(body.value.hir_id)
169+
&& let output = typeck.expr_ty(body.value)
170+
&& let ty::Tuple(tys) = *subs.type_at(1).kind()
171+
{
172+
cx.tcx.mk_fn_sig(tys, output, false, Safety::Safe, Abi::Rust)
173+
} else {
174+
return;
175+
}
176+
},
177+
};
178+
if let Some(outer) = outer_receiver
179+
&& ty_has_static(sig.output())
180+
&& let generic_args = typeck.node_args(outer.hir_id)
181+
// HACK: Given a closure in `T.method(|| f())`, where `fn f() -> U where U: 'static`, `T.method(f)`
182+
// will succeed iff `T: 'static`. But the region of `T` is always erased by `typeck.expr_ty()` when
183+
// T is a generic type. For example, return type of `Option<String>::as_deref()` is a generic.
184+
// So we have a hack like this.
185+
&& generic_args.len() > 0
186+
{
187+
return;
188+
}
189+
if check_sig(closure_sig, sig)
190+
&& let generic_args = typeck.node_args(callee.hir_id)
191+
// Given some trait fn `fn f() -> ()` and some type `T: Trait`, `T::f` is not
192+
// `'static` unless `T: 'static`. The cast `T::f as fn()` will, however, result
193+
// in a type which is `'static`.
194+
// For now ignore all callee types which reference a type parameter.
195+
&& !generic_args.types().any(|t| matches!(t.kind(), ty::Param(_)))
196+
{
197+
span_lint_and_then(cx, REDUNDANT_CLOSURE, expr.span, "redundant closure", |diag| {
198+
if let Some(mut snippet) = snippet_opt(cx, callee.span) {
199+
if path_to_local(callee).map_or(false, |l| {
200+
// FIXME: Do we really need this `local_used_in` check?
201+
// Isn't it checking something like... `callee(callee)`?
202+
// If somehow this check is needed, add some test for it,
203+
// 'cuz currently nothing changes after deleting this check.
204+
local_used_in(cx, l, args) || local_used_after_expr(cx, l, expr)
205+
}) {
206+
match cx.tcx.infer_ctxt().build().err_ctxt().type_implements_fn_trait(
207+
cx.param_env,
208+
Binder::bind_with_vars(callee_ty_adjusted, List::empty()),
209+
ty::PredicatePolarity::Positive,
210+
) {
211+
// Mutable closure is used after current expr; we cannot consume it.
212+
Ok((ClosureKind::FnMut, _)) => snippet = format!("&mut {snippet}"),
213+
Ok((ClosureKind::Fn, _)) if !callee_ty_raw.is_ref() => {
214+
snippet = format!("&{snippet}");
215+
},
216+
_ => (),
193217
}
194-
diag.span_suggestion(
195-
expr.span,
196-
"replace the closure with the function itself",
197-
snippet,
198-
Applicability::MachineApplicable,
199-
);
200218
}
201-
});
202-
}
203-
},
204-
ExprKind::MethodCall(path, self_, args, _) if check_inputs(typeck, body.params, Some(self_), args) => {
205-
if let Some(method_def_id) = typeck.type_dependent_def_id(body.value.hir_id)
206-
&& !cx.tcx.has_attr(method_def_id, sym::track_caller)
207-
&& check_sig(cx, closure, cx.tcx.fn_sig(method_def_id).skip_binder().skip_binder())
208-
{
209-
span_lint_and_then(
210-
cx,
211-
REDUNDANT_CLOSURE_FOR_METHOD_CALLS,
212-
expr.span,
213-
"redundant closure",
214-
|diag| {
215-
let args = typeck.node_args(body.value.hir_id);
216-
let caller = self_.hir_id.owner.def_id;
217-
let type_name = get_path_from_caller_to_method_type(cx.tcx, caller, method_def_id, args);
218-
diag.span_suggestion(
219-
expr.span,
220-
"replace the closure with the method itself",
221-
format!("{}::{}", type_name, path.ident.name),
222-
Applicability::MachineApplicable,
223-
);
224-
},
225-
);
226-
}
227-
},
228-
_ => (),
229-
}
219+
diag.span_suggestion(
220+
expr.span,
221+
"replace the closure with the function itself",
222+
snippet,
223+
Applicability::MachineApplicable,
224+
);
225+
}
226+
});
227+
}
228+
},
229+
ExprKind::MethodCall(path, self_, args, _) if check_inputs(typeck, body.params, Some(self_), args) => {
230+
if let Some(method_def_id) = typeck.type_dependent_def_id(body.value.hir_id)
231+
&& !cx.tcx.has_attr(method_def_id, sym::track_caller)
232+
&& check_sig(closure_sig, cx.tcx.fn_sig(method_def_id).skip_binder().skip_binder())
233+
{
234+
span_lint_and_then(
235+
cx,
236+
REDUNDANT_CLOSURE_FOR_METHOD_CALLS,
237+
expr.span,
238+
"redundant closure",
239+
|diag| {
240+
let args = typeck.node_args(body.value.hir_id);
241+
let caller = self_.hir_id.owner.def_id;
242+
let type_name = get_path_from_caller_to_method_type(cx.tcx, caller, method_def_id, args);
243+
diag.span_suggestion(
244+
expr.span,
245+
"replace the closure with the method itself",
246+
format!("{}::{}", type_name, path.ident.name),
247+
Applicability::MachineApplicable,
248+
);
249+
},
250+
);
251+
}
252+
},
253+
_ => (),
230254
}
231255
}
232256

@@ -251,12 +275,8 @@ fn check_inputs(
251275
})
252276
}
253277

254-
fn check_sig<'tcx>(cx: &LateContext<'tcx>, closure: ClosureArgs<TyCtxt<'tcx>>, call_sig: FnSig<'_>) -> bool {
255-
call_sig.safety == Safety::Safe
256-
&& !has_late_bound_to_non_late_bound_regions(
257-
cx.tcx.signature_unclosure(closure.sig(), Safety::Safe).skip_binder(),
258-
call_sig,
259-
)
278+
fn check_sig<'tcx>(closure_sig: FnSig<'tcx>, call_sig: FnSig<'tcx>) -> bool {
279+
call_sig.safety == Safety::Safe && !has_late_bound_to_non_late_bound_regions(closure_sig, call_sig)
260280
}
261281

262282
/// This walks through both signatures and checks for any time a late-bound region is expected by an
@@ -265,7 +285,7 @@ fn check_sig<'tcx>(cx: &LateContext<'tcx>, closure: ClosureArgs<TyCtxt<'tcx>>, c
265285
/// This is needed because rustc is unable to late bind early-bound regions in a function signature.
266286
fn has_late_bound_to_non_late_bound_regions(from_sig: FnSig<'_>, to_sig: FnSig<'_>) -> bool {
267287
fn check_region(from_region: Region<'_>, to_region: Region<'_>) -> bool {
268-
matches!(from_region.kind(), RegionKind::ReBound(..)) && !matches!(to_region.kind(), RegionKind::ReBound(..))
288+
from_region.is_bound() && !to_region.is_bound()
269289
}
270290

271291
fn check_subs(from_subs: &[GenericArg<'_>], to_subs: &[GenericArg<'_>]) -> bool {
@@ -318,3 +338,8 @@ fn has_late_bound_to_non_late_bound_regions(from_sig: FnSig<'_>, to_sig: FnSig<'
318338
.zip(to_sig.inputs_and_output)
319339
.any(|(from_ty, to_ty)| check_ty(from_ty, to_ty))
320340
}
341+
342+
fn ty_has_static(ty: Ty<'_>) -> bool {
343+
ty.walk()
344+
.any(|arg| matches!(arg.unpack(), GenericArgKind::Lifetime(re) if re.is_static()))
345+
}

‎tests/ui/eta.fixed

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@
22
#![allow(unused)]
33
#![allow(
44
clippy::needless_borrow,
5+
clippy::needless_option_as_deref,
56
clippy::needless_pass_by_value,
67
clippy::no_effect,
78
clippy::option_map_unit_fn,
@@ -482,3 +483,19 @@ mod issue_12853 {
482483
x();
483484
}
484485
}
486+
487+
mod issue_13073 {
488+
fn get_default() -> Option<&'static str> {
489+
Some("foo")
490+
}
491+
492+
pub fn foo() {
493+
// shouldn't lint
494+
let bind: Option<String> = None;
495+
let _field = bind.as_deref().or_else(|| get_default()).unwrap();
496+
let bind: Option<&'static str> = None;
497+
let _field = bind.as_deref().or_else(|| get_default()).unwrap();
498+
// should lint
499+
let _field = bind.or_else(get_default).unwrap();
500+
}
501+
}

0 commit comments

Comments
 (0)