diff --git a/CHANGELOG.md b/CHANGELOG.md index 1ab879ba9ee..7f18a00a9dd 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -3225,6 +3225,7 @@ Released 2018-09-13 [`iter_nth_zero`]: https://rust-lang.github.io/rust-clippy/master/index.html#iter_nth_zero [`iter_overeager_cloned`]: https://rust-lang.github.io/rust-clippy/master/index.html#iter_overeager_cloned [`iter_skip_next`]: https://rust-lang.github.io/rust-clippy/master/index.html#iter_skip_next +[`iter_with_drain`]: https://rust-lang.github.io/rust-clippy/master/index.html#iter_with_drain [`iterator_step_by_zero`]: https://rust-lang.github.io/rust-clippy/master/index.html#iterator_step_by_zero [`just_underscores_and_digits`]: https://rust-lang.github.io/rust-clippy/master/index.html#just_underscores_and_digits [`large_const_arrays`]: https://rust-lang.github.io/rust-clippy/master/index.html#large_const_arrays diff --git a/clippy_lints/src/lib.register_all.rs b/clippy_lints/src/lib.register_all.rs index 4484f6e1960..f388ae7ce60 100644 --- a/clippy_lints/src/lib.register_all.rs +++ b/clippy_lints/src/lib.register_all.rs @@ -164,6 +164,7 @@ store.register_group(true, "clippy::all", Some("clippy_all"), vec![ LintId::of(methods::ITER_NTH_ZERO), LintId::of(methods::ITER_OVEREAGER_CLONED), LintId::of(methods::ITER_SKIP_NEXT), + LintId::of(methods::ITER_WITH_DRAIN), LintId::of(methods::MANUAL_FILTER_MAP), LintId::of(methods::MANUAL_FIND_MAP), LintId::of(methods::MANUAL_SATURATING_ARITHMETIC), diff --git a/clippy_lints/src/lib.register_lints.rs b/clippy_lints/src/lib.register_lints.rs index c34d8f46e09..d248c2a41ee 100644 --- a/clippy_lints/src/lib.register_lints.rs +++ b/clippy_lints/src/lib.register_lints.rs @@ -299,6 +299,7 @@ store.register_lints(&[ methods::ITER_NTH_ZERO, methods::ITER_OVEREAGER_CLONED, methods::ITER_SKIP_NEXT, + methods::ITER_WITH_DRAIN, methods::MANUAL_FILTER_MAP, methods::MANUAL_FIND_MAP, methods::MANUAL_SATURATING_ARITHMETIC, diff --git a/clippy_lints/src/lib.register_perf.rs b/clippy_lints/src/lib.register_perf.rs index f2f5c988d8f..6e9c0ee33a1 100644 --- a/clippy_lints/src/lib.register_perf.rs +++ b/clippy_lints/src/lib.register_perf.rs @@ -16,6 +16,7 @@ store.register_group(true, "clippy::perf", Some("clippy_perf"), vec![ LintId::of(methods::EXTEND_WITH_DRAIN), LintId::of(methods::ITER_NTH), LintId::of(methods::ITER_OVEREAGER_CLONED), + LintId::of(methods::ITER_WITH_DRAIN), LintId::of(methods::MANUAL_STR_REPEAT), LintId::of(methods::OR_FUN_CALL), LintId::of(methods::SINGLE_CHAR_PATTERN), diff --git a/clippy_lints/src/methods/iter_with_drain.rs b/clippy_lints/src/methods/iter_with_drain.rs new file mode 100644 index 00000000000..958c3773087 --- /dev/null +++ b/clippy_lints/src/methods/iter_with_drain.rs @@ -0,0 +1,72 @@ +use clippy_utils::diagnostics::span_lint_and_sugg; +use clippy_utils::is_integer_const; +use clippy_utils::ty::is_type_diagnostic_item; +use clippy_utils::{ + higher::{self, Range}, + SpanlessEq, +}; +use rustc_ast::ast::RangeLimits; +use rustc_errors::Applicability; +use rustc_hir::{Expr, ExprKind, QPath}; +use rustc_lint::LateContext; +use rustc_span::symbol::{sym, Symbol}; +use rustc_span::Span; + +use super::ITER_WITH_DRAIN; + +const DRAIN_TYPES: &[Symbol] = &[sym::Vec, sym::VecDeque]; + +pub(super) fn check(cx: &LateContext<'_>, expr: &Expr<'_>, recv: &Expr<'_>, span: Span, arg: &Expr<'_>) { + let ty = cx.typeck_results().expr_ty(recv).peel_refs(); + if let Some(drained_type) = DRAIN_TYPES.iter().find(|&&sym| is_type_diagnostic_item(cx, ty, sym)) { + // Refuse to emit `into_iter` suggestion on draining struct fields due + // to the strong possibility of processing unmovable field. + if let ExprKind::Field(..) = recv.kind { + return; + } + + if let Some(range) = higher::Range::hir(arg) { + let left_full = match range { + Range { start: Some(start), .. } if is_integer_const(cx, start, 0) => true, + Range { start: None, .. } => true, + _ => false, + }; + let full = left_full + && match range { + Range { + end: Some(end), + limits: RangeLimits::HalfOpen, + .. + } => { + // `x.drain(..x.len())` call + if_chain! { + if let ExprKind::MethodCall(len_path, len_args, _) = end.kind; + if len_path.ident.name == sym::len && len_args.len() == 1; + if let ExprKind::Path(QPath::Resolved(_, drain_path)) = recv.kind; + if let ExprKind::Path(QPath::Resolved(_, len_path)) = len_args[0].kind; + if SpanlessEq::new(cx).eq_path(drain_path, len_path); + then { true } + else { false } + } + }, + Range { + end: None, + limits: RangeLimits::HalfOpen, + .. + } => true, + _ => false, + }; + if full { + span_lint_and_sugg( + cx, + ITER_WITH_DRAIN, + span.with_hi(expr.span.hi()), + &format!("`drain(..)` used on a `{}`", drained_type), + "try this", + "into_iter()".to_string(), + Applicability::MaybeIncorrect, + ); + } + } + } +} diff --git a/clippy_lints/src/methods/mod.rs b/clippy_lints/src/methods/mod.rs index 581f4c8b0e2..5edd22cd14c 100644 --- a/clippy_lints/src/methods/mod.rs +++ b/clippy_lints/src/methods/mod.rs @@ -32,6 +32,7 @@ mod iter_nth; mod iter_nth_zero; mod iter_overeager_cloned; mod iter_skip_next; +mod iter_with_drain; mod iterator_step_by_zero; mod manual_saturating_arithmetic; mod manual_str_repeat; @@ -1118,6 +1119,31 @@ declare_clippy_lint! { "using `.skip(x).next()` on an iterator" } +declare_clippy_lint! { + /// ### What it does + /// Checks for use of `.drain(..)` on `Vec` and `VecDeque` for iteration. + /// + /// ### Why is this bad? + /// `.into_iter()` is simpler with better performance. + /// + /// ### Example + /// ```rust + /// # use std::collections::HashSet; + /// let mut foo = vec![0, 1, 2, 3]; + /// let bar: HashSet = foo.drain(..).collect(); + /// ``` + /// Use instead: + /// ```rust + /// # use std::collections::HashSet; + /// let foo = vec![0, 1, 2, 3]; + /// let bar: HashSet = foo.into_iter().collect(); + /// ``` + #[clippy::version = "1.61.0"] + pub ITER_WITH_DRAIN, + perf, + "replace `.drain(..)` with `.into_iter()`" +} + declare_clippy_lint! { /// ### What it does /// Checks for use of `.get().unwrap()` (or @@ -2047,6 +2073,7 @@ impl_lint_pass!(Methods => [ GET_UNWRAP, STRING_EXTEND_CHARS, ITER_CLONED_COLLECT, + ITER_WITH_DRAIN, USELESS_ASREF, UNNECESSARY_FOLD, UNNECESSARY_FILTER_MAP, @@ -2327,6 +2354,9 @@ fn check_methods<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>, msrv: Optio Some(("map", [_, arg], _)) => suspicious_map::check(cx, expr, recv, arg), _ => {}, }, + ("drain", [arg]) => { + iter_with_drain::check(cx, expr, recv, span, arg); + }, ("expect", [_]) => match method_call(recv) { Some(("ok", [recv], _)) => ok_expect::check(cx, expr, recv), _ => expect_used::check(cx, expr, recv), diff --git a/clippy_lints/src/suspicious_operation_groupings.rs b/clippy_lints/src/suspicious_operation_groupings.rs index c3d984fb5ae..940a8428f77 100644 --- a/clippy_lints/src/suspicious_operation_groupings.rs +++ b/clippy_lints/src/suspicious_operation_groupings.rs @@ -399,9 +399,9 @@ fn if_statment_binops(kind: &ExprKind) -> Option>> { fn append_opt_vecs(target_opt: Option>, source_opt: Option>) -> Option> { match (target_opt, source_opt) { - (Some(mut target), Some(mut source)) => { + (Some(mut target), Some(source)) => { target.reserve(source.len()); - for op in source.drain(..) { + for op in source { target.push(op); } Some(target) @@ -436,9 +436,9 @@ fn chained_binops_helper<'expr>(left_outer: &'expr Expr, right_outer: &'expr Exp chained_binops_helper(left_left, left_right), chained_binops_helper(right_left, right_right), ) { - (Some(mut left_ops), Some(mut right_ops)) => { + (Some(mut left_ops), Some(right_ops)) => { left_ops.reserve(right_ops.len()); - for op in right_ops.drain(..) { + for op in right_ops { left_ops.push(op); } Some(left_ops) diff --git a/clippy_lints/src/utils/internal_lints/metadata_collector.rs b/clippy_lints/src/utils/internal_lints/metadata_collector.rs index 56633490eaa..a617422bbeb 100644 --- a/clippy_lints/src/utils/internal_lints/metadata_collector.rs +++ b/clippy_lints/src/utils/internal_lints/metadata_collector.rs @@ -473,7 +473,7 @@ impl<'hir> LateLintPass<'hir> for MetadataCollector { /// ``` fn check_expr(&mut self, cx: &LateContext<'hir>, expr: &'hir hir::Expr<'_>) { if let Some(args) = match_lint_emission(cx, expr) { - let mut emission_info = extract_emission_info(cx, args); + let emission_info = extract_emission_info(cx, args); if emission_info.is_empty() { // See: // - src/misc.rs:734:9 @@ -483,7 +483,7 @@ impl<'hir> LateLintPass<'hir> for MetadataCollector { return; } - for (lint_name, applicability, is_multi_part) in emission_info.drain(..) { + for (lint_name, applicability, is_multi_part) in emission_info { let app_info = self.applicability_info.entry(lint_name).or_default(); app_info.applicability = applicability; app_info.is_multi_part_suggestion = is_multi_part; @@ -693,7 +693,7 @@ fn extract_emission_info<'hir>( } lints - .drain(..) + .into_iter() .map(|lint_name| (lint_name, applicability, multi_part)) .collect() } diff --git a/clippy_utils/src/hir_utils.rs b/clippy_utils/src/hir_utils.rs index 9654895060f..00594f4d42a 100644 --- a/clippy_utils/src/hir_utils.rs +++ b/clippy_utils/src/hir_utils.rs @@ -74,6 +74,10 @@ impl<'a, 'tcx> SpanlessEq<'a, 'tcx> { self.inter_expr().eq_expr(left, right) } + pub fn eq_path(&mut self, left: &Path<'_>, right: &Path<'_>) -> bool { + self.inter_expr().eq_path(left, right) + } + pub fn eq_path_segment(&mut self, left: &PathSegment<'_>, right: &PathSegment<'_>) -> bool { self.inter_expr().eq_path_segment(left, right) } @@ -362,7 +366,7 @@ impl HirEqInterExpr<'_, '_, '_> { } } - fn eq_path(&mut self, left: &Path<'_>, right: &Path<'_>) -> bool { + pub fn eq_path(&mut self, left: &Path<'_>, right: &Path<'_>) -> bool { match (left.res, right.res) { (Res::Local(l), Res::Local(r)) => l == r || self.locals.get(&l) == Some(&r), (Res::Local(_), _) | (_, Res::Local(_)) => false, diff --git a/tests/ui/extend_with_drain.fixed b/tests/ui/extend_with_drain.fixed index e863870e7d6..71ebad24c16 100644 --- a/tests/ui/extend_with_drain.fixed +++ b/tests/ui/extend_with_drain.fixed @@ -1,11 +1,11 @@ // run-rustfix #![warn(clippy::extend_with_drain)] +#![allow(clippy::iter_with_drain)] use std::collections::BinaryHeap; fn main() { //gets linted let mut vec1 = vec![0u8; 1024]; let mut vec2: std::vec::Vec = Vec::new(); - vec2.append(&mut vec1); let mut vec3 = vec![0u8; 1024]; @@ -17,7 +17,7 @@ fn main() { vec11.append(&mut return_vector()); - //won't get linted it dosen't move the entire content of a vec into another + //won't get linted it doesn't move the entire content of a vec into another let mut test1 = vec![0u8, 10]; let mut test2: std::vec::Vec = Vec::new(); diff --git a/tests/ui/extend_with_drain.rs b/tests/ui/extend_with_drain.rs index dcb36b5951c..e9f011abb0e 100644 --- a/tests/ui/extend_with_drain.rs +++ b/tests/ui/extend_with_drain.rs @@ -1,11 +1,11 @@ // run-rustfix #![warn(clippy::extend_with_drain)] +#![allow(clippy::iter_with_drain)] use std::collections::BinaryHeap; fn main() { //gets linted let mut vec1 = vec![0u8; 1024]; let mut vec2: std::vec::Vec = Vec::new(); - vec2.extend(vec1.drain(..)); let mut vec3 = vec![0u8; 1024]; @@ -17,7 +17,7 @@ fn main() { vec11.extend(return_vector().drain(..)); - //won't get linted it dosen't move the entire content of a vec into another + //won't get linted it doesn't move the entire content of a vec into another let mut test1 = vec![0u8, 10]; let mut test2: std::vec::Vec = Vec::new(); diff --git a/tests/ui/iter_with_drain.fixed b/tests/ui/iter_with_drain.fixed new file mode 100644 index 00000000000..aea4dba9dd5 --- /dev/null +++ b/tests/ui/iter_with_drain.fixed @@ -0,0 +1,56 @@ +// run-rustfix +// will emits unused mut warnings after fixing +#![allow(unused_mut)] +// will emits needless collect warnings after fixing +#![allow(clippy::needless_collect)] +#![warn(clippy::iter_with_drain)] +use std::collections::{BinaryHeap, HashMap, HashSet, VecDeque}; + +fn full() { + let mut a = vec!["aaa".to_string(), "bbb".to_string()]; + let mut a: BinaryHeap<_> = a.into_iter().collect(); + let mut a: HashSet<_> = a.drain().collect(); + let mut a: VecDeque<_> = a.drain().collect(); + let mut a: Vec<_> = a.into_iter().collect(); + let mut a: HashMap<_, _> = a.into_iter().map(|x| (x.clone(), x)).collect(); + let _: Vec<(String, String)> = a.drain().collect(); +} + +fn closed() { + let mut a = vec!["aaa".to_string(), "bbb".to_string()]; + let mut a: BinaryHeap<_> = a.into_iter().collect(); + let mut a: HashSet<_> = a.drain().collect(); + let mut a: VecDeque<_> = a.drain().collect(); + let mut a: Vec<_> = a.into_iter().collect(); + let mut a: HashMap<_, _> = a.into_iter().map(|x| (x.clone(), x)).collect(); + let _: Vec<(String, String)> = a.drain().collect(); +} + +fn should_not_help() { + let mut a = vec!["aaa".to_string(), "bbb".to_string()]; + let mut a: BinaryHeap<_> = a.drain(1..).collect(); + let mut a: HashSet<_> = a.drain().collect(); + let mut a: VecDeque<_> = a.drain().collect(); + let mut a: Vec<_> = a.drain(..a.len() - 1).collect(); + let mut a: HashMap<_, _> = a.drain(1..a.len() - 1).map(|x| (x.clone(), x)).collect(); + let _: Vec<(String, String)> = a.drain().collect(); + + let mut b = vec!["aaa".to_string(), "bbb".to_string()]; + let _: Vec<_> = b.drain(0..a.len()).collect(); +} + +#[derive(Default)] +struct Bomb { + fire: Vec, +} + +fn should_not_help_0(bomb: &mut Bomb) { + let _: Vec = bomb.fire.drain(..).collect(); +} + +fn main() { + full(); + closed(); + should_not_help(); + should_not_help_0(&mut Bomb::default()); +} diff --git a/tests/ui/iter_with_drain.rs b/tests/ui/iter_with_drain.rs new file mode 100644 index 00000000000..271878cffb4 --- /dev/null +++ b/tests/ui/iter_with_drain.rs @@ -0,0 +1,56 @@ +// run-rustfix +// will emits unused mut warnings after fixing +#![allow(unused_mut)] +// will emits needless collect warnings after fixing +#![allow(clippy::needless_collect)] +#![warn(clippy::iter_with_drain)] +use std::collections::{BinaryHeap, HashMap, HashSet, VecDeque}; + +fn full() { + let mut a = vec!["aaa".to_string(), "bbb".to_string()]; + let mut a: BinaryHeap<_> = a.drain(..).collect(); + let mut a: HashSet<_> = a.drain().collect(); + let mut a: VecDeque<_> = a.drain().collect(); + let mut a: Vec<_> = a.drain(..).collect(); + let mut a: HashMap<_, _> = a.drain(..).map(|x| (x.clone(), x)).collect(); + let _: Vec<(String, String)> = a.drain().collect(); +} + +fn closed() { + let mut a = vec!["aaa".to_string(), "bbb".to_string()]; + let mut a: BinaryHeap<_> = a.drain(0..).collect(); + let mut a: HashSet<_> = a.drain().collect(); + let mut a: VecDeque<_> = a.drain().collect(); + let mut a: Vec<_> = a.drain(..a.len()).collect(); + let mut a: HashMap<_, _> = a.drain(0..a.len()).map(|x| (x.clone(), x)).collect(); + let _: Vec<(String, String)> = a.drain().collect(); +} + +fn should_not_help() { + let mut a = vec!["aaa".to_string(), "bbb".to_string()]; + let mut a: BinaryHeap<_> = a.drain(1..).collect(); + let mut a: HashSet<_> = a.drain().collect(); + let mut a: VecDeque<_> = a.drain().collect(); + let mut a: Vec<_> = a.drain(..a.len() - 1).collect(); + let mut a: HashMap<_, _> = a.drain(1..a.len() - 1).map(|x| (x.clone(), x)).collect(); + let _: Vec<(String, String)> = a.drain().collect(); + + let mut b = vec!["aaa".to_string(), "bbb".to_string()]; + let _: Vec<_> = b.drain(0..a.len()).collect(); +} + +#[derive(Default)] +struct Bomb { + fire: Vec, +} + +fn should_not_help_0(bomb: &mut Bomb) { + let _: Vec = bomb.fire.drain(..).collect(); +} + +fn main() { + full(); + closed(); + should_not_help(); + should_not_help_0(&mut Bomb::default()); +} diff --git a/tests/ui/iter_with_drain.stderr b/tests/ui/iter_with_drain.stderr new file mode 100644 index 00000000000..aa394439fa6 --- /dev/null +++ b/tests/ui/iter_with_drain.stderr @@ -0,0 +1,40 @@ +error: `drain(..)` used on a `Vec` + --> $DIR/iter_with_drain.rs:11:34 + | +LL | let mut a: BinaryHeap<_> = a.drain(..).collect(); + | ^^^^^^^^^ help: try this: `into_iter()` + | + = note: `-D clippy::iter-with-drain` implied by `-D warnings` + +error: `drain(..)` used on a `VecDeque` + --> $DIR/iter_with_drain.rs:14:27 + | +LL | let mut a: Vec<_> = a.drain(..).collect(); + | ^^^^^^^^^ help: try this: `into_iter()` + +error: `drain(..)` used on a `Vec` + --> $DIR/iter_with_drain.rs:15:34 + | +LL | let mut a: HashMap<_, _> = a.drain(..).map(|x| (x.clone(), x)).collect(); + | ^^^^^^^^^ help: try this: `into_iter()` + +error: `drain(..)` used on a `Vec` + --> $DIR/iter_with_drain.rs:21:34 + | +LL | let mut a: BinaryHeap<_> = a.drain(0..).collect(); + | ^^^^^^^^^^ help: try this: `into_iter()` + +error: `drain(..)` used on a `VecDeque` + --> $DIR/iter_with_drain.rs:24:27 + | +LL | let mut a: Vec<_> = a.drain(..a.len()).collect(); + | ^^^^^^^^^^^^^^^^ help: try this: `into_iter()` + +error: `drain(..)` used on a `Vec` + --> $DIR/iter_with_drain.rs:25:34 + | +LL | let mut a: HashMap<_, _> = a.drain(0..a.len()).map(|x| (x.clone(), x)).collect(); + | ^^^^^^^^^^^^^^^^^ help: try this: `into_iter()` + +error: aborting due to 6 previous errors + diff --git a/tests/ui/range_plus_minus_one.fixed b/tests/ui/range_plus_minus_one.fixed index 19b253b0fe2..40d7791df28 100644 --- a/tests/ui/range_plus_minus_one.fixed +++ b/tests/ui/range_plus_minus_one.fixed @@ -1,7 +1,7 @@ // run-rustfix #![allow(unused_parens)] - +#![allow(clippy::iter_with_drain)] fn f() -> usize { 42 } diff --git a/tests/ui/range_plus_minus_one.rs b/tests/ui/range_plus_minus_one.rs index 7d034117547..a8ddd9b5f75 100644 --- a/tests/ui/range_plus_minus_one.rs +++ b/tests/ui/range_plus_minus_one.rs @@ -1,7 +1,7 @@ // run-rustfix #![allow(unused_parens)] - +#![allow(clippy::iter_with_drain)] fn f() -> usize { 42 }