From 46c5a5d234f13dcf4bb4cf4241b2addedbf0be14 Mon Sep 17 00:00:00 2001 From: Philipp Krones Date: Mon, 21 Nov 2022 20:34:47 +0100 Subject: Merge commit 'f4850f7292efa33759b4f7f9b7621268979e9914' into clippyup --- clippy_lints/src/methods/chars_cmp_with_unwrap.rs | 4 +- clippy_lints/src/methods/chars_last_cmp.rs | 2 +- .../src/methods/chars_last_cmp_with_unwrap.rs | 2 +- clippy_lints/src/methods/chars_next_cmp.rs | 2 +- .../src/methods/chars_next_cmp_with_unwrap.rs | 2 +- .../src/methods/collapsible_str_replace.rs | 6 +- clippy_lints/src/methods/expect_used.rs | 10 +- clippy_lints/src/methods/filter_map.rs | 8 +- clippy_lints/src/methods/inefficient_to_string.rs | 4 +- clippy_lints/src/methods/iter_nth_zero.rs | 2 +- .../methods/iter_on_single_or_empty_collections.rs | 2 +- .../src/methods/manual_saturating_arithmetic.rs | 2 +- clippy_lints/src/methods/manual_str_repeat.rs | 2 - clippy_lints/src/methods/map_clone.rs | 4 +- .../src/methods/map_collect_result_unit.rs | 15 +- clippy_lints/src/methods/map_err_ignore.rs | 2 +- clippy_lints/src/methods/mod.rs | 281 ++++++++----- clippy_lints/src/methods/needless_collect.rs | 456 +++++++++++++++++++++ clippy_lints/src/methods/option_as_ref_deref.rs | 4 +- clippy_lints/src/methods/or_fun_call.rs | 57 ++- clippy_lints/src/methods/seek_from_current.rs | 48 +++ .../src/methods/seek_to_start_instead_of_rewind.rs | 45 ++ clippy_lints/src/methods/string_extend_chars.rs | 6 +- clippy_lints/src/methods/suspicious_map.rs | 2 +- clippy_lints/src/methods/unnecessary_join.rs | 2 +- clippy_lints/src/methods/unwrap_used.rs | 10 +- 26 files changed, 811 insertions(+), 169 deletions(-) create mode 100644 clippy_lints/src/methods/needless_collect.rs create mode 100644 clippy_lints/src/methods/seek_from_current.rs create mode 100644 clippy_lints/src/methods/seek_to_start_instead_of_rewind.rs (limited to 'clippy_lints/src/methods') diff --git a/clippy_lints/src/methods/chars_cmp_with_unwrap.rs b/clippy_lints/src/methods/chars_cmp_with_unwrap.rs index 7e808760663..27a05337a29 100644 --- a/clippy_lints/src/methods/chars_cmp_with_unwrap.rs +++ b/clippy_lints/src/methods/chars_cmp_with_unwrap.rs @@ -9,8 +9,8 @@ use rustc_lint::LateContext; use rustc_lint::Lint; /// Wrapper fn for `CHARS_NEXT_CMP` and `CHARS_LAST_CMP` lints with `unwrap()`. -pub(super) fn check<'tcx>( - cx: &LateContext<'tcx>, +pub(super) fn check( + cx: &LateContext<'_>, info: &crate::methods::BinaryExprInfo<'_>, chain_methods: &[&str], lint: &'static Lint, diff --git a/clippy_lints/src/methods/chars_last_cmp.rs b/clippy_lints/src/methods/chars_last_cmp.rs index 07bbc5ca1bf..2efff4c3c54 100644 --- a/clippy_lints/src/methods/chars_last_cmp.rs +++ b/clippy_lints/src/methods/chars_last_cmp.rs @@ -4,7 +4,7 @@ use rustc_lint::LateContext; use super::CHARS_LAST_CMP; /// Checks for the `CHARS_LAST_CMP` lint. -pub(super) fn check<'tcx>(cx: &LateContext<'tcx>, info: &crate::methods::BinaryExprInfo<'_>) -> bool { +pub(super) fn check(cx: &LateContext<'_>, info: &crate::methods::BinaryExprInfo<'_>) -> bool { if chars_cmp::check(cx, info, &["chars", "last"], CHARS_LAST_CMP, "ends_with") { true } else { diff --git a/clippy_lints/src/methods/chars_last_cmp_with_unwrap.rs b/clippy_lints/src/methods/chars_last_cmp_with_unwrap.rs index c29ee0ec8c8..5b8713f7d79 100644 --- a/clippy_lints/src/methods/chars_last_cmp_with_unwrap.rs +++ b/clippy_lints/src/methods/chars_last_cmp_with_unwrap.rs @@ -4,7 +4,7 @@ use rustc_lint::LateContext; use super::CHARS_LAST_CMP; /// Checks for the `CHARS_LAST_CMP` lint with `unwrap()`. -pub(super) fn check<'tcx>(cx: &LateContext<'tcx>, info: &crate::methods::BinaryExprInfo<'_>) -> bool { +pub(super) fn check(cx: &LateContext<'_>, info: &crate::methods::BinaryExprInfo<'_>) -> bool { if chars_cmp_with_unwrap::check(cx, info, &["chars", "last", "unwrap"], CHARS_LAST_CMP, "ends_with") { true } else { diff --git a/clippy_lints/src/methods/chars_next_cmp.rs b/clippy_lints/src/methods/chars_next_cmp.rs index a6701d8830e..b631fecab97 100644 --- a/clippy_lints/src/methods/chars_next_cmp.rs +++ b/clippy_lints/src/methods/chars_next_cmp.rs @@ -3,6 +3,6 @@ use rustc_lint::LateContext; use super::CHARS_NEXT_CMP; /// Checks for the `CHARS_NEXT_CMP` lint. -pub(super) fn check<'tcx>(cx: &LateContext<'tcx>, info: &crate::methods::BinaryExprInfo<'_>) -> bool { +pub(super) fn check(cx: &LateContext<'_>, info: &crate::methods::BinaryExprInfo<'_>) -> bool { crate::methods::chars_cmp::check(cx, info, &["chars", "next"], CHARS_NEXT_CMP, "starts_with") } diff --git a/clippy_lints/src/methods/chars_next_cmp_with_unwrap.rs b/clippy_lints/src/methods/chars_next_cmp_with_unwrap.rs index 28ede28e935..caf21d3ff3b 100644 --- a/clippy_lints/src/methods/chars_next_cmp_with_unwrap.rs +++ b/clippy_lints/src/methods/chars_next_cmp_with_unwrap.rs @@ -3,6 +3,6 @@ use rustc_lint::LateContext; use super::CHARS_NEXT_CMP; /// Checks for the `CHARS_NEXT_CMP` lint with `unwrap()`. -pub(super) fn check<'tcx>(cx: &LateContext<'tcx>, info: &crate::methods::BinaryExprInfo<'_>) -> bool { +pub(super) fn check(cx: &LateContext<'_>, info: &crate::methods::BinaryExprInfo<'_>) -> bool { crate::methods::chars_cmp_with_unwrap::check(cx, info, &["chars", "next", "unwrap"], CHARS_NEXT_CMP, "starts_with") } diff --git a/clippy_lints/src/methods/collapsible_str_replace.rs b/clippy_lints/src/methods/collapsible_str_replace.rs index 501646863fe..ac61b437788 100644 --- a/clippy_lints/src/methods/collapsible_str_replace.rs +++ b/clippy_lints/src/methods/collapsible_str_replace.rs @@ -23,7 +23,7 @@ pub(super) fn check<'tcx>( // If the parent node's `to` argument is the same as the `to` argument // of the last replace call in the current chain, don't lint as it was already linted if let Some(parent) = get_parent_expr(cx, expr) - && let Some(("replace", _, [current_from, current_to], _)) = method_call(parent) + && let Some(("replace", _, [current_from, current_to], _, _)) = method_call(parent) && eq_expr_value(cx, to, current_to) && from_kind == cx.typeck_results().expr_ty(current_from).peel_refs().kind() { @@ -48,7 +48,7 @@ fn collect_replace_calls<'tcx>( let mut from_args = VecDeque::new(); let _: Option<()> = for_each_expr(expr, |e| { - if let Some(("replace", _, [from, to], _)) = method_call(e) { + if let Some(("replace", _, [from, to], _, _)) = method_call(e) { if eq_expr_value(cx, to_arg, to) && cx.typeck_results().expr_ty(from).peel_refs().is_char() { methods.push_front(e); from_args.push_front(from); @@ -78,7 +78,7 @@ fn check_consecutive_replace_calls<'tcx>( .collect(); let app = Applicability::MachineApplicable; let earliest_replace_call = replace_methods.methods.front().unwrap(); - if let Some((_, _, [..], span_lo)) = method_call(earliest_replace_call) { + if let Some((_, _, [..], span_lo, _)) = method_call(earliest_replace_call) { span_lint_and_sugg( cx, COLLAPSIBLE_STR_REPLACE, diff --git a/clippy_lints/src/methods/expect_used.rs b/clippy_lints/src/methods/expect_used.rs index d59fefa1ddc..cce8f797e98 100644 --- a/clippy_lints/src/methods/expect_used.rs +++ b/clippy_lints/src/methods/expect_used.rs @@ -1,5 +1,5 @@ use clippy_utils::diagnostics::span_lint_and_help; -use clippy_utils::is_in_test_function; +use clippy_utils::is_in_cfg_test; use clippy_utils::ty::is_type_diagnostic_item; use rustc_hir as hir; use rustc_lint::LateContext; @@ -18,16 +18,16 @@ pub(super) fn check( let obj_ty = cx.typeck_results().expr_ty(recv).peel_refs(); let mess = if is_type_diagnostic_item(cx, obj_ty, sym::Option) && !is_err { - Some((EXPECT_USED, "an Option", "None", "")) + Some((EXPECT_USED, "an `Option`", "None", "")) } else if is_type_diagnostic_item(cx, obj_ty, sym::Result) { - Some((EXPECT_USED, "a Result", if is_err { "Ok" } else { "Err" }, "an ")) + Some((EXPECT_USED, "a `Result`", if is_err { "Ok" } else { "Err" }, "an ")) } else { None }; let method = if is_err { "expect_err" } else { "expect" }; - if allow_expect_in_tests && is_in_test_function(cx.tcx, expr.hir_id) { + if allow_expect_in_tests && is_in_cfg_test(cx.tcx, expr.hir_id) { return; } @@ -36,7 +36,7 @@ pub(super) fn check( cx, lint, expr.span, - &format!("used `{method}()` on `{kind}` value"), + &format!("used `{method}()` on {kind} value"), None, &format!("if this value is {none_prefix}`{none_value}`, it will panic"), ); diff --git a/clippy_lints/src/methods/filter_map.rs b/clippy_lints/src/methods/filter_map.rs index 9719b2f1c51..f888c58a72d 100644 --- a/clippy_lints/src/methods/filter_map.rs +++ b/clippy_lints/src/methods/filter_map.rs @@ -17,7 +17,7 @@ use super::MANUAL_FILTER_MAP; use super::MANUAL_FIND_MAP; use super::OPTION_FILTER_MAP; -fn is_method<'tcx>(cx: &LateContext<'tcx>, expr: &hir::Expr<'_>, method_name: Symbol) -> bool { +fn is_method(cx: &LateContext<'_>, expr: &hir::Expr<'_>, method_name: Symbol) -> bool { match &expr.kind { hir::ExprKind::Path(QPath::TypeRelative(_, mname)) => mname.ident.name == method_name, hir::ExprKind::Path(QPath::Resolved(_, segments)) => { @@ -46,7 +46,7 @@ fn is_method<'tcx>(cx: &LateContext<'tcx>, expr: &hir::Expr<'_>, method_name: Sy } } -fn is_option_filter_map<'tcx>(cx: &LateContext<'tcx>, filter_arg: &hir::Expr<'_>, map_arg: &hir::Expr<'_>) -> bool { +fn is_option_filter_map(cx: &LateContext<'_>, filter_arg: &hir::Expr<'_>, map_arg: &hir::Expr<'_>) -> bool { is_method(cx, map_arg, sym::unwrap) && is_method(cx, filter_arg, sym!(is_some)) } @@ -66,8 +66,8 @@ fn is_filter_some_map_unwrap( /// lint use of `filter().map()` or `find().map()` for `Iterators` #[allow(clippy::too_many_arguments)] -pub(super) fn check<'tcx>( - cx: &LateContext<'tcx>, +pub(super) fn check( + cx: &LateContext<'_>, expr: &hir::Expr<'_>, filter_recv: &hir::Expr<'_>, filter_arg: &hir::Expr<'_>, diff --git a/clippy_lints/src/methods/inefficient_to_string.rs b/clippy_lints/src/methods/inefficient_to_string.rs index 4f4f543e8a9..d8c821bc9ee 100644 --- a/clippy_lints/src/methods/inefficient_to_string.rs +++ b/clippy_lints/src/methods/inefficient_to_string.rs @@ -12,8 +12,8 @@ use rustc_span::symbol::{Symbol, sym}; use super::INEFFICIENT_TO_STRING; /// Checks for the `INEFFICIENT_TO_STRING` lint -pub fn check<'tcx>( - cx: &LateContext<'tcx>, +pub fn check( + cx: &LateContext<'_>, expr: &hir::Expr<'_>, method_name: Symbol, receiver: &hir::Expr<'_>, diff --git a/clippy_lints/src/methods/iter_nth_zero.rs b/clippy_lints/src/methods/iter_nth_zero.rs index 68d906c3ea3..c830958d5c8 100644 --- a/clippy_lints/src/methods/iter_nth_zero.rs +++ b/clippy_lints/src/methods/iter_nth_zero.rs @@ -10,7 +10,7 @@ use rustc_span::sym; use super::ITER_NTH_ZERO; -pub(super) fn check<'tcx>(cx: &LateContext<'tcx>, expr: &hir::Expr<'_>, recv: &hir::Expr<'_>, arg: &hir::Expr<'_>) { +pub(super) fn check(cx: &LateContext<'_>, expr: &hir::Expr<'_>, recv: &hir::Expr<'_>, arg: &hir::Expr<'_>) { if_chain! { if is_trait_method(cx, expr, sym::Iterator); if let Some((Constant::Int(0), _)) = constant(cx, cx.typeck_results(), arg); diff --git a/clippy_lints/src/methods/iter_on_single_or_empty_collections.rs b/clippy_lints/src/methods/iter_on_single_or_empty_collections.rs index 4f73b3ec422..70abe4891d9 100644 --- a/clippy_lints/src/methods/iter_on_single_or_empty_collections.rs +++ b/clippy_lints/src/methods/iter_on_single_or_empty_collections.rs @@ -25,7 +25,7 @@ impl IterType { } } -pub(super) fn check<'tcx>(cx: &LateContext<'tcx>, expr: &Expr<'_>, method_name: &str, recv: &Expr<'_>) { +pub(super) fn check(cx: &LateContext<'_>, expr: &Expr<'_>, method_name: &str, recv: &Expr<'_>) { let item = match recv.kind { ExprKind::Array([]) => None, ExprKind::Array([e]) => Some(e), diff --git a/clippy_lints/src/methods/manual_saturating_arithmetic.rs b/clippy_lints/src/methods/manual_saturating_arithmetic.rs index b80541b8647..a7284c64497 100644 --- a/clippy_lints/src/methods/manual_saturating_arithmetic.rs +++ b/clippy_lints/src/methods/manual_saturating_arithmetic.rs @@ -67,7 +67,7 @@ enum MinMax { Max, } -fn is_min_or_max<'tcx>(cx: &LateContext<'tcx>, expr: &hir::Expr<'_>) -> Option { +fn is_min_or_max(cx: &LateContext<'_>, expr: &hir::Expr<'_>) -> Option { // `T::max_value()` `T::min_value()` inherent methods if_chain! { if let hir::ExprKind::Call(func, args) = &expr.kind; diff --git a/clippy_lints/src/methods/manual_str_repeat.rs b/clippy_lints/src/methods/manual_str_repeat.rs index 13c47c03a80..a08f7254053 100644 --- a/clippy_lints/src/methods/manual_str_repeat.rs +++ b/clippy_lints/src/methods/manual_str_repeat.rs @@ -59,10 +59,8 @@ pub(super) fn check( if let ExprKind::Call(repeat_fn, [repeat_arg]) = take_self_arg.kind; if is_path_diagnostic_item(cx, repeat_fn, sym::iter_repeat); if is_type_lang_item(cx, cx.typeck_results().expr_ty(collect_expr), LangItem::String); - if let Some(collect_id) = cx.typeck_results().type_dependent_def_id(collect_expr.hir_id); if let Some(take_id) = cx.typeck_results().type_dependent_def_id(take_expr.hir_id); if let Some(iter_trait_id) = cx.tcx.get_diagnostic_item(sym::Iterator); - if cx.tcx.trait_of_item(collect_id) == Some(iter_trait_id); if cx.tcx.trait_of_item(take_id) == Some(iter_trait_id); if let Some(repeat_kind) = parse_repeat_arg(cx, repeat_arg); let ctxt = collect_expr.span.ctxt(); diff --git a/clippy_lints/src/methods/map_clone.rs b/clippy_lints/src/methods/map_clone.rs index 7ce14ec080b..6bc783c6d50 100644 --- a/clippy_lints/src/methods/map_clone.rs +++ b/clippy_lints/src/methods/map_clone.rs @@ -15,11 +15,11 @@ use rustc_span::{sym, Span}; use super::MAP_CLONE; -pub(super) fn check<'tcx>( +pub(super) fn check( cx: &LateContext<'_>, e: &hir::Expr<'_>, recv: &hir::Expr<'_>, - arg: &'tcx hir::Expr<'_>, + arg: &hir::Expr<'_>, msrv: Option, ) { if_chain! { diff --git a/clippy_lints/src/methods/map_collect_result_unit.rs b/clippy_lints/src/methods/map_collect_result_unit.rs index d420f144eea..a0300d27870 100644 --- a/clippy_lints/src/methods/map_collect_result_unit.rs +++ b/clippy_lints/src/methods/map_collect_result_unit.rs @@ -1,5 +1,4 @@ use clippy_utils::diagnostics::span_lint_and_sugg; -use clippy_utils::is_trait_method; use clippy_utils::source::snippet; use clippy_utils::ty::is_type_diagnostic_item; use if_chain::if_chain; @@ -11,18 +10,10 @@ use rustc_span::symbol::sym; use super::MAP_COLLECT_RESULT_UNIT; -pub(super) fn check( - cx: &LateContext<'_>, - expr: &hir::Expr<'_>, - iter: &hir::Expr<'_>, - map_fn: &hir::Expr<'_>, - collect_recv: &hir::Expr<'_>, -) { +pub(super) fn check(cx: &LateContext<'_>, expr: &hir::Expr<'_>, iter: &hir::Expr<'_>, map_fn: &hir::Expr<'_>) { + // return of collect `Result<(),_>` + let collect_ret_ty = cx.typeck_results().expr_ty(expr); if_chain! { - // called on Iterator - if is_trait_method(cx, collect_recv, sym::Iterator); - // return of collect `Result<(),_>` - let collect_ret_ty = cx.typeck_results().expr_ty(expr); if is_type_diagnostic_item(cx, collect_ret_ty, sym::Result); if let ty::Adt(_, substs) = collect_ret_ty.kind(); if let Some(result_t) = substs.types().next(); diff --git a/clippy_lints/src/methods/map_err_ignore.rs b/clippy_lints/src/methods/map_err_ignore.rs index 1fb6617145e..b773b3e423f 100644 --- a/clippy_lints/src/methods/map_err_ignore.rs +++ b/clippy_lints/src/methods/map_err_ignore.rs @@ -6,7 +6,7 @@ use rustc_span::sym; use super::MAP_ERR_IGNORE; -pub(super) fn check<'tcx>(cx: &LateContext<'_>, e: &Expr<'_>, arg: &'tcx Expr<'_>) { +pub(super) fn check(cx: &LateContext<'_>, e: &Expr<'_>, arg: &Expr<'_>) { if let Some(method_id) = cx.typeck_results().type_dependent_def_id(e.hir_id) && let Some(impl_id) = cx.tcx.impl_of_method(method_id) && is_type_diagnostic_item(cx, cx.tcx.type_of(impl_id), sym::Result) diff --git a/clippy_lints/src/methods/mod.rs b/clippy_lints/src/methods/mod.rs index 8a76ba0b064..38165ab4fb2 100644 --- a/clippy_lints/src/methods/mod.rs +++ b/clippy_lints/src/methods/mod.rs @@ -54,6 +54,7 @@ mod map_flatten; mod map_identity; mod map_unwrap_or; mod mut_mutex_lock; +mod needless_collect; mod needless_option_as_deref; mod needless_option_take; mod no_effect_replace; @@ -69,6 +70,8 @@ mod path_buf_push_overwrite; mod range_zip_with_len; mod repeat_once; mod search_is_some; +mod seek_from_current; +mod seek_to_start_instead_of_rewind; mod single_char_add_str; mod single_char_insert_string; mod single_char_pattern; @@ -101,12 +104,11 @@ mod zst_offset; use bind_instead_of_map::BindInsteadOfMap; use clippy_utils::consts::{constant, Constant}; use clippy_utils::diagnostics::{span_lint, span_lint_and_help}; -use clippy_utils::ty::{contains_adt_constructor, implements_trait, is_copy, is_type_diagnostic_item}; -use clippy_utils::{contains_return, is_trait_method, iter_input_pats, meets_msrv, msrvs, return_ty}; +use clippy_utils::ty::{contains_ty_adt_constructor_opaque, implements_trait, is_copy, is_type_diagnostic_item}; +use clippy_utils::{contains_return, is_bool, is_trait_method, iter_input_pats, meets_msrv, msrvs, return_ty}; use if_chain::if_chain; use rustc_hir as hir; -use rustc_hir::def::Res; -use rustc_hir::{Expr, ExprKind, PrimTy, QPath, TraitItem, TraitItemKind}; +use rustc_hir::{Expr, ExprKind, TraitItem, TraitItemKind}; use rustc_hir_analysis::hir_ty_to_ty; use rustc_lint::{LateContext, LateLintPass, LintContext}; use rustc_middle::lint::in_external_macro; @@ -156,9 +158,9 @@ declare_clippy_lint! { /// ``` /// Use instead: /// ```rust - /// let hello = "hesuo worpd".replace(&['s', 'u', 'p'], "l"); + /// let hello = "hesuo worpd".replace(['s', 'u', 'p'], "l"); /// ``` - #[clippy::version = "1.64.0"] + #[clippy::version = "1.65.0"] pub COLLAPSIBLE_STR_REPLACE, perf, "collapse consecutive calls to str::replace (2 or more) into a single call" @@ -829,32 +831,30 @@ declare_clippy_lint! { /// etc. instead. /// /// ### Why is this bad? - /// The function will always be called and potentially - /// allocate an object acting as the default. + /// The function will always be called. This is only bad if it allocates or + /// does some non-trivial amount of work. /// /// ### Known problems - /// If the function has side-effects, not calling it will - /// change the semantic of the program, but you shouldn't rely on that anyway. + /// If the function has side-effects, not calling it will change the + /// semantic of the program, but you shouldn't rely on that. + /// + /// The lint also cannot figure out whether the function you call is + /// actually expensive to call or not. /// /// ### Example /// ```rust /// # let foo = Some(String::new()); - /// foo.unwrap_or(String::new()); + /// foo.unwrap_or(String::from("empty")); /// ``` /// /// Use instead: /// ```rust /// # let foo = Some(String::new()); - /// foo.unwrap_or_else(String::new); - /// - /// // or - /// - /// # let foo = Some(String::new()); - /// foo.unwrap_or_default(); + /// foo.unwrap_or_else(|| String::from("empty")); /// ``` #[clippy::version = "pre 1.29.0"] pub OR_FUN_CALL, - perf, + nursery, "using any `*or` method with a function call, which suggests `*or_else`" } @@ -1728,7 +1728,7 @@ declare_clippy_lint! { declare_clippy_lint! { /// ### What it does - /// Checks for usage of `_.as_ref().map(Deref::deref)` or it's aliases (such as String::as_str). + /// Checks for usage of `_.as_ref().map(Deref::deref)` or its aliases (such as String::as_str). /// /// ### Why is this bad? /// Readability, this can be written more concisely as @@ -2094,8 +2094,7 @@ declare_clippy_lint! { /// let s = "Hello world!"; /// let cow = Cow::Borrowed(s); /// - /// let data = cow.into_owned(); - /// assert!(matches!(data, String)) + /// let _data: String = cow.into_owned(); /// ``` #[clippy::version = "1.65.0"] pub SUSPICIOUS_TO_OWNED, @@ -2426,7 +2425,7 @@ declare_clippy_lint! { /// ### Known problems /// /// The type of the resulting iterator might become incompatible with its usage - #[clippy::version = "1.64.0"] + #[clippy::version = "1.65.0"] pub ITER_ON_SINGLE_ITEMS, nursery, "Iterator for array of length 1" @@ -2458,7 +2457,7 @@ declare_clippy_lint! { /// ### Known problems /// /// The type of the resulting iterator might become incompatible with its usage - #[clippy::version = "1.64.0"] + #[clippy::version = "1.65.0"] pub ITER_ON_EMPTY_COLLECTIONS, nursery, "Iterator for empty array" @@ -3066,6 +3065,102 @@ declare_clippy_lint! { "iterating on map using `iter` when `keys` or `values` would do" } +declare_clippy_lint! { + /// ### What it does + /// + /// Checks an argument of `seek` method of `Seek` trait + /// and if it start seek from `SeekFrom::Current(0)`, suggests `stream_position` instead. + /// + /// ### Why is this bad? + /// + /// Readability. Use dedicated method. + /// + /// ### Example + /// + /// ```rust,no_run + /// use std::fs::File; + /// use std::io::{self, Write, Seek, SeekFrom}; + /// + /// fn main() -> io::Result<()> { + /// let mut f = File::create("foo.txt")?; + /// f.write_all(b"Hello")?; + /// eprintln!("Written {} bytes", f.seek(SeekFrom::Current(0))?); + /// + /// Ok(()) + /// } + /// ``` + /// Use instead: + /// ```rust,no_run + /// use std::fs::File; + /// use std::io::{self, Write, Seek, SeekFrom}; + /// + /// fn main() -> io::Result<()> { + /// let mut f = File::create("foo.txt")?; + /// f.write_all(b"Hello")?; + /// eprintln!("Written {} bytes", f.stream_position()?); + /// + /// Ok(()) + /// } + /// ``` + #[clippy::version = "1.66.0"] + pub SEEK_FROM_CURRENT, + complexity, + "use dedicated method for seek from current position" +} + +declare_clippy_lint! { + /// ### What it does + /// + /// Checks for jumps to the start of a stream that implements `Seek` + /// and uses the `seek` method providing `Start` as parameter. + /// + /// ### Why is this bad? + /// + /// Readability. There is a specific method that was implemented for + /// this exact scenario. + /// + /// ### Example + /// ```rust + /// # use std::io; + /// fn foo(t: &mut T) { + /// t.seek(io::SeekFrom::Start(0)); + /// } + /// ``` + /// Use instead: + /// ```rust + /// # use std::io; + /// fn foo(t: &mut T) { + /// t.rewind(); + /// } + /// ``` + #[clippy::version = "1.66.0"] + pub SEEK_TO_START_INSTEAD_OF_REWIND, + complexity, + "jumping to the start of stream using `seek` method" +} + +declare_clippy_lint! { + /// ### What it does + /// Checks for functions collecting an iterator when collect + /// is not needed. + /// + /// ### Why is this bad? + /// `collect` causes the allocation of a new data structure, + /// when this allocation may not be needed. + /// + /// ### Example + /// ```rust + /// # let iterator = vec![1].into_iter(); + /// let len = iterator.clone().collect::>().len(); + /// // should be + /// let len = iterator.count(); + /// ``` + #[clippy::version = "1.30.0"] + pub NEEDLESS_COLLECT, + nursery, + "collecting an iterator when collect is not needed" +} + pub struct Methods { avoid_breaking_exported_api: bool, msrv: Option, @@ -3190,16 +3285,19 @@ impl_lint_pass!(Methods => [ VEC_RESIZE_TO_ZERO, VERBOSE_FILE_READS, ITER_KV_MAP, + SEEK_FROM_CURRENT, + SEEK_TO_START_INSTEAD_OF_REWIND, + NEEDLESS_COLLECT, ]); /// Extracts a method call name, args, and `Span` of the method name. fn method_call<'tcx>( recv: &'tcx hir::Expr<'tcx>, -) -> Option<(&'tcx str, &'tcx hir::Expr<'tcx>, &'tcx [hir::Expr<'tcx>], Span)> { - if let ExprKind::MethodCall(path, receiver, args, _) = recv.kind { +) -> Option<(&'tcx str, &'tcx hir::Expr<'tcx>, &'tcx [hir::Expr<'tcx>], Span, Span)> { + if let ExprKind::MethodCall(path, receiver, args, call_span) = recv.kind { if !args.iter().any(|e| e.span.from_expansion()) && !receiver.span.from_expansion() { let name = path.ident.name.as_str(); - return Some((name, receiver, args, path.ident.span)); + return Some((name, receiver, args, path.ident.span, call_span)); } } None @@ -3316,36 +3414,10 @@ impl<'tcx> LateLintPass<'tcx> for Methods { if let hir::ImplItemKind::Fn(_, _) = impl_item.kind { let ret_ty = return_ty(cx, impl_item.hir_id()); - // walk the return type and check for Self (this does not check associated types) - if let Some(self_adt) = self_ty.ty_adt_def() { - if contains_adt_constructor(ret_ty, self_adt) { - return; - } - } else if ret_ty.contains(self_ty) { + if contains_ty_adt_constructor_opaque(cx, ret_ty, self_ty) { return; } - // if return type is impl trait, check the associated types - if let ty::Opaque(def_id, _) = *ret_ty.kind() { - // one of the associated types must be Self - for &(predicate, _span) in cx.tcx.explicit_item_bounds(def_id) { - if let ty::PredicateKind::Projection(projection_predicate) = predicate.kind().skip_binder() { - let assoc_ty = match projection_predicate.term.unpack() { - ty::TermKind::Ty(ty) => ty, - ty::TermKind::Const(_c) => continue, - }; - // walk the associated type and check for Self - if let Some(self_adt) = self_ty.ty_adt_def() { - if contains_adt_constructor(assoc_ty, self_adt) { - return; - } - } else if assoc_ty.contains(self_ty) { - return; - } - } - } - } - if name == "new" && ret_ty != self_ty { span_lint( cx, @@ -3411,7 +3483,7 @@ impl<'tcx> LateLintPass<'tcx> for Methods { impl Methods { #[allow(clippy::too_many_lines)] fn check_methods<'tcx>(&self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) { - if let Some((name, recv, args, span)) = method_call(expr) { + if let Some((name, recv, args, span, call_span)) = method_call(expr) { match (name, args) { ("add" | "offset" | "sub" | "wrapping_offset" | "wrapping_add" | "wrapping_sub", [_arg]) => { zst_offset::check(cx, expr, recv); @@ -3430,28 +3502,31 @@ impl Methods { ("as_ref", []) => useless_asref::check(cx, expr, "as_ref", recv), ("assume_init", []) => uninit_assumed_init::check(cx, expr, recv), ("cloned", []) => cloned_instead_of_copied::check(cx, expr, recv, span, self.msrv), - ("collect", []) => match method_call(recv) { - Some((name @ ("cloned" | "copied"), recv2, [], _)) => { - iter_cloned_collect::check(cx, name, expr, recv2); - }, - Some(("map", m_recv, [m_arg], _)) => { - map_collect_result_unit::check(cx, expr, m_recv, m_arg, recv); - }, - Some(("take", take_self_arg, [take_arg], _)) => { - if meets_msrv(self.msrv, msrvs::STR_REPEAT) { - manual_str_repeat::check(cx, expr, recv, take_self_arg, take_arg); - } - }, - _ => {}, + ("collect", []) if is_trait_method(cx, expr, sym::Iterator) => { + needless_collect::check(cx, span, expr, recv, call_span); + match method_call(recv) { + Some((name @ ("cloned" | "copied"), recv2, [], _, _)) => { + iter_cloned_collect::check(cx, name, expr, recv2); + }, + Some(("map", m_recv, [m_arg], _, _)) => { + map_collect_result_unit::check(cx, expr, m_recv, m_arg); + }, + Some(("take", take_self_arg, [take_arg], _, _)) => { + if meets_msrv(self.msrv, msrvs::STR_REPEAT) { + manual_str_repeat::check(cx, expr, recv, take_self_arg, take_arg); + } + }, + _ => {}, + } }, ("count", []) if is_trait_method(cx, expr, sym::Iterator) => match method_call(recv) { - Some(("cloned", recv2, [], _)) => iter_overeager_cloned::check(cx, expr, recv, recv2, true, false), - Some((name2 @ ("into_iter" | "iter" | "iter_mut"), recv2, [], _)) => { + Some(("cloned", recv2, [], _, _)) => iter_overeager_cloned::check(cx, expr, recv, recv2, true, false), + Some((name2 @ ("into_iter" | "iter" | "iter_mut"), recv2, [], _, _)) => { iter_count::check(cx, expr, recv2, name2); }, - Some(("map", _, [arg], _)) => suspicious_map::check(cx, expr, recv, arg), - Some(("filter", recv2, [arg], _)) => bytecount::check(cx, expr, recv2, arg), - Some(("bytes", recv2, [], _)) => bytes_count_to_len::check(cx, expr, recv, recv2), + Some(("map", _, [arg], _, _)) => suspicious_map::check(cx, expr, recv, arg), + Some(("filter", recv2, [arg], _, _)) => bytecount::check(cx, expr, recv2, arg), + Some(("bytes", recv2, [], _, _)) => bytes_count_to_len::check(cx, expr, recv, recv2), _ => {}, }, ("drain", [arg]) => { @@ -3463,8 +3538,8 @@ impl Methods { } }, ("expect", [_]) => match method_call(recv) { - Some(("ok", recv, [], _)) => ok_expect::check(cx, expr, recv), - Some(("err", recv, [], err_span)) => err_expect::check(cx, expr, recv, self.msrv, span, err_span), + Some(("ok", recv, [], _, _)) => ok_expect::check(cx, expr, recv), + Some(("err", recv, [], err_span, _)) => err_expect::check(cx, expr, recv, self.msrv, span, err_span), _ => expect_used::check(cx, expr, recv, false, self.allow_expect_in_tests), }, ("expect_err", [_]) => expect_used::check(cx, expr, recv, true, self.allow_expect_in_tests), @@ -3484,13 +3559,13 @@ impl Methods { flat_map_option::check(cx, expr, arg, span); }, ("flatten", []) => match method_call(recv) { - Some(("map", recv, [map_arg], map_span)) => map_flatten::check(cx, expr, recv, map_arg, map_span), - Some(("cloned", recv2, [], _)) => iter_overeager_cloned::check(cx, expr, recv, recv2, false, true), + Some(("map", recv, [map_arg], map_span, _)) => map_flatten::check(cx, expr, recv, map_arg, map_span), + Some(("cloned", recv2, [], _, _)) => iter_overeager_cloned::check(cx, expr, recv, recv2, false, true), _ => {}, }, ("fold", [init, acc]) => unnecessary_fold::check(cx, expr, init, acc, span), ("for_each", [_]) => { - if let Some(("inspect", _, [_], span2)) = method_call(recv) { + if let Some(("inspect", _, [_], span2, _)) = method_call(recv) { inspect_for_each::check(cx, expr, span2); } }, @@ -3510,12 +3585,12 @@ impl Methods { iter_on_single_or_empty_collections::check(cx, expr, name, recv); }, ("join", [join_arg]) => { - if let Some(("collect", _, _, span)) = method_call(recv) { + if let Some(("collect", _, _, span, _)) = method_call(recv) { unnecessary_join::check(cx, expr, recv, join_arg, span); } }, ("last", []) | ("skip", [_]) => { - if let Some((name2, recv2, args2, _span2)) = method_call(recv) { + if let Some((name2, recv2, args2, _span2, _)) = method_call(recv) { if let ("cloned", []) = (name2, args2) { iter_overeager_cloned::check(cx, expr, recv, recv2, false, false); } @@ -3527,13 +3602,13 @@ impl Methods { (name @ ("map" | "map_err"), [m_arg]) => { if name == "map" { map_clone::check(cx, expr, recv, m_arg, self.msrv); - if let Some((map_name @ ("iter" | "into_iter"), recv2, _, _)) = method_call(recv) { + if let Some((map_name @ ("iter" | "into_iter"), recv2, _, _, _)) = method_call(recv) { iter_kv_map::check(cx, map_name, expr, recv2, m_arg); } } else { map_err_ignore::check(cx, expr, m_arg); } - if let Some((name, recv2, args, span2)) = method_call(recv) { + if let Some((name, recv2, args, span2,_)) = method_call(recv) { match (name, args) { ("as_mut", []) => option_as_ref_deref::check(cx, expr, recv2, m_arg, true, self.msrv), ("as_ref", []) => option_as_ref_deref::check(cx, expr, recv2, m_arg, false, self.msrv), @@ -3553,7 +3628,7 @@ impl Methods { manual_ok_or::check(cx, expr, recv, def, map); }, ("next", []) => { - if let Some((name2, recv2, args2, _)) = method_call(recv) { + if let Some((name2, recv2, args2, _, _)) = method_call(recv) { match (name2, args2) { ("cloned", []) => iter_overeager_cloned::check(cx, expr, recv, recv2, false, false), ("filter", [arg]) => filter_next::check(cx, expr, recv2, arg), @@ -3566,10 +3641,10 @@ impl Methods { } }, ("nth", [n_arg]) => match method_call(recv) { - Some(("bytes", recv2, [], _)) => bytes_nth::check(cx, expr, recv2, n_arg), - Some(("cloned", recv2, [], _)) => iter_overeager_cloned::check(cx, expr, recv, recv2, false, false), - Some(("iter", recv2, [], _)) => iter_nth::check(cx, expr, recv2, recv, n_arg, false), - Some(("iter_mut", recv2, [], _)) => iter_nth::check(cx, expr, recv2, recv, n_arg, true), + Some(("bytes", recv2, [], _, _)) => bytes_nth::check(cx, expr, recv2, n_arg), + Some(("cloned", recv2, [], _, _)) => iter_overeager_cloned::check(cx, expr, recv, recv2, false, false), + Some(("iter", recv2, [], _, _)) => iter_nth::check(cx, expr, recv2, recv, n_arg, false), + Some(("iter_mut", recv2, [], _, _)) => iter_nth::check(cx, expr, recv2, recv, n_arg, true), _ => iter_nth_zero::check(cx, expr, recv, n_arg), }, ("ok_or_else", [arg]) => unnecessary_lazy_eval::check(cx, expr, recv, arg, "ok_or"), @@ -3604,6 +3679,14 @@ impl Methods { ("resize", [count_arg, default_arg]) => { vec_resize_to_zero::check(cx, expr, count_arg, default_arg, span); }, + ("seek", [arg]) => { + if meets_msrv(self.msrv, msrvs::SEEK_FROM_CURRENT) { + seek_from_current::check(cx, expr, recv, arg); + } + if meets_msrv(self.msrv, msrvs::SEEK_REWIND) { + seek_to_start_instead_of_rewind::check(cx, expr, recv, arg, span); + } + }, ("sort", []) => { stable_sort_primitive::check(cx, expr, recv); }, @@ -3626,7 +3709,7 @@ impl Methods { }, ("step_by", [arg]) => iterator_step_by_zero::check(cx, expr, arg), ("take", [_arg]) => { - if let Some((name2, recv2, args2, _span2)) = method_call(recv) { + if let Some((name2, recv2, args2, _span2, _)) = method_call(recv) { if let ("cloned", []) = (name2, args2) { iter_overeager_cloned::check(cx, expr, recv, recv2, false, false); } @@ -3649,13 +3732,13 @@ impl Methods { }, ("unwrap", []) => { match method_call(recv) { - Some(("get", recv, [get_arg], _)) => { + Some(("get", recv, [get_arg], _, _)) => { get_unwrap::check(cx, expr, recv, get_arg, false); }, - Some(("get_mut", recv, [get_arg], _)) => { + Some(("get_mut", recv, [get_arg], _, _)) => { get_unwrap::check(cx, expr, recv, get_arg, true); }, - Some(("or", recv, [or_arg], or_span)) => { + Some(("or", recv, [or_arg], or_span, _)) => { or_then_unwrap::check(cx, expr, recv, or_arg, or_span); }, _ => {}, @@ -3664,19 +3747,19 @@ impl Methods { }, ("unwrap_err", []) => unwrap_used::check(cx, expr, recv, true, self.allow_unwrap_in_tests), ("unwrap_or", [u_arg]) => match method_call(recv) { - Some((arith @ ("checked_add" | "checked_sub" | "checked_mul"), lhs, [rhs], _)) => { + Some((arith @ ("checked_add" | "checked_sub" | "checked_mul"), lhs, [rhs], _, _)) => { manual_saturating_arithmetic::check(cx, expr, lhs, rhs, u_arg, &arith["checked_".len()..]); }, - Some(("map", m_recv, [m_arg], span)) => { + Some(("map", m_recv, [m_arg], span, _)) => { option_map_unwrap_or::check(cx, expr, m_recv, m_arg, recv, u_arg, span); }, - Some(("then_some", t_recv, [t_arg], _)) => { + Some(("then_some", t_recv, [t_arg], _, _)) => { obfuscated_if_else::check(cx, expr, t_recv, t_arg, u_arg); }, _ => {}, }, ("unwrap_or_else", [u_arg]) => match method_call(recv) { - Some(("map", recv, [map_arg], _)) + Some(("map", recv, [map_arg], _, _)) if map_unwrap_or::check(cx, expr, recv, map_arg, u_arg, self.msrv) => {}, _ => { unwrap_or_else_default::check(cx, expr, recv, u_arg); @@ -3697,7 +3780,7 @@ impl Methods { } fn check_is_some_is_none(cx: &LateContext<'_>, expr: &Expr<'_>, recv: &Expr<'_>, is_some: bool) { - if let Some((name @ ("find" | "position" | "rposition"), f_recv, [arg], span)) = method_call(recv) { + if let Some((name @ ("find" | "position" | "rposition"), f_recv, [arg], span, _)) = method_call(recv) { search_is_some::check(cx, expr, name, is_some, f_recv, arg, recv, span); } } @@ -3906,14 +3989,6 @@ impl OutType { } } -fn is_bool(ty: &hir::Ty<'_>) -> bool { - if let hir::TyKind::Path(QPath::Resolved(_, path)) = ty.kind { - matches!(path.res, Res::PrimTy(PrimTy::Bool)) - } else { - false - } -} - fn fn_header_equals(expected: hir::FnHeader, actual: hir::FnHeader) -> bool { expected.constness == actual.constness && expected.unsafety == actual.unsafety diff --git a/clippy_lints/src/methods/needless_collect.rs b/clippy_lints/src/methods/needless_collect.rs new file mode 100644 index 00000000000..b088e642e0e --- /dev/null +++ b/clippy_lints/src/methods/needless_collect.rs @@ -0,0 +1,456 @@ +use super::NEEDLESS_COLLECT; +use clippy_utils::diagnostics::{span_lint_and_sugg, span_lint_hir_and_then}; +use clippy_utils::higher; +use clippy_utils::source::{snippet, snippet_with_applicability}; +use clippy_utils::sugg::Sugg; +use clippy_utils::ty::{is_type_diagnostic_item, make_normalized_projection, make_projection}; +use clippy_utils::{ + can_move_expr_to_closure, get_enclosing_block, get_parent_node, is_trait_method, path_to_local, path_to_local_id, + CaptureKind, +}; +use rustc_data_structures::fx::FxHashMap; +use rustc_errors::{Applicability, MultiSpan}; +use rustc_hir::intravisit::{walk_block, walk_expr, Visitor}; +use rustc_hir::{ + BindingAnnotation, Block, Expr, ExprKind, HirId, HirIdSet, Local, Mutability, Node, PatKind, Stmt, StmtKind, +}; +use rustc_lint::LateContext; +use rustc_middle::hir::nested_filter; +use rustc_middle::ty::{self, AssocKind, EarlyBinder, GenericArg, GenericArgKind, Ty}; +use rustc_span::symbol::Ident; +use rustc_span::{sym, Span, Symbol}; + +const NEEDLESS_COLLECT_MSG: &str = "avoid using `collect()` when not needed"; + +pub(super) fn check<'tcx>( + cx: &LateContext<'tcx>, + name_span: Span, + collect_expr: &'tcx Expr<'_>, + iter_expr: &'tcx Expr<'tcx>, + call_span: Span, +) { + if let Some(parent) = get_parent_node(cx.tcx, collect_expr.hir_id) { + match parent { + Node::Expr(parent) => { + if let ExprKind::MethodCall(name, _, args @ ([] | [_]), _) = parent.kind { + let mut app = Applicability::MachineApplicable; + let name = name.ident.as_str(); + let collect_ty = cx.typeck_results().expr_ty(collect_expr); + + let sugg: String = match name { + "len" => { + if let Some(adt) = collect_ty.ty_adt_def() + && matches!( + cx.tcx.get_diagnostic_name(adt.did()), + Some(sym::Vec | sym::VecDeque | sym::LinkedList | sym::BinaryHeap) + ) + { + "count()".into() + } else { + return; + } + }, + "is_empty" + if is_is_empty_sig(cx, parent.hir_id) + && iterates_same_ty(cx, cx.typeck_results().expr_ty(iter_expr), collect_ty) => + { + "next().is_none()".into() + }, + "contains" => { + if is_contains_sig(cx, parent.hir_id, iter_expr) + && let Some(arg) = args.first() + { + let (span, prefix) = if let ExprKind::AddrOf(_, _, arg) = arg.kind { + (arg.span, "") + } else { + (arg.span, "*") + }; + let snip = snippet_with_applicability(cx, span, "??", &mut app); + format!("any(|x| x == {prefix}{snip})") + } else { + return; + } + }, + _ => return, + }; + + span_lint_and_sugg( + cx, + NEEDLESS_COLLECT, + call_span.with_hi(parent.span.hi()), + NEEDLESS_COLLECT_MSG, + "replace with", + sugg, + app, + ); + } + }, + Node::Local(l) => { + if let PatKind::Binding(BindingAnnotation::NONE | BindingAnnotation::MUT, id, _, None) + = l.pat.kind + && let ty = cx.typeck_results().expr_ty(collect_expr) + && [sym::Vec, sym::VecDeque, sym::BinaryHeap, sym::LinkedList].into_iter() + .any(|item| is_type_diagnostic_item(cx, ty, item)) + && let iter_ty = cx.typeck_results().expr_ty(iter_expr) + && let Some(block) = get_enclosing_block(cx, l.hir_id) + && let Some(iter_calls) = detect_iter_and_into_iters(block, id, cx, get_captured_ids(cx, iter_ty)) + && let [iter_call] = &*iter_calls + { + let mut used_count_visitor = UsedCountVisitor { + cx, + id, + count: 0, + }; + walk_block(&mut used_count_visitor, block); + if used_count_visitor.count > 1 { + return; + } + + // Suggest replacing iter_call with iter_replacement, and removing stmt + let mut span = MultiSpan::from_span(name_span); + span.push_span_label(iter_call.span, "the iterator could be used here instead"); + span_lint_hir_and_then( + cx, + super::NEEDLESS_COLLECT, + collect_expr.hir_id, + span, + NEEDLESS_COLLECT_MSG, + |diag| { + let iter_replacement = format!("{}{}", Sugg::hir(cx, iter_expr, ".."), iter_call.get_iter_method(cx)); + diag.multipart_suggestion( + iter_call.get_suggestion_text(), + vec![ + (l.span, String::new()), + (iter_call.span, iter_replacement) + ], + Applicability::MaybeIncorrect, + ); + }, + ); + } + }, + _ => (), + } + } +} + +/// Checks if the given method call matches the expected signature of `([&[mut]] self) -> bool` +fn is_is_empty_sig(cx: &LateContext<'_>, call_id: HirId) -> bool { + cx.typeck_results().type_dependent_def_id(call_id).map_or(false, |id| { + let sig = cx.tcx.fn_sig(id).skip_binder(); + sig.inputs().len() == 1 && sig.output().is_bool() + }) +} + +/// Checks if `::Item` is the same as `::Item` +fn iterates_same_ty<'tcx>(cx: &LateContext<'tcx>, iter_ty: Ty<'tcx>, collect_ty: Ty<'tcx>) -> bool { + let item = Symbol::intern("Item"); + if let Some(iter_trait) = cx.tcx.get_diagnostic_item(sym::Iterator) + && let Some(into_iter_trait) = cx.tcx.get_diagnostic_item(sym::IntoIterator) + && let Some(iter_item_ty) = make_normalized_projection(cx.tcx, cx.param_env, iter_trait, item, [iter_ty]) + && let Some(into_iter_item_proj) = make_projection(cx.tcx, into_iter_trait, item, [collect_ty]) + && let Ok(into_iter_item_ty) = cx.tcx.try_normalize_erasing_regions( + cx.param_env, + cx.tcx.mk_projection(into_iter_item_proj.item_def_id, into_iter_item_proj.substs) + ) + { + iter_item_ty == into_iter_item_ty + } else { + false + } +} + +/// Checks if the given method call matches the expected signature of +/// `([&[mut]] self, &::Item) -> bool` +fn is_contains_sig(cx: &LateContext<'_>, call_id: HirId, iter_expr: &Expr<'_>) -> bool { + let typeck = cx.typeck_results(); + if let Some(id) = typeck.type_dependent_def_id(call_id) + && let sig = cx.tcx.fn_sig(id) + && sig.skip_binder().output().is_bool() + && let [_, search_ty] = *sig.skip_binder().inputs() + && let ty::Ref(_, search_ty, Mutability::Not) = *cx.tcx.erase_late_bound_regions(sig.rebind(search_ty)).kind() + && let Some(iter_trait) = cx.tcx.get_diagnostic_item(sym::Iterator) + && let Some(iter_item) = cx.tcx + .associated_items(iter_trait) + .find_by_name_and_kind(cx.tcx, Ident::with_dummy_span(Symbol::intern("Item")), AssocKind::Type, iter_trait) + && let substs = cx.tcx.mk_substs([GenericArg::from(typeck.expr_ty_adjusted(iter_expr))].into_iter()) + && let proj_ty = cx.tcx.mk_projection(iter_item.def_id, substs) + && let Ok(item_ty) = cx.tcx.try_normalize_erasing_regions(cx.param_env, proj_ty) + { + item_ty == EarlyBinder(search_ty).subst(cx.tcx, cx.typeck_results().node_substs(call_id)) + } else { + false + } +} + +struct IterFunction { + func: IterFunctionKind, + span: Span, +} +impl IterFunction { + fn get_iter_method(&self, cx: &LateContext<'_>) -> String { + match &self.func { + IterFunctionKind::IntoIter => String::new(), + IterFunctionKind::Len => String::from(".count()"), + IterFunctionKind::IsEmpty => String::from(".next().is_none()"), + IterFunctionKind::Contains(span) => { + let s = snippet(cx, *span, ".."); + if let Some(stripped) = s.strip_prefix('&') { + format!(".any(|x| x == {stripped})") + } else { + format!(".any(|x| x == *{s})") + } + }, + } + } + fn get_suggestion_text(&self) -> &'static str { + match &self.func { + IterFunctionKind::IntoIter => { + "use the original Iterator instead of collecting it and then producing a new one" + }, + IterFunctionKind::Len => { + "take the original Iterator's count instead of collecting it and finding the length" + }, + IterFunctionKind::IsEmpty => { + "check if the original Iterator has anything instead of collecting it and seeing if it's empty" + }, + IterFunctionKind::Contains(_) => { + "check if the original Iterator contains an element instead of collecting then checking" + }, + } + } +} +enum IterFunctionKind { + IntoIter, + Len, + IsEmpty, + Contains(Span), +} + +struct IterFunctionVisitor<'a, 'tcx> { + illegal_mutable_capture_ids: HirIdSet, + current_mutably_captured_ids: HirIdSet, + cx: &'a LateContext<'tcx>, + uses: Vec>, + hir_id_uses_map: FxHashMap, + current_statement_hir_id: Option, + seen_other: bool, + target: HirId, +} +impl<'tcx> Visitor<'tcx> for IterFunctionVisitor<'_, 'tcx> { + fn visit_block(&mut self, block: &'tcx Block<'tcx>) { + for (expr, hir_id) in block.stmts.iter().filter_map(get_expr_and_hir_id_from_stmt) { + if check_loop_kind(expr).is_some() { + continue; + } + self.visit_block_expr(expr, hir_id); + } + if let Some(expr) = block.expr { + if let Some(loop_kind) = check_loop_kind(expr) { + if let LoopKind::Conditional(block_expr) = loop_kind { + self.visit_block_expr(block_expr, None); + } + } else { + self.visit_block_expr(expr, None); + } + } + } + + fn visit_expr(&mut self, expr: &'tcx Expr<'tcx>) { + // Check function calls on our collection + if let ExprKind::MethodCall(method_name, recv, [args @ ..], _) = &expr.kind { + if method_name.ident.name == sym!(collect) && is_trait_method(self.cx, expr, sym::Iterator) { + self.current_mutably_captured_ids = get_captured_ids(self.cx, self.cx.typeck_results().expr_ty(recv)); + self.visit_expr(recv); + return; + } + + if path_to_local_id(recv, self.target) { + if self + .illegal_mutable_capture_ids + .intersection(&self.current_mutably_captured_ids) + .next() + .is_none() + { + if let Some(hir_id) = self.current_statement_hir_id { + self.hir_id_uses_map.insert(hir_id, self.uses.len()); + } + match method_name.ident.name.as_str() { + "into_iter" => self.uses.push(Some(IterFunction { + func: IterFunctionKind::IntoIter, + span: expr.span, + })), + "len" => self.uses.push(Some(IterFunction { + func: IterFunctionKind::Len, + span: expr.span, + })), + "is_empty" => self.uses.push(Some(IterFunction { + func: IterFunctionKind::IsEmpty, + span: expr.span, + })), + "contains" => self.uses.push(Some(IterFunction { + func: IterFunctionKind::Contains(args[0].span), + span: expr.span, + })), + _ => { + self.seen_other = true; + if let Some(hir_id) = self.current_statement_hir_id { + self.hir_id_uses_map.remove(&hir_id); + } + }, + } + } + return; + } + + if let Some(hir_id) = path_to_local(recv) { + if let Some(index) = self.hir_id_uses_map.remove(&hir_id) { + if self + .illegal_mutable_capture_ids + .intersection(&self.current_mutably_captured_ids) + .next() + .is_none() + { + if let Some(hir_id) = self.current_statement_hir_id { + self.hir_id_uses_map.insert(hir_id, index); + } + } else { + self.uses[index] = None; + } + } + } + } + // Check if the collection is used for anything else + if path_to_local_id(expr, self.target) { + self.seen_other = true; + } else { + walk_expr(self, expr); + } + } +} + +enum LoopKind<'tcx> { + Conditional(&'tcx Expr<'tcx>), + Loop, +} + +fn check_loop_kind<'tcx>(expr: &Expr<'tcx>) -> Option> { + if let Some(higher::WhileLet { let_expr, .. }) = higher::WhileLet::hir(expr) { + return Some(LoopKind::Conditional(let_expr)); + } + if let Some(higher::While { condition, .. }) = higher::While::hir(expr) { + return Some(LoopKind::Conditional(condition)); + } + if let Some(higher::ForLoop { arg, .. }) = higher::ForLoop::hir(expr) { + return Some(LoopKind::Conditional(arg)); + } + if let ExprKind::Loop { .. } = expr.kind { + return Some(LoopKind::Loop); + } + + None +} + +impl<'tcx> IterFunctionVisitor<'_, 'tcx> { + fn visit_block_expr(&mut self, expr: &'tcx Expr<'tcx>, hir_id: Option) { + self.current_statement_hir_id = hir_id; + self.current_mutably_captured_ids = get_captured_ids(self.cx, self.cx.typeck_results().expr_ty(expr)); + self.visit_expr(expr); + } +} + +fn get_expr_and_hir_id_from_stmt<'v>(stmt: &'v Stmt<'v>) -> Option<(&'v Expr<'v>, Option)> { + match stmt.kind { + StmtKind::Expr(expr) | StmtKind::Semi(expr) => Some((expr, None)), + StmtKind::Item(..) => None, + StmtKind::Local(Local { init, pat, .. }) => { + if let PatKind::Binding(_, hir_id, ..) = pat.kind { + init.map(|init_expr| (init_expr, Some(hir_id))) + } else { + init.map(|init_expr| (init_expr, None)) + } + }, + } +} + +struct UsedCountVisitor<'a, 'tcx> { + cx: &'a LateContext<'tcx>, + id: HirId, + count: usize, +} + +impl<'a, 'tcx> Visitor<'tcx> for UsedCountVisitor<'a, 'tcx> { + type NestedFilter = nested_filter::OnlyBodies; + + fn visit_expr(&mut self, expr: &'tcx Expr<'_>) { + if path_to_local_id(expr, self.id) { + self.count += 1; + } else { + walk_expr(self, expr); + } + } + + fn nested_visit_map(&mut self) -> Self::Map { + self.cx.tcx.hir() + } +} + +/// Detect the occurrences of calls to `iter` or `into_iter` for the +/// given identifier +fn detect_iter_and_into_iters<'tcx: 'a, 'a>( + block: &'tcx Block<'tcx>, + id: HirId, + cx: &'a LateContext<'tcx>, + captured_ids: HirIdSet, +) -> Option> { + let mut visitor = IterFunctionVisitor { + uses: Vec::new(), + target: id, + seen_other: false, + cx, + current_mutably_captured_ids: HirIdSet::default(), + illegal_mutable_capture_ids: captured_ids, + hir_id_uses_map: FxHashMap::default(), + current_statement_hir_id: None, + }; + visitor.visit_block(block); + if visitor.seen_other { + None + } else { + Some(visitor.uses.into_iter().flatten().collect()) + } +} + +fn get_captured_ids(cx: &LateContext<'_>, ty: Ty<'_>) -> HirIdSet { + fn get_captured_ids_recursive(cx: &LateContext<'_>, ty: Ty<'_>, set: &mut HirIdSet) { + match ty.kind() { + ty::Adt(_, generics) => { + for generic in *generics { + if let GenericArgKind::Type(ty) = generic.unpack() { + get_captured_ids_recursive(cx, ty, set); + } + } + }, + ty::Closure(def_id, _) => { + let closure_hir_node = cx.tcx.hir().get_if_local(*def_id).unwrap(); + if let Node::Expr(closure_expr) = closure_hir_node { + can_move_expr_to_closure(cx, closure_expr) + .unwrap() + .into_iter() + .for_each(|(hir_id, capture_kind)| { + if matches!(capture_kind, CaptureKind::Ref(Mutability::Mut)) { + set.insert(hir_id); + } + }); + } + }, + _ => (), + } + } + + let mut set = HirIdSet::default(); + + get_captured_ids_recursive(cx, ty, &mut set); + + set +} diff --git a/clippy_lints/src/methods/option_as_ref_deref.rs b/clippy_lints/src/methods/option_as_ref_deref.rs index 742483e6b2e..e6eb64bcbde 100644 --- a/clippy_lints/src/methods/option_as_ref_deref.rs +++ b/clippy_lints/src/methods/option_as_ref_deref.rs @@ -13,8 +13,8 @@ use rustc_span::sym; use super::OPTION_AS_REF_DEREF; /// lint use of `_.as_ref().map(Deref::deref)` for `Option`s -pub(super) fn check<'tcx>( - cx: &LateContext<'tcx>, +pub(super) fn check( + cx: &LateContext<'_>, expr: &hir::Expr<'_>, as_ref_recv: &hir::Expr<'_>, map_arg: &hir::Expr<'_>, diff --git a/clippy_lints/src/methods/or_fun_call.rs b/clippy_lints/src/methods/or_fun_call.rs index 991d3dd538b..4460f38fcc1 100644 --- a/clippy_lints/src/methods/or_fun_call.rs +++ b/clippy_lints/src/methods/or_fun_call.rs @@ -83,6 +83,8 @@ pub(super) fn check<'tcx>( method_span: Span, self_expr: &hir::Expr<'_>, arg: &'tcx hir::Expr<'_>, + // `Some` if fn has second argument + second_arg: Option<&hir::Expr<'_>>, span: Span, // None if lambda is required fun_span: Option, @@ -109,30 +111,40 @@ pub(super) fn check<'tcx>( if poss.contains(&name); then { - let macro_expanded_snipped; - let sugg: Cow<'_, str> = { + let sugg = { let (snippet_span, use_lambda) = match (fn_has_arguments, fun_span) { (false, Some(fun_span)) => (fun_span, false), _ => (arg.span, true), }; - let snippet = { - let not_macro_argument_snippet = snippet_with_macro_callsite(cx, snippet_span, ".."); - if not_macro_argument_snippet == "vec![]" { - macro_expanded_snipped = snippet(cx, snippet_span, ".."); + + let format_span = |span: Span| { + let not_macro_argument_snippet = snippet_with_macro_callsite(cx, span, ".."); + let snip = if not_macro_argument_snippet == "vec![]" { + let macro_expanded_snipped = snippet(cx, snippet_span, ".."); match macro_expanded_snipped.strip_prefix("$crate::vec::") { - Some(stripped) => Cow::from(stripped), + Some(stripped) => Cow::Owned(stripped.to_owned()), None => macro_expanded_snipped, } } else { not_macro_argument_snippet - } + }; + + snip.to_string() }; - if use_lambda { + let snip = format_span(snippet_span); + let snip = if use_lambda { let l_arg = if fn_has_arguments { "_" } else { "" }; - format!("|{l_arg}| {snippet}").into() + format!("|{l_arg}| {snip}") } else { - snippet + snip + }; + + if let Some(f) = second_arg { + let f = format_span(f.span); + format!("{snip}, {f}") + } else { + snip } }; let span_replace_word = method_span.with_hi(span.hi()); @@ -149,8 +161,8 @@ pub(super) fn check<'tcx>( } } - if let [arg] = args { - let inner_arg = if let hir::ExprKind::Block( + let extract_inner_arg = |arg: &'tcx hir::Expr<'_>| { + if let hir::ExprKind::Block( hir::Block { stmts: [], expr: Some(expr), @@ -162,19 +174,32 @@ pub(super) fn check<'tcx>( expr } else { arg - }; + } + }; + + if let [arg] = args { + let inner_arg = extract_inner_arg(arg); match inner_arg.kind { hir::ExprKind::Call(fun, or_args) => { let or_has_args = !or_args.is_empty(); if !check_unwrap_or_default(cx, name, fun, arg, or_has_args, expr.span, method_span) { let fun_span = if or_has_args { None } else { Some(fun.span) }; - check_general_case(cx, name, method_span, receiver, arg, expr.span, fun_span); + check_general_case(cx, name, method_span, receiver, arg, None, expr.span, fun_span); } }, hir::ExprKind::Index(..) | hir::ExprKind::MethodCall(..) => { - check_general_case(cx, name, method_span, receiver, arg, expr.span, None); + check_general_case(cx, name, method_span, receiver, arg, None, expr.span, None); }, _ => (), } } + + // `map_or` takes two arguments + if let [arg, lambda] = args { + let inner_arg = extract_inner_arg(arg); + if let hir::ExprKind::Call(fun, or_args) = inner_arg.kind { + let fun_span = if or_args.is_empty() { Some(fun.span) } else { None }; + check_general_case(cx, name, method_span, receiver, arg, Some(lambda), expr.span, fun_span); + } + } } diff --git a/clippy_lints/src/methods/seek_from_current.rs b/clippy_lints/src/methods/seek_from_current.rs new file mode 100644 index 00000000000..361a3082f94 --- /dev/null +++ b/clippy_lints/src/methods/seek_from_current.rs @@ -0,0 +1,48 @@ +use rustc_ast::ast::{LitIntType, LitKind}; +use rustc_errors::Applicability; +use rustc_hir::{Expr, ExprKind}; +use rustc_lint::LateContext; + +use clippy_utils::{ + diagnostics::span_lint_and_sugg, get_trait_def_id, match_def_path, paths, source::snippet_with_applicability, + ty::implements_trait, +}; + +use super::SEEK_FROM_CURRENT; + +pub(super) fn check<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>, recv: &'tcx Expr<'_>, arg: &'tcx Expr<'_>) { + let ty = cx.typeck_results().expr_ty(recv); + + if let Some(def_id) = get_trait_def_id(cx, &paths::STD_IO_SEEK) { + if implements_trait(cx, ty, def_id, &[]) && arg_is_seek_from_current(cx, arg) { + let mut applicability = Applicability::MachineApplicable; + let snip = snippet_with_applicability(cx, recv.span, "..", &mut applicability); + + span_lint_and_sugg( + cx, + SEEK_FROM_CURRENT, + expr.span, + "using `SeekFrom::Current` to start from current position", + "replace with", + format!("{snip}.stream_position()"), + applicability, + ); + } + } +} + +fn arg_is_seek_from_current<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) -> bool { + if let ExprKind::Call(f, args) = expr.kind && + let ExprKind::Path(ref path) = f.kind && + let Some(def_id) = cx.qpath_res(path, f.hir_id).opt_def_id() && + match_def_path(cx, def_id, &paths::STD_IO_SEEK_FROM_CURRENT) { + // check if argument of `SeekFrom::Current` is `0` + if args.len() == 1 && + let ExprKind::Lit(ref lit) = args[0].kind && + let LitKind::Int(0, LitIntType::Unsuffixed) = lit.node { + return true + } + } + + false +} diff --git a/clippy_lints/src/methods/seek_to_start_instead_of_rewind.rs b/clippy_lints/src/methods/seek_to_start_instead_of_rewind.rs new file mode 100644 index 00000000000..7e3bed1e41a --- /dev/null +++ b/clippy_lints/src/methods/seek_to_start_instead_of_rewind.rs @@ -0,0 +1,45 @@ +use clippy_utils::diagnostics::span_lint_and_then; +use clippy_utils::ty::implements_trait; +use clippy_utils::{get_trait_def_id, match_def_path, paths}; +use rustc_ast::ast::{LitIntType, LitKind}; +use rustc_errors::Applicability; +use rustc_hir::{Expr, ExprKind}; +use rustc_lint::LateContext; +use rustc_span::Span; + +use super::SEEK_TO_START_INSTEAD_OF_REWIND; + +pub(super) fn check<'tcx>( + cx: &LateContext<'tcx>, + expr: &'tcx Expr<'_>, + recv: &'tcx Expr<'_>, + arg: &'tcx Expr<'_>, + name_span: Span, +) { + // Get receiver type + let ty = cx.typeck_results().expr_ty(recv).peel_refs(); + + if let Some(seek_trait_id) = get_trait_def_id(cx, &paths::STD_IO_SEEK) && + implements_trait(cx, ty, seek_trait_id, &[]) && + let ExprKind::Call(func, args1) = arg.kind && + let ExprKind::Path(ref path) = func.kind && + let Some(def_id) = cx.qpath_res(path, func.hir_id).opt_def_id() && + match_def_path(cx, def_id, &paths::STD_IO_SEEKFROM_START) && + args1.len() == 1 && + let ExprKind::Lit(ref lit) = args1[0].kind && + let LitKind::Int(0, LitIntType::Unsuffixed) = lit.node + { + let method_call_span = expr.span.with_lo(name_span.lo()); + span_lint_and_then( + cx, + SEEK_TO_START_INSTEAD_OF_REWIND, + method_call_span, + "used `seek` to go to the start of the stream", + |diag| { + let app = Applicability::MachineApplicable; + + diag.span_suggestion(method_call_span, "replace with", "rewind()", app); + }, + ); + } +} diff --git a/clippy_lints/src/methods/string_extend_chars.rs b/clippy_lints/src/methods/string_extend_chars.rs index 6f4cec546e9..f35d81cee8e 100644 --- a/clippy_lints/src/methods/string_extend_chars.rs +++ b/clippy_lints/src/methods/string_extend_chars.rs @@ -18,7 +18,11 @@ pub(super) fn check(cx: &LateContext<'_>, expr: &hir::Expr<'_>, recv: &hir::Expr let target = &arglists[0].0; let self_ty = cx.typeck_results().expr_ty(target).peel_refs(); let ref_str = if *self_ty.kind() == ty::Str { - "" + if matches!(target.kind, hir::ExprKind::Index(..)) { + "&" + } else { + "" + } } else if is_type_lang_item(cx, self_ty, hir::LangItem::String) { "&" } else { diff --git a/clippy_lints/src/methods/suspicious_map.rs b/clippy_lints/src/methods/suspicious_map.rs index 851cdf54455..2ac0786b37b 100644 --- a/clippy_lints/src/methods/suspicious_map.rs +++ b/clippy_lints/src/methods/suspicious_map.rs @@ -8,7 +8,7 @@ use rustc_span::sym; use super::SUSPICIOUS_MAP; -pub fn check<'tcx>(cx: &LateContext<'tcx>, expr: &hir::Expr<'_>, count_recv: &hir::Expr<'_>, map_arg: &hir::Expr<'_>) { +pub fn check(cx: &LateContext<'_>, expr: &hir::Expr<'_>, count_recv: &hir::Expr<'_>, map_arg: &hir::Expr<'_>) { if_chain! { if is_trait_method(cx, count_recv, sym::Iterator); let closure = expr_or_init(cx, map_arg); diff --git a/clippy_lints/src/methods/unnecessary_join.rs b/clippy_lints/src/methods/unnecessary_join.rs index c9b87bc6bf2..087e1e4343b 100644 --- a/clippy_lints/src/methods/unnecessary_join.rs +++ b/clippy_lints/src/methods/unnecessary_join.rs @@ -31,7 +31,7 @@ pub(super) fn check<'tcx>( cx, UNNECESSARY_JOIN, span.with_hi(expr.span.hi()), - r#"called `.collect>().join("")` on an iterator"#, + r#"called `.collect::>().join("")` on an iterator"#, "try using", "collect::()".to_owned(), applicability, diff --git a/clippy_lints/src/methods/unwrap_used.rs b/clippy_lints/src/methods/unwrap_used.rs index ee17f2d7889..90983f249cd 100644 --- a/clippy_lints/src/methods/unwrap_used.rs +++ b/clippy_lints/src/methods/unwrap_used.rs @@ -1,6 +1,6 @@ use clippy_utils::diagnostics::span_lint_and_help; use clippy_utils::ty::is_type_diagnostic_item; -use clippy_utils::{is_in_test_function, is_lint_allowed}; +use clippy_utils::{is_in_cfg_test, is_lint_allowed}; use rustc_hir as hir; use rustc_lint::LateContext; use rustc_span::sym; @@ -18,16 +18,16 @@ pub(super) fn check( let obj_ty = cx.typeck_results().expr_ty(recv).peel_refs(); let mess = if is_type_diagnostic_item(cx, obj_ty, sym::Option) && !is_err { - Some((UNWRAP_USED, "an Option", "None", "")) + Some((UNWRAP_USED, "an `Option`", "None", "")) } else if is_type_diagnostic_item(cx, obj_ty, sym::Result) { - Some((UNWRAP_USED, "a Result", if is_err { "Ok" } else { "Err" }, "an ")) + Some((UNWRAP_USED, "a `Result`", if is_err { "Ok" } else { "Err" }, "an ")) } else { None }; let method_suffix = if is_err { "_err" } else { "" }; - if allow_unwrap_in_tests && is_in_test_function(cx.tcx, expr.hir_id) { + if allow_unwrap_in_tests && is_in_cfg_test(cx.tcx, expr.hir_id) { return; } @@ -45,7 +45,7 @@ pub(super) fn check( cx, lint, expr.span, - &format!("used `unwrap{method_suffix}()` on `{kind}` value"), + &format!("used `unwrap{method_suffix}()` on {kind} value"), None, &help, ); -- cgit 1.4.1-3-g733a5