Skip to content

Commit 0c7a202

Browse files
JarchoJarcho
committed
Rework redundant_closure
* Better track when a early-bound region appears when a late-bound region is required * Don't lint when the closure gives explicit types.
1 parent 18c6818 commit 0c7a202

File tree

5 files changed

+310
-119
lines changed

5 files changed

+310
-119
lines changed

‎clippy_lints/src/eta_reduction.rs

Lines changed: 184 additions & 114 deletions
Original file line numberDiff line numberDiff line change
@@ -1,20 +1,18 @@
11
use clippy_utils::diagnostics::{span_lint_and_sugg, span_lint_and_then};
22
use clippy_utils::higher::VecArgs;
33
use clippy_utils::source::snippet_opt;
4-
use clippy_utils::ty::is_type_diagnostic_item;
4+
use clippy_utils::ty::type_diagnostic_name;
55
use clippy_utils::usage::local_used_after_expr;
6-
use clippy_utils::{higher, is_adjusted, path_to_local, path_to_local_id};
7-
use if_chain::if_chain;
6+
use clippy_utils::{get_enclosing_loop_or_multi_call_closure, higher, is_adjusted, path_to_local, path_to_local_id};
87
use rustc_errors::Applicability;
98
use rustc_hir::def_id::DefId;
10-
use rustc_hir::{Closure, Expr, ExprKind, Param, PatKind, Unsafety};
9+
use rustc_hir::{BindingAnnotation, Expr, ExprKind, FnRetTy, Param, PatKind, TyKind, Unsafety};
1110
use rustc_lint::{LateContext, LateLintPass};
12-
use rustc_middle::ty::adjustment::{Adjust, Adjustment, AutoBorrow};
13-
use rustc_middle::ty::binding::BindingMode;
14-
use rustc_middle::ty::subst::Subst;
15-
use rustc_middle::ty::{self, ClosureKind, Ty, TypeVisitable};
11+
use rustc_middle::ty::subst::{GenericArg, GenericArgKind};
12+
use rustc_middle::ty::{self, ClosureKind, ClosureSubsts, FnSig, Region, RegionKind, Ty, TypeVisitable, TypeckResults};
1613
use rustc_session::{declare_lint_pass, declare_tool_lint};
1714
use rustc_span::symbol::sym;
15+
use rustc_target::spec::abi::Abi;
1816

1917
declare_clippy_lint! {
2018
/// ### What it does
@@ -73,14 +71,18 @@ declare_clippy_lint! {
7371
declare_lint_pass!(EtaReduction => [REDUNDANT_CLOSURE, REDUNDANT_CLOSURE_FOR_METHOD_CALLS]);
7472

7573
impl<'tcx> LateLintPass<'tcx> for EtaReduction {
74+
#[allow(clippy::too_many_lines)]
7675
fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) {
77-
if expr.span.from_expansion() {
76+
let body = if let ExprKind::Closure(c) = expr.kind
77+
&& c.fn_decl.inputs.iter().all(|ty| matches!(ty.kind, TyKind::Infer))
78+
&& matches!(c.fn_decl.output, FnRetTy::DefaultReturn(_))
79+
&& !expr.span.from_expansion()
80+
{
81+
cx.tcx.hir().body(c.body)
82+
} else {
7883
return;
79-
}
80-
let body = match expr.kind {
81-
ExprKind::Closure(&Closure { body, .. }) => cx.tcx.hir().body(body),
82-
_ => return,
8384
};
85+
8486
if body.value.span.from_expansion() {
8587
if body.params.is_empty() {
8688
if let Some(VecArgs::Vec(&[])) = higher::VecArgs::hir(cx, &body.value) {
@@ -100,123 +102,191 @@ impl<'tcx> LateLintPass<'tcx> for EtaReduction {
100102
return;
101103
}
102104

103-
let closure_ty = cx.typeck_results().expr_ty(expr);
105+
let typeck = cx.typeck_results();
106+
let closure = if let ty::Closure(_, closure_subs) = typeck.expr_ty(expr).kind() {
107+
closure_subs.as_closure()
108+
} else {
109+
return;
110+
};
104111

105-
if_chain!(
106-
if !is_adjusted(cx, &body.value);
107-
if let ExprKind::Call(callee, args) = body.value.kind;
108-
if let ExprKind::Path(_) = callee.kind;
109-
if check_inputs(cx, body.params, args);
110-
let callee_ty = cx.typeck_results().expr_ty_adjusted(callee);
111-
let call_ty = cx.typeck_results().type_dependent_def_id(body.value.hir_id)
112-
.map_or(callee_ty, |id| cx.tcx.type_of(id));
113-
if check_sig(cx, closure_ty, call_ty);
114-
let substs = cx.typeck_results().node_substs(callee.hir_id);
115-
// This fixes some false positives that I don't entirely understand
116-
if substs.is_empty() || !cx.typeck_results().expr_ty(expr).has_late_bound_regions();
117-
// A type param function ref like `T::f` is not 'static, however
118-
// it is if cast like `T::f as fn()`. This seems like a rustc bug.
119-
if !substs.types().any(|t| matches!(t.kind(), ty::Param(_)));
120-
let callee_ty_unadjusted = cx.typeck_results().expr_ty(callee).peel_refs();
121-
if !is_type_diagnostic_item(cx, callee_ty_unadjusted, sym::Arc);
122-
if !is_type_diagnostic_item(cx, callee_ty_unadjusted, sym::Rc);
123-
then {
124-
span_lint_and_then(cx, REDUNDANT_CLOSURE, expr.span, "redundant closure", |diag| {
125-
if let Some(mut snippet) = snippet_opt(cx, callee.span) {
126-
if_chain! {
127-
if let ty::Closure(_, substs) = callee_ty.peel_refs().kind();
128-
if substs.as_closure().kind() == ClosureKind::FnMut;
129-
if path_to_local(callee).map_or(false, |l| local_used_after_expr(cx, l, expr));
112+
if is_adjusted(cx, &body.value) {
113+
return;
114+
}
130115

131-
then {
132-
// Mutable closure is used after current expr; we cannot consume it.
133-
snippet = format!("&mut {}", snippet);
116+
match body.value.kind {
117+
ExprKind::Call(callee, args)
118+
if matches!(callee.kind, ExprKind::Path(..))
119+
&& !matches!(
120+
type_diagnostic_name(cx, typeck.expr_ty(callee).peel_refs()),
121+
Some(sym::Arc | sym::Rc)
122+
)
123+
&& check_inputs(typeck, body.params, args) =>
124+
{
125+
let sig = match *typeck.expr_ty_adjusted(callee).kind() {
126+
ty::FnDef(def, _) => cx.tcx.fn_sig(def).skip_binder(),
127+
ty::FnPtr(sig) => sig.skip_binder(),
128+
ty::Closure(_, subs) => cx
129+
.tcx
130+
.signature_unclosure(subs.as_closure().sig(), Unsafety::Normal)
131+
.skip_binder(),
132+
_ => {
133+
if typeck.type_dependent_def_id(body.value.hir_id).is_some()
134+
&& let subs = typeck.node_substs(body.value.hir_id)
135+
&& let output = typeck.expr_ty(&body.value)
136+
&& let ty::Tuple(tys) = *subs.type_at(1).kind()
137+
{
138+
cx.tcx.mk_fn_sig(tys.into_iter(), output, false, Unsafety::Normal, Abi::Rust)
139+
} else {
140+
return;
141+
}
142+
},
143+
};
144+
if check_sig(cx, closure, sig)
145+
// Given some trait fn `fn f() -> ()` and some type `T: Trait`, `T::f` is not
146+
// `'static` unless `T: 'static`. The cast `T::f as fn()` will, however, result
147+
// in a type which is `'static`.
148+
// For now ignore all callee types which reference a type parameter.
149+
&& !typeck.node_substs(callee.hir_id).types().any(|t| matches!(t.kind(), ty::Param(_)))
150+
{
151+
span_lint_and_then(
152+
cx,
153+
REDUNDANT_CLOSURE,
154+
expr.span,
155+
"redundant closure",
156+
|diag| {
157+
if let Some(mut snippet) = snippet_opt(cx, callee.span) {
158+
if let ty::Closure(_, substs) = *typeck.expr_ty(callee).peel_refs().kind()
159+
&& substs.as_closure().kind() == ClosureKind::FnMut
160+
&& (
161+
get_enclosing_loop_or_multi_call_closure(cx, expr).is_some()
162+
|| path_to_local(callee)
163+
.map_or(false, |l| local_used_after_expr(cx, l, callee))
164+
)
165+
{
166+
// Mutable closure is used after current expr; we cannot consume it.
167+
snippet = format!("&mut {}", snippet);
168+
}
169+
diag.span_suggestion(
170+
expr.span,
171+
"replace the closure with the function itself",
172+
snippet,
173+
Applicability::MachineApplicable,
174+
);
134175
}
135176
}
136-
diag.span_suggestion(
137-
expr.span,
138-
"replace the closure with the function itself",
139-
snippet,
140-
Applicability::MachineApplicable,
141-
);
142-
}
143-
});
144-
}
145-
);
146-
147-
if_chain!(
148-
if !is_adjusted(cx, &body.value);
149-
if let ExprKind::MethodCall(path, args, _) = body.value.kind;
150-
if check_inputs(cx, body.params, args);
151-
let method_def_id = cx.typeck_results().type_dependent_def_id(body.value.hir_id).unwrap();
152-
let substs = cx.typeck_results().node_substs(body.value.hir_id);
153-
let call_ty = cx.tcx.bound_type_of(method_def_id).subst(cx.tcx, substs);
154-
if check_sig(cx, closure_ty, call_ty);
155-
then {
156-
span_lint_and_then(cx, REDUNDANT_CLOSURE_FOR_METHOD_CALLS, expr.span, "redundant closure", |diag| {
157-
let name = get_ufcs_type_name(cx, method_def_id);
158-
diag.span_suggestion(
177+
);
178+
}
179+
},
180+
ExprKind::MethodCall(path, args, _) if check_inputs(typeck, body.params, args) => {
181+
if let Some(method_def_id) = typeck.type_dependent_def_id(body.value.hir_id)
182+
&& check_sig(cx, closure, cx.tcx.fn_sig(method_def_id).skip_binder())
183+
{
184+
span_lint_and_then(
185+
cx,
186+
REDUNDANT_CLOSURE_FOR_METHOD_CALLS,
159187
expr.span,
160-
"replace the closure with the method itself",
161-
format!("{}::{}", name, path.ident.name),
162-
Applicability::MachineApplicable,
188+
"redundant closure",
189+
|diag| {
190+
let name = get_ufcs_type_name(cx, method_def_id);
191+
diag.span_suggestion(
192+
expr.span,
193+
"replace the closure with the method itself",
194+
format!("{}::{}", name, path.ident.name),
195+
Applicability::MachineApplicable,
196+
);
197+
},
163198
);
164-
})
165-
}
166-
);
199+
}
200+
},
201+
_ => (),
202+
}
167203
}
168204
}
169205

170-
fn check_inputs(cx: &LateContext<'_>, params: &[Param<'_>], call_args: &[Expr<'_>]) -> bool {
171-
if params.len() != call_args.len() {
172-
return false;
206+
fn check_inputs(typeck: &TypeckResults<'_>, params: &[Param<'_>], call_args: &[Expr<'_>]) -> bool {
207+
params.len() == call_args.len()
208+
&& params.iter().zip(call_args).all(|(p, arg)| {
209+
matches!(
210+
p.pat.kind,PatKind::Binding(BindingAnnotation::Unannotated, id, _, None)
211+
if path_to_local_id(arg, id)
212+
)
213+
// Only allow adjustments which change regions (i.e. re-borrowing).
214+
&& typeck
215+
.expr_adjustments(arg)
216+
.last()
217+
.map_or(true, |a| a.target == typeck.expr_ty(arg))
218+
})
219+
}
220+
221+
fn check_sig<'tcx>(cx: &LateContext<'tcx>, closure: ClosureSubsts<'tcx>, call_sig: FnSig<'_>) -> bool {
222+
call_sig.unsafety == Unsafety::Normal
223+
&& !has_late_bound_to_non_late_bound_regions(
224+
cx.tcx
225+
.signature_unclosure(closure.sig(), Unsafety::Normal)
226+
.skip_binder(),
227+
call_sig,
228+
)
229+
}
230+
231+
/// This walks through both signatures and checks for any time a late-bound region is expected by an
232+
/// `impl Fn` type, but the target signature does not have a late-bound region in the same position.
233+
///
234+
/// This is needed because rustc is unable to late bind early-bound regions in a function signature.
235+
fn has_late_bound_to_non_late_bound_regions(from_sig: FnSig<'_>, to_sig: FnSig<'_>) -> bool {
236+
fn check_region(from_region: Region<'_>, to_region: Region<'_>) -> bool {
237+
matches!(from_region.kind(), RegionKind::ReLateBound(..))
238+
&& !matches!(to_region.kind(), RegionKind::ReLateBound(..))
173239
}
174-
let binding_modes = cx.typeck_results().pat_binding_modes();
175-
std::iter::zip(params, call_args).all(|(param, arg)| {
176-
match param.pat.kind {
177-
PatKind::Binding(_, id, ..) if path_to_local_id(arg, id) => {},
178-
_ => return false,
179-
}
180-
// checks that parameters are not bound as `ref` or `ref mut`
181-
if let Some(BindingMode::BindByReference(_)) = binding_modes.get(param.pat.hir_id) {
182-
return false;
183-
}
184240

185-
match *cx.typeck_results().expr_adjustments(arg) {
186-
[] => true,
187-
[
188-
Adjustment {
189-
kind: Adjust::Deref(None),
190-
..
241+
fn check_subs(from_subs: &[GenericArg<'_>], to_subs: &[GenericArg<'_>]) -> bool {
242+
if from_subs.len() != to_subs.len() {
243+
return true;
244+
}
245+
for (from_arg, to_arg) in to_subs.iter().zip(from_subs) {
246+
match (from_arg.unpack(), to_arg.unpack()) {
247+
(GenericArgKind::Lifetime(from_region), GenericArgKind::Lifetime(to_region)) => {
248+
if check_region(from_region, to_region) {
249+
return true;
250+
}
191251
},
192-
Adjustment {
193-
kind: Adjust::Borrow(AutoBorrow::Ref(_, mu2)),
194-
..
252+
(GenericArgKind::Type(from_ty), GenericArgKind::Type(to_ty)) => {
253+
if check_ty(from_ty, to_ty) {
254+
return true;
255+
}
195256
},
196-
] => {
197-
// re-borrow with the same mutability is allowed
198-
let ty = cx.typeck_results().expr_ty(arg);
199-
matches!(*ty.kind(), ty::Ref(.., mu1) if mu1 == mu2.into())
200-
},
201-
_ => false,
257+
(GenericArgKind::Const(_), GenericArgKind::Const(_)) => (),
258+
_ => return true,
259+
}
202260
}
203-
})
204-
}
205-
206-
fn check_sig<'tcx>(cx: &LateContext<'tcx>, closure_ty: Ty<'tcx>, call_ty: Ty<'tcx>) -> bool {
207-
let call_sig = call_ty.fn_sig(cx.tcx);
208-
if call_sig.unsafety() == Unsafety::Unsafe {
209-
return false;
261+
false
210262
}
211-
if !closure_ty.has_late_bound_regions() {
212-
return true;
263+
264+
fn check_ty(from_ty: Ty<'_>, to_ty: Ty<'_>) -> bool {
265+
match (from_ty.kind(), to_ty.kind()) {
266+
(&ty::Adt(_, from_subs), &ty::Adt(_, to_subs)) => check_subs(from_subs, to_subs),
267+
(&ty::Array(from_ty, _), &ty::Array(to_ty, _)) | (&ty::Slice(from_ty), &ty::Slice(to_ty)) => {
268+
check_ty(from_ty, to_ty)
269+
},
270+
(&ty::Ref(from_region, from_ty, _), &ty::Ref(to_region, to_ty, _)) => {
271+
check_region(from_region, to_region) || check_ty(from_ty, to_ty)
272+
},
273+
(&ty::Tuple(from_tys), &ty::Tuple(to_tys)) => {
274+
from_tys.len() != to_tys.len()
275+
|| from_tys
276+
.iter()
277+
.zip(to_tys)
278+
.any(|(from_ty, to_ty)| check_ty(from_ty, to_ty))
279+
},
280+
_ => from_ty.has_late_bound_regions(),
281+
}
213282
}
214-
let substs = match closure_ty.kind() {
215-
ty::Closure(_, substs) => substs,
216-
_ => return false,
217-
};
218-
let closure_sig = cx.tcx.signature_unclosure(substs.as_closure().sig(), Unsafety::Normal);
219-
cx.tcx.erase_late_bound_regions(closure_sig) == cx.tcx.erase_late_bound_regions(call_sig)
283+
284+
assert!(from_sig.inputs_and_output.len() == to_sig.inputs_and_output.len());
285+
from_sig
286+
.inputs_and_output
287+
.iter()
288+
.zip(to_sig.inputs_and_output)
289+
.any(|(from_ty, to_ty)| check_ty(from_ty, to_ty))
220290
}
221291

222292
fn get_ufcs_type_name(cx: &LateContext<'_>, method_def_id: DefId) -> String {

‎clippy_utils/src/ty.rs

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -330,6 +330,11 @@ pub fn is_type_lang_item(cx: &LateContext<'_>, ty: Ty<'_>, lang_item: hir::LangI
330330
}
331331
}
332332

333+
/// Gets the diagnostic name of the type, if it has one
334+
pub fn type_diagnostic_name(cx: &LateContext<'_>, ty: Ty<'_>) -> Option<Symbol> {
335+
ty.ty_adt_def().and_then(|adt| cx.tcx.get_diagnostic_name(adt.did()))
336+
}
337+
333338
/// Return `true` if the passed `typ` is `isize` or `usize`.
334339
pub fn is_isize_or_usize(typ: Ty<'_>) -> bool {
335340
matches!(typ.kind(), ty::Int(IntTy::Isize) | ty::Uint(UintTy::Usize))

0 commit comments

Comments
 (0)