From 77ed0241c8c57a3b5899278d9589a9e17170d6a3 Mon Sep 17 00:00:00 2001 From: wowinter13 Date: Thu, 30 Jan 2025 14:02:13 +0100 Subject: [PATCH 1/7] new lint: unnecessary_reserve --- CHANGELOG.md | 1 + clippy_lints/src/declared_lints.rs | 1 + clippy_lints/src/lib.rs | 2 + clippy_lints/src/unnecessary_reserve.rs | 129 ++++++++++++++++++++++++ clippy_utils/src/msrvs.rs | 2 +- clippy_utils/src/paths.rs | 1 + tests/ui/unnecessary_reserve.fixed | 99 ++++++++++++++++++ tests/ui/unnecessary_reserve.rs | 100 ++++++++++++++++++ tests/ui/unnecessary_reserve.stderr | 96 ++++++++++++++++++ 9 files changed, 430 insertions(+), 1 deletion(-) create mode 100644 clippy_lints/src/unnecessary_reserve.rs create mode 100644 tests/ui/unnecessary_reserve.fixed create mode 100644 tests/ui/unnecessary_reserve.rs create mode 100644 tests/ui/unnecessary_reserve.stderr diff --git a/CHANGELOG.md b/CHANGELOG.md index 51441ab9fc0d..98c70466c0e6 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6229,6 +6229,7 @@ Released 2018-09-13 [`unnecessary_mut_passed`]: https://rust-lang.github.io/rust-clippy/master/index.html#unnecessary_mut_passed [`unnecessary_operation`]: https://rust-lang.github.io/rust-clippy/master/index.html#unnecessary_operation [`unnecessary_owned_empty_strings`]: https://rust-lang.github.io/rust-clippy/master/index.html#unnecessary_owned_empty_strings +[`unnecessary_reserve`]: https://rust-lang.github.io/rust-clippy/master/index.html#unnecessary_reserve [`unnecessary_result_map_or_else`]: https://rust-lang.github.io/rust-clippy/master/index.html#unnecessary_result_map_or_else [`unnecessary_safety_comment`]: https://rust-lang.github.io/rust-clippy/master/index.html#unnecessary_safety_comment [`unnecessary_safety_doc`]: https://rust-lang.github.io/rust-clippy/master/index.html#unnecessary_safety_doc diff --git a/clippy_lints/src/declared_lints.rs b/clippy_lints/src/declared_lints.rs index 0834618499c7..5559126f7e7a 100644 --- a/clippy_lints/src/declared_lints.rs +++ b/clippy_lints/src/declared_lints.rs @@ -634,6 +634,7 @@ pub static LINTS: &[&crate::LintInfo] = &[ crate::pathbuf_init_then_push::PATHBUF_INIT_THEN_PUSH_INFO, crate::pattern_type_mismatch::PATTERN_TYPE_MISMATCH_INFO, crate::permissions_set_readonly_false::PERMISSIONS_SET_READONLY_FALSE_INFO, + crate::unnecessary_reserve::UNNECESSARY_RESERVE_INFO, crate::pointers_in_nomem_asm_block::POINTERS_IN_NOMEM_ASM_BLOCK_INFO, crate::precedence::PRECEDENCE_INFO, crate::precedence::PRECEDENCE_BITS_INFO, diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index 3fe3cd67e167..765ae054c1d9 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -375,6 +375,7 @@ mod unnecessary_box_returns; mod unnecessary_literal_bound; mod unnecessary_map_on_constructor; mod unnecessary_owned_empty_strings; +mod unnecessary_reserve; mod unnecessary_self_imports; mod unnecessary_semicolon; mod unnecessary_struct_initialization; @@ -961,6 +962,7 @@ pub fn register_lints(store: &mut rustc_lint::LintStore, conf: &'static Conf) { store.register_late_pass(move |_| Box::new(assigning_clones::AssigningClones::new(conf))); store.register_late_pass(|_| Box::new(zero_repeat_side_effects::ZeroRepeatSideEffects)); store.register_late_pass(|_| Box::new(manual_unwrap_or_default::ManualUnwrapOrDefault)); + store.register_late_pass(move |_| Box::new(unnecessary_reserve::UnnecessaryReserve::new(conf))); store.register_late_pass(|_| Box::new(integer_division_remainder_used::IntegerDivisionRemainderUsed)); store.register_late_pass(move |_| Box::new(macro_metavars_in_unsafe::ExprMetavarsInUnsafe::new(conf))); store.register_late_pass(move |_| Box::new(string_patterns::StringPatterns::new(conf))); diff --git a/clippy_lints/src/unnecessary_reserve.rs b/clippy_lints/src/unnecessary_reserve.rs new file mode 100644 index 000000000000..7c519eb0e288 --- /dev/null +++ b/clippy_lints/src/unnecessary_reserve.rs @@ -0,0 +1,129 @@ +use clippy_config::Conf; +use clippy_utils::diagnostics::span_lint_and_then; +use clippy_utils::msrvs::{self, Msrv}; +use clippy_utils::visitors::for_each_expr; +use clippy_utils::{SpanlessEq, get_enclosing_block, match_def_path, paths}; +use core::ops::ControlFlow; +use rustc_errors::Applicability; +use rustc_hir::{Block, Expr, ExprKind, PathSegment}; +use rustc_lint::{LateContext, LateLintPass}; +use rustc_middle::ty; +use rustc_session::impl_lint_pass; +use rustc_span::sym; + +declare_clippy_lint! { + /// ### What it does + /// + /// This lint checks for a call to `reserve` before `extend` on a `Vec` or `VecDeque`. + /// ### Why is this bad? + /// Since Rust 1.62, `extend` implicitly calls `reserve` + /// + /// ### Example + /// ```rust + /// let mut vec: Vec = vec![]; + /// let array: &[usize] = &[1, 2]; + /// vec.reserve(array.len()); + /// vec.extend(array); + /// ``` + /// Use instead: + /// ```rust + /// let mut vec: Vec = vec![]; + /// let array: &[usize] = &[1, 2]; + /// vec.extend(array); + /// ``` + #[clippy::version = "1.64.0"] + pub UNNECESSARY_RESERVE, + pedantic, + "calling `reserve` before `extend` on a `Vec` or `VecDeque`, when it will be called implicitly" +} + +impl_lint_pass!(UnnecessaryReserve => [UNNECESSARY_RESERVE]); + +pub struct UnnecessaryReserve { + msrv: Msrv, +} +impl UnnecessaryReserve { + pub fn new(conf: &'static Conf) -> Self { + Self { + msrv: conf.msrv.clone(), + } + } +} + +impl<'tcx> LateLintPass<'tcx> for UnnecessaryReserve { + fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &Expr<'tcx>) { + if !self.msrv.meets(msrvs::EXTEND_IMPLICIT_RESERVE) { + return; + } + + if let ExprKind::MethodCall(PathSegment { ident: method, .. }, struct_calling_on, args_a, _) = expr.kind + && method.name.as_str() == "reserve" + && acceptable_type(cx, struct_calling_on) + && let Some(block) = get_enclosing_block(cx, expr.hir_id) + && let Some(next_stmt_span) = check_extend_method(cx, block, struct_calling_on, &args_a[0]) + && !next_stmt_span.from_expansion() + { + span_lint_and_then( + cx, + UNNECESSARY_RESERVE, + next_stmt_span, + "unnecessary call to `reserve`", + |diag| { + diag.span_suggestion( + expr.span, + "remove this line", + String::new(), + Applicability::MaybeIncorrect, + ); + }, + ); + } + } + + extract_msrv_attr!(LateContext); +} + +#[must_use] +fn acceptable_type<'tcx>(cx: &LateContext<'tcx>, struct_calling_on: &Expr<'_>) -> bool { + let acceptable_types = [sym::Vec, sym::VecDeque]; + acceptable_types.iter().any(|&acceptable_ty| { + match cx.typeck_results().expr_ty(struct_calling_on).peel_refs().kind() { + ty::Adt(def, _) => cx.tcx.is_diagnostic_item(acceptable_ty, def.did()), + _ => false, + } + }) +} + +#[must_use] +fn check_extend_method<'tcx>( + cx: &LateContext<'tcx>, + block: &'tcx Block<'tcx>, + struct_expr: &Expr<'tcx>, + args_a: &Expr<'tcx>, +) -> Option { + let args_a_kind = &args_a.kind; + let mut read_found = false; + let mut spanless_eq = SpanlessEq::new(cx); + + let _: Option = for_each_expr(cx, block, |expr: &Expr<'tcx>| { + if let ExprKind::MethodCall(_, struct_calling_on, _, _) = expr.kind + && let Some(expr_def_id) = cx.typeck_results().type_dependent_def_id(expr.hir_id) + && let ExprKind::MethodCall( + PathSegment { + ident: method_call_a, .. + }, + .., + ) = args_a_kind + && method_call_a.name == sym::len + && match_def_path(cx, expr_def_id, &paths::ITER_EXTEND) + && acceptable_type(cx, struct_calling_on) + && spanless_eq.eq_expr(struct_calling_on, struct_expr) + { + read_found = true; + } + let _: bool = !read_found; + ControlFlow::Continue(()) + }); + + if read_found { Some(block.span) } else { None } +} diff --git a/clippy_utils/src/msrvs.rs b/clippy_utils/src/msrvs.rs index 5bb2b12988a6..488e19b5d1bd 100644 --- a/clippy_utils/src/msrvs.rs +++ b/clippy_utils/src/msrvs.rs @@ -39,7 +39,7 @@ msrv_aliases! { 1,68,0 { PATH_MAIN_SEPARATOR_STR } 1,65,0 { LET_ELSE, POINTER_CAST_CONSTNESS } 1,63,0 { CLONE_INTO } - 1,62,0 { BOOL_THEN_SOME, DEFAULT_ENUM_ATTRIBUTE, CONST_EXTERN_C_FN } + 1,62,0 { BOOL_THEN_SOME, DEFAULT_ENUM_ATTRIBUTE, CONST_EXTERN_C_FN, EXTEND_IMPLICIT_RESERVE } 1,59,0 { THREAD_LOCAL_CONST_INIT } 1,58,0 { FORMAT_ARGS_CAPTURE, PATTERN_TRAIT_CHAR_ARRAY, CONST_RAW_PTR_DEREF } 1,57,0 { MAP_WHILE } diff --git a/clippy_utils/src/paths.rs b/clippy_utils/src/paths.rs index 51d06ad9b1aa..019f6ff68179 100644 --- a/clippy_utils/src/paths.rs +++ b/clippy_utils/src/paths.rs @@ -15,6 +15,7 @@ pub const APPLICABILITY_VALUES: [[&str; 3]; 4] = [ pub const DIAG: [&str; 2] = ["rustc_errors", "Diag"]; pub const EARLY_CONTEXT: [&str; 2] = ["rustc_lint", "EarlyContext"]; pub const EARLY_LINT_PASS: [&str; 3] = ["rustc_lint", "passes", "EarlyLintPass"]; +pub const ITER_EXTEND: [&str; 6] = ["core", "iter", "traits", "collect", "Extend", "extend"]; pub const IDENT: [&str; 3] = ["rustc_span", "symbol", "Ident"]; pub const IDENT_AS_STR: [&str; 4] = ["rustc_span", "symbol", "Ident", "as_str"]; pub const KW_MODULE: [&str; 3] = ["rustc_span", "symbol", "kw"]; diff --git a/tests/ui/unnecessary_reserve.fixed b/tests/ui/unnecessary_reserve.fixed new file mode 100644 index 000000000000..e383b779c9c2 --- /dev/null +++ b/tests/ui/unnecessary_reserve.fixed @@ -0,0 +1,99 @@ +#![warn(clippy::unnecessary_reserve)] +#![feature(custom_inner_attributes)] + +#![allow(clippy::unnecessary_operation)] +use std::collections::{HashMap, VecDeque}; + +fn main() { + vec_reserve(); + vec_deque_reserve(); + hash_map_reserve(); + msrv_1_62(); +} + +fn vec_reserve() { + let mut vec: Vec = vec![]; + let array1: &[usize] = &[1, 2]; + let array2: &[usize] = &[3, 4]; + + // do not lint - different arrays + ; + vec.extend(array2); + + // do not lint + vec.reserve(1); + vec.extend([1]); + + //// do lint + ; + vec.extend(array1); + + // do lint + { + ; + vec.extend(array1) + }; + + // do not lint + ; + vec.push(1); + vec.extend(array1); + + // do not lint + let mut other_vec: Vec = Vec::with_capacity(1); + vec.extend([1]) +} + +fn vec_deque_reserve() { + let mut vec_deque: VecDeque = [1].into(); + let array: &[usize] = &[1, 2]; + + // do not lint + vec_deque.reserve(1); + vec_deque.extend([1]); + + // do lint + ; + vec_deque.extend(array); + + // do not lint + { + vec_deque.reserve(1); + vec_deque.extend([1]) + }; + + // do not lint + vec_deque.reserve(array.len() + 1); + vec_deque.push_back(1); + vec_deque.extend(array); + + // do not lint + let mut other_vec_deque: VecDeque = [1].into(); + other_vec_deque.reserve(1); + vec_deque.extend([1]) +} + +fn hash_map_reserve() { + let mut map: HashMap = HashMap::new(); + let mut other_map: HashMap = HashMap::new(); + // do not lint + map.reserve(other_map.len()); + map.extend(other_map); +} + +fn msrv_1_62() { + #![clippy::msrv = "1.61"] + let mut vec: Vec = vec![]; + let array: &[usize] = &[1, 2]; + + // do not lint + vec.reserve(1); + vec.extend([1]); + + let mut vec_deque: VecDeque = [1].into(); + let array: &[usize] = &[1, 2]; + + // do not lint + vec_deque.reserve(1); + vec_deque.extend([1]); +} diff --git a/tests/ui/unnecessary_reserve.rs b/tests/ui/unnecessary_reserve.rs new file mode 100644 index 000000000000..d6d220299374 --- /dev/null +++ b/tests/ui/unnecessary_reserve.rs @@ -0,0 +1,100 @@ +#![warn(clippy::unnecessary_reserve)] +#![feature(custom_inner_attributes)] + +#[allow(clippy::unnecessary_operation)] +use std::collections::{HashMap, VecDeque}; + +fn main() { + vec_reserve(); + vec_deque_reserve(); + hash_map_reserve(); + msrv_1_62(); +} + +fn vec_reserve() { + let mut vec: Vec = vec![]; + let array1: &[usize] = &[1, 2]; + let array2: &[usize] = &[3, 4]; + + // do not lint - different arrays + vec.reserve(array1.len()); + vec.extend(array2); + + // do not lint + vec.reserve(1); + vec.extend([1]); + + //// do lint + vec.reserve(array1.len()); + vec.extend(array1); + + // do lint + { + vec.reserve(array1.len()); + vec.extend(array1) + }; + + // do not lint + vec.reserve(array1.len()); + vec.push(1); + vec.extend(array1); + + // do not lint + let mut other_vec: Vec = vec![]; + other_vec.reserve(1); + vec.extend([1]) +} + +fn vec_deque_reserve() { + let mut vec_deque: VecDeque = [1].into(); + let array: &[usize] = &[1, 2]; + + // do not lint + vec_deque.reserve(1); + vec_deque.extend([1]); + + // do lint + vec_deque.reserve(array.len()); + vec_deque.extend(array); + + // do not lint + { + vec_deque.reserve(1); + vec_deque.extend([1]) + }; + + // do not lint + vec_deque.reserve(array.len() + 1); + vec_deque.push_back(1); + vec_deque.extend(array); + + // do not lint + let mut other_vec_deque: VecDeque = [1].into(); + other_vec_deque.reserve(1); + vec_deque.extend([1]) +} + +fn hash_map_reserve() { + let mut map: HashMap = HashMap::new(); + let mut other_map: HashMap = HashMap::new(); + // do not lint + map.reserve(other_map.len()); + map.extend(other_map); +} + +fn msrv_1_62() { + #![clippy::msrv = "1.61"] + let mut vec: Vec = vec![]; + let array: &[usize] = &[1, 2]; + + // do not lint + vec.reserve(1); + vec.extend([1]); + + let mut vec_deque: VecDeque = [1].into(); + let array: &[usize] = &[1, 2]; + + // do not lint + vec_deque.reserve(1); + vec_deque.extend([1]); +} diff --git a/tests/ui/unnecessary_reserve.stderr b/tests/ui/unnecessary_reserve.stderr new file mode 100644 index 000000000000..4f33fed963d3 --- /dev/null +++ b/tests/ui/unnecessary_reserve.stderr @@ -0,0 +1,96 @@ +error: useless lint attribute + --> tests/ui/unnecessary_reserve.rs:4:1 + | +LL | #[allow(clippy::unnecessary_operation)] + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: if you just forgot a `!`, use: `#![allow(clippy::unnecessary_operation)]` + | + = note: `#[deny(clippy::useless_attribute)]` on by default + +error: unnecessary call to `reserve` + --> tests/ui/unnecessary_reserve.rs:14:18 + | +LL | fn vec_reserve() { + | __________________^ +LL | | let mut vec: Vec = vec![]; +LL | | let array1: &[usize] = &[1, 2]; +LL | | let array2: &[usize] = &[3, 4]; +... | +LL | | vec.reserve(array1.len()); + | | ------------------------- help: remove this line +... | +LL | | vec.extend([1]) +LL | | } + | |_^ + | + = note: `-D clippy::unnecessary-reserve` implied by `-D warnings` + = help: to override `-D warnings` add `#[allow(clippy::unnecessary_reserve)]` + +error: unnecessary call to `reserve` + --> tests/ui/unnecessary_reserve.rs:14:18 + | +LL | fn vec_reserve() { + | __________________^ +LL | | let mut vec: Vec = vec![]; +LL | | let array1: &[usize] = &[1, 2]; +LL | | let array2: &[usize] = &[3, 4]; +... | +LL | | vec.reserve(array1.len()); + | | ------------------------- help: remove this line +... | +LL | | vec.extend([1]) +LL | | } + | |_^ + +error: unnecessary call to `reserve` + --> tests/ui/unnecessary_reserve.rs:32:5 + | +LL | / { +LL | | vec.reserve(array1.len()); + | | ------------------------- help: remove this line +LL | | vec.extend(array1) +LL | | }; + | |_____^ + +error: unnecessary call to `reserve` + --> tests/ui/unnecessary_reserve.rs:14:18 + | +LL | fn vec_reserve() { + | __________________^ +LL | | let mut vec: Vec = vec![]; +LL | | let array1: &[usize] = &[1, 2]; +LL | | let array2: &[usize] = &[3, 4]; +... | +LL | | vec.reserve(array1.len()); + | | ------------------------- help: remove this line +... | +LL | | vec.extend([1]) +LL | | } + | |_^ + +error: call to `reserve` immediately after creation + --> tests/ui/unnecessary_reserve.rs:43:5 + | +LL | / let mut other_vec: Vec = vec![]; +LL | | other_vec.reserve(1); + | |_________________________^ help: consider using `Vec::with_capacity(/* Space hint */)`: `let mut other_vec: Vec = Vec::with_capacity(1);` + | + = note: `-D clippy::reserve-after-initialization` implied by `-D warnings` + = help: to override `-D warnings` add `#[allow(clippy::reserve_after_initialization)]` + +error: unnecessary call to `reserve` + --> tests/ui/unnecessary_reserve.rs:48:24 + | +LL | fn vec_deque_reserve() { + | ________________________^ +LL | | let mut vec_deque: VecDeque = [1].into(); +LL | | let array: &[usize] = &[1, 2]; +... | +LL | | vec_deque.reserve(array.len()); + | | ------------------------------ help: remove this line +... | +LL | | vec_deque.extend([1]) +LL | | } + | |_^ + +error: aborting due to 7 previous errors + From 6beddf49523b93047e442a0fe3f52fd3b80ec8f0 Mon Sep 17 00:00:00 2001 From: wowinter13 Date: Thu, 30 Jan 2025 14:05:03 +0100 Subject: [PATCH 2/7] fix: declared lints --- clippy_lints/src/declared_lints.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/clippy_lints/src/declared_lints.rs b/clippy_lints/src/declared_lints.rs index 5559126f7e7a..7db48f20652e 100644 --- a/clippy_lints/src/declared_lints.rs +++ b/clippy_lints/src/declared_lints.rs @@ -634,7 +634,6 @@ pub static LINTS: &[&crate::LintInfo] = &[ crate::pathbuf_init_then_push::PATHBUF_INIT_THEN_PUSH_INFO, crate::pattern_type_mismatch::PATTERN_TYPE_MISMATCH_INFO, crate::permissions_set_readonly_false::PERMISSIONS_SET_READONLY_FALSE_INFO, - crate::unnecessary_reserve::UNNECESSARY_RESERVE_INFO, crate::pointers_in_nomem_asm_block::POINTERS_IN_NOMEM_ASM_BLOCK_INFO, crate::precedence::PRECEDENCE_INFO, crate::precedence::PRECEDENCE_BITS_INFO, @@ -772,6 +771,7 @@ pub static LINTS: &[&crate::LintInfo] = &[ crate::unnecessary_literal_bound::UNNECESSARY_LITERAL_BOUND_INFO, crate::unnecessary_map_on_constructor::UNNECESSARY_MAP_ON_CONSTRUCTOR_INFO, crate::unnecessary_owned_empty_strings::UNNECESSARY_OWNED_EMPTY_STRINGS_INFO, + crate::unnecessary_reserve::UNNECESSARY_RESERVE_INFO, crate::unnecessary_self_imports::UNNECESSARY_SELF_IMPORTS_INFO, crate::unnecessary_semicolon::UNNECESSARY_SEMICOLON_INFO, crate::unnecessary_struct_initialization::UNNECESSARY_STRUCT_INITIALIZATION_INFO, From 9cecf4177b5a12a0797e404feee1be2245661e24 Mon Sep 17 00:00:00 2001 From: wowinter13 Date: Thu, 30 Jan 2025 14:38:09 +0100 Subject: [PATCH 3/7] fix: linter --- clippy_lints/src/unnecessary_reserve.rs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/clippy_lints/src/unnecessary_reserve.rs b/clippy_lints/src/unnecessary_reserve.rs index 7c519eb0e288..b45bbcb1c502 100644 --- a/clippy_lints/src/unnecessary_reserve.rs +++ b/clippy_lints/src/unnecessary_reserve.rs @@ -83,8 +83,7 @@ impl<'tcx> LateLintPass<'tcx> for UnnecessaryReserve { extract_msrv_attr!(LateContext); } -#[must_use] -fn acceptable_type<'tcx>(cx: &LateContext<'tcx>, struct_calling_on: &Expr<'_>) -> bool { +fn acceptable_type(cx: &LateContext<'_>, struct_calling_on: &Expr<'_>) -> bool { let acceptable_types = [sym::Vec, sym::VecDeque]; acceptable_types.iter().any(|&acceptable_ty| { match cx.typeck_results().expr_ty(struct_calling_on).peel_refs().kind() { From fcbf8908eba8aff80e3003b9749cec664074801a Mon Sep 17 00:00:00 2001 From: wowinter13 Date: Tue, 4 Feb 2025 17:18:10 +0100 Subject: [PATCH 4/7] unnecessary_reserve: address review comments --- clippy_config/src/conf.rs | 1 + clippy_lints/src/unnecessary_reserve.rs | 31 +++++++++++++---------- clippy_utils/src/ty/mod.rs | 5 ++++ clippy_utils/src/ty/type_certainty/mod.rs | 7 ++--- tests/ui/unnecessary_reserve.fixed | 10 ++++++++ tests/ui/unnecessary_reserve.rs | 10 ++++++++ tests/ui/unnecessary_reserve.stderr | 28 +++++++++++++++----- 7 files changed, 67 insertions(+), 25 deletions(-) diff --git a/clippy_config/src/conf.rs b/clippy_config/src/conf.rs index a61acbaa96bc..a9d3bd48ea07 100644 --- a/clippy_config/src/conf.rs +++ b/clippy_config/src/conf.rs @@ -666,6 +666,7 @@ define_Conf! { unchecked_duration_subtraction, uninlined_format_args, unnecessary_lazy_evaluations, + unnecessary_reserve, unnested_or_patterns, unused_trait_names, use_self, diff --git a/clippy_lints/src/unnecessary_reserve.rs b/clippy_lints/src/unnecessary_reserve.rs index b45bbcb1c502..4e9c50c0816e 100644 --- a/clippy_lints/src/unnecessary_reserve.rs +++ b/clippy_lints/src/unnecessary_reserve.rs @@ -1,13 +1,13 @@ use clippy_config::Conf; use clippy_utils::diagnostics::span_lint_and_then; use clippy_utils::msrvs::{self, Msrv}; +use clippy_utils::ty::adt_def_id; use clippy_utils::visitors::for_each_expr; use clippy_utils::{SpanlessEq, get_enclosing_block, match_def_path, paths}; use core::ops::ControlFlow; use rustc_errors::Applicability; use rustc_hir::{Block, Expr, ExprKind, PathSegment}; use rustc_lint::{LateContext, LateLintPass}; -use rustc_middle::ty; use rustc_session::impl_lint_pass; use rustc_span::sym; @@ -16,7 +16,7 @@ declare_clippy_lint! { /// /// This lint checks for a call to `reserve` before `extend` on a `Vec` or `VecDeque`. /// ### Why is this bad? - /// Since Rust 1.62, `extend` implicitly calls `reserve` + /// `extend` implicitly calls `reserve` /// /// ### Example /// ```rust @@ -31,9 +31,9 @@ declare_clippy_lint! { /// let array: &[usize] = &[1, 2]; /// vec.extend(array); /// ``` - #[clippy::version = "1.64.0"] + #[clippy::version = "1.86.0"] pub UNNECESSARY_RESERVE, - pedantic, + complexity, "calling `reserve` before `extend` on a `Vec` or `VecDeque`, when it will be called implicitly" } @@ -84,13 +84,11 @@ impl<'tcx> LateLintPass<'tcx> for UnnecessaryReserve { } fn acceptable_type(cx: &LateContext<'_>, struct_calling_on: &Expr<'_>) -> bool { - let acceptable_types = [sym::Vec, sym::VecDeque]; - acceptable_types.iter().any(|&acceptable_ty| { - match cx.typeck_results().expr_ty(struct_calling_on).peel_refs().kind() { - ty::Adt(def, _) => cx.tcx.is_diagnostic_item(acceptable_ty, def.did()), - _ => false, - } - }) + if let Some(did) = adt_def_id(cx.typeck_results().expr_ty_adjusted(struct_calling_on)) { + matches!(cx.tcx.get_diagnostic_name(did), Some(sym::Vec | sym::VecDeque)) + } else { + false + } } #[must_use] @@ -100,11 +98,18 @@ fn check_extend_method<'tcx>( struct_expr: &Expr<'tcx>, args_a: &Expr<'tcx>, ) -> Option { - let args_a_kind = &args_a.kind; + let mut found_reserve = false; let mut read_found = false; let mut spanless_eq = SpanlessEq::new(cx); let _: Option = for_each_expr(cx, block, |expr: &Expr<'tcx>| { + if !found_reserve { + if expr.hir_id == args_a.hir_id { + found_reserve = true; + } + return ControlFlow::Continue(()); + } + if let ExprKind::MethodCall(_, struct_calling_on, _, _) = expr.kind && let Some(expr_def_id) = cx.typeck_results().type_dependent_def_id(expr.hir_id) && let ExprKind::MethodCall( @@ -112,7 +117,7 @@ fn check_extend_method<'tcx>( ident: method_call_a, .. }, .., - ) = args_a_kind + ) = args_a.kind && method_call_a.name == sym::len && match_def_path(cx, expr_def_id, &paths::ITER_EXTEND) && acceptable_type(cx, struct_calling_on) diff --git a/clippy_utils/src/ty/mod.rs b/clippy_utils/src/ty/mod.rs index 2611fb8a78d8..0efeab96ae88 100644 --- a/clippy_utils/src/ty/mod.rs +++ b/clippy_utils/src/ty/mod.rs @@ -1376,3 +1376,8 @@ pub fn option_arg_ty<'tcx>(cx: &LateContext<'tcx>, ty: Ty<'tcx>) -> Option None, } } + +/// Check if `ty` with references peeled is an `Adt` and return its `DefId`. +pub fn adt_def_id(ty: Ty<'_>) -> Option { + ty.peel_refs().ty_adt_def().map(AdtDef::did) +} diff --git a/clippy_utils/src/ty/type_certainty/mod.rs b/clippy_utils/src/ty/type_certainty/mod.rs index 3398ff8af2f5..753f86b61b50 100644 --- a/clippy_utils/src/ty/type_certainty/mod.rs +++ b/clippy_utils/src/ty/type_certainty/mod.rs @@ -11,13 +11,14 @@ //! As a heuristic, `expr_type_is_certain` may produce false negatives, but a false positive should //! be considered a bug. +use super::adt_def_id; use crate::def_path_res; use rustc_hir::def::{DefKind, Res}; use rustc_hir::def_id::DefId; use rustc_hir::intravisit::{InferKind, Visitor, VisitorExt, walk_qpath, walk_ty}; use rustc_hir::{self as hir, AmbigArg, Expr, ExprKind, GenericArgs, HirId, Node, PathSegment, QPath, TyKind}; use rustc_lint::LateContext; -use rustc_middle::ty::{self, AdtDef, GenericArgKind, Ty}; +use rustc_middle::ty::{self, GenericArgKind, Ty}; use rustc_span::{Span, Symbol}; mod certainty; @@ -313,10 +314,6 @@ fn self_ty<'tcx>(cx: &LateContext<'tcx>, method_def_id: DefId) -> Ty<'tcx> { cx.tcx.fn_sig(method_def_id).skip_binder().inputs().skip_binder()[0] } -fn adt_def_id(ty: Ty<'_>) -> Option { - ty.peel_refs().ty_adt_def().map(AdtDef::did) -} - fn contains_param(ty: Ty<'_>, index: u32) -> bool { ty.walk() .any(|arg| matches!(arg.unpack(), GenericArgKind::Type(ty) if ty.is_param(index))) diff --git a/tests/ui/unnecessary_reserve.fixed b/tests/ui/unnecessary_reserve.fixed index e383b779c9c2..ae536713c02f 100644 --- a/tests/ui/unnecessary_reserve.fixed +++ b/tests/ui/unnecessary_reserve.fixed @@ -9,6 +9,7 @@ fn main() { vec_deque_reserve(); hash_map_reserve(); msrv_1_62(); + box_vec_reserve(); } fn vec_reserve() { @@ -97,3 +98,12 @@ fn msrv_1_62() { vec_deque.reserve(1); vec_deque.extend([1]); } + +fn box_vec_reserve() { + let mut vec: Box> = Box::default(); + let array: &[usize] = &[1, 2]; + + // do lint + ; + vec.extend(array); +} diff --git a/tests/ui/unnecessary_reserve.rs b/tests/ui/unnecessary_reserve.rs index d6d220299374..bdbab78ec4bb 100644 --- a/tests/ui/unnecessary_reserve.rs +++ b/tests/ui/unnecessary_reserve.rs @@ -9,6 +9,7 @@ fn main() { vec_deque_reserve(); hash_map_reserve(); msrv_1_62(); + box_vec_reserve(); } fn vec_reserve() { @@ -98,3 +99,12 @@ fn msrv_1_62() { vec_deque.reserve(1); vec_deque.extend([1]); } + +fn box_vec_reserve() { + let mut vec: Box> = Box::default(); + let array: &[usize] = &[1, 2]; + + // do lint + vec.reserve(array.len()); + vec.extend(array); +} diff --git a/tests/ui/unnecessary_reserve.stderr b/tests/ui/unnecessary_reserve.stderr index 4f33fed963d3..3b9b42545e93 100644 --- a/tests/ui/unnecessary_reserve.stderr +++ b/tests/ui/unnecessary_reserve.stderr @@ -7,7 +7,7 @@ LL | #[allow(clippy::unnecessary_operation)] = note: `#[deny(clippy::useless_attribute)]` on by default error: unnecessary call to `reserve` - --> tests/ui/unnecessary_reserve.rs:14:18 + --> tests/ui/unnecessary_reserve.rs:15:18 | LL | fn vec_reserve() { | __________________^ @@ -26,7 +26,7 @@ LL | | } = help: to override `-D warnings` add `#[allow(clippy::unnecessary_reserve)]` error: unnecessary call to `reserve` - --> tests/ui/unnecessary_reserve.rs:14:18 + --> tests/ui/unnecessary_reserve.rs:15:18 | LL | fn vec_reserve() { | __________________^ @@ -42,7 +42,7 @@ LL | | } | |_^ error: unnecessary call to `reserve` - --> tests/ui/unnecessary_reserve.rs:32:5 + --> tests/ui/unnecessary_reserve.rs:33:5 | LL | / { LL | | vec.reserve(array1.len()); @@ -52,7 +52,7 @@ LL | | }; | |_____^ error: unnecessary call to `reserve` - --> tests/ui/unnecessary_reserve.rs:14:18 + --> tests/ui/unnecessary_reserve.rs:15:18 | LL | fn vec_reserve() { | __________________^ @@ -68,7 +68,7 @@ LL | | } | |_^ error: call to `reserve` immediately after creation - --> tests/ui/unnecessary_reserve.rs:43:5 + --> tests/ui/unnecessary_reserve.rs:44:5 | LL | / let mut other_vec: Vec = vec![]; LL | | other_vec.reserve(1); @@ -78,7 +78,7 @@ LL | | other_vec.reserve(1); = help: to override `-D warnings` add `#[allow(clippy::reserve_after_initialization)]` error: unnecessary call to `reserve` - --> tests/ui/unnecessary_reserve.rs:48:24 + --> tests/ui/unnecessary_reserve.rs:49:24 | LL | fn vec_deque_reserve() { | ________________________^ @@ -92,5 +92,19 @@ LL | | vec_deque.extend([1]) LL | | } | |_^ -error: aborting due to 7 previous errors +error: unnecessary call to `reserve` + --> tests/ui/unnecessary_reserve.rs:103:22 + | +LL | fn box_vec_reserve() { + | ______________________^ +LL | | let mut vec: Box> = Box::default(); +LL | | let array: &[usize] = &[1, 2]; +... | +LL | | vec.reserve(array.len()); + | | ------------------------ help: remove this line +LL | | vec.extend(array); +LL | | } + | |_^ + +error: aborting due to 8 previous errors From bdf6640ce72e9d42583c1000228d2a3bcdffa74a Mon Sep 17 00:00:00 2001 From: wowinter13 Date: Tue, 4 Feb 2025 17:35:07 +0100 Subject: [PATCH 5/7] unnecessary_reserve: lint configuration --- book/src/lint_configuration.md | 1 + 1 file changed, 1 insertion(+) diff --git a/book/src/lint_configuration.md b/book/src/lint_configuration.md index 28b613ea3295..6223164e9673 100644 --- a/book/src/lint_configuration.md +++ b/book/src/lint_configuration.md @@ -818,6 +818,7 @@ The minimum rust version that the project supports. Defaults to the `rust-versio * [`unchecked_duration_subtraction`](https://rust-lang.github.io/rust-clippy/master/index.html#unchecked_duration_subtraction) * [`uninlined_format_args`](https://rust-lang.github.io/rust-clippy/master/index.html#uninlined_format_args) * [`unnecessary_lazy_evaluations`](https://rust-lang.github.io/rust-clippy/master/index.html#unnecessary_lazy_evaluations) +* [`unnecessary_reserve`](https://rust-lang.github.io/rust-clippy/master/index.html#unnecessary_reserve) * [`unnested_or_patterns`](https://rust-lang.github.io/rust-clippy/master/index.html#unnested_or_patterns) * [`unused_trait_names`](https://rust-lang.github.io/rust-clippy/master/index.html#unused_trait_names) * [`use_self`](https://rust-lang.github.io/rust-clippy/master/index.html#use_self) From e08b89a4f33bb6a79063c91d66c0a11941c2d56e Mon Sep 17 00:00:00 2001 From: wowinter13 Date: Fri, 7 Feb 2025 23:55:30 +0100 Subject: [PATCH 6/7] unnecessary_reserve: address review comments --- clippy_lints/src/unnecessary_reserve.rs | 9 ++++++++- tests/ui/unnecessary_reserve.fixed | 27 +++++++++++++++---------- tests/ui/unnecessary_reserve.rs | 15 +++++++++----- tests/ui/unnecessary_reserve.stderr | 22 ++++++++++---------- 4 files changed, 45 insertions(+), 28 deletions(-) diff --git a/clippy_lints/src/unnecessary_reserve.rs b/clippy_lints/src/unnecessary_reserve.rs index 4e9c50c0816e..441d80dfc785 100644 --- a/clippy_lints/src/unnecessary_reserve.rs +++ b/clippy_lints/src/unnecessary_reserve.rs @@ -63,6 +63,13 @@ impl<'tcx> LateLintPass<'tcx> for UnnecessaryReserve { && let Some(next_stmt_span) = check_extend_method(cx, block, struct_calling_on, &args_a[0]) && !next_stmt_span.from_expansion() { + let stmt_span = cx + .tcx + .hir() + .parent_id_iter(expr.hir_id) + .next() + .map_or(expr.span, |parent| cx.tcx.hir().span(parent)); + span_lint_and_then( cx, UNNECESSARY_RESERVE, @@ -70,7 +77,7 @@ impl<'tcx> LateLintPass<'tcx> for UnnecessaryReserve { "unnecessary call to `reserve`", |diag| { diag.span_suggestion( - expr.span, + stmt_span, "remove this line", String::new(), Applicability::MaybeIncorrect, diff --git a/tests/ui/unnecessary_reserve.fixed b/tests/ui/unnecessary_reserve.fixed index ae536713c02f..b2d8ee3f1049 100644 --- a/tests/ui/unnecessary_reserve.fixed +++ b/tests/ui/unnecessary_reserve.fixed @@ -8,7 +8,7 @@ fn main() { vec_reserve(); vec_deque_reserve(); hash_map_reserve(); - msrv_1_62(); + insufficient_msrv(); box_vec_reserve(); } @@ -18,31 +18,36 @@ fn vec_reserve() { let array2: &[usize] = &[3, 4]; // do not lint - different arrays - ; + vec.extend(array2); // do not lint vec.reserve(1); vec.extend([1]); - //// do lint - ; + // do lint + vec.extend(array1); // do lint { - ; + vec.extend(array1) }; // do not lint - ; + vec.push(1); vec.extend(array1); // do not lint let mut other_vec: Vec = Vec::with_capacity(1); - vec.extend([1]) + other_vec.extend([1]); + + // do not lint + let mut vec2: Vec = vec![]; + vec2.extend(array1); + vec2.reserve(array1.len()); } fn vec_deque_reserve() { @@ -54,7 +59,7 @@ fn vec_deque_reserve() { vec_deque.extend([1]); // do lint - ; + vec_deque.extend(array); // do not lint @@ -82,8 +87,8 @@ fn hash_map_reserve() { map.extend(other_map); } -fn msrv_1_62() { - #![clippy::msrv = "1.61"] +#[clippy::msrv = "1.61"] +fn insufficient_msrv() { let mut vec: Vec = vec![]; let array: &[usize] = &[1, 2]; @@ -104,6 +109,6 @@ fn box_vec_reserve() { let array: &[usize] = &[1, 2]; // do lint - ; + vec.extend(array); } diff --git a/tests/ui/unnecessary_reserve.rs b/tests/ui/unnecessary_reserve.rs index bdbab78ec4bb..bf675ab98ac8 100644 --- a/tests/ui/unnecessary_reserve.rs +++ b/tests/ui/unnecessary_reserve.rs @@ -8,7 +8,7 @@ fn main() { vec_reserve(); vec_deque_reserve(); hash_map_reserve(); - msrv_1_62(); + insufficient_msrv(); box_vec_reserve(); } @@ -25,7 +25,7 @@ fn vec_reserve() { vec.reserve(1); vec.extend([1]); - //// do lint + // do lint vec.reserve(array1.len()); vec.extend(array1); @@ -43,7 +43,12 @@ fn vec_reserve() { // do not lint let mut other_vec: Vec = vec![]; other_vec.reserve(1); - vec.extend([1]) + other_vec.extend([1]); + + // do not lint + let mut vec2: Vec = vec![]; + vec2.extend(array1); + vec2.reserve(array1.len()); } fn vec_deque_reserve() { @@ -83,8 +88,8 @@ fn hash_map_reserve() { map.extend(other_map); } -fn msrv_1_62() { - #![clippy::msrv = "1.61"] +#[clippy::msrv = "1.61"] +fn insufficient_msrv() { let mut vec: Vec = vec![]; let array: &[usize] = &[1, 2]; diff --git a/tests/ui/unnecessary_reserve.stderr b/tests/ui/unnecessary_reserve.stderr index 3b9b42545e93..583af88b6f23 100644 --- a/tests/ui/unnecessary_reserve.stderr +++ b/tests/ui/unnecessary_reserve.stderr @@ -16,9 +16,9 @@ LL | | let array1: &[usize] = &[1, 2]; LL | | let array2: &[usize] = &[3, 4]; ... | LL | | vec.reserve(array1.len()); - | | ------------------------- help: remove this line + | | -------------------------- help: remove this line ... | -LL | | vec.extend([1]) +LL | | vec2.reserve(array1.len()); LL | | } | |_^ | @@ -35,9 +35,9 @@ LL | | let array1: &[usize] = &[1, 2]; LL | | let array2: &[usize] = &[3, 4]; ... | LL | | vec.reserve(array1.len()); - | | ------------------------- help: remove this line + | | -------------------------- help: remove this line ... | -LL | | vec.extend([1]) +LL | | vec2.reserve(array1.len()); LL | | } | |_^ @@ -46,7 +46,7 @@ error: unnecessary call to `reserve` | LL | / { LL | | vec.reserve(array1.len()); - | | ------------------------- help: remove this line + | | -------------------------- help: remove this line LL | | vec.extend(array1) LL | | }; | |_____^ @@ -61,9 +61,9 @@ LL | | let array1: &[usize] = &[1, 2]; LL | | let array2: &[usize] = &[3, 4]; ... | LL | | vec.reserve(array1.len()); - | | ------------------------- help: remove this line + | | -------------------------- help: remove this line ... | -LL | | vec.extend([1]) +LL | | vec2.reserve(array1.len()); LL | | } | |_^ @@ -78,7 +78,7 @@ LL | | other_vec.reserve(1); = help: to override `-D warnings` add `#[allow(clippy::reserve_after_initialization)]` error: unnecessary call to `reserve` - --> tests/ui/unnecessary_reserve.rs:49:24 + --> tests/ui/unnecessary_reserve.rs:54:24 | LL | fn vec_deque_reserve() { | ________________________^ @@ -86,14 +86,14 @@ LL | | let mut vec_deque: VecDeque = [1].into(); LL | | let array: &[usize] = &[1, 2]; ... | LL | | vec_deque.reserve(array.len()); - | | ------------------------------ help: remove this line + | | ------------------------------- help: remove this line ... | LL | | vec_deque.extend([1]) LL | | } | |_^ error: unnecessary call to `reserve` - --> tests/ui/unnecessary_reserve.rs:103:22 + --> tests/ui/unnecessary_reserve.rs:108:22 | LL | fn box_vec_reserve() { | ______________________^ @@ -101,7 +101,7 @@ LL | | let mut vec: Box> = Box::default(); LL | | let array: &[usize] = &[1, 2]; ... | LL | | vec.reserve(array.len()); - | | ------------------------ help: remove this line + | | ------------------------- help: remove this line LL | | vec.extend(array); LL | | } | |_^ From 37dfd0aa92bb23930a0117beb8ceb8d7311a0655 Mon Sep 17 00:00:00 2001 From: wowinter13 Date: Sat, 1 Mar 2025 16:46:53 +0100 Subject: [PATCH 7/7] unnecessary_reserve: address review comments --- clippy_lints/src/unnecessary_reserve.rs | 46 +++++++++---------------- 1 file changed, 16 insertions(+), 30 deletions(-) diff --git a/clippy_lints/src/unnecessary_reserve.rs b/clippy_lints/src/unnecessary_reserve.rs index 441d80dfc785..841cdb78092d 100644 --- a/clippy_lints/src/unnecessary_reserve.rs +++ b/clippy_lints/src/unnecessary_reserve.rs @@ -6,7 +6,7 @@ use clippy_utils::visitors::for_each_expr; use clippy_utils::{SpanlessEq, get_enclosing_block, match_def_path, paths}; use core::ops::ControlFlow; use rustc_errors::Applicability; -use rustc_hir::{Block, Expr, ExprKind, PathSegment}; +use rustc_hir::{Block, Expr, ExprKind}; use rustc_lint::{LateContext, LateLintPass}; use rustc_session::impl_lint_pass; use rustc_span::sym; @@ -56,11 +56,11 @@ impl<'tcx> LateLintPass<'tcx> for UnnecessaryReserve { return; } - if let ExprKind::MethodCall(PathSegment { ident: method, .. }, struct_calling_on, args_a, _) = expr.kind - && method.name.as_str() == "reserve" - && acceptable_type(cx, struct_calling_on) + if let ExprKind::MethodCall(method, receiver, [arg], _) = expr.kind + && method.ident.name.as_str() == "reserve" + && acceptable_type(cx, receiver) && let Some(block) = get_enclosing_block(cx, expr.hir_id) - && let Some(next_stmt_span) = check_extend_method(cx, block, struct_calling_on, &args_a[0]) + && let Some(next_stmt_span) = check_extend_method(cx, block, receiver, arg) && !next_stmt_span.from_expansion() { let stmt_span = cx @@ -91,11 +91,8 @@ impl<'tcx> LateLintPass<'tcx> for UnnecessaryReserve { } fn acceptable_type(cx: &LateContext<'_>, struct_calling_on: &Expr<'_>) -> bool { - if let Some(did) = adt_def_id(cx.typeck_results().expr_ty_adjusted(struct_calling_on)) { - matches!(cx.tcx.get_diagnostic_name(did), Some(sym::Vec | sym::VecDeque)) - } else { - false - } + adt_def_id(cx.typeck_results().expr_ty_adjusted(struct_calling_on)) + .is_some_and(|did| matches!(cx.tcx.get_diagnostic_name(did), Some(sym::Vec | sym::VecDeque))) } #[must_use] @@ -103,38 +100,27 @@ fn check_extend_method<'tcx>( cx: &LateContext<'tcx>, block: &'tcx Block<'tcx>, struct_expr: &Expr<'tcx>, - args_a: &Expr<'tcx>, + arg: &Expr<'tcx>, ) -> Option { let mut found_reserve = false; - let mut read_found = false; - let mut spanless_eq = SpanlessEq::new(cx); + let mut spanless_eq = SpanlessEq::new(cx).deny_side_effects(); - let _: Option = for_each_expr(cx, block, |expr: &Expr<'tcx>| { + for_each_expr(cx, block, |expr: &Expr<'tcx>| { if !found_reserve { - if expr.hir_id == args_a.hir_id { - found_reserve = true; - } + found_reserve = expr.hir_id == arg.hir_id; return ControlFlow::Continue(()); } - if let ExprKind::MethodCall(_, struct_calling_on, _, _) = expr.kind + if let ExprKind::MethodCall(_method, struct_calling_on, _, _) = expr.kind && let Some(expr_def_id) = cx.typeck_results().type_dependent_def_id(expr.hir_id) - && let ExprKind::MethodCall( - PathSegment { - ident: method_call_a, .. - }, - .., - ) = args_a.kind - && method_call_a.name == sym::len + && let ExprKind::MethodCall(len_method, ..) = arg.kind + && len_method.ident.name == sym::len && match_def_path(cx, expr_def_id, &paths::ITER_EXTEND) && acceptable_type(cx, struct_calling_on) && spanless_eq.eq_expr(struct_calling_on, struct_expr) { - read_found = true; + return ControlFlow::Break(block.span); } - let _: bool = !read_found; ControlFlow::Continue(()) - }); - - if read_found { Some(block.span) } else { None } + }) }