Skip to content

Commit 5782dc0

Browse files
camsteffencamsteffen
committed
Fix redundant closure bugs
1 parent 1a52478 commit 5782dc0

File tree

9 files changed

+227
-230
lines changed

9 files changed

+227
-230
lines changed

‎clippy_lints/src/eta_reduction.rs‎

Lines changed: 92 additions & 148 deletions
Original file line numberDiff line numberDiff line change
@@ -1,16 +1,16 @@
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::{implements_trait, type_is_unsafe_function};
54
use clippy_utils::usage::UsedAfterExprVisitor;
6-
use clippy_utils::{get_enclosing_loop_or_closure, higher};
7-
use clippy_utils::{is_adjusted, iter_input_pats};
5+
use clippy_utils::{get_enclosing_loop_or_closure, higher, path_to_local_id};
86
use if_chain::if_chain;
97
use rustc_errors::Applicability;
10-
use rustc_hir::{def_id, Expr, ExprKind, Param, PatKind, QPath};
11-
use rustc_lint::{LateContext, LateLintPass, LintContext};
12-
use rustc_middle::lint::in_external_macro;
13-
use rustc_middle::ty::{self, ClosureKind, Ty};
8+
use rustc_hir::def_id::DefId;
9+
use rustc_hir::{Expr, ExprKind, Param, PatKind, Unsafety};
10+
use rustc_lint::{LateContext, LateLintPass};
11+
use rustc_middle::ty::adjustment::{Adjust, Adjustment, AutoBorrow};
12+
use rustc_middle::ty::subst::Subst;
13+
use rustc_middle::ty::{self, ClosureKind, Ty, TypeFoldable};
1414
use rustc_session::{declare_lint_pass, declare_tool_lint};
1515

1616
declare_clippy_lint! {
@@ -52,12 +52,6 @@ declare_clippy_lint! {
5252
/// ### Why is this bad?
5353
/// It's unnecessary to create the closure.
5454
///
55-
/// ### Known problems
56-
/// [#3071](https://github.com/rust-lang/rust-clippy/issues/3071),
57-
/// [#3942](https://github.com/rust-lang/rust-clippy/issues/3942),
58-
/// [#4002](https://github.com/rust-lang/rust-clippy/issues/4002)
59-
///
60-
///
6155
/// ### Example
6256
/// ```rust,ignore
6357
/// Some('a').map(|s| s.to_uppercase());
@@ -75,32 +69,16 @@ declare_lint_pass!(EtaReduction => [REDUNDANT_CLOSURE, REDUNDANT_CLOSURE_FOR_MET
7569

7670
impl<'tcx> LateLintPass<'tcx> for EtaReduction {
7771
fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) {
78-
if in_external_macro(cx.sess(), expr.span) {
72+
if expr.span.from_expansion() {
7973
return;
8074
}
81-
82-
match expr.kind {
83-
ExprKind::Call(_, args) | ExprKind::MethodCall(_, _, args, _) => {
84-
for arg in args {
85-
// skip `foo(macro!())`
86-
if arg.span.ctxt() == expr.span.ctxt() {
87-
check_closure(cx, arg);
88-
}
89-
}
90-
},
91-
_ => (),
92-
}
93-
}
94-
}
95-
96-
fn check_closure<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) {
97-
if let ExprKind::Closure(_, decl, eid, _, _) = expr.kind {
98-
let body = cx.tcx.hir().body(eid);
99-
let ex = &body.value;
100-
101-
if ex.span.ctxt() != expr.span.ctxt() {
102-
if decl.inputs.is_empty() {
103-
if let Some(VecArgs::Vec(&[])) = higher::VecArgs::hir(cx, ex) {
75+
let body = match expr.kind {
76+
ExprKind::Closure(_, _, id, _, _) => cx.tcx.hir().body(id),
77+
_ => return,
78+
};
79+
if body.value.span.from_expansion() {
80+
if body.params.is_empty() {
81+
if let Some(VecArgs::Vec(&[])) = higher::VecArgs::hir(cx, &body.value) {
10482
// replace `|| vec![]` with `Vec::new`
10583
span_lint_and_sugg(
10684
cx,
@@ -117,33 +95,30 @@ fn check_closure<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) {
11795
return;
11896
}
11997

120-
if_chain!(
121-
if let ExprKind::Call(caller, args) = ex.kind;
122-
123-
if let ExprKind::Path(_) = caller.kind;
124-
125-
// Not the same number of arguments, there is no way the closure is the same as the function return;
126-
if args.len() == decl.inputs.len();
127-
128-
// Are the expression or the arguments type-adjusted? Then we need the closure
129-
if !(is_adjusted(cx, ex) || args.iter().any(|arg| is_adjusted(cx, arg)));
130-
131-
let fn_ty = cx.typeck_results().expr_ty(caller);
132-
133-
if matches!(fn_ty.kind(), ty::FnDef(_, _) | ty::FnPtr(_) | ty::Closure(_, _));
134-
135-
if !type_is_unsafe_function(cx, fn_ty);
136-
137-
if compare_inputs(&mut iter_input_pats(decl, body), &mut args.iter());
98+
let closure_ty = cx.typeck_results().expr_ty(expr);
13899

100+
if_chain!(
101+
if let ExprKind::Call(callee, args) = body.value.kind;
102+
if let ExprKind::Path(_) = callee.kind;
103+
if check_inputs(cx, body.params, args);
104+
let callee_ty = cx.typeck_results().expr_ty_adjusted(callee);
105+
let call_ty = cx.typeck_results().type_dependent_def_id(body.value.hir_id)
106+
.map_or(callee_ty, |id| cx.tcx.type_of(id));
107+
if check_sig(cx, closure_ty, call_ty);
108+
let substs = cx.typeck_results().node_substs(callee.hir_id);
109+
// This fixes some false positives that I don't entirely understand
110+
if substs.is_empty() || !cx.typeck_results().expr_ty(expr).has_late_bound_regions();
111+
// A type param function ref like `T::f` is not 'static, however
112+
// it is if cast like `T::f as fn()`. This seems like a rustc bug.
113+
if !substs.types().any(|t| matches!(t.kind(), ty::Param(_)));
139114
then {
140115
span_lint_and_then(cx, REDUNDANT_CLOSURE, expr.span, "redundant closure", |diag| {
141-
if let Some(mut snippet) = snippet_opt(cx, caller.span) {
116+
if let Some(mut snippet) = snippet_opt(cx, callee.span) {
142117
if_chain! {
143-
if let ty::Closure(_, substs) = fn_ty.kind();
118+
if let ty::Closure(_, substs) = callee_ty.peel_refs().kind();
144119
if let ClosureKind::FnMut = substs.as_closure().kind();
145-
if UsedAfterExprVisitor::is_found(cx, caller)
146-
|| get_enclosing_loop_or_closure(cx.tcx, expr).is_some();
120+
if get_enclosing_loop_or_closure(cx.tcx, expr).is_some()
121+
|| UsedAfterExprVisitor::is_found(cx, callee);
147122

148123
then {
149124
// Mutable closure is used after current expr; we cannot consume it.
@@ -162,110 +137,79 @@ fn check_closure<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) {
162137
);
163138

164139
if_chain!(
165-
if let ExprKind::MethodCall(path, _, args, _) = ex.kind;
166-
167-
// Not the same number of arguments, there is no way the closure is the same as the function return;
168-
if args.len() == decl.inputs.len();
169-
170-
// Are the expression or the arguments type-adjusted? Then we need the closure
171-
if !(is_adjusted(cx, ex) || args.iter().skip(1).any(|arg| is_adjusted(cx, arg)));
172-
173-
let method_def_id = cx.typeck_results().type_dependent_def_id(ex.hir_id).unwrap();
174-
if !type_is_unsafe_function(cx, cx.tcx.type_of(method_def_id));
175-
176-
if compare_inputs(&mut iter_input_pats(decl, body), &mut args.iter());
177-
178-
if let Some(name) = get_ufcs_type_name(cx, method_def_id, &args[0]);
179-
140+
if let ExprKind::MethodCall(path, _, args, _) = body.value.kind;
141+
if check_inputs(cx, body.params, args);
142+
let method_def_id = cx.typeck_results().type_dependent_def_id(body.value.hir_id).unwrap();
143+
let substs = cx.typeck_results().node_substs(body.value.hir_id);
144+
let call_ty = cx.tcx.type_of(method_def_id).subst(cx.tcx, substs);
145+
if check_sig(cx, closure_ty, call_ty);
180146
then {
181-
span_lint_and_sugg(
182-
cx,
183-
REDUNDANT_CLOSURE_FOR_METHOD_CALLS,
184-
expr.span,
185-
"redundant closure",
186-
"replace the closure with the method itself",
187-
format!("{}::{}", name, path.ident.name),
188-
Applicability::MachineApplicable,
189-
);
147+
span_lint_and_then(cx, REDUNDANT_CLOSURE_FOR_METHOD_CALLS, expr.span, "redundant closure", |diag| {
148+
let name = get_ufcs_type_name(cx, method_def_id);
149+
diag.span_suggestion(
150+
expr.span,
151+
"replace the closure with the method itself",
152+
format!("{}::{}", name, path.ident.name),
153+
Applicability::MachineApplicable,
154+
);
155+
})
190156
}
191157
);
192158
}
193159
}
194160

195-
/// Tries to determine the type for universal function call to be used instead of the closure
196-
fn get_ufcs_type_name(cx: &LateContext<'_>, method_def_id: def_id::DefId, self_arg: &Expr<'_>) -> Option<String> {
197-
let expected_type_of_self = &cx.tcx.fn_sig(method_def_id).inputs_and_output().skip_binder()[0];
198-
let actual_type_of_self = &cx.typeck_results().node_type(self_arg.hir_id);
199-
200-
if let Some(trait_id) = cx.tcx.trait_of_item(method_def_id) {
201-
if match_borrow_depth(expected_type_of_self, actual_type_of_self)
202-
&& implements_trait(cx, actual_type_of_self, trait_id, &[])
203-
{
204-
return Some(cx.tcx.def_path_str(trait_id));
205-
}
161+
fn check_inputs(cx: &LateContext<'_>, params: &[Param<'_>], call_args: &[Expr<'_>]) -> bool {
162+
if params.len() != call_args.len() {
163+
return false;
206164
}
207-
208-
cx.tcx.impl_of_method(method_def_id).and_then(|_| {
209-
//a type may implicitly implement other type's methods (e.g. Deref)
210-
if match_types(expected_type_of_self, actual_type_of_self) {
211-
Some(get_type_name(cx, actual_type_of_self))
212-
} else {
213-
None
165+
std::iter::zip(params, call_args).all(|(param, arg)| {
166+
match param.pat.kind {
167+
PatKind::Binding(_, id, ..) if path_to_local_id(arg, id) => {},
168+
_ => return false,
169+
}
170+
match *cx.typeck_results().expr_adjustments(arg) {
171+
[] => true,
172+
[Adjustment {
173+
kind: Adjust::Deref(None),
174+
..
175+
}, Adjustment {
176+
kind: Adjust::Borrow(AutoBorrow::Ref(_, mu2)),
177+
..
178+
}] => {
179+
// re-borrow with the same mutability is allowed
180+
let ty = cx.typeck_results().expr_ty(arg);
181+
matches!(*ty.kind(), ty::Ref(.., mu1) if mu1 == mu2.into())
182+
},
183+
_ => false,
214184
}
215185
})
216186
}
217187

218-
fn match_borrow_depth(lhs: Ty<'_>, rhs: Ty<'_>) -> bool {
219-
match (&lhs.kind(), &rhs.kind()) {
220-
(ty::Ref(_, t1, mut1), ty::Ref(_, t2, mut2)) => mut1 == mut2 && match_borrow_depth(t1, t2),
221-
(l, r) => !matches!((l, r), (ty::Ref(_, _, _), _) | (_, ty::Ref(_, _, _))),
222-
}
223-
}
224-
225-
fn match_types(lhs: Ty<'_>, rhs: Ty<'_>) -> bool {
226-
match (&lhs.kind(), &rhs.kind()) {
227-
(ty::Bool, ty::Bool)
228-
| (ty::Char, ty::Char)
229-
| (ty::Int(_), ty::Int(_))
230-
| (ty::Uint(_), ty::Uint(_))
231-
| (ty::Str, ty::Str) => true,
232-
(ty::Ref(_, t1, mut1), ty::Ref(_, t2, mut2)) => mut1 == mut2 && match_types(t1, t2),
233-
(ty::Array(t1, _), ty::Array(t2, _)) | (ty::Slice(t1), ty::Slice(t2)) => match_types(t1, t2),
234-
(ty::Adt(def1, _), ty::Adt(def2, _)) => def1 == def2,
235-
(_, _) => false,
188+
fn check_sig<'tcx>(cx: &LateContext<'tcx>, closure_ty: Ty<'tcx>, call_ty: Ty<'tcx>) -> bool {
189+
let call_sig = call_ty.fn_sig(cx.tcx);
190+
if call_sig.unsafety() == Unsafety::Unsafe {
191+
return false;
236192
}
237-
}
238-
239-
fn get_type_name(cx: &LateContext<'_>, ty: Ty<'_>) -> String {
240-
match ty.kind() {
241-
ty::Adt(t, _) => cx.tcx.def_path_str(t.did),
242-
ty::Ref(_, r, _) => get_type_name(cx, r),
243-
_ => ty.to_string(),
193+
if !closure_ty.has_late_bound_regions() {
194+
return true;
244195
}
196+
let substs = match closure_ty.kind() {
197+
ty::Closure(_, substs) => substs,
198+
_ => return false,
199+
};
200+
let closure_sig = cx.tcx.signature_unclosure(substs.as_closure().sig(), Unsafety::Normal);
201+
cx.tcx.erase_late_bound_regions(closure_sig) == cx.tcx.erase_late_bound_regions(call_sig)
245202
}
246203

247-
fn compare_inputs(
248-
closure_inputs: &mut dyn Iterator<Item = &Param<'_>>,
249-
call_args: &mut dyn Iterator<Item = &Expr<'_>>,
250-
) -> bool {
251-
for (closure_input, function_arg) in closure_inputs.zip(call_args) {
252-
if let PatKind::Binding(_, _, ident, _) = closure_input.pat.kind {
253-
// XXXManishearth Should I be checking the binding mode here?
254-
if let ExprKind::Path(QPath::Resolved(None, p)) = function_arg.kind {
255-
if p.segments.len() != 1 {
256-
// If it's a proper path, it can't be a local variable
257-
return false;
258-
}
259-
if p.segments[0].ident.name != ident.name {
260-
// The two idents should be the same
261-
return false;
262-
}
263-
} else {
264-
return false;
204+
fn get_ufcs_type_name(cx: &LateContext<'_>, method_def_id: DefId) -> String {
205+
match cx.tcx.associated_item(method_def_id).container {
206+
ty::TraitContainer(def_id) => cx.tcx.def_path_str(def_id),
207+
ty::ImplContainer(def_id) => {
208+
let ty = cx.tcx.type_of(def_id);
209+
match ty.kind() {
210+
ty::Adt(adt, _) => cx.tcx.def_path_str(adt.did),
211+
_ => ty.to_string(),
265212
}
266-
} else {
267-
return false;
268-
}
213+
},
269214
}
270-
true
271215
}

0 commit comments

Comments
 (0)