mikros/rust
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

Auto merge of #8422 - buttercrab:only_used_in_recursion, r=llogiq

Browse Source 
new lint: `only_used_in_recursion`

changed:
- added `only_used_in_recursion`.
- fixed code that variables are only used in recursion.
- this would not lint when `unused_variable`

This fixes: #8390

-----

changelog: add lint [`only_used_in_recursion`]
This commit is contained in:
bors 2022-03-13 16:11:25 +00:00
commit e2e492c10e
12 changed files with 903 additions and 39 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

1
CHANGELOG.md
View File

@ -3356,6 +3356,7 @@ Released 2018-09-13
[`not_unsafe_ptr_arg_deref`]: https://rust-lang.github.io/rust-clippy/master/index.html#not_unsafe_ptr_arg_deref
[`octal_escapes`]: https://rust-lang.github.io/rust-clippy/master/index.html#octal_escapes
[`ok_expect`]: https://rust-lang.github.io/rust-clippy/master/index.html#ok_expect
[`only_used_in_recursion`]: https://rust-lang.github.io/rust-clippy/master/index.html#only_used_in_recursion
[`op_ref`]: https://rust-lang.github.io/rust-clippy/master/index.html#op_ref
[`option_as_ref_deref`]: https://rust-lang.github.io/rust-clippy/master/index.html#option_as_ref_deref
[`option_env_unwrap`]: https://rust-lang.github.io/rust-clippy/master/index.html#option_env_unwrap

1
clippy_lints/src/lib.register_all.rs
View File

@ -236,6 +236,7 @@ store.register_group(true, "clippy::all", Some("clippy_all"), vec![
LintId::of(non_expressive_names::JUST_UNDERSCORES_AND_DIGITS),
LintId::of(non_octal_unix_permissions::NON_OCTAL_UNIX_PERMISSIONS),
LintId::of(octal_escapes::OCTAL_ESCAPES),
LintId::of(only_used_in_recursion::ONLY_USED_IN_RECURSION),
LintId::of(open_options::NONSENSICAL_OPEN_OPTIONS),
LintId::of(option_env_unwrap::OPTION_ENV_UNWRAP),
LintId::of(overflow_check_conditional::OVERFLOW_CHECK_CONDITIONAL),

1
clippy_lints/src/lib.register_complexity.rs
View File

@ -65,6 +65,7 @@ store.register_group(true, "clippy::complexity", Some("clippy_complexity"), vec!
LintId::of(neg_cmp_op_on_partial_ord::NEG_CMP_OP_ON_PARTIAL_ORD),
LintId::of(no_effect::NO_EFFECT),
LintId::of(no_effect::UNNECESSARY_OPERATION),
LintId::of(only_used_in_recursion::ONLY_USED_IN_RECURSION),
LintId::of(overflow_check_conditional::OVERFLOW_CHECK_CONDITIONAL),
LintId::of(partialeq_ne_impl::PARTIALEQ_NE_IMPL),
LintId::of(precedence::PRECEDENCE),

1
clippy_lints/src/lib.register_lints.rs
View File

@ -400,6 +400,7 @@ store.register_lints(&[
non_send_fields_in_send_ty::NON_SEND_FIELDS_IN_SEND_TY,
nonstandard_macro_braces::NONSTANDARD_MACRO_BRACES,
octal_escapes::OCTAL_ESCAPES,
only_used_in_recursion::ONLY_USED_IN_RECURSION,
open_options::NONSENSICAL_OPEN_OPTIONS,
option_env_unwrap::OPTION_ENV_UNWRAP,
option_if_let_else::OPTION_IF_LET_ELSE,

2
clippy_lints/src/lib.rs
View File

@ -318,6 +318,7 @@ mod non_octal_unix_permissions;
mod non_send_fields_in_send_ty;
mod nonstandard_macro_braces;
mod octal_escapes;
mod only_used_in_recursion;
mod open_options;
mod option_env_unwrap;
mod option_if_let_else;
@ -857,6 +858,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
store.register_late_pass(move || Box::new(borrow_as_ptr::BorrowAsPtr::new(msrv)));
store.register_late_pass(move || Box::new(manual_bits::ManualBits::new(msrv)));
store.register_late_pass(|| Box::new(default_union_representation::DefaultUnionRepresentation));
store.register_late_pass(|| Box::new(only_used_in_recursion::OnlyUsedInRecursion));
store.register_late_pass(|| Box::new(dbg_macro::DbgMacro));
let cargo_ignore_publish = conf.cargo_ignore_publish;
store.register_late_pass(move || {

9
clippy_lints/src/methods/option_map_or_none.rs
View File

@ -14,10 +14,7 @@ use super::RESULT_MAP_OR_INTO_OPTION;
// The expression inside a closure may or may not have surrounding braces
// which causes problems when generating a suggestion.
fn reduce_unit_expression<'a>(
cx: &LateContext<'_>,
expr: &'a hir::Expr<'_>,
) -> Option<(&'a hir::Expr<'a>, &'a [hir::Expr<'a>])> {
fn reduce_unit_expression<'a>(expr: &'a hir::Expr<'_>) -> Option<(&'a hir::Expr<'a>, &'a [hir::Expr<'a>])> {
match expr.kind {
hir::ExprKind::Call(func, arg_char) => Some((func, arg_char)),
hir::ExprKind::Block(block, _) => {
@ -25,7 +22,7 @@ fn reduce_unit_expression<'a>(
(&[], Some(inner_expr)) => {
// If block only contains an expression,
// reduce `|x| { x + 1 }` to `|x| x + 1`
reduce_unit_expression(cx, inner_expr)
reduce_unit_expression(inner_expr)
},
_ => None,
}
@ -77,7 +74,7 @@ pub(super) fn check<'tcx>(
if let hir::ExprKind::Closure(_, _, id, span, _) = map_arg.kind;
let arg_snippet = snippet(cx, span, "..");
let body = cx.tcx.hir().body(id);
if let Some((func, [arg_char])) = reduce_unit_expression(cx, &body.value);
if let Some((func, [arg_char])) = reduce_unit_expression(&body.value);
if let Some(id) = path_def_id(cx, func).and_then(|ctor_id| cx.tcx.parent(ctor_id));
if Some(id) == cx.tcx.lang_items().option_some_variant();
then {

668
clippy_lints/src/only_used_in_recursion.rs Normal file
View File

@ -0,0 +1,668 @@
use std::collections::VecDeque;
use clippy_utils::diagnostics::span_lint_and_sugg;
use itertools::{izip, Itertools};
use rustc_ast::{walk_list, Label, Mutability};
use rustc_data_structures::fx::{FxHashMap, FxHashSet};
use rustc_errors::Applicability;
use rustc_hir::def::{DefKind, Res};
use rustc_hir::def_id::DefId;
use rustc_hir::definitions::{DefPathData, DisambiguatedDefPathData};
use rustc_hir::intravisit::{walk_expr, FnKind, Visitor};
use rustc_hir::{
Arm, Block, Body, Expr, ExprKind, Guard, HirId, ImplicitSelfKind, Let, Local, Pat, PatKind, Path, PathSegment,
QPath, Stmt, StmtKind, TyKind, UnOp,
};
use rustc_lint::{LateContext, LateLintPass};
use rustc_middle::ty;
use rustc_middle::ty::{Ty, TyCtxt, TypeckResults};
use rustc_session::{declare_lint_pass, declare_tool_lint};
use rustc_span::symbol::kw;
use rustc_span::symbol::Ident;
use rustc_span::Span;
declare_clippy_lint! {
/// ### What it does
/// Checks for arguments that are only used in recursion with no side-effects.
///
/// ### Why is this bad?
/// It could contain a useless calculation and can make function simpler.
///
/// The arguments can be involved in calculations and assignments but as long as
/// the calculations have no side-effects (function calls or mutating dereference)
/// and the assigned variables are also only in recursion, it is useless.
///
/// ### Known problems
/// In some cases, this would not catch all useless arguments.
///
/// ```rust
/// fn foo(a: usize, b: usize) -> usize {
/// let f = |x| x + 1;
///
/// if a == 0 {
/// 1
/// } else {
/// foo(a - 1, f(b))
/// }
/// }
/// ```
///
/// For example, the argument `b` is only used in recursion, but the lint would not catch it.
///
/// List of some examples that can not be caught:
/// - binary operation of non-primitive types
/// - closure usage
/// - some `break` relative operations
/// - struct pattern binding
///
/// Also, when you recurse the function name with path segments, it is not possible to detect.
///
/// ### Example
/// ```rust
/// fn f(a: usize, b: usize) -> usize {
/// if a == 0 {
/// 1
/// } else {
/// f(a - 1, b + 1)
/// }
/// }
/// # fn main() {
/// # print!("{}", f(1, 1));
/// # }
/// ```
/// Use instead:
/// ```rust
/// fn f(a: usize) -> usize {
/// if a == 0 {
/// 1
/// } else {
/// f(a - 1)
/// }
/// }
/// # fn main() {
/// # print!("{}", f(1));
/// # }
/// ```
#[clippy::version = "1.60.0"]
pub ONLY_USED_IN_RECURSION,
complexity,
"arguments that is only used in recursion can be removed"
}
declare_lint_pass!(OnlyUsedInRecursion => [ONLY_USED_IN_RECURSION]);
impl<'tcx> LateLintPass<'tcx> for OnlyUsedInRecursion {
fn check_fn(
&mut self,
cx: &LateContext<'tcx>,
kind: FnKind<'tcx>,
decl: &'tcx rustc_hir::FnDecl<'tcx>,
body: &'tcx Body<'tcx>,
_: Span,
id: HirId,
) {
if let FnKind::ItemFn(ident, ..) | FnKind::Method(ident, ..) = kind {
let def_id = id.owner.to_def_id();
let data = cx.tcx.def_path(def_id).data;
if data.len() > 1 {
match data.get(data.len() - 2) {
Some(DisambiguatedDefPathData {
data: DefPathData::Impl,
disambiguator,
}) if *disambiguator != 0 => return,
_ => {},
}
}
let has_self = !matches!(decl.implicit_self, ImplicitSelfKind::None);
let ty_res = cx.typeck_results();
let param_span = body
.params
.iter()
.flat_map(|param| {
let mut v = Vec::new();
param.pat.each_binding(|_, hir_id, span, ident| {
v.push((hir_id, span, ident));
});
v
})
.skip(if has_self { 1 } else { 0 })
.filter(|(_, _, ident)| !ident.name.as_str().starts_with('_'))
.collect_vec();
let params = body.params.iter().map(|param| param.pat).collect();
let mut visitor = SideEffectVisit {
graph: FxHashMap::default(),
has_side_effect: FxHashSet::default(),
ret_vars: Vec::new(),
contains_side_effect: false,
break_vars: FxHashMap::default(),
params,
fn_ident: ident,
fn_def_id: def_id,
is_method: matches!(kind, FnKind::Method(..)),
has_self,
ty_res,
ty_ctx: cx.tcx,
};
visitor.visit_expr(&body.value);
let vars = std::mem::take(&mut visitor.ret_vars);
// this would set the return variables to side effect
visitor.add_side_effect(vars);
let mut queue = visitor.has_side_effect.iter().copied().collect::<VecDeque<_>>();
// a simple BFS to check all the variables that have side effect
while let Some(id) = queue.pop_front() {
if let Some(next) = visitor.graph.get(&id) {
for i in next {
if !visitor.has_side_effect.contains(i) {
visitor.has_side_effect.insert(*i);
queue.push_back(*i);
}
}
}
}
for (id, span, ident) in param_span {
// if the variable is not used in recursion, it would be marked as unused
if !visitor.has_side_effect.contains(&id) {
let mut queue = VecDeque::new();
let mut visited = FxHashSet::default();
queue.push_back(id);
// a simple BFS to check the graph can reach to itself
// if it can't, it means the variable is never used in recursion
while let Some(id) = queue.pop_front() {
if let Some(next) = visitor.graph.get(&id) {
for i in next {
if !visited.contains(i) {
visited.insert(id);
queue.push_back(*i);
}
}
}
}
if visited.contains(&id) {
span_lint_and_sugg(
cx,
ONLY_USED_IN_RECURSION,
span,
"parameter is only used in recursion",
"if this is intentional, prefix with an underscore",
format!("_{}", ident.name.as_str()),
Applicability::MaybeIncorrect,
);
}
}
}
}
}
}
pub fn is_primitive(ty: Ty<'_>) -> bool {
match ty.kind() {
ty::Bool | ty::Char | ty::Int(_) | ty::Uint(_) | ty::Float(_) | ty::Str => true,
ty::Ref(_, t, _) => is_primitive(*t),
_ => false,
}
}
pub fn is_array(ty: Ty<'_>) -> bool {
match ty.kind() {
ty::Array(..) | ty::Slice(..) => true,
ty::Ref(_, t, _) => is_array(*t),
_ => false,
}
}
/// This builds the graph of side effect.
/// The edge `a -> b` means if `a` has side effect, `b` will have side effect.
///
/// There are some exmaple in following code:
/// ```rust, ignore
/// let b = 1;
/// let a = b; // a -> b
/// let (c, d) = (a, b); // c -> b, d -> b
///
/// let e = if a == 0 { // e -> a
/// c // e -> c
/// } else {
/// d // e -> d
/// };
/// ```
pub struct SideEffectVisit<'tcx> {
graph: FxHashMap<HirId, FxHashSet<HirId>>,
has_side_effect: FxHashSet<HirId>,
// bool for if the variable was dereferenced from mutable reference
ret_vars: Vec<(HirId, bool)>,
contains_side_effect: bool,
// break label
break_vars: FxHashMap<Ident, Vec<(HirId, bool)>>,
params: Vec<&'tcx Pat<'tcx>>,
fn_ident: Ident,
fn_def_id: DefId,
is_method: bool,
has_self: bool,
ty_res: &'tcx TypeckResults<'tcx>,
ty_ctx: TyCtxt<'tcx>,
}
impl<'tcx> Visitor<'tcx> for SideEffectVisit<'tcx> {
fn visit_block(&mut self, b: &'tcx Block<'tcx>) {
b.stmts.iter().for_each(|stmt| {
self.visit_stmt(stmt);
self.ret_vars.clear();
});
walk_list!(self, visit_expr, b.expr);
}
fn visit_stmt(&mut self, s: &'tcx Stmt<'tcx>) {
match s.kind {
StmtKind::Local(Local {
pat, init: Some(init), ..
}) => {
self.visit_pat_expr(pat, init, false);
self.ret_vars.clear();
},
StmtKind::Item(i) => {
let item = self.ty_ctx.hir().item(i);
self.visit_item(item);
self.ret_vars.clear();
},
StmtKind::Expr(e) | StmtKind::Semi(e) => {
self.visit_expr(e);
self.ret_vars.clear();
},
StmtKind::Local(_) => {},
}
}
fn visit_expr(&mut self, ex: &'tcx Expr<'tcx>) {
match ex.kind {
ExprKind::Array(exprs) | ExprKind::Tup(exprs) => {
self.ret_vars = exprs
.iter()
.flat_map(|expr| {
self.visit_expr(expr);
std::mem::take(&mut self.ret_vars)
})
.collect();
},
ExprKind::Call(callee, args) => self.visit_fn(callee, args),
ExprKind::MethodCall(path, args, _) => self.visit_method_call(path, args),
ExprKind::Binary(_, lhs, rhs) => {
self.visit_bin_op(lhs, rhs);
},
ExprKind::Unary(op, expr) => self.visit_un_op(op, expr),
ExprKind::Let(Let { pat, init, .. }) => self.visit_pat_expr(pat, init, false),
ExprKind::If(bind, then_expr, else_expr) => {
self.visit_if(bind, then_expr, else_expr);
},
ExprKind::Match(expr, arms, _) => self.visit_match(expr, arms),
// since analysing the closure is not easy, just set all variables in it to side-effect
ExprKind::Closure(_, _, body_id, _, _) => {
let body = self.ty_ctx.hir().body(body_id);
self.visit_body(body);
let vars = std::mem::take(&mut self.ret_vars);
self.add_side_effect(vars);
},
ExprKind::Loop(block, label, _, _) | ExprKind::Block(block, label) => {
self.visit_block_label(block, label);
},
ExprKind::Assign(bind, expr, _) => {
self.visit_assign(bind, expr);
},
ExprKind::AssignOp(_, bind, expr) => {
self.visit_assign(bind, expr);
self.visit_bin_op(bind, expr);
},
ExprKind::Field(expr, _) => {
self.visit_expr(expr);
if matches!(self.ty_res.expr_ty(expr).kind(), ty::Ref(_, _, Mutability::Mut)) {
self.ret_vars.iter_mut().for_each(|(_, b)| *b = true);
}
},
ExprKind::Index(expr, index) => {
self.visit_expr(expr);
let mut vars = std::mem::take(&mut self.ret_vars);
self.visit_expr(index);
self.ret_vars.append(&mut vars);
if !is_array(self.ty_res.expr_ty(expr)) {
self.add_side_effect(self.ret_vars.clone());
} else if matches!(self.ty_res.expr_ty(expr).kind(), ty::Ref(_, _, Mutability::Mut)) {
self.ret_vars.iter_mut().for_each(|(_, b)| *b = true);
}
},
ExprKind::Break(dest, Some(expr)) => {
self.visit_expr(expr);
if let Some(label) = dest.label {
self.break_vars
.entry(label.ident)
.or_insert(Vec::new())
.append(&mut self.ret_vars);
}
self.contains_side_effect = true;
},
ExprKind::Ret(Some(expr)) => {
self.visit_expr(expr);
let vars = std::mem::take(&mut self.ret_vars);
self.add_side_effect(vars);
self.contains_side_effect = true;
},
ExprKind::Break(_, None) | ExprKind::Continue(_) | ExprKind::Ret(None) => {
self.contains_side_effect = true;
},
ExprKind::Struct(_, exprs, expr) => {
let mut ret_vars = exprs
.iter()
.flat_map(|field| {
self.visit_expr(field.expr);
std::mem::take(&mut self.ret_vars)
})
.collect();
walk_list!(self, visit_expr, expr);
self.ret_vars.append(&mut ret_vars);
},
_ => walk_expr(self, ex),
}
}
fn visit_path(&mut self, path: &'tcx Path<'tcx>, _id: HirId) {
if let Res::Local(id) = path.res {
self.ret_vars.push((id, false));
}
}
}
impl<'tcx> SideEffectVisit<'tcx> {
fn visit_assign(&mut self, lhs: &'tcx Expr<'tcx>, rhs: &'tcx Expr<'tcx>) {
// Just support array and tuple unwrapping for now.
//
// ex) `(a, b) = (c, d);`
// The graph would look like this:
// a -> c
// b -> d
//
// This would minimize the connection of the side-effect graph.
match (&lhs.kind, &rhs.kind) {
(ExprKind::Array(lhs), ExprKind::Array(rhs)) | (ExprKind::Tup(lhs), ExprKind::Tup(rhs)) => {
// if not, it is a compile error
debug_assert!(lhs.len() == rhs.len());
izip!(*lhs, *rhs).for_each(|(lhs, rhs)| self.visit_assign(lhs, rhs));
},
// in other assigns, we have to connect all each other
// because they can be connected somehow
_ => {
self.visit_expr(lhs);
let lhs_vars = std::mem::take(&mut self.ret_vars);
self.visit_expr(rhs);
let rhs_vars = std::mem::take(&mut self.ret_vars);
self.connect_assign(&lhs_vars, &rhs_vars, false);
},
}
}
fn visit_block_label(&mut self, block: &'tcx Block<'tcx>, label: Option<Label>) {
self.visit_block(block);
let _ = label.and_then(|label| {
self.break_vars
.remove(&label.ident)
.map(|mut break_vars| self.ret_vars.append(&mut break_vars))
});
}
fn visit_bin_op(&mut self, lhs: &'tcx Expr<'tcx>, rhs: &'tcx Expr<'tcx>) {
self.visit_expr(lhs);
let mut ret_vars = std::mem::take(&mut self.ret_vars);
self.visit_expr(rhs);
self.ret_vars.append(&mut ret_vars);
// the binary operation between non primitive values are overloaded operators
// so they can have side-effects
if !is_primitive(self.ty_res.expr_ty(lhs)) || !is_primitive(self.ty_res.expr_ty(rhs)) {
self.ret_vars.iter().for_each(|id| {
self.has_side_effect.insert(id.0);
});
self.contains_side_effect = true;
}
}
fn visit_un_op(&mut self, op: UnOp, expr: &'tcx Expr<'tcx>) {
self.visit_expr(expr);
let ty = self.ty_res.expr_ty(expr);
// dereferencing a reference has no side-effect
if !is_primitive(ty) && !matches!((op, ty.kind()), (UnOp::Deref, ty::Ref(..))) {
self.add_side_effect(self.ret_vars.clone());
}
if matches!((op, ty.kind()), (UnOp::Deref, ty::Ref(_, _, Mutability::Mut))) {
self.ret_vars.iter_mut().for_each(|(_, b)| *b = true);
}
}
fn visit_pat_expr(&mut self, pat: &'tcx Pat<'tcx>, expr: &'tcx Expr<'tcx>, connect_self: bool) {
match (&pat.kind, &expr.kind) {
(PatKind::Tuple(pats, _), ExprKind::Tup(exprs)) => {
self.ret_vars = izip!(*pats, *exprs)
.flat_map(|(pat, expr)| {
self.visit_pat_expr(pat, expr, connect_self);
std::mem::take(&mut self.ret_vars)
})
.collect();
},
(PatKind::Slice(front_exprs, _, back_exprs), ExprKind::Array(exprs)) => {
let mut vars = izip!(*front_exprs, *exprs)
.flat_map(|(pat, expr)| {
self.visit_pat_expr(pat, expr, connect_self);
std::mem::take(&mut self.ret_vars)
})
.collect();
self.ret_vars = izip!(back_exprs.iter().rev(), exprs.iter().rev())
.flat_map(|(pat, expr)| {
self.visit_pat_expr(pat, expr, connect_self);
std::mem::take(&mut self.ret_vars)
})
.collect();
self.ret_vars.append(&mut vars);
},
_ => {
let mut lhs_vars = Vec::new();
pat.each_binding(|_, id, _, _| lhs_vars.push((id, false)));
self.visit_expr(expr);
let rhs_vars = std::mem::take(&mut self.ret_vars);
self.connect_assign(&lhs_vars, &rhs_vars, connect_self);
self.ret_vars = rhs_vars;
},
}
}
fn visit_fn(&mut self, callee: &'tcx Expr<'tcx>, args: &'tcx [Expr<'tcx>]) {
self.visit_expr(callee);
let mut ret_vars = std::mem::take(&mut self.ret_vars);
self.add_side_effect(ret_vars.clone());
let mut is_recursive = false;
if_chain! {
if !self.has_self;
if let ExprKind::Path(QPath::Resolved(_, path)) = callee.kind;
if let Res::Def(DefKind::Fn, def_id) = path.res;
if self.fn_def_id == def_id;
then {
is_recursive = true;
}
}
if_chain! {
if !self.has_self && self.is_method;
if let ExprKind::Path(QPath::TypeRelative(ty, segment)) = callee.kind;
if segment.ident == self.fn_ident;
if let TyKind::Path(QPath::Resolved(_, path)) = ty.kind;
if let Res::SelfTy{ .. } = path.res;
then {
is_recursive = true;
}
}
if is_recursive {
izip!(self.params.clone(), args).for_each(|(pat, expr)| {
self.visit_pat_expr(pat, expr, true);
self.ret_vars.clear();
});
} else {
// This would set arguments used in closure that does not have side-effect.
// Closure itself can be detected whether there is a side-effect, but the
// value of variable that is holding closure can change.
// So, we just check the variables.
self.ret_vars = args
.iter()
.flat_map(|expr| {
self.visit_expr(expr);
std::mem::take(&mut self.ret_vars)
})
.collect_vec()
.into_iter()
.map(|id| {
self.has_side_effect.insert(id.0);
id
})
.collect();
self.contains_side_effect = true;
}
self.ret_vars.append(&mut ret_vars);
}
fn visit_method_call(&mut self, path: &'tcx PathSegment<'tcx>, args: &'tcx [Expr<'tcx>]) {
if_chain! {
if self.is_method;
if path.ident == self.fn_ident;
if let ExprKind::Path(QPath::Resolved(_, path)) = args.first().unwrap().kind;
if let Res::Local(..) = path.res;
let ident = path.segments.last().unwrap().ident;
if ident.name == kw::SelfLower;
then {
izip!(self.params.clone(), args.iter())
.for_each(|(pat, expr)| {
self.visit_pat_expr(pat, expr, true);
self.ret_vars.clear();
});
} else {
self.ret_vars = args
.iter()
.flat_map(|expr| {
self.visit_expr(expr);
std::mem::take(&mut self.ret_vars)
})
.collect_vec()
.into_iter()
.map(|a| {
self.has_side_effect.insert(a.0);
a
})
.collect();
self.contains_side_effect = true;
}
}
}
fn visit_if(&mut self, bind: &'tcx Expr<'tcx>, then_expr: &'tcx Expr<'tcx>, else_expr: Option<&'tcx Expr<'tcx>>) {
let contains_side_effect = self.contains_side_effect;
self.contains_side_effect = false;
self.visit_expr(bind);
let mut vars = std::mem::take(&mut self.ret_vars);
self.visit_expr(then_expr);
let mut then_vars = std::mem::take(&mut self.ret_vars);
walk_list!(self, visit_expr, else_expr);
if self.contains_side_effect {
self.add_side_effect(vars.clone());
}
self.contains_side_effect |= contains_side_effect;
self.ret_vars.append(&mut vars);
self.ret_vars.append(&mut then_vars);
}
fn visit_match(&mut self, expr: &'tcx Expr<'tcx>, arms: &'tcx [Arm<'tcx>]) {
self.visit_expr(expr);
let mut expr_vars = std::mem::take(&mut self.ret_vars);
self.ret_vars = arms
.iter()
.flat_map(|arm| {
let contains_side_effect = self.contains_side_effect;
self.contains_side_effect = false;
// this would visit `expr` multiple times
// but couldn't think of a better way
self.visit_pat_expr(arm.pat, expr, false);
let mut vars = std::mem::take(&mut self.ret_vars);
let _ = arm.guard.as_ref().map(|guard| {
self.visit_expr(match guard {
Guard::If(expr) | Guard::IfLet(_, expr) => expr,
});
vars.append(&mut self.ret_vars);
});
self.visit_expr(arm.body);
if self.contains_side_effect {
self.add_side_effect(vars.clone());
self.add_side_effect(expr_vars.clone());
}
self.contains_side_effect |= contains_side_effect;
vars.append(&mut self.ret_vars);
vars
})
.collect();
self.ret_vars.append(&mut expr_vars);
}
fn connect_assign(&mut self, lhs: &[(HirId, bool)], rhs: &[(HirId, bool)], connect_self: bool) {
// if mutable dereference is on assignment it can have side-effect
// (this can lead to parameter mutable dereference and change the original value)
// too hard to detect whether this value is from parameter, so this would all
// check mutable dereference assignment to side effect
lhs.iter().filter(|(_, b)| *b).for_each(|(id, _)| {
self.has_side_effect.insert(*id);
self.contains_side_effect = true;
});
// there is no connection
if lhs.is_empty() || rhs.is_empty() {
return;
}
// by connected rhs in cycle, the connections would decrease
// from `n * m` to `n + m`
// where `n` and `m` are length of `lhs` and `rhs`.
// unwrap is possible since rhs is not empty
let rhs_first = rhs.first().unwrap();
for (id, _) in lhs.iter() {
if connect_self || *id != rhs_first.0 {
self.graph
.entry(*id)
.or_insert_with(FxHashSet::default)
.insert(rhs_first.0);
}
}
let rhs = rhs.iter();
izip!(rhs.clone().cycle().skip(1), rhs).for_each(|(from, to)| {
if connect_self || from.0 != to.0 {
self.graph.entry(from.0).or_insert_with(FxHashSet::default).insert(to.0);
}
});
}
fn add_side_effect(&mut self, v: Vec<(HirId, bool)>) {
for (id, _) in v {
self.has_side_effect.insert(id);
self.contains_side_effect = true;
}
}
}

6
clippy_lints/src/question_mark.rs
View File

@ -123,7 +123,7 @@ impl QuestionMark {
}
fn result_check_and_early_return(cx: &LateContext<'_>, expr: &Expr<'_>, nested_expr: &Expr<'_>) -> bool {
Self::is_result(cx, expr) && Self::expression_returns_unmodified_err(cx, nested_expr, expr)
Self::is_result(cx, expr) && Self::expression_returns_unmodified_err(nested_expr, expr)
}
fn option_check_and_early_return(cx: &LateContext<'_>, expr: &Expr<'_>, nested_expr: &Expr<'_>) -> bool {
@ -156,9 +156,9 @@ impl QuestionMark {
}
}
fn expression_returns_unmodified_err(cx: &LateContext<'_>, expr: &Expr<'_>, cond_expr: &Expr<'_>) -> bool {
fn expression_returns_unmodified_err(expr: &Expr<'_>, cond_expr: &Expr<'_>) -> bool {
match peel_blocks_with_stmt(expr).kind {
ExprKind::Ret(Some(ret_expr)) => Self::expression_returns_unmodified_err(cx, ret_expr, cond_expr),
ExprKind::Ret(Some(ret_expr)) => Self::expression_returns_unmodified_err(ret_expr, cond_expr),
ExprKind::Path(_) => path_to_local(expr).is_some() && path_to_local(expr) == path_to_local(cond_expr),
_ => false,
}

45
clippy_lints/src/unnecessary_sort_by.rs
View File

@ -67,59 +67,51 @@ struct SortByKeyDetection {
/// Detect if the two expressions are mirrored (identical, except one
/// contains a and the other replaces it with b)
fn mirrored_exprs(
cx: &LateContext<'_>,
a_expr: &Expr<'_>,
a_ident: &Ident,
b_expr: &Expr<'_>,
b_ident: &Ident,
) -> bool {
fn mirrored_exprs(a_expr: &Expr<'_>, a_ident: &Ident, b_expr: &Expr<'_>, b_ident: &Ident) -> bool {
match (&a_expr.kind, &b_expr.kind) {
// Two boxes with mirrored contents
(ExprKind::Box(left_expr), ExprKind::Box(right_expr)) => {
mirrored_exprs(cx, left_expr, a_ident, right_expr, b_ident)
mirrored_exprs(left_expr, a_ident, right_expr, b_ident)
},
// Two arrays with mirrored contents
(ExprKind::Array(left_exprs), ExprKind::Array(right_exprs)) => {
iter::zip(*left_exprs, *right_exprs).all(|(left, right)| mirrored_exprs(cx, left, a_ident, right, b_ident))
iter::zip(*left_exprs, *right_exprs).all(|(left, right)| mirrored_exprs(left, a_ident, right, b_ident))
},
// The two exprs are function calls.
// Check to see that the function itself and its arguments are mirrored
(ExprKind::Call(left_expr, left_args), ExprKind::Call(right_expr, right_args)) => {
mirrored_exprs(cx, left_expr, a_ident, right_expr, b_ident)
&& iter::zip(*left_args, *right_args)
.all(|(left, right)| mirrored_exprs(cx, left, a_ident, right, b_ident))
mirrored_exprs(left_expr, a_ident, right_expr, b_ident)
&& iter::zip(*left_args, *right_args).all(|(left, right)| mirrored_exprs(left, a_ident, right, b_ident))
},
// The two exprs are method calls.
// Check to see that the function is the same and the arguments are mirrored
// This is enough because the receiver of the method is listed in the arguments
(ExprKind::MethodCall(left_segment, left_args, _), ExprKind::MethodCall(right_segment, right_args, _)) => {
left_segment.ident == right_segment.ident
&& iter::zip(*left_args, *right_args)
.all(|(left, right)| mirrored_exprs(cx, left, a_ident, right, b_ident))
&& iter::zip(*left_args, *right_args).all(|(left, right)| mirrored_exprs(left, a_ident, right, b_ident))
},
// Two tuples with mirrored contents
(ExprKind::Tup(left_exprs), ExprKind::Tup(right_exprs)) => {
iter::zip(*left_exprs, *right_exprs).all(|(left, right)| mirrored_exprs(cx, left, a_ident, right, b_ident))
iter::zip(*left_exprs, *right_exprs).all(|(left, right)| mirrored_exprs(left, a_ident, right, b_ident))
},
// Two binary ops, which are the same operation and which have mirrored arguments
(ExprKind::Binary(left_op, left_left, left_right), ExprKind::Binary(right_op, right_left, right_right)) => {
left_op.node == right_op.node
&& mirrored_exprs(cx, left_left, a_ident, right_left, b_ident)
&& mirrored_exprs(cx, left_right, a_ident, right_right, b_ident)
&& mirrored_exprs(left_left, a_ident, right_left, b_ident)
&& mirrored_exprs(left_right, a_ident, right_right, b_ident)
},
// Two unary ops, which are the same operation and which have the same argument
(ExprKind::Unary(left_op, left_expr), ExprKind::Unary(right_op, right_expr)) => {
left_op == right_op && mirrored_exprs(cx, left_expr, a_ident, right_expr, b_ident)
left_op == right_op && mirrored_exprs(left_expr, a_ident, right_expr, b_ident)
},
// The two exprs are literals of some kind
(ExprKind::Lit(left_lit), ExprKind::Lit(right_lit)) => left_lit.node == right_lit.node,
(ExprKind::Cast(left, _), ExprKind::Cast(right, _)) => mirrored_exprs(cx, left, a_ident, right, b_ident),
(ExprKind::Cast(left, _), ExprKind::Cast(right, _)) => mirrored_exprs(left, a_ident, right, b_ident),
(ExprKind::DropTemps(left_block), ExprKind::DropTemps(right_block)) => {
mirrored_exprs(cx, left_block, a_ident, right_block, b_ident)
mirrored_exprs(left_block, a_ident, right_block, b_ident)
},
(ExprKind::Field(left_expr, left_ident), ExprKind::Field(right_expr, right_ident)) => {
left_ident.name == right_ident.name && mirrored_exprs(cx, left_expr, a_ident, right_expr, right_ident)
left_ident.name == right_ident.name && mirrored_exprs(left_expr, a_ident, right_expr, right_ident)
},
// Two paths: either one is a and the other is b, or they're identical to each other
(
@ -151,11 +143,9 @@ fn mirrored_exprs(
(
ExprKind::AddrOf(left_kind, Mutability::Not, left_expr),
ExprKind::AddrOf(right_kind, Mutability::Not, right_expr),
) => left_kind == right_kind && mirrored_exprs(cx, left_expr, a_ident, right_expr, b_ident),
(_, ExprKind::AddrOf(_, Mutability::Not, right_expr)) => {
mirrored_exprs(cx, a_expr, a_ident, right_expr, b_ident)
},
(ExprKind::AddrOf(_, Mutability::Not, left_expr), _) => mirrored_exprs(cx, left_expr, a_ident, b_expr, b_ident),
) => left_kind == right_kind && mirrored_exprs(left_expr, a_ident, right_expr, b_ident),
(_, ExprKind::AddrOf(_, Mutability::Not, right_expr)) => mirrored_exprs(a_expr, a_ident, right_expr, b_ident),
(ExprKind::AddrOf(_, Mutability::Not, left_expr), _) => mirrored_exprs(left_expr, a_ident, b_expr, b_ident),
_ => false,
}
}
@ -176,14 +166,13 @@ fn detect_lint(cx: &LateContext<'_>, expr: &Expr<'_>) -> Option<LintTrigger> {
if method_path.ident.name == sym::cmp;
then {
let (closure_body, closure_arg, reverse) = if mirrored_exprs(
cx,
left_expr,
left_ident,
right_expr,
right_ident
) {
(Sugg::hir(cx, left_expr, "..").to_string(), left_ident.name.to_string(), false)
} else if mirrored_exprs(cx, left_expr, right_ident, right_expr, left_ident) {
} else if mirrored_exprs(left_expr, right_ident, right_expr, left_ident) {
(Sugg::hir(cx, left_expr, "..").to_string(), right_ident.name.to_string(), true)
} else {
return None;

4
tests/ui/crate_level_checks/no_std_main_recursion.rs
View File

@ -12,12 +12,12 @@ static N: AtomicUsize = AtomicUsize::new(0);
#[warn(clippy::main_recursion)]
#[start]
fn main(argc: isize, argv: *const *const u8) -> isize {
fn main(_argc: isize, _argv: *const *const u8) -> isize {
let x = N.load(Ordering::Relaxed);
N.store(x + 1, Ordering::Relaxed);
if x < 3 {
main(argc, argv);
main(_argc, _argv);
}
0

122
tests/ui/only_used_in_recursion.rs Normal file
View File

@ -0,0 +1,122 @@
#![warn(clippy::only_used_in_recursion)]
fn simple(a: usize, b: usize) -> usize {
if a == 0 { 1 } else { simple(a - 1, b) }
}
fn with_calc(a: usize, b: isize) -> usize {
if a == 0 { 1 } else { with_calc(a - 1, -b + 1) }
}
fn tuple((a, b): (usize, usize)) -> usize {
if a == 0 { 1 } else { tuple((a - 1, b + 1)) }
}
fn let_tuple(a: usize, b: usize) -> usize {
let (c, d) = (a, b);
if c == 0 { 1 } else { let_tuple(c - 1, d + 1) }
}
fn array([a, b]: [usize; 2]) -> usize {
if a == 0 { 1 } else { array([a - 1, b + 1]) }
}
fn index(a: usize, mut b: &[usize], c: usize) -> usize {
if a == 0 { 1 } else { index(a - 1, b, c + b[0]) }
}
fn break_(a: usize, mut b: usize, mut c: usize) -> usize {
let c = loop {
b += 1;
c += 1;
if c == 10 {
break b;
}
};
if a == 0 { 1 } else { break_(a - 1, c, c) }
}
// this has a side effect
fn mut_ref(a: usize, b: &mut usize) -> usize {
*b = 1;
if a == 0 { 1 } else { mut_ref(a - 1, b) }
}
fn mut_ref2(a: usize, b: &mut usize) -> usize {
let mut c = *b;
if a == 0 { 1 } else { mut_ref2(a - 1, &mut c) }
}
fn not_primitive(a: usize, b: String) -> usize {
if a == 0 { 1 } else { not_primitive(a - 1, b) }
}
// this doesn't have a side effect,
// but `String` is not primitive.
fn not_primitive_op(a: usize, b: String, c: &str) -> usize {
if a == 1 { 1 } else { not_primitive_op(a, b + c, c) }
}
struct A;
impl A {
fn method(a: usize, b: usize) -> usize {
if a == 0 { 1 } else { A::method(a - 1, b - 1) }
}
fn method2(&self, a: usize, b: usize) -> usize {
if a == 0 { 1 } else { self.method2(a - 1, b + 1) }
}
}
trait B {
fn hello(a: usize, b: usize) -> usize;
fn hello2(&self, a: usize, b: usize) -> usize;
}
impl B for A {
fn hello(a: usize, b: usize) -> usize {
if a == 0 { 1 } else { A::hello(a - 1, b + 1) }
}
fn hello2(&self, a: usize, b: usize) -> usize {
if a == 0 { 1 } else { self.hello2(a - 1, b + 1) }
}
}
trait C {
fn hello(a: usize, b: usize) -> usize {
if a == 0 { 1 } else { Self::hello(a - 1, b + 1) }
}
fn hello2(&self, a: usize, b: usize) -> usize {
if a == 0 { 1 } else { self.hello2(a - 1, b + 1) }
}
}
fn ignore(a: usize, _: usize) -> usize {
if a == 1 { 1 } else { ignore(a - 1, 0) }
}
fn ignore2(a: usize, _b: usize) -> usize {
if a == 1 { 1 } else { ignore2(a - 1, _b) }
}
fn f1(a: u32) -> u32 {
a
}
fn f2(a: u32) -> u32 {
f1(a)
}
fn inner_fn(a: u32) -> u32 {
fn inner_fn(a: u32) -> u32 {
a
}
inner_fn(a)
}
fn main() {}

82
tests/ui/only_used_in_recursion.stderr Normal file
View File

@ -0,0 +1,82 @@
error: parameter is only used in recursion
--> $DIR/only_used_in_recursion.rs:3:21
|
LL | fn simple(a: usize, b: usize) -> usize {
| ^ help: if this is intentional, prefix with an underscore: `_b`
|
= note: `-D clippy::only-used-in-recursion` implied by `-D warnings`
error: parameter is only used in recursion
--> $DIR/only_used_in_recursion.rs:7:24
|
LL | fn with_calc(a: usize, b: isize) -> usize {
| ^ help: if this is intentional, prefix with an underscore: `_b`
error: parameter is only used in recursion
--> $DIR/only_used_in_recursion.rs:11:14
|
LL | fn tuple((a, b): (usize, usize)) -> usize {
| ^ help: if this is intentional, prefix with an underscore: `_b`
error: parameter is only used in recursion
--> $DIR/only_used_in_recursion.rs:15:24
|
LL | fn let_tuple(a: usize, b: usize) -> usize {
| ^ help: if this is intentional, prefix with an underscore: `_b`
error: parameter is only used in recursion
--> $DIR/only_used_in_recursion.rs:20:14
|
LL | fn array([a, b]: [usize; 2]) -> usize {
| ^ help: if this is intentional, prefix with an underscore: `_b`
error: parameter is only used in recursion
--> $DIR/only_used_in_recursion.rs:24:20
|
LL | fn index(a: usize, mut b: &[usize], c: usize) -> usize {
| ^^^^^ help: if this is intentional, prefix with an underscore: `_b`
error: parameter is only used in recursion
--> $DIR/only_used_in_recursion.rs:24:37
|
LL | fn index(a: usize, mut b: &[usize], c: usize) -> usize {
| ^ help: if this is intentional, prefix with an underscore: `_c`
error: parameter is only used in recursion
--> $DIR/only_used_in_recursion.rs:28:21
|
LL | fn break_(a: usize, mut b: usize, mut c: usize) -> usize {
| ^^^^^ help: if this is intentional, prefix with an underscore: `_b`
error: parameter is only used in recursion
--> $DIR/only_used_in_recursion.rs:46:23
|
LL | fn mut_ref2(a: usize, b: &mut usize) -> usize {
| ^ help: if this is intentional, prefix with an underscore: `_b`
error: parameter is only used in recursion
--> $DIR/only_used_in_recursion.rs:51:28
|
LL | fn not_primitive(a: usize, b: String) -> usize {
| ^ help: if this is intentional, prefix with an underscore: `_b`
error: parameter is only used in recursion
--> $DIR/only_used_in_recursion.rs:68:33
|
LL | fn method2(&self, a: usize, b: usize) -> usize {
| ^ help: if this is intentional, prefix with an underscore: `_b`
error: parameter is only used in recursion
--> $DIR/only_used_in_recursion.rs:90:24
|
LL | fn hello(a: usize, b: usize) -> usize {
| ^ help: if this is intentional, prefix with an underscore: `_b`
error: parameter is only used in recursion
--> $DIR/only_used_in_recursion.rs:94:32
|
LL | fn hello2(&self, a: usize, b: usize) -> usize {
| ^ help: if this is intentional, prefix with an underscore: `_b`
error: aborting due to 13 previous errors