Skip to content

Commit

Permalink
Rework redundant_closure
Browse files Browse the repository at this point in the history
* Better track when a early-bound region appears when a late-bound region is required
* Don't lint when the closure gives explicit types.
  • Loading branch information
Jarcho committed Jun 9, 2023
1 parent 476efe9 commit 0395535
Show file tree
Hide file tree
Showing 9 changed files with 477 additions and 222 deletions.
4 changes: 2 additions & 2 deletions clippy_lints/src/derive.rs
Original file line number Diff line number Diff line change
Expand Up @@ -470,12 +470,12 @@ fn check_partial_eq_without_eq<'tcx>(cx: &LateContext<'tcx>, span: Span, trait_r
if let Some(def_id) = trait_ref.trait_def_id();
if cx.tcx.is_diagnostic_item(sym::PartialEq, def_id);
let param_env = param_env_for_derived_eq(cx.tcx, adt.did(), eq_trait_def_id);
if !implements_trait_with_env(cx.tcx, param_env, ty, eq_trait_def_id, []);
if !implements_trait_with_env(cx.tcx, param_env, ty, eq_trait_def_id, &[]);
// If all of our fields implement `Eq`, we can implement `Eq` too
if adt
.all_fields()
.map(|f| f.ty(cx.tcx, substs))
.all(|ty| implements_trait_with_env(cx.tcx, param_env, ty, eq_trait_def_id, []));
.all(|ty| implements_trait_with_env(cx.tcx, param_env, ty, eq_trait_def_id, &[]));
then {
span_lint_and_sugg(
cx,
Expand Down
343 changes: 214 additions & 129 deletions clippy_lints/src/eta_reduction.rs

Large diffs are not rendered by default.

10 changes: 8 additions & 2 deletions clippy_lints/src/needless_pass_by_value.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ use clippy_utils::diagnostics::{multispan_sugg, span_lint_and_then};
use clippy_utils::ptr::get_spans;
use clippy_utils::source::{snippet, snippet_opt};
use clippy_utils::ty::{
implements_trait, implements_trait_with_env, is_copy, is_type_diagnostic_item, is_type_lang_item,
implements_trait, implements_trait_with_env_from_iter, is_copy, is_type_diagnostic_item, is_type_lang_item,
};
use clippy_utils::{get_trait_def_id, is_self, paths};
use if_chain::if_chain;
Expand Down Expand Up @@ -189,7 +189,13 @@ impl<'tcx> LateLintPass<'tcx> for NeedlessPassByValue {
if !ty.is_mutable_ptr();
if !is_copy(cx, ty);
if ty.is_sized(cx.tcx, cx.param_env);
if !allowed_traits.iter().any(|&t| implements_trait_with_env(cx.tcx, cx.param_env, ty, t, [None]));
if !allowed_traits.iter().any(|&t| implements_trait_with_env_from_iter(
cx.tcx,
cx.param_env,
ty,
t,
[Option::<ty::GenericArg<'tcx>>::None],
));
if !implements_borrow_trait;
if !all_borrowable_trait;

Expand Down
169 changes: 103 additions & 66 deletions clippy_utils/src/ty.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
#![allow(clippy::module_name_repetitions)]

use core::ops::ControlFlow;
use itertools::Itertools;
use rustc_ast::ast::Mutability;
use rustc_data_structures::fx::{FxHashMap, FxHashSet};
use rustc_hir as hir;
Expand All @@ -15,17 +16,19 @@ use rustc_infer::infer::{
};
use rustc_lint::LateContext;
use rustc_middle::mir::interpret::{ConstValue, Scalar};
use rustc_middle::traits::EvaluationResult;
use rustc_middle::ty::{
self, layout::ValidityRequirement, AdtDef, AliasTy, AssocKind, Binder, BoundRegion, FnSig, IntTy, List, ParamEnv,
Predicate, PredicateKind, Region, RegionKind, SubstsRef, Ty, TyCtxt, TypeSuperVisitable, TypeVisitable,
TypeVisitableExt, TypeVisitor, UintTy, VariantDef, VariantDiscr,
self, layout::ValidityRequirement, AdtDef, AliasTy, AssocKind, Binder, BoundRegion, FnSig, GenericArg,
GenericArgKind, GenericParamDefKind, IntTy, List, ParamEnv, Predicate, PredicateKind, Region, RegionKind,
SubstsRef, ToPredicate, TraitRef, Ty, TyCtxt, TypeSuperVisitable, TypeVisitable, TypeVisitableExt, TypeVisitor,
UintTy, VariantDef, VariantDiscr,
};
use rustc_middle::ty::{GenericArg, GenericArgKind};
use rustc_span::symbol::Ident;
use rustc_span::{sym, Span, Symbol, DUMMY_SP};
use rustc_target::abi::{Size, VariantIdx};
use rustc_trait_selection::infer::InferCtxtExt;
use rustc_trait_selection::traits::query::evaluate_obligation::InferCtxtExt as _;
use rustc_trait_selection::traits::query::normalize::QueryNormalizeExt;
use rustc_trait_selection::traits::{Obligation, ObligationCause};
use std::iter;

use crate::{match_def_path, path_res, paths};
Expand Down Expand Up @@ -206,15 +209,9 @@ pub fn implements_trait<'tcx>(
cx: &LateContext<'tcx>,
ty: Ty<'tcx>,
trait_id: DefId,
ty_params: &[GenericArg<'tcx>],
substs: &[GenericArg<'tcx>],
) -> bool {
implements_trait_with_env(
cx.tcx,
cx.param_env,
ty,
trait_id,
ty_params.iter().map(|&arg| Some(arg)),
)
implements_trait_with_env_from_iter(cx.tcx, cx.param_env, ty, trait_id, substs.iter().map(|&x| Some(x)))
}

/// Same as `implements_trait` but allows using a `ParamEnv` different from the lint context.
Expand All @@ -223,7 +220,18 @@ pub fn implements_trait_with_env<'tcx>(
param_env: ParamEnv<'tcx>,
ty: Ty<'tcx>,
trait_id: DefId,
ty_params: impl IntoIterator<Item = Option<GenericArg<'tcx>>>,
substs: &[GenericArg<'tcx>],
) -> bool {
implements_trait_with_env_from_iter(tcx, param_env, ty, trait_id, substs.iter().map(|&x| Some(x)))
}

/// Same as `implements_trait_from_env` but takes the substitutions as an iterator.
pub fn implements_trait_with_env_from_iter<'tcx>(
tcx: TyCtxt<'tcx>,
param_env: ParamEnv<'tcx>,
ty: Ty<'tcx>,
trait_id: DefId,
substs: impl IntoIterator<Item = impl Into<Option<GenericArg<'tcx>>>>,
) -> bool {
// Clippy shouldn't have infer types
assert!(!ty.has_infer());
Expand All @@ -232,19 +240,37 @@ pub fn implements_trait_with_env<'tcx>(
if ty.has_escaping_bound_vars() {
return false;
}

let infcx = tcx.infer_ctxt().build();
let orig = TypeVariableOrigin {
kind: TypeVariableOriginKind::MiscVariable,
span: DUMMY_SP,
};
let ty_params = tcx.mk_substs_from_iter(
ty_params
let trait_ref = TraitRef::new(
tcx,
trait_id,
Some(GenericArg::from(ty))
.into_iter()
.map(|arg| arg.unwrap_or_else(|| infcx.next_ty_var(orig).into())),
.chain(substs.into_iter().map(|subst| {
subst.into().unwrap_or_else(|| {
let orig = TypeVariableOrigin {
kind: TypeVariableOriginKind::MiscVariable,
span: DUMMY_SP,
};
infcx.next_ty_var(orig).into()
})
})),
);

debug_assert_eq!(tcx.def_kind(trait_id), DefKind::Trait);
#[cfg(debug_assertions)]
assert_substs_match(tcx, trait_id, trait_ref.substs);

let obligation = Obligation {
cause: ObligationCause::dummy(),
param_env,
recursion_depth: 0,
predicate: ty::Binder::dummy(trait_ref).without_const().to_predicate(tcx),
};
infcx
.type_implements_trait(trait_id, [ty.into()].into_iter().chain(ty_params), param_env)
.must_apply_modulo_regions()
.evaluate_obligation(&obligation)
.is_ok_and(EvaluationResult::must_apply_modulo_regions)
}

/// Checks whether this type implements `Drop`.
Expand Down Expand Up @@ -392,6 +418,11 @@ pub fn is_type_lang_item(cx: &LateContext<'_>, ty: Ty<'_>, lang_item: hir::LangI
}
}

/// Gets the diagnostic name of the type, if it has one
pub fn type_diagnostic_name(cx: &LateContext<'_>, ty: Ty<'_>) -> Option<Symbol> {
ty.ty_adt_def().and_then(|adt| cx.tcx.get_diagnostic_name(adt.did()))
}

/// Return `true` if the passed `typ` is `isize` or `usize`.
pub fn is_isize_or_usize(typ: Ty<'_>) -> bool {
matches!(typ.kind(), ty::Int(IntTy::Isize) | ty::Uint(UintTy::Usize))
Expand Down Expand Up @@ -1016,6 +1047,54 @@ pub fn approx_ty_size<'tcx>(cx: &LateContext<'tcx>, ty: Ty<'tcx>) -> u64 {
}
}

/// Asserts that the given substitutions matches the generic parameters of the given item.
#[allow(dead_code)]
fn assert_substs_match<'tcx>(tcx: TyCtxt<'tcx>, did: DefId, substs: &[GenericArg<'tcx>]) {
let g = tcx.generics_of(did);
let parent = g.parent.map(|did| tcx.generics_of(did));
let count = g.parent_count + g.params.len();
let params = parent
.map_or([].as_slice(), |p| p.params.as_slice())
.iter()
.chain(&g.params)
.map(|x| &x.kind);

assert!(
count == substs.len(),
"wrong number substs for `{did:?}`: expected `{count}`, found {}\n\
note: the expected parameters are: `[{}]`\n\
the given substs are: `{substs:#?}`",
substs.len(),
params.clone().map(GenericParamDefKind::descr).format(", "),
);

if let Some((idx, (param, arg))) =
params
.clone()
.zip(substs.iter().map(|&x| x.unpack()))
.enumerate()
.find(|(_, (param, arg))| match (param, arg) {
(GenericParamDefKind::Lifetime, GenericArgKind::Lifetime(_))
| (GenericParamDefKind::Type { .. }, GenericArgKind::Type(_))
| (GenericParamDefKind::Const { .. }, GenericArgKind::Const(_)) => false,
(
GenericParamDefKind::Lifetime
| GenericParamDefKind::Type { .. }
| GenericParamDefKind::Const { .. },
_,
) => true,
})
{
panic!(
"incorrect subst for `{did:?}` at index `{idx}`: expected a {}, found `{arg:?}`\n\
note: the expected parameters are `[{}]`\n\
the given arguments are `{substs:#?}`",
param.descr(),
params.clone().map(GenericParamDefKind::descr).format(", "),
);
}
}

/// Makes the projection type for the named associated type in the given impl or trait impl.
///
/// This function is for associated types which are "known" to exist, and as such, will only return
Expand Down Expand Up @@ -1043,49 +1122,7 @@ pub fn make_projection<'tcx>(
return None;
};
#[cfg(debug_assertions)]
{
let generics = tcx.generics_of(assoc_item.def_id);
let generic_count = generics.parent_count + generics.params.len();
let params = generics
.parent
.map_or([].as_slice(), |id| &*tcx.generics_of(id).params)
.iter()
.chain(&generics.params)
.map(|x| &x.kind);

debug_assert!(
generic_count == substs.len(),
"wrong number of substs for `{:?}`: found `{}` expected `{generic_count}`.\n\
note: the expected parameters are: {:#?}\n\
the given arguments are: `{substs:#?}`",
assoc_item.def_id,
substs.len(),
params.map(ty::GenericParamDefKind::descr).collect::<Vec<_>>(),
);

if let Some((idx, (param, arg))) = params
.clone()
.zip(substs.iter().map(GenericArg::unpack))
.enumerate()
.find(|(_, (param, arg))| {
!matches!(
(param, arg),
(ty::GenericParamDefKind::Lifetime, GenericArgKind::Lifetime(_))
| (ty::GenericParamDefKind::Type { .. }, GenericArgKind::Type(_))
| (ty::GenericParamDefKind::Const { .. }, GenericArgKind::Const(_))
)
})
{
debug_assert!(
false,
"mismatched subst type at index {idx}: expected a {}, found `{arg:?}`\n\
note: the expected parameters are {:#?}\n\
the given arguments are {substs:#?}",
param.descr(),
params.map(ty::GenericParamDefKind::descr).collect::<Vec<_>>()
);
}
}
assert_substs_match(tcx, assoc_item.def_id, substs);

Some(tcx.mk_alias_ty(assoc_item.def_id, substs))
}
Expand Down
45 changes: 23 additions & 22 deletions clippy_utils/src/usage.rs
Original file line number Diff line number Diff line change
@@ -1,10 +1,11 @@
use crate as utils;
use crate::visitors::{for_each_expr, for_each_expr_with_closures, Descend};
use crate::get_enclosing_loop_or_multi_call_closure;
use crate::visitors::{for_each_expr, for_each_expr_with_closures, Descend, Visitable};
use core::ops::ControlFlow;
use rustc_hir as hir;
use rustc_hir::intravisit::{self, Visitor};
use rustc_hir::HirIdSet;
use rustc_hir::{Expr, ExprKind, HirId, Node};
use rustc_hir::{Expr, ExprKind, HirId};
use rustc_hir_typeck::expr_use_visitor::{Delegate, ExprUseVisitor, PlaceBase, PlaceWithHirId};
use rustc_infer::infer::TyCtxtInferExt;
use rustc_lint::LateContext;
Expand Down Expand Up @@ -155,6 +156,17 @@ pub fn contains_return_break_continue_macro(expression: &Expr<'_>) -> bool {
.is_some()
}

pub fn local_used_in<'tcx>(cx: &LateContext<'tcx>, local_id: HirId, v: impl Visitable<'tcx>) -> bool {
for_each_expr_with_closures(cx, v, |e| {
if utils::path_to_local_id(e, local_id) {
ControlFlow::Break(())
} else {
ControlFlow::Continue(())
}
})
.is_some()
}

pub fn local_used_after_expr(cx: &LateContext<'_>, local_id: HirId, after: &Expr<'_>) -> bool {
let Some(block) = utils::get_enclosing_block(cx, local_id) else { return false };

Expand All @@ -165,32 +177,21 @@ pub fn local_used_after_expr(cx: &LateContext<'_>, local_id: HirId, after: &Expr
// let closure = || local;
// closure();
// closure();
let in_loop_or_closure = cx
.tcx
.hir()
.parent_iter(after.hir_id)
.take_while(|&(id, _)| id != block.hir_id)
.any(|(_, node)| {
matches!(
node,
Node::Expr(Expr {
kind: ExprKind::Loop(..) | ExprKind::Closure { .. },
..
})
)
});
if in_loop_or_closure {
return true;
}
let loop_start = get_enclosing_loop_or_multi_call_closure(cx, after).map(|e| e.hir_id);

let mut past_expr = false;
for_each_expr_with_closures(cx, block, |e| {
if e.hir_id == after.hir_id {
if past_expr {
if utils::path_to_local_id(e, local_id) {
ControlFlow::Break(())
} else {
ControlFlow::Continue(Descend::Yes)
}
} else if e.hir_id == after.hir_id {
past_expr = true;
ControlFlow::Continue(Descend::No)
} else if past_expr && utils::path_to_local_id(e, local_id) {
ControlFlow::Break(())
} else {
past_expr = Some(e.hir_id) == loop_start;
ControlFlow::Continue(Descend::Yes)
}
})
Expand Down
10 changes: 10 additions & 0 deletions clippy_utils/src/visitors.rs
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,16 @@ pub trait Visitable<'tcx> {
/// Calls the corresponding `visit_*` function on the visitor.
fn visit<V: Visitor<'tcx>>(self, visitor: &mut V);
}
impl<'tcx, T> Visitable<'tcx> for &'tcx [T]
where
&'tcx T: Visitable<'tcx>,
{
fn visit<V: Visitor<'tcx>>(self, visitor: &mut V) {
for x in self {
x.visit(visitor);
}
}
}
macro_rules! visitable_ref {
($t:ident, $f:ident) => {
impl<'tcx> Visitable<'tcx> for &'tcx $t<'tcx> {
Expand Down
Loading

0 comments on commit 0395535

Please sign in to comment.