diff options
| -rw-r--r-- | clippy_lints/src/methods/mod.rs | 2 | ||||
| -rw-r--r-- | clippy_lints/src/methods/suspicious_map.rs | 45 | ||||
| -rw-r--r-- | clippy_utils/src/lib.rs | 64 | ||||
| -rw-r--r-- | tests/ui/suspicious_map.rs | 27 | ||||
| -rw-r--r-- | tests/ui/suspicious_map.stderr | 10 |
5 files changed, 132 insertions, 16 deletions
diff --git a/clippy_lints/src/methods/mod.rs b/clippy_lints/src/methods/mod.rs index 7fd14c4f9b1..45e906cf468 100644 --- a/clippy_lints/src/methods/mod.rs +++ b/clippy_lints/src/methods/mod.rs @@ -1739,7 +1739,7 @@ impl<'tcx> LateLintPass<'tcx> for Methods { unnecessary_filter_map::check(cx, expr, arg_lists[0]); filter_map_identity::check(cx, expr, arg_lists[0], method_spans[0]); }, - ["count", "map"] => suspicious_map::check(cx, expr), + ["count", "map"] => suspicious_map::check(cx, expr, arg_lists[1], arg_lists[0]), ["assume_init"] => uninit_assumed_init::check(cx, &arg_lists[0][0], expr), ["unwrap_or", arith @ ("checked_add" | "checked_sub" | "checked_mul")] => { manual_saturating_arithmetic::check(cx, expr, &arg_lists, &arith["checked_".len()..]) diff --git a/clippy_lints/src/methods/suspicious_map.rs b/clippy_lints/src/methods/suspicious_map.rs index e135a826dc4..0ffa71de30c 100644 --- a/clippy_lints/src/methods/suspicious_map.rs +++ b/clippy_lints/src/methods/suspicious_map.rs @@ -1,16 +1,41 @@ -use crate::utils::span_lint_and_help; +use crate::utils::usage::mutated_variables; +use crate::utils::{expr_or_init, is_trait_method, span_lint_and_help}; +use if_chain::if_chain; use rustc_hir as hir; use rustc_lint::LateContext; +use rustc_span::sym; use super::SUSPICIOUS_MAP; -pub(super) fn check(cx: &LateContext<'_>, expr: &hir::Expr<'_>) { - span_lint_and_help( - cx, - SUSPICIOUS_MAP, - expr.span, - "this call to `map()` won't have an effect on the call to `count()`", - None, - "make sure you did not confuse `map` with `filter` or `for_each`", - ); +pub fn check<'tcx>( + cx: &LateContext<'tcx>, + expr: &hir::Expr<'_>, + map_args: &[hir::Expr<'_>], + count_args: &[hir::Expr<'_>], +) { + if_chain! { + if let [count_recv] = count_args; + if let [_, map_arg] = map_args; + if is_trait_method(cx, count_recv, sym::Iterator); + let closure = expr_or_init(cx, map_arg); + if let Some(body_id) = cx.tcx.hir().maybe_body_owned_by(closure.hir_id); + let closure_body = cx.tcx.hir().body(body_id); + if !cx.typeck_results().expr_ty(&closure_body.value).is_unit(); + then { + if let Some(map_mutated_vars) = mutated_variables(&closure_body.value, cx) { + // A variable is used mutably inside of the closure. Suppress the lint. + if !map_mutated_vars.is_empty() { + return; + } + } + span_lint_and_help( + cx, + SUSPICIOUS_MAP, + expr.span, + "this call to `map()` won't have an effect on the call to `count()`", + None, + "make sure you did not confuse `map` with `filter` or `for_each`", + ); + } + } } diff --git a/clippy_utils/src/lib.rs b/clippy_utils/src/lib.rs index 0ddc915862c..98cdeae739f 100644 --- a/clippy_utils/src/lib.rs +++ b/clippy_utils/src/lib.rs @@ -62,10 +62,10 @@ use rustc_hir::def::{CtorKind, CtorOf, DefKind, Res}; use rustc_hir::def_id::{DefId, LOCAL_CRATE}; use rustc_hir::intravisit::{self, NestedVisitorMap, Visitor}; use rustc_hir::{ - def, Arm, Block, Body, Constness, CrateItem, Expr, ExprKind, FnDecl, ForeignItem, GenericArgs, GenericParam, HirId, - Impl, ImplItem, ImplItemKind, Item, ItemKind, LangItem, Lifetime, Local, MacroDef, MatchSource, Node, Param, Pat, - PatKind, Path, PathSegment, QPath, Stmt, StructField, TraitItem, TraitItemKind, TraitRef, TyKind, Unsafety, - Variant, Visibility, + def, Arm, BindingAnnotation, Block, Body, Constness, CrateItem, Expr, ExprKind, FnDecl, ForeignItem, GenericArgs, + GenericParam, HirId, Impl, ImplItem, ImplItemKind, Item, ItemKind, LangItem, Lifetime, Local, MacroDef, + MatchSource, Node, Param, Pat, PatKind, Path, PathSegment, QPath, Stmt, StructField, TraitItem, TraitItemKind, + TraitRef, TyKind, Unsafety, Variant, Visibility, }; use rustc_infer::infer::TyCtxtInferExt; use rustc_lint::{LateContext, Level, Lint, LintContext}; @@ -138,6 +138,62 @@ pub fn differing_macro_contexts(lhs: Span, rhs: Span) -> bool { rhs.ctxt() != lhs.ctxt() } +/// If the given expression is a local binding, find the initializer expression. +/// If that initializer expression is another local binding, find its initializer again. +/// This process repeats as long as possible (but usually no more than once). Initializer +/// expressions with adjustments are ignored. If this is not desired, use [`find_binding_init`] +/// instead. +/// +/// Examples: +/// ```ignore +/// let abc = 1; +/// // ^ output +/// let def = abc; +/// dbg!(def) +/// // ^^^ input +/// +/// // or... +/// let abc = 1; +/// let def = abc + 2; +/// // ^^^^^^^ output +/// dbg!(def) +/// // ^^^ input +/// ``` +pub fn expr_or_init<'a, 'b, 'tcx: 'b>(cx: &LateContext<'tcx>, mut expr: &'a Expr<'b>) -> &'a Expr<'b> { + while let Some(init) = path_to_local(expr) + .and_then(|id| find_binding_init(cx, id)) + .filter(|init| cx.typeck_results().expr_adjustments(init).is_empty()) + { + expr = init; + } + expr +} + +/// Finds the initializer expression for a local binding. Returns `None` if the binding is mutable. +/// By only considering immutable bindings, we guarantee that the returned expression represents the +/// value of the binding wherever it is referenced. +/// +/// Example: +/// ```ignore +/// let abc = 1; +/// // ^ output +/// dbg!(abc) +/// // ^^^ input +/// ``` +pub fn find_binding_init<'tcx>(cx: &LateContext<'tcx>, hir_id: HirId) -> Option<&'tcx Expr<'tcx>> { + let hir = cx.tcx.hir(); + if_chain! { + if let Some(Node::Binding(pat)) = hir.find(hir_id); + if matches!(pat.kind, PatKind::Binding(BindingAnnotation::Unannotated, ..)); + let parent = hir.get_parent_node(hir_id); + if let Some(Node::Local(local)) = hir.find(parent); + then { + return local.init; + } + } + None +} + /// Returns `true` if the given `NodeId` is inside a constant context /// /// # Example diff --git a/tests/ui/suspicious_map.rs b/tests/ui/suspicious_map.rs index d838d8fde21..3a2a10cf09e 100644 --- a/tests/ui/suspicious_map.rs +++ b/tests/ui/suspicious_map.rs @@ -2,4 +2,31 @@ fn main() { let _ = (0..3).map(|x| x + 2).count(); + + let f = |x| x + 1; + let _ = (0..3).map(f).count(); +} + +fn negative() { + // closure with side effects + let mut sum = 0; + let _ = (0..3).map(|x| sum += x).count(); + + // closure variable with side effects + let ext_closure = |x| sum += x; + let _ = (0..3).map(ext_closure).count(); + + // closure that returns unit + let _ = (0..3) + .map(|x| { + // do nothing + }) + .count(); + + // external function + let _ = (0..3).map(do_something).count(); +} + +fn do_something<T>(t: T) -> String { + unimplemented!() } diff --git a/tests/ui/suspicious_map.stderr b/tests/ui/suspicious_map.stderr index e1b4ba40376..8c3f36584a5 100644 --- a/tests/ui/suspicious_map.stderr +++ b/tests/ui/suspicious_map.stderr @@ -7,5 +7,13 @@ LL | let _ = (0..3).map(|x| x + 2).count(); = note: `-D clippy::suspicious-map` implied by `-D warnings` = help: make sure you did not confuse `map` with `filter` or `for_each` -error: aborting due to previous error +error: this call to `map()` won't have an effect on the call to `count()` + --> $DIR/suspicious_map.rs:7:13 + | +LL | let _ = (0..3).map(f).count(); + | ^^^^^^^^^^^^^^^^^^^^^ + | + = help: make sure you did not confuse `map` with `filter` or `for_each` + +error: aborting due to 2 previous errors |