diff --git a/CHANGELOG.md b/CHANGELOG.md index bc42c07224e1..b839a90656bf 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6173,6 +6173,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/book/src/lint_configuration.md b/book/src/lint_configuration.md index b8f9fff9c613..3dea1827f5b5 100644 --- a/book/src/lint_configuration.md +++ b/book/src/lint_configuration.md @@ -778,6 +778,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) diff --git a/clippy_config/src/conf.rs b/clippy_config/src/conf.rs index 552141476f3a..7dffd13218d5 100644 --- a/clippy_config/src/conf.rs +++ b/clippy_config/src/conf.rs @@ -647,6 +647,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/declared_lints.rs b/clippy_lints/src/declared_lints.rs index 86bcf8edd578..e6300d4b0660 100644 --- a/clippy_lints/src/declared_lints.rs +++ b/clippy_lints/src/declared_lints.rs @@ -758,6 +758,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, diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index 4b700673d0f8..c3932b4fccec 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -372,6 +372,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; @@ -956,6 +957,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..441d80dfc785 --- /dev/null +++ b/clippy_lints/src/unnecessary_reserve.rs @@ -0,0 +1,140 @@ +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_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? + /// `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.86.0"] + pub UNNECESSARY_RESERVE, + complexity, + "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() + { + 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, + next_stmt_span, + "unnecessary call to `reserve`", + |diag| { + diag.span_suggestion( + stmt_span, + "remove this line", + String::new(), + Applicability::MaybeIncorrect, + ); + }, + ); + } + } + + extract_msrv_attr!(LateContext); +} + +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 + } +} + +#[must_use] +fn check_extend_method<'tcx>( + cx: &LateContext<'tcx>, + block: &'tcx Block<'tcx>, + struct_expr: &Expr<'tcx>, + args_a: &Expr<'tcx>, +) -> Option { + 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( + 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 d73cb7e35611..92a6719a61f4 100644 --- a/clippy_utils/src/msrvs.rs +++ b/clippy_utils/src/msrvs.rs @@ -31,7 +31,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,56,0 { CONST_FN_UNION } diff --git a/clippy_utils/src/paths.rs b/clippy_utils/src/paths.rs index f15fffc09e8d..a535116d6a78 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/clippy_utils/src/ty/mod.rs b/clippy_utils/src/ty/mod.rs index f2bbdda70585..a776961b9e2e 100644 --- a/clippy_utils/src/ty/mod.rs +++ b/clippy_utils/src/ty/mod.rs @@ -1352,3 +1352,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 new file mode 100644 index 000000000000..b2d8ee3f1049 --- /dev/null +++ b/tests/ui/unnecessary_reserve.fixed @@ -0,0 +1,114 @@ +#![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(); + insufficient_msrv(); + box_vec_reserve(); +} + +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); + other_vec.extend([1]); + + // do not lint + let mut vec2: Vec = vec![]; + vec2.extend(array1); + vec2.reserve(array1.len()); +} + +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); +} + +#[clippy::msrv = "1.61"] +fn insufficient_msrv() { + 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]); +} + +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 new file mode 100644 index 000000000000..bf675ab98ac8 --- /dev/null +++ b/tests/ui/unnecessary_reserve.rs @@ -0,0 +1,115 @@ +#![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(); + insufficient_msrv(); + box_vec_reserve(); +} + +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); + other_vec.extend([1]); + + // do not lint + let mut vec2: Vec = vec![]; + vec2.extend(array1); + vec2.reserve(array1.len()); +} + +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); +} + +#[clippy::msrv = "1.61"] +fn insufficient_msrv() { + 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]); +} + +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 new file mode 100644 index 000000000000..583af88b6f23 --- /dev/null +++ b/tests/ui/unnecessary_reserve.stderr @@ -0,0 +1,110 @@ +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:15: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 | | vec2.reserve(array1.len()); +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:15: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 | | vec2.reserve(array1.len()); +LL | | } + | |_^ + +error: unnecessary call to `reserve` + --> tests/ui/unnecessary_reserve.rs:33: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:15: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 | | vec2.reserve(array1.len()); +LL | | } + | |_^ + +error: call to `reserve` immediately after creation + --> tests/ui/unnecessary_reserve.rs:44: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:54: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: unnecessary call to `reserve` + --> tests/ui/unnecessary_reserve.rs:108: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 +