Skip to content

Commit

Permalink
Auto merge of rust-lang#13108 - tesuji:fix_redundant_closure, r=xFrednet
Browse files Browse the repository at this point in the history 
Fix `redundant_closure` false positive with closures has return type contains  `'static`

Fix rust-lang#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`
  • Loading branch information
@bors
bors committed Aug 1, 2024
2 parents 5542309 + 0bc9f00 commit 2fc74a3
Show file tree
Hide file tree
Showing 4 changed files with 250 additions and 185 deletions.
327 changes: 176 additions & 151 deletions clippy_lints/src/eta_reduction.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,8 +9,7 @@ use rustc_hir::{BindingMode, Expr, ExprKind, FnRetTy, Param, PatKind, QPath, Saf
use rustc_infer::infer::TyCtxtInferExt;
use rustc_lint::{LateContext, LateLintPass};
use rustc_middle::ty::{
self, Binder, ClosureArgs, ClosureKind, FnSig, GenericArg, GenericArgKind, List, Region, RegionKind, Ty, TyCtxt,
TypeVisitableExt, TypeckResults,
self, Binder, ClosureKind, FnSig, GenericArg, GenericArgKind, List, Region, Ty, TypeVisitableExt, TypeckResults,
};
use rustc_session::declare_lint_pass;
use rustc_span::symbol::sym;
Expand Down Expand Up @@ -74,159 +73,184 @@ declare_clippy_lint! {
declare_lint_pass!(EtaReduction => [REDUNDANT_CLOSURE, REDUNDANT_CLOSURE_FOR_METHOD_CALLS]);

impl<'tcx> LateLintPass<'tcx> for EtaReduction {
#[allow(clippy::too_many_lines)]
fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) {
let body = if let ExprKind::Closure(c) = expr.kind
&& c.fn_decl.inputs.iter().all(|ty| matches!(ty.kind, TyKind::Infer))
&& matches!(c.fn_decl.output, FnRetTy::DefaultReturn(_))
&& !expr.span.from_expansion()
{
cx.tcx.hir().body(c.body)
} else {
return;
};

if body.value.span.from_expansion() {
if body.params.is_empty() {
if let Some(VecArgs::Vec(&[])) = VecArgs::hir(cx, body.value) {
// replace `|| vec![]` with `Vec::new`
span_lint_and_sugg(
cx,
REDUNDANT_CLOSURE,
expr.span,
"redundant closure",
"replace the closure with `Vec::new`",
"std::vec::Vec::new".into(),
Applicability::MachineApplicable,
);
}
fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &Expr<'tcx>) {
if let ExprKind::MethodCall(_method, receiver, args, _) = expr.kind {
for arg in args {
check_clousure(cx, Some(receiver), arg);
}
// skip `foo(|| macro!())`
return;
}
if let ExprKind::Call(func, args) = expr.kind {
check_clousure(cx, None, func);
for arg in args {
check_clousure(cx, None, arg);
}
}
}
}

let typeck = cx.typeck_results();
let closure = if let ty::Closure(_, closure_subs) = typeck.expr_ty(expr).kind() {
closure_subs.as_closure()
} else {
return;
};
#[allow(clippy::too_many_lines)]
fn check_clousure<'tcx>(cx: &LateContext<'tcx>, outer_receiver: Option<&Expr<'tcx>>, expr: &Expr<'tcx>) {
let body = if let ExprKind::Closure(c) = expr.kind
&& c.fn_decl.inputs.iter().all(|ty| matches!(ty.kind, TyKind::Infer))
&& matches!(c.fn_decl.output, FnRetTy::DefaultReturn(_))
&& !expr.span.from_expansion()
{
cx.tcx.hir().body(c.body)
} else {
return;
};

if is_adjusted(cx, body.value) {
return;
if body.value.span.from_expansion() {
if body.params.is_empty() {
if let Some(VecArgs::Vec(&[])) = VecArgs::hir(cx, body.value) {
// replace `|| vec![]` with `Vec::new`
span_lint_and_sugg(
cx,
REDUNDANT_CLOSURE,
expr.span,
"redundant closure",
"replace the closure with `Vec::new`",
"std::vec::Vec::new".into(),
Applicability::MachineApplicable,
);
}
}
// skip `foo(|| macro!())`
return;
}

match body.value.kind {
ExprKind::Call(callee, args)
if matches!(
callee.kind,
ExprKind::Path(QPath::Resolved(..) | QPath::TypeRelative(..))
) =>
if is_adjusted(cx, body.value) {
return;
}

let typeck = cx.typeck_results();
let closure = if let ty::Closure(_, closure_subs) = typeck.expr_ty(expr).kind() {
closure_subs.as_closure()
} else {
return;
};
let closure_sig = cx.tcx.signature_unclosure(closure.sig(), Safety::Safe).skip_binder();
match body.value.kind {
ExprKind::Call(callee, args)
if matches!(
callee.kind,
ExprKind::Path(QPath::Resolved(..) | QPath::TypeRelative(..))
) =>
{
let callee_ty_raw = typeck.expr_ty(callee);
let callee_ty = callee_ty_raw.peel_refs();
if matches!(type_diagnostic_name(cx, callee_ty), Some(sym::Arc | sym::Rc))
|| !check_inputs(typeck, body.params, None, args)
{
let callee_ty_raw = typeck.expr_ty(callee);
let callee_ty = callee_ty_raw.peel_refs();
if matches!(type_diagnostic_name(cx, callee_ty), Some(sym::Arc | sym::Rc))
|| !check_inputs(typeck, body.params, None, args)
{
return;
}
let callee_ty_adjusted = typeck
.expr_adjustments(callee)
.last()
.map_or(callee_ty, |a| a.target.peel_refs());
return;
}
let callee_ty_adjusted = typeck
.expr_adjustments(callee)
.last()
.map_or(callee_ty, |a| a.target.peel_refs());

let sig = match callee_ty_adjusted.kind() {
ty::FnDef(def, _) => {
// Rewriting `x(|| f())` to `x(f)` where f is marked `#[track_caller]` moves the `Location`
if cx.tcx.has_attr(*def, sym::track_caller) {
return;
}
let sig = match callee_ty_adjusted.kind() {
ty::FnDef(def, _) => {
// Rewriting `x(|| f())` to `x(f)` where f is marked `#[track_caller]` moves the `Location`
if cx.tcx.has_attr(*def, sym::track_caller) {
return;
}

cx.tcx.fn_sig(def).skip_binder().skip_binder()
},
ty::FnPtr(sig) => sig.skip_binder(),
ty::Closure(_, subs) => cx
.tcx
.signature_unclosure(subs.as_closure().sig(), Safety::Safe)
.skip_binder(),
_ => {
if typeck.type_dependent_def_id(body.value.hir_id).is_some()
&& let subs = typeck.node_args(body.value.hir_id)
&& let output = typeck.expr_ty(body.value)
&& let ty::Tuple(tys) = *subs.type_at(1).kind()
{
cx.tcx.mk_fn_sig(tys, output, false, Safety::Safe, Abi::Rust)
} else {
return;
}
},
};
if check_sig(cx, closure, sig)
&& let generic_args = typeck.node_args(callee.hir_id)
// Given some trait fn `fn f() -> ()` and some type `T: Trait`, `T::f` is not
// `'static` unless `T: 'static`. The cast `T::f as fn()` will, however, result
// in a type which is `'static`.
// For now ignore all callee types which reference a type parameter.
&& !generic_args.types().any(|t| matches!(t.kind(), ty::Param(_)))
{
span_lint_and_then(cx, REDUNDANT_CLOSURE, expr.span, "redundant closure", |diag| {
if let Some(mut snippet) = snippet_opt(cx, callee.span) {
if path_to_local(callee).map_or(false, |l| {
// FIXME: Do we really need this `local_used_in` check?
// Isn't it checking something like... `callee(callee)`?
// If somehow this check is needed, add some test for it,
// 'cuz currently nothing changes after deleting this check.
local_used_in(cx, l, args) || local_used_after_expr(cx, l, expr)
}) {
match cx.tcx.infer_ctxt().build().err_ctxt().type_implements_fn_trait(
cx.param_env,
Binder::bind_with_vars(callee_ty_adjusted, List::empty()),
ty::PredicatePolarity::Positive,
) {
// Mutable closure is used after current expr; we cannot consume it.
Ok((ClosureKind::FnMut, _)) => snippet = format!("&mut {snippet}"),
Ok((ClosureKind::Fn, _)) if !callee_ty_raw.is_ref() => {
snippet = format!("&{snippet}");
},
_ => (),
}
cx.tcx.fn_sig(def).skip_binder().skip_binder()
},
ty::FnPtr(sig) => sig.skip_binder(),
ty::Closure(_, subs) => cx
.tcx
.signature_unclosure(subs.as_closure().sig(), Safety::Safe)
.skip_binder(),
_ => {
if typeck.type_dependent_def_id(body.value.hir_id).is_some()
&& let subs = typeck.node_args(body.value.hir_id)
&& let output = typeck.expr_ty(body.value)
&& let ty::Tuple(tys) = *subs.type_at(1).kind()
{
cx.tcx.mk_fn_sig(tys, output, false, Safety::Safe, Abi::Rust)
} else {
return;
}
},
};
if let Some(outer) = outer_receiver
&& ty_has_static(sig.output())
&& let generic_args = typeck.node_args(outer.hir_id)
// HACK: Given a closure in `T.method(|| f())`, where `fn f() -> U where U: 'static`, `T.method(f)`
// will succeed iff `T: 'static`. But the region of `T` is always erased by `typeck.expr_ty()` when
// T is a generic type. For example, return type of `Option<String>::as_deref()` is a generic.
// So we have a hack like this.
&& generic_args.len() > 0
{
return;
}
if check_sig(closure_sig, sig)
&& let generic_args = typeck.node_args(callee.hir_id)
// Given some trait fn `fn f() -> ()` and some type `T: Trait`, `T::f` is not
// `'static` unless `T: 'static`. The cast `T::f as fn()` will, however, result
// in a type which is `'static`.
// For now ignore all callee types which reference a type parameter.
&& !generic_args.types().any(|t| matches!(t.kind(), ty::Param(_)))
{
span_lint_and_then(cx, REDUNDANT_CLOSURE, expr.span, "redundant closure", |diag| {
if let Some(mut snippet) = snippet_opt(cx, callee.span) {
if path_to_local(callee).map_or(false, |l| {
// FIXME: Do we really need this `local_used_in` check?
// Isn't it checking something like... `callee(callee)`?
// If somehow this check is needed, add some test for it,
// 'cuz currently nothing changes after deleting this check.
local_used_in(cx, l, args) || local_used_after_expr(cx, l, expr)
}) {
match cx.tcx.infer_ctxt().build().err_ctxt().type_implements_fn_trait(
cx.param_env,
Binder::bind_with_vars(callee_ty_adjusted, List::empty()),
ty::PredicatePolarity::Positive,
) {
// Mutable closure is used after current expr; we cannot consume it.
Ok((ClosureKind::FnMut, _)) => snippet = format!("&mut {snippet}"),
Ok((ClosureKind::Fn, _)) if !callee_ty_raw.is_ref() => {
snippet = format!("&{snippet}");
},
_ => (),
}
diag.span_suggestion(
expr.span,
"replace the closure with the function itself",
snippet,
Applicability::MachineApplicable,
);
}
});
}
},
ExprKind::MethodCall(path, self_, args, _) if check_inputs(typeck, body.params, Some(self_), args) => {
if let Some(method_def_id) = typeck.type_dependent_def_id(body.value.hir_id)
&& !cx.tcx.has_attr(method_def_id, sym::track_caller)
&& check_sig(cx, closure, cx.tcx.fn_sig(method_def_id).skip_binder().skip_binder())
{
span_lint_and_then(
cx,
REDUNDANT_CLOSURE_FOR_METHOD_CALLS,
expr.span,
"redundant closure",
|diag| {
let args = typeck.node_args(body.value.hir_id);
let caller = self_.hir_id.owner.def_id;
let type_name = get_path_from_caller_to_method_type(cx.tcx, caller, method_def_id, args);
diag.span_suggestion(
expr.span,
"replace the closure with the method itself",
format!("{}::{}", type_name, path.ident.name),
Applicability::MachineApplicable,
);
},
);
}
},
_ => (),
}
diag.span_suggestion(
expr.span,
"replace the closure with the function itself",
snippet,
Applicability::MachineApplicable,
);
}
});
}
},
ExprKind::MethodCall(path, self_, args, _) if check_inputs(typeck, body.params, Some(self_), args) => {
if let Some(method_def_id) = typeck.type_dependent_def_id(body.value.hir_id)
&& !cx.tcx.has_attr(method_def_id, sym::track_caller)
&& check_sig(closure_sig, cx.tcx.fn_sig(method_def_id).skip_binder().skip_binder())
{
span_lint_and_then(
cx,
REDUNDANT_CLOSURE_FOR_METHOD_CALLS,
expr.span,
"redundant closure",
|diag| {
let args = typeck.node_args(body.value.hir_id);
let caller = self_.hir_id.owner.def_id;
let type_name = get_path_from_caller_to_method_type(cx.tcx, caller, method_def_id, args);
diag.span_suggestion(
expr.span,
"replace the closure with the method itself",
format!("{}::{}", type_name, path.ident.name),
Applicability::MachineApplicable,
);
},
);
}
},
_ => (),
}
}

Expand All @@ -251,12 +275,8 @@ fn check_inputs(
})
}

fn check_sig<'tcx>(cx: &LateContext<'tcx>, closure: ClosureArgs<TyCtxt<'tcx>>, call_sig: FnSig<'_>) -> bool {
call_sig.safety == Safety::Safe
&& !has_late_bound_to_non_late_bound_regions(
cx.tcx.signature_unclosure(closure.sig(), Safety::Safe).skip_binder(),
call_sig,
)
fn check_sig<'tcx>(closure_sig: FnSig<'tcx>, call_sig: FnSig<'tcx>) -> bool {
call_sig.safety == Safety::Safe && !has_late_bound_to_non_late_bound_regions(closure_sig, call_sig)
}

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

fn check_subs(from_subs: &[GenericArg<'_>], to_subs: &[GenericArg<'_>]) -> bool {
Expand Down Expand Up @@ -318,3 +338,8 @@ fn has_late_bound_to_non_late_bound_regions(from_sig: FnSig<'_>, to_sig: FnSig<'
.zip(to_sig.inputs_and_output)
.any(|(from_ty, to_ty)| check_ty(from_ty, to_ty))
}

fn ty_has_static(ty: Ty<'_>) -> bool {
ty.walk()
.any(|arg| matches!(arg.unpack(), GenericArgKind::Lifetime(re) if re.is_static()))
}
17 changes: 17 additions & 0 deletions tests/ui/eta.fixed
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
#![allow(unused)]
#![allow(
clippy::needless_borrow,
clippy::needless_option_as_deref,
clippy::needless_pass_by_value,
clippy::no_effect,
clippy::option_map_unit_fn,
Expand Down Expand Up @@ -482,3 +483,19 @@ mod issue_12853 {
x();
}
}

mod issue_13073 {
fn get_default() -> Option<&'static str> {
Some("foo")
}

pub fn foo() {
// shouldn't lint
let bind: Option<String> = None;
let _field = bind.as_deref().or_else(|| get_default()).unwrap();
let bind: Option<&'static str> = None;
let _field = bind.as_deref().or_else(|| get_default()).unwrap();
// should lint
let _field = bind.or_else(get_default).unwrap();
}
}
Loading

0 comments on commit 2fc74a3

Please sign in to comment.