From 7f44530f54002401ba771bcc19f56f4950b22bb1 Mon Sep 17 00:00:00 2001 From: bluthej Date: Sat, 18 Mar 2023 10:43:02 +0100 Subject: [PATCH 01/11] Create `clear_with_drain` lint --- CHANGELOG.md | 1 + clippy_lints/src/declared_lints.rs | 1 + clippy_lints/src/methods/clear_with_drain.rs | 8 ++++++++ clippy_lints/src/methods/mod.rs | 21 ++++++++++++++++++++ tests/ui/clear_with_drain.rs | 12 +++++++++++ 5 files changed, 43 insertions(+) create mode 100644 clippy_lints/src/methods/clear_with_drain.rs create mode 100644 tests/ui/clear_with_drain.rs diff --git a/CHANGELOG.md b/CHANGELOG.md index 47a503510c1..81d0ff80968 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4441,6 +4441,7 @@ Released 2018-09-13 [`chars_last_cmp`]: https://rust-lang.github.io/rust-clippy/master/index.html#chars_last_cmp [`chars_next_cmp`]: https://rust-lang.github.io/rust-clippy/master/index.html#chars_next_cmp [`checked_conversions`]: https://rust-lang.github.io/rust-clippy/master/index.html#checked_conversions +[`clear_with_drain`]: https://rust-lang.github.io/rust-clippy/master/index.html#clear_with_drain [`clone_double_ref`]: https://rust-lang.github.io/rust-clippy/master/index.html#clone_double_ref [`clone_on_copy`]: https://rust-lang.github.io/rust-clippy/master/index.html#clone_on_copy [`clone_on_ref_ptr`]: https://rust-lang.github.io/rust-clippy/master/index.html#clone_on_ref_ptr diff --git a/clippy_lints/src/declared_lints.rs b/clippy_lints/src/declared_lints.rs index 2331e857b1f..f7b108c923a 100644 --- a/clippy_lints/src/declared_lints.rs +++ b/clippy_lints/src/declared_lints.rs @@ -307,6 +307,7 @@ pub(crate) static LINTS: &[&crate::LintInfo] = &[ crate::methods::CASE_SENSITIVE_FILE_EXTENSION_COMPARISONS_INFO, crate::methods::CHARS_LAST_CMP_INFO, crate::methods::CHARS_NEXT_CMP_INFO, + crate::methods::CLEAR_WITH_DRAIN_INFO, crate::methods::CLONED_INSTEAD_OF_COPIED_INFO, crate::methods::CLONE_DOUBLE_REF_INFO, crate::methods::CLONE_ON_COPY_INFO, diff --git a/clippy_lints/src/methods/clear_with_drain.rs b/clippy_lints/src/methods/clear_with_drain.rs new file mode 100644 index 00000000000..9e55852ef7a --- /dev/null +++ b/clippy_lints/src/methods/clear_with_drain.rs @@ -0,0 +1,8 @@ +use rustc_lint::{LateContext, LintContext}; + +use super::CLEAR_WITH_DRAIN; + +// TODO: Adjust the parameters as necessary +pub(super) fn check(cx: &LateContext) { + todo!(); +} diff --git a/clippy_lints/src/methods/mod.rs b/clippy_lints/src/methods/mod.rs index 56e3988bf09..859284b6a5c 100644 --- a/clippy_lints/src/methods/mod.rs +++ b/clippy_lints/src/methods/mod.rs @@ -9,6 +9,7 @@ mod chars_last_cmp; mod chars_last_cmp_with_unwrap; mod chars_next_cmp; mod chars_next_cmp_with_unwrap; +mod clear_with_drain; mod clone_on_copy; mod clone_on_ref_ptr; mod cloned_instead_of_copied; @@ -3190,6 +3191,25 @@ declare_clippy_lint! { "single command line argument that looks like it should be multiple arguments" } +declare_clippy_lint! { + /// ### What it does + /// + /// ### Why is this bad? + /// + /// ### Example + /// ```rust + /// // example code where clippy issues a warning + /// ``` + /// Use instead: + /// ```rust + /// // example code which does not raise clippy warning + /// ``` + #[clippy::version = "1.69.0"] + pub CLEAR_WITH_DRAIN, + nursery, + "default lint description" +} + pub struct Methods { avoid_breaking_exported_api: bool, msrv: Msrv, @@ -3318,6 +3338,7 @@ impl_lint_pass!(Methods => [ SEEK_TO_START_INSTEAD_OF_REWIND, NEEDLESS_COLLECT, SUSPICIOUS_COMMAND_ARG_SPACE, + CLEAR_WITH_DRAIN, ]); /// Extracts a method call name, args, and `Span` of the method name. diff --git a/tests/ui/clear_with_drain.rs b/tests/ui/clear_with_drain.rs new file mode 100644 index 00000000000..bb3a371cc08 --- /dev/null +++ b/tests/ui/clear_with_drain.rs @@ -0,0 +1,12 @@ +#![allow(unused)] +#![warn(clippy::clear_with_drain)] + +fn main() { + let mut vec: Vec = Vec::new(); + //Lint + vec.drain(..); + vec.drain(0..vec.len()); + + // Dont Lint + let iter = vec.drain(..); +} From 85d428101dc757f7aa41e47696c94683d8398903 Mon Sep 17 00:00:00 2001 From: bluthej Date: Sat, 18 Mar 2023 11:34:30 +0100 Subject: [PATCH 02/11] Pull `is_full_range` method from `iter_with_drain` Rename method to `is_range_full` because the type is actually `RangeFull`. Method moved to `clippy_utils` for reuse in `clear_with_drain`. --- clippy_lints/src/methods/iter_with_drain.rs | 23 +++------------------ clippy_utils/src/lib.rs | 21 ++++++++++++++++++- 2 files changed, 23 insertions(+), 21 deletions(-) diff --git a/clippy_lints/src/methods/iter_with_drain.rs b/clippy_lints/src/methods/iter_with_drain.rs index 3da230e12d7..ea92e3a549f 100644 --- a/clippy_lints/src/methods/iter_with_drain.rs +++ b/clippy_lints/src/methods/iter_with_drain.rs @@ -1,9 +1,8 @@ use clippy_utils::diagnostics::span_lint_and_sugg; use clippy_utils::higher::Range; -use clippy_utils::is_integer_const; -use rustc_ast::ast::RangeLimits; +use clippy_utils::is_range_full; use rustc_errors::Applicability; -use rustc_hir::{Expr, ExprKind, QPath}; +use rustc_hir::{Expr, ExprKind}; use rustc_lint::LateContext; use rustc_span::symbol::sym; use rustc_span::Span; @@ -16,7 +15,7 @@ pub(super) fn check(cx: &LateContext<'_>, expr: &Expr<'_>, recv: &Expr<'_>, span && let Some(ty_name) = cx.tcx.get_diagnostic_name(adt.did()) && matches!(ty_name, sym::Vec | sym::VecDeque) && let Some(range) = Range::hir(arg) - && is_full_range(cx, recv, range) + && is_range_full(cx, recv, range) { span_lint_and_sugg( cx, @@ -29,19 +28,3 @@ pub(super) fn check(cx: &LateContext<'_>, expr: &Expr<'_>, recv: &Expr<'_>, span ); }; } - -fn is_full_range(cx: &LateContext<'_>, container: &Expr<'_>, range: Range<'_>) -> bool { - range.start.map_or(true, |e| is_integer_const(cx, e, 0)) - && range.end.map_or(true, |e| { - if range.limits == RangeLimits::HalfOpen - && let ExprKind::Path(QPath::Resolved(None, container_path)) = container.kind - && let ExprKind::MethodCall(name, self_arg, [], _) = e.kind - && name.ident.name == sym::len - && let ExprKind::Path(QPath::Resolved(None, path)) = self_arg.kind - { - container_path.res == path.res - } else { - false - } - }) -} diff --git a/clippy_utils/src/lib.rs b/clippy_utils/src/lib.rs index 44b6b9f7b0b..0fdbd7a6341 100644 --- a/clippy_utils/src/lib.rs +++ b/clippy_utils/src/lib.rs @@ -78,7 +78,7 @@ use std::sync::OnceLock; use std::sync::{Mutex, MutexGuard}; use if_chain::if_chain; -use rustc_ast::ast::{self, LitKind}; +use rustc_ast::ast::{self, LitKind, RangeLimits}; use rustc_ast::Attribute; use rustc_data_structures::fx::FxHashMap; use rustc_data_structures::unhash::UnhashMap; @@ -115,6 +115,7 @@ use rustc_span::Span; use rustc_target::abi::Integer; use crate::consts::{constant, Constant}; +use crate::higher::Range; use crate::ty::{can_partially_move_ty, expr_sig, is_copy, is_recursively_primitive_type, ty_is_fn_once_param}; use crate::visitors::for_each_expr; @@ -1491,6 +1492,24 @@ pub fn is_else_clause(tcx: TyCtxt<'_>, expr: &Expr<'_>) -> bool { } } +/// Checks whether the given `Range` is equivalent to a `RangeFull`. +/// Inclusive ranges are not considered because they already constitute a lint. +pub fn is_range_full(cx: &LateContext<'_>, container: &Expr<'_>, range: Range<'_>) -> bool { + range.start.map_or(true, |e| is_integer_const(cx, e, 0)) + && range.end.map_or(true, |e| { + if range.limits == RangeLimits::HalfOpen + && let ExprKind::Path(QPath::Resolved(None, container_path)) = container.kind + && let ExprKind::MethodCall(name, self_arg, [], _) = e.kind + && name.ident.name == sym::len + && let ExprKind::Path(QPath::Resolved(None, path)) = self_arg.kind + { + container_path.res == path.res + } else { + false + } + }) +} + /// Checks whether the given expression is a constant integer of the given value. /// unlike `is_integer_literal`, this version does const folding pub fn is_integer_const(cx: &LateContext<'_>, e: &Expr<'_>, value: u128) -> bool { From 589d7e1ab40af9950d33715ef6cf866efa04f9a2 Mon Sep 17 00:00:00 2001 From: bluthej Date: Sat, 18 Mar 2023 14:45:57 +0100 Subject: [PATCH 03/11] Add tests to cover all cases --- tests/ui/clear_with_drain.rs | 32 ++++++++++++++++++++++++-------- 1 file changed, 24 insertions(+), 8 deletions(-) diff --git a/tests/ui/clear_with_drain.rs b/tests/ui/clear_with_drain.rs index bb3a371cc08..3f8517011cd 100644 --- a/tests/ui/clear_with_drain.rs +++ b/tests/ui/clear_with_drain.rs @@ -1,12 +1,28 @@ #![allow(unused)] #![warn(clippy::clear_with_drain)] -fn main() { - let mut vec: Vec = Vec::new(); - //Lint - vec.drain(..); - vec.drain(0..vec.len()); - - // Dont Lint - let iter = vec.drain(..); +fn range() { + let (mut u, mut v) = (vec![1, 2, 3], vec![1, 2, 3]); + let iter = u.drain(0..u.len()); // Yay + v.drain(0..v.len()); // Nay } + +fn range_from() { + let (mut u, mut v) = (vec![1, 2, 3], vec![1, 2, 3]); + let iter = u.drain(0..); // Yay + v.drain(0..); // Nay +} + +fn range_full() { + let (mut u, mut v) = (vec![1, 2, 3], vec![1, 2, 3]); + let iter = u.drain(..); // Yay + v.drain(..); // Nay +} + +fn range_to() { + let (mut u, mut v) = (vec![1, 2, 3], vec![1, 2, 3]); + let iter = u.drain(..u.len()); // Yay + v.drain(..v.len()); // Nay +} + +fn main() {} From 484c82e0418325df5d25e8d0e11e10a5d1371502 Mon Sep 17 00:00:00 2001 From: bluthej Date: Mon, 20 Mar 2023 18:46:18 +0100 Subject: [PATCH 04/11] Update lint declaration --- clippy_lints/src/methods/mod.rs | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/clippy_lints/src/methods/mod.rs b/clippy_lints/src/methods/mod.rs index 859284b6a5c..4e74ac6af2b 100644 --- a/clippy_lints/src/methods/mod.rs +++ b/clippy_lints/src/methods/mod.rs @@ -3193,21 +3193,27 @@ declare_clippy_lint! { declare_clippy_lint! { /// ### What it does + /// Checks for usage of `.drain(..)` for the sole purpose of clearing a `Vec`. /// /// ### Why is this bad? + /// This creates an unnecessary iterator that is dropped immediately. + /// + /// Calling `.clear()` also makes the intent clearer. /// /// ### Example /// ```rust - /// // example code where clippy issues a warning + /// let mut v = vec![1, 2, 3]; + /// v.drain(..); /// ``` /// Use instead: /// ```rust - /// // example code which does not raise clippy warning + /// let mut v = vec![1, 2, 3]; + /// v.clear(); /// ``` #[clippy::version = "1.69.0"] pub CLEAR_WITH_DRAIN, nursery, - "default lint description" + "calling `drain` in order to `clear` a `Vec`" } pub struct Methods { From c7e3e304d5d2ac82dbebe5f3d3e0611b407d81be Mon Sep 17 00:00:00 2001 From: bluthej Date: Tue, 21 Mar 2023 22:08:37 +0100 Subject: [PATCH 05/11] Add the lint logic --- clippy_lints/src/methods/clear_with_drain.rs | 27 +++++++++++++++++--- clippy_lints/src/methods/mod.rs | 10 ++++++-- 2 files changed, 32 insertions(+), 5 deletions(-) diff --git a/clippy_lints/src/methods/clear_with_drain.rs b/clippy_lints/src/methods/clear_with_drain.rs index 9e55852ef7a..ac6c0e57257 100644 --- a/clippy_lints/src/methods/clear_with_drain.rs +++ b/clippy_lints/src/methods/clear_with_drain.rs @@ -1,8 +1,29 @@ -use rustc_lint::{LateContext, LintContext}; +use clippy_utils::diagnostics::span_lint_and_sugg; +use clippy_utils::higher::Range; +use clippy_utils::is_range_full; +use clippy_utils::ty::is_type_diagnostic_item; +use rustc_errors::Applicability; +use rustc_hir::Expr; +use rustc_lint::LateContext; +use rustc_span::symbol::sym; +use rustc_span::Span; use super::CLEAR_WITH_DRAIN; // TODO: Adjust the parameters as necessary -pub(super) fn check(cx: &LateContext) { - todo!(); +// see clippy_lints/src/methods/mod.rs to add call to this check in `check_methods` +pub(super) fn check(cx: &LateContext<'_>, expr: &Expr<'_>, recv: &Expr<'_>, span: Span, arg: &Expr<'_>) { + let ty = cx.typeck_results().expr_ty(recv); + if is_type_diagnostic_item(cx, ty, sym::Vec) && let Some(range) = Range::hir(arg) && is_range_full(cx, recv, range) + { + span_lint_and_sugg( + cx, + CLEAR_WITH_DRAIN, + span.with_hi(expr.span.hi()), + "`drain` used to clear a `Vec`", + "try", + "clear()".to_string(), + Applicability::MachineApplicable, + ); + } } diff --git a/clippy_lints/src/methods/mod.rs b/clippy_lints/src/methods/mod.rs index 4e74ac6af2b..257bc4eccc3 100644 --- a/clippy_lints/src/methods/mod.rs +++ b/clippy_lints/src/methods/mod.rs @@ -111,7 +111,7 @@ use clippy_utils::ty::{contains_ty_adt_constructor_opaque, implements_trait, is_ use clippy_utils::{contains_return, is_bool, is_trait_method, iter_input_pats, return_ty}; use if_chain::if_chain; use rustc_hir as hir; -use rustc_hir::{Expr, ExprKind, TraitItem, TraitItemKind}; +use rustc_hir::{Expr, ExprKind, Node, Stmt, StmtKind, TraitItem, TraitItemKind}; use rustc_hir_analysis::hir_ty_to_ty; use rustc_lint::{LateContext, LateLintPass, LintContext}; use rustc_middle::lint::in_external_macro; @@ -3590,7 +3590,13 @@ impl Methods { _ => {}, }, ("drain", [arg]) => { - iter_with_drain::check(cx, expr, recv, span, arg); + if let Node::Stmt(Stmt { hir_id: _, kind, .. }) = cx.tcx.hir().get_parent(expr.hir_id) + && matches!(kind, StmtKind::Semi(_)) + { + clear_with_drain::check(cx, expr, recv, span, arg); + } else { + iter_with_drain::check(cx, expr, recv, span, arg); + } }, ("ends_with", [arg]) => { if let ExprKind::MethodCall(.., span) = expr.kind { From 1d0acce984f9be9f085fd5ebcb44dd6988b1f295 Mon Sep 17 00:00:00 2001 From: bluthej Date: Tue, 21 Mar 2023 22:09:26 +0100 Subject: [PATCH 06/11] Add test for partial range --- tests/ui/clear_with_drain.rs | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/tests/ui/clear_with_drain.rs b/tests/ui/clear_with_drain.rs index 3f8517011cd..28119deaebb 100644 --- a/tests/ui/clear_with_drain.rs +++ b/tests/ui/clear_with_drain.rs @@ -25,4 +25,15 @@ fn range_to() { v.drain(..v.len()); // Nay } +fn partial_drains() { + let mut v = vec![1, 2, 3]; + v.drain(1..); // Yay + + let mut v = vec![1, 2, 3]; + v.drain(..v.len() - 1); // Yay + + let mut v = vec![1, 2, 3]; + v.drain(1..v.len() - 1); // Yay +} + fn main() {} From e8ec242a61e7fcadf1a8cb3c6f7ec162b5e2726e Mon Sep 17 00:00:00 2001 From: bluthej Date: Tue, 21 Mar 2023 22:46:59 +0100 Subject: [PATCH 07/11] Finish tests - add rustfix --- clippy_lints/src/methods/clear_with_drain.rs | 2 - tests/ui/clear_with_drain.fixed | 40 ++++++++++++++++++++ tests/ui/clear_with_drain.rs | 1 + tests/ui/clear_with_drain.stderr | 28 ++++++++++++++ 4 files changed, 69 insertions(+), 2 deletions(-) create mode 100644 tests/ui/clear_with_drain.fixed create mode 100644 tests/ui/clear_with_drain.stderr diff --git a/clippy_lints/src/methods/clear_with_drain.rs b/clippy_lints/src/methods/clear_with_drain.rs index ac6c0e57257..bf7e7d12405 100644 --- a/clippy_lints/src/methods/clear_with_drain.rs +++ b/clippy_lints/src/methods/clear_with_drain.rs @@ -10,8 +10,6 @@ use rustc_span::Span; use super::CLEAR_WITH_DRAIN; -// TODO: Adjust the parameters as necessary -// see clippy_lints/src/methods/mod.rs to add call to this check in `check_methods` pub(super) fn check(cx: &LateContext<'_>, expr: &Expr<'_>, recv: &Expr<'_>, span: Span, arg: &Expr<'_>) { let ty = cx.typeck_results().expr_ty(recv); if is_type_diagnostic_item(cx, ty, sym::Vec) && let Some(range) = Range::hir(arg) && is_range_full(cx, recv, range) diff --git a/tests/ui/clear_with_drain.fixed b/tests/ui/clear_with_drain.fixed new file mode 100644 index 00000000000..62b0af08eea --- /dev/null +++ b/tests/ui/clear_with_drain.fixed @@ -0,0 +1,40 @@ +// run-rustfix +#![allow(unused)] +#![warn(clippy::clear_with_drain)] + +fn range() { + let (mut u, mut v) = (vec![1, 2, 3], vec![1, 2, 3]); + let iter = u.drain(0..u.len()); // Yay + v.clear(); // Nay +} + +fn range_from() { + let (mut u, mut v) = (vec![1, 2, 3], vec![1, 2, 3]); + let iter = u.drain(0..); // Yay + v.clear(); // Nay +} + +fn range_full() { + let (mut u, mut v) = (vec![1, 2, 3], vec![1, 2, 3]); + let iter = u.drain(..); // Yay + v.clear(); // Nay +} + +fn range_to() { + let (mut u, mut v) = (vec![1, 2, 3], vec![1, 2, 3]); + let iter = u.drain(..u.len()); // Yay + v.clear(); // Nay +} + +fn partial_drains() { + let mut v = vec![1, 2, 3]; + v.drain(1..); // Yay + + let mut v = vec![1, 2, 3]; + v.drain(..v.len() - 1); // Yay + + let mut v = vec![1, 2, 3]; + v.drain(1..v.len() - 1); // Yay +} + +fn main() {} diff --git a/tests/ui/clear_with_drain.rs b/tests/ui/clear_with_drain.rs index 28119deaebb..721af7d2ea7 100644 --- a/tests/ui/clear_with_drain.rs +++ b/tests/ui/clear_with_drain.rs @@ -1,3 +1,4 @@ +// run-rustfix #![allow(unused)] #![warn(clippy::clear_with_drain)] diff --git a/tests/ui/clear_with_drain.stderr b/tests/ui/clear_with_drain.stderr new file mode 100644 index 00000000000..0d6d8263e10 --- /dev/null +++ b/tests/ui/clear_with_drain.stderr @@ -0,0 +1,28 @@ +error: `drain` used to clear a `Vec` + --> $DIR/clear_with_drain.rs:8:7 + | +LL | v.drain(0..v.len()); // Nay + | ^^^^^^^^^^^^^^^^^ help: try: `clear()` + | + = note: `-D clippy::clear-with-drain` implied by `-D warnings` + +error: `drain` used to clear a `Vec` + --> $DIR/clear_with_drain.rs:14:7 + | +LL | v.drain(0..); // Nay + | ^^^^^^^^^^ help: try: `clear()` + +error: `drain` used to clear a `Vec` + --> $DIR/clear_with_drain.rs:20:7 + | +LL | v.drain(..); // Nay + | ^^^^^^^^^ help: try: `clear()` + +error: `drain` used to clear a `Vec` + --> $DIR/clear_with_drain.rs:26:7 + | +LL | v.drain(..v.len()); // Nay + | ^^^^^^^^^^^^^^^^ help: try: `clear()` + +error: aborting due to 4 previous errors + From ee0de538d4196596986328311bb097a07f473273 Mon Sep 17 00:00:00 2001 From: bluthej Date: Thu, 23 Mar 2023 22:29:30 +0100 Subject: [PATCH 08/11] Add some tests --- tests/ui/clear_with_drain.fixed | 48 ++++++++++++++++++++++++++------ tests/ui/clear_with_drain.rs | 48 ++++++++++++++++++++++++++------ tests/ui/clear_with_drain.stderr | 8 +++--- 3 files changed, 84 insertions(+), 20 deletions(-) diff --git a/tests/ui/clear_with_drain.fixed b/tests/ui/clear_with_drain.fixed index 62b0af08eea..6ab05d9baca 100644 --- a/tests/ui/clear_with_drain.fixed +++ b/tests/ui/clear_with_drain.fixed @@ -3,38 +3,70 @@ #![warn(clippy::clear_with_drain)] fn range() { - let (mut u, mut v) = (vec![1, 2, 3], vec![1, 2, 3]); - let iter = u.drain(0..u.len()); // Yay + let mut v = vec![1, 2, 3]; + let iter = v.drain(0..v.len()); // Yay + + let mut v = vec![1, 2, 3]; + let n = v.drain(0..v.len()).count(); // Yay + + let mut v = vec![1, 2, 3]; v.clear(); // Nay } fn range_from() { - let (mut u, mut v) = (vec![1, 2, 3], vec![1, 2, 3]); - let iter = u.drain(0..); // Yay + let mut v = vec![1, 2, 3]; + let iter = v.drain(0..); // Yay + + let mut v = vec![1, 2, 3]; + let next = v.drain(0..).next(); // Yay + + let mut v = vec![1, 2, 3]; v.clear(); // Nay } fn range_full() { - let (mut u, mut v) = (vec![1, 2, 3], vec![1, 2, 3]); - let iter = u.drain(..); // Yay + let mut v = vec![1, 2, 3]; + let iter = v.drain(..); // Yay + + let mut v = vec![1, 2, 3]; + // Yay + for x in v.drain(..) { + let y = format!("x = {x}"); + } + + let mut v = vec![1, 2, 3]; v.clear(); // Nay } fn range_to() { - let (mut u, mut v) = (vec![1, 2, 3], vec![1, 2, 3]); - let iter = u.drain(..u.len()); // Yay + let mut v = vec![1, 2, 3]; + let iter = v.drain(..v.len()); // Yay + + let mut v = vec![1, 2, 3]; + // Yay + for x in v.drain(..v.len()) { + let y = format!("x = {x}"); + } + + let mut v = vec![1, 2, 3]; v.clear(); // Nay } fn partial_drains() { let mut v = vec![1, 2, 3]; v.drain(1..); // Yay + let mut v = vec![1, 2, 3]; + v.drain(1..).max(); // Yay let mut v = vec![1, 2, 3]; v.drain(..v.len() - 1); // Yay + let mut v = vec![1, 2, 3]; + v.drain(..v.len() - 1).min(); // Yay let mut v = vec![1, 2, 3]; v.drain(1..v.len() - 1); // Yay + let mut v = vec![1, 2, 3]; + let w: Vec = v.drain(1..v.len() - 1).collect(); // Yay } fn main() {} diff --git a/tests/ui/clear_with_drain.rs b/tests/ui/clear_with_drain.rs index 721af7d2ea7..6926a4b60e8 100644 --- a/tests/ui/clear_with_drain.rs +++ b/tests/ui/clear_with_drain.rs @@ -3,38 +3,70 @@ #![warn(clippy::clear_with_drain)] fn range() { - let (mut u, mut v) = (vec![1, 2, 3], vec![1, 2, 3]); - let iter = u.drain(0..u.len()); // Yay + let mut v = vec![1, 2, 3]; + let iter = v.drain(0..v.len()); // Yay + + let mut v = vec![1, 2, 3]; + let n = v.drain(0..v.len()).count(); // Yay + + let mut v = vec![1, 2, 3]; v.drain(0..v.len()); // Nay } fn range_from() { - let (mut u, mut v) = (vec![1, 2, 3], vec![1, 2, 3]); - let iter = u.drain(0..); // Yay + let mut v = vec![1, 2, 3]; + let iter = v.drain(0..); // Yay + + let mut v = vec![1, 2, 3]; + let next = v.drain(0..).next(); // Yay + + let mut v = vec![1, 2, 3]; v.drain(0..); // Nay } fn range_full() { - let (mut u, mut v) = (vec![1, 2, 3], vec![1, 2, 3]); - let iter = u.drain(..); // Yay + let mut v = vec![1, 2, 3]; + let iter = v.drain(..); // Yay + + let mut v = vec![1, 2, 3]; + // Yay + for x in v.drain(..) { + let y = format!("x = {x}"); + } + + let mut v = vec![1, 2, 3]; v.drain(..); // Nay } fn range_to() { - let (mut u, mut v) = (vec![1, 2, 3], vec![1, 2, 3]); - let iter = u.drain(..u.len()); // Yay + let mut v = vec![1, 2, 3]; + let iter = v.drain(..v.len()); // Yay + + let mut v = vec![1, 2, 3]; + // Yay + for x in v.drain(..v.len()) { + let y = format!("x = {x}"); + } + + let mut v = vec![1, 2, 3]; v.drain(..v.len()); // Nay } fn partial_drains() { let mut v = vec![1, 2, 3]; v.drain(1..); // Yay + let mut v = vec![1, 2, 3]; + v.drain(1..).max(); // Yay let mut v = vec![1, 2, 3]; v.drain(..v.len() - 1); // Yay + let mut v = vec![1, 2, 3]; + v.drain(..v.len() - 1).min(); // Yay let mut v = vec![1, 2, 3]; v.drain(1..v.len() - 1); // Yay + let mut v = vec![1, 2, 3]; + let w: Vec = v.drain(1..v.len() - 1).collect(); // Yay } fn main() {} diff --git a/tests/ui/clear_with_drain.stderr b/tests/ui/clear_with_drain.stderr index 0d6d8263e10..b7206cd9007 100644 --- a/tests/ui/clear_with_drain.stderr +++ b/tests/ui/clear_with_drain.stderr @@ -1,5 +1,5 @@ error: `drain` used to clear a `Vec` - --> $DIR/clear_with_drain.rs:8:7 + --> $DIR/clear_with_drain.rs:13:7 | LL | v.drain(0..v.len()); // Nay | ^^^^^^^^^^^^^^^^^ help: try: `clear()` @@ -7,19 +7,19 @@ LL | v.drain(0..v.len()); // Nay = note: `-D clippy::clear-with-drain` implied by `-D warnings` error: `drain` used to clear a `Vec` - --> $DIR/clear_with_drain.rs:14:7 + --> $DIR/clear_with_drain.rs:24:7 | LL | v.drain(0..); // Nay | ^^^^^^^^^^ help: try: `clear()` error: `drain` used to clear a `Vec` - --> $DIR/clear_with_drain.rs:20:7 + --> $DIR/clear_with_drain.rs:38:7 | LL | v.drain(..); // Nay | ^^^^^^^^^ help: try: `clear()` error: `drain` used to clear a `Vec` - --> $DIR/clear_with_drain.rs:26:7 + --> $DIR/clear_with_drain.rs:52:7 | LL | v.drain(..v.len()); // Nay | ^^^^^^^^^^^^^^^^ help: try: `clear()` From 2493be2196f6ba16eaea25915c25d93f1c2664b5 Mon Sep 17 00:00:00 2001 From: bluthej Date: Sun, 26 Mar 2023 18:25:15 +0200 Subject: [PATCH 09/11] Improve `is_range_full` implementation Make this function work with signed integer types by extracting the underlying type and finding the min and max values. Change the signature to make it more consistent: - The range is now given as an `Expr` in order to extract the type - The container's path is now passed, and only as an `Option` so that the function can be called in the general case without a container --- clippy_lints/src/methods/clear_with_drain.rs | 7 +- clippy_lints/src/methods/iter_with_drain.rs | 7 +- clippy_utils/src/lib.rs | 71 ++++++++++++++++---- 3 files changed, 65 insertions(+), 20 deletions(-) diff --git a/clippy_lints/src/methods/clear_with_drain.rs b/clippy_lints/src/methods/clear_with_drain.rs index bf7e7d12405..24496bd4689 100644 --- a/clippy_lints/src/methods/clear_with_drain.rs +++ b/clippy_lints/src/methods/clear_with_drain.rs @@ -1,9 +1,8 @@ use clippy_utils::diagnostics::span_lint_and_sugg; -use clippy_utils::higher::Range; use clippy_utils::is_range_full; use clippy_utils::ty::is_type_diagnostic_item; use rustc_errors::Applicability; -use rustc_hir::Expr; +use rustc_hir::{Expr, ExprKind, QPath}; use rustc_lint::LateContext; use rustc_span::symbol::sym; use rustc_span::Span; @@ -12,7 +11,9 @@ use super::CLEAR_WITH_DRAIN; pub(super) fn check(cx: &LateContext<'_>, expr: &Expr<'_>, recv: &Expr<'_>, span: Span, arg: &Expr<'_>) { let ty = cx.typeck_results().expr_ty(recv); - if is_type_diagnostic_item(cx, ty, sym::Vec) && let Some(range) = Range::hir(arg) && is_range_full(cx, recv, range) + if is_type_diagnostic_item(cx, ty, sym::Vec) + && let ExprKind::Path(QPath::Resolved(None, container_path)) = recv.kind + && is_range_full(cx, arg, Some(container_path)) { span_lint_and_sugg( cx, diff --git a/clippy_lints/src/methods/iter_with_drain.rs b/clippy_lints/src/methods/iter_with_drain.rs index ea92e3a549f..f6772c5c6b3 100644 --- a/clippy_lints/src/methods/iter_with_drain.rs +++ b/clippy_lints/src/methods/iter_with_drain.rs @@ -1,8 +1,7 @@ use clippy_utils::diagnostics::span_lint_and_sugg; -use clippy_utils::higher::Range; use clippy_utils::is_range_full; use rustc_errors::Applicability; -use rustc_hir::{Expr, ExprKind}; +use rustc_hir::{Expr, ExprKind, QPath}; use rustc_lint::LateContext; use rustc_span::symbol::sym; use rustc_span::Span; @@ -14,8 +13,8 @@ pub(super) fn check(cx: &LateContext<'_>, expr: &Expr<'_>, recv: &Expr<'_>, span && let Some(adt) = cx.typeck_results().expr_ty(recv).ty_adt_def() && let Some(ty_name) = cx.tcx.get_diagnostic_name(adt.did()) && matches!(ty_name, sym::Vec | sym::VecDeque) - && let Some(range) = Range::hir(arg) - && is_range_full(cx, recv, range) + && let ExprKind::Path(QPath::Resolved(None, container_path)) = recv.kind + && is_range_full(cx, arg, Some(container_path)) { span_lint_and_sugg( cx, diff --git a/clippy_utils/src/lib.rs b/clippy_utils/src/lib.rs index e8225feb33d..2e839fdf472 100644 --- a/clippy_utils/src/lib.rs +++ b/clippy_utils/src/lib.rs @@ -96,6 +96,7 @@ use rustc_hir::{ use rustc_lexer::{tokenize, TokenKind}; use rustc_lint::{LateContext, Level, Lint, LintContext}; use rustc_middle::hir::place::PlaceBase; +use rustc_middle::mir::ConstantKind; use rustc_middle::ty as rustc_ty; use rustc_middle::ty::adjustment::{Adjust, Adjustment, AutoBorrow}; use rustc_middle::ty::binding::BindingMode; @@ -114,7 +115,7 @@ use rustc_span::symbol::{kw, Ident, Symbol}; use rustc_span::Span; use rustc_target::abi::Integer; -use crate::consts::{constant, Constant}; +use crate::consts::{constant, miri_to_const, Constant}; use crate::higher::Range; use crate::ty::{can_partially_move_ty, expr_sig, is_copy, is_recursively_primitive_type, ty_is_fn_once_param}; use crate::visitors::for_each_expr; @@ -1492,22 +1493,66 @@ pub fn is_else_clause(tcx: TyCtxt<'_>, expr: &Expr<'_>) -> bool { } } -/// Checks whether the given `Range` is equivalent to a `RangeFull`. -/// Inclusive ranges are not considered because they already constitute a lint. -pub fn is_range_full(cx: &LateContext<'_>, container: &Expr<'_>, range: Range<'_>) -> bool { - range.start.map_or(true, |e| is_integer_const(cx, e, 0)) - && range.end.map_or(true, |e| { - if range.limits == RangeLimits::HalfOpen - && let ExprKind::Path(QPath::Resolved(None, container_path)) = container.kind - && let ExprKind::MethodCall(name, self_arg, [], _) = e.kind - && name.ident.name == sym::len - && let ExprKind::Path(QPath::Resolved(None, path)) = self_arg.kind +/// Checks whether the given `Expr` is a range equivalent to a `RangeFull`. +/// For the lower bound, this means that: +/// - either there is none +/// - or it is the smallest value that can be represented by the range's integer type +/// For the upper bound, this means that: +/// - either there is none +/// - or it is the largest value that can be represented by the range's integer type and is +/// inclusive +/// - or it is a call to some container's `len` method and is exclusive, and the range is passed to +/// a method call on that same container (e.g. `v.drain(..v.len())`) +/// If the given `Expr` is not some kind of range, the function returns `false`. +pub fn is_range_full(cx: &LateContext<'_>, expr: &Expr<'_>, container_path: Option<&Path<'_>>) -> bool { + let ty = cx.typeck_results().expr_ty(expr); + if let Some(Range { start, end, limits }) = Range::hir(expr) { + let start_is_none_or_min = start.map_or(true, |start| { + if let rustc_ty::Adt(_, subst) = ty.kind() + && let bnd_ty = subst.type_at(0) + && let Some(min_val) = bnd_ty.numeric_min_val(cx.tcx) + && let const_val = cx.tcx.valtree_to_const_val((bnd_ty, min_val.to_valtree())) + && let min_const_kind = ConstantKind::from_value(const_val, bnd_ty) + && let Some(min_const) = miri_to_const(cx.tcx, min_const_kind) + && let Some((start_const, _)) = constant(cx, cx.typeck_results(), start) { - container_path.res == path.res + start_const == min_const } else { false } - }) + }); + let end_is_none_or_max = end.map_or(true, |end| { + match limits { + RangeLimits::Closed => { + if let rustc_ty::Adt(_, subst) = ty.kind() + && let bnd_ty = subst.type_at(0) + && let Some(max_val) = bnd_ty.numeric_max_val(cx.tcx) + && let const_val = cx.tcx.valtree_to_const_val((bnd_ty, max_val.to_valtree())) + && let max_const_kind = ConstantKind::from_value(const_val, bnd_ty) + && let Some(max_const) = miri_to_const(cx.tcx, max_const_kind) + && let Some((end_const, _)) = constant(cx, cx.typeck_results(), end) + { + end_const == max_const + } else { + false + } + }, + RangeLimits::HalfOpen => { + if let Some(container_path) = container_path + && let ExprKind::MethodCall(name, self_arg, [], _) = end.kind + && name.ident.name == sym::len + && let ExprKind::Path(QPath::Resolved(None, path)) = self_arg.kind + { + container_path.res == path.res + } else { + false + } + }, + } + }); + return start_is_none_or_min && end_is_none_or_max; + } + false } /// Checks whether the given expression is a constant integer of the given value. From 3966580c9d146b6938e691c8a1abefc89e0aa7b7 Mon Sep 17 00:00:00 2001 From: bluthej Date: Sun, 26 Mar 2023 18:59:20 +0200 Subject: [PATCH 10/11] Add tests with `usize::MIN` --- tests/ui/clear_with_drain.fixed | 12 ++++++++++++ tests/ui/clear_with_drain.rs | 12 ++++++++++++ tests/ui/clear_with_drain.stderr | 22 +++++++++++++++++----- 3 files changed, 41 insertions(+), 5 deletions(-) diff --git a/tests/ui/clear_with_drain.fixed b/tests/ui/clear_with_drain.fixed index 6ab05d9baca..0a15463ddf7 100644 --- a/tests/ui/clear_with_drain.fixed +++ b/tests/ui/clear_with_drain.fixed @@ -9,6 +9,12 @@ fn range() { let mut v = vec![1, 2, 3]; let n = v.drain(0..v.len()).count(); // Yay + let mut v = vec![1, 2, 3]; + let n = v.drain(usize::MIN..v.len()).count(); // Yay + + let mut v = vec![1, 2, 3]; + v.clear(); // Nay + let mut v = vec![1, 2, 3]; v.clear(); // Nay } @@ -20,6 +26,12 @@ fn range_from() { let mut v = vec![1, 2, 3]; let next = v.drain(0..).next(); // Yay + let mut v = vec![1, 2, 3]; + let next = v.drain(usize::MIN..).next(); // Yay + + let mut v = vec![1, 2, 3]; + v.clear(); // Nay + let mut v = vec![1, 2, 3]; v.clear(); // Nay } diff --git a/tests/ui/clear_with_drain.rs b/tests/ui/clear_with_drain.rs index 6926a4b60e8..40201d9cc4d 100644 --- a/tests/ui/clear_with_drain.rs +++ b/tests/ui/clear_with_drain.rs @@ -9,8 +9,14 @@ fn range() { let mut v = vec![1, 2, 3]; let n = v.drain(0..v.len()).count(); // Yay + let mut v = vec![1, 2, 3]; + let n = v.drain(usize::MIN..v.len()).count(); // Yay + let mut v = vec![1, 2, 3]; v.drain(0..v.len()); // Nay + + let mut v = vec![1, 2, 3]; + v.drain(usize::MIN..v.len()); // Nay } fn range_from() { @@ -20,8 +26,14 @@ fn range_from() { let mut v = vec![1, 2, 3]; let next = v.drain(0..).next(); // Yay + let mut v = vec![1, 2, 3]; + let next = v.drain(usize::MIN..).next(); // Yay + let mut v = vec![1, 2, 3]; v.drain(0..); // Nay + + let mut v = vec![1, 2, 3]; + v.drain(usize::MIN..); // Nay } fn range_full() { diff --git a/tests/ui/clear_with_drain.stderr b/tests/ui/clear_with_drain.stderr index b7206cd9007..e7152a1cb68 100644 --- a/tests/ui/clear_with_drain.stderr +++ b/tests/ui/clear_with_drain.stderr @@ -1,5 +1,5 @@ error: `drain` used to clear a `Vec` - --> $DIR/clear_with_drain.rs:13:7 + --> $DIR/clear_with_drain.rs:16:7 | LL | v.drain(0..v.len()); // Nay | ^^^^^^^^^^^^^^^^^ help: try: `clear()` @@ -7,22 +7,34 @@ LL | v.drain(0..v.len()); // Nay = note: `-D clippy::clear-with-drain` implied by `-D warnings` error: `drain` used to clear a `Vec` - --> $DIR/clear_with_drain.rs:24:7 + --> $DIR/clear_with_drain.rs:19:7 + | +LL | v.drain(usize::MIN..v.len()); // Nay + | ^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `clear()` + +error: `drain` used to clear a `Vec` + --> $DIR/clear_with_drain.rs:33:7 | LL | v.drain(0..); // Nay | ^^^^^^^^^^ help: try: `clear()` error: `drain` used to clear a `Vec` - --> $DIR/clear_with_drain.rs:38:7 + --> $DIR/clear_with_drain.rs:36:7 + | +LL | v.drain(usize::MIN..); // Nay + | ^^^^^^^^^^^^^^^^^^^ help: try: `clear()` + +error: `drain` used to clear a `Vec` + --> $DIR/clear_with_drain.rs:50:7 | LL | v.drain(..); // Nay | ^^^^^^^^^ help: try: `clear()` error: `drain` used to clear a `Vec` - --> $DIR/clear_with_drain.rs:52:7 + --> $DIR/clear_with_drain.rs:64:7 | LL | v.drain(..v.len()); // Nay | ^^^^^^^^^^^^^^^^ help: try: `clear()` -error: aborting due to 4 previous errors +error: aborting due to 6 previous errors From df65d21f4c98a19573f1cd89d9ff01ee8812cf43 Mon Sep 17 00:00:00 2001 From: bluthej Date: Mon, 27 Mar 2023 11:11:40 +0200 Subject: [PATCH 11/11] Include tests where the iterator is used later --- tests/ui/clear_with_drain.fixed | 10 ++++++---- tests/ui/clear_with_drain.rs | 10 ++++++---- tests/ui/clear_with_drain.stderr | 12 ++++++------ 3 files changed, 18 insertions(+), 14 deletions(-) diff --git a/tests/ui/clear_with_drain.fixed b/tests/ui/clear_with_drain.fixed index 0a15463ddf7..9c4dc010ca7 100644 --- a/tests/ui/clear_with_drain.fixed +++ b/tests/ui/clear_with_drain.fixed @@ -10,7 +10,8 @@ fn range() { let n = v.drain(0..v.len()).count(); // Yay let mut v = vec![1, 2, 3]; - let n = v.drain(usize::MIN..v.len()).count(); // Yay + let iter = v.drain(usize::MIN..v.len()); // Yay + let n = iter.count(); let mut v = vec![1, 2, 3]; v.clear(); // Nay @@ -24,7 +25,8 @@ fn range_from() { let iter = v.drain(0..); // Yay let mut v = vec![1, 2, 3]; - let next = v.drain(0..).next(); // Yay + let mut iter = v.drain(0..); // Yay + let next = iter.next(); let mut v = vec![1, 2, 3]; let next = v.drain(usize::MIN..).next(); // Yay @@ -55,8 +57,8 @@ fn range_to() { let iter = v.drain(..v.len()); // Yay let mut v = vec![1, 2, 3]; - // Yay - for x in v.drain(..v.len()) { + let iter = v.drain(..v.len()); // Yay + for x in iter { let y = format!("x = {x}"); } diff --git a/tests/ui/clear_with_drain.rs b/tests/ui/clear_with_drain.rs index 40201d9cc4d..f00dbab234c 100644 --- a/tests/ui/clear_with_drain.rs +++ b/tests/ui/clear_with_drain.rs @@ -10,7 +10,8 @@ fn range() { let n = v.drain(0..v.len()).count(); // Yay let mut v = vec![1, 2, 3]; - let n = v.drain(usize::MIN..v.len()).count(); // Yay + let iter = v.drain(usize::MIN..v.len()); // Yay + let n = iter.count(); let mut v = vec![1, 2, 3]; v.drain(0..v.len()); // Nay @@ -24,7 +25,8 @@ fn range_from() { let iter = v.drain(0..); // Yay let mut v = vec![1, 2, 3]; - let next = v.drain(0..).next(); // Yay + let mut iter = v.drain(0..); // Yay + let next = iter.next(); let mut v = vec![1, 2, 3]; let next = v.drain(usize::MIN..).next(); // Yay @@ -55,8 +57,8 @@ fn range_to() { let iter = v.drain(..v.len()); // Yay let mut v = vec![1, 2, 3]; - // Yay - for x in v.drain(..v.len()) { + let iter = v.drain(..v.len()); // Yay + for x in iter { let y = format!("x = {x}"); } diff --git a/tests/ui/clear_with_drain.stderr b/tests/ui/clear_with_drain.stderr index e7152a1cb68..c88aa1a23cb 100644 --- a/tests/ui/clear_with_drain.stderr +++ b/tests/ui/clear_with_drain.stderr @@ -1,5 +1,5 @@ error: `drain` used to clear a `Vec` - --> $DIR/clear_with_drain.rs:16:7 + --> $DIR/clear_with_drain.rs:17:7 | LL | v.drain(0..v.len()); // Nay | ^^^^^^^^^^^^^^^^^ help: try: `clear()` @@ -7,31 +7,31 @@ LL | v.drain(0..v.len()); // Nay = note: `-D clippy::clear-with-drain` implied by `-D warnings` error: `drain` used to clear a `Vec` - --> $DIR/clear_with_drain.rs:19:7 + --> $DIR/clear_with_drain.rs:20:7 | LL | v.drain(usize::MIN..v.len()); // Nay | ^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `clear()` error: `drain` used to clear a `Vec` - --> $DIR/clear_with_drain.rs:33:7 + --> $DIR/clear_with_drain.rs:35:7 | LL | v.drain(0..); // Nay | ^^^^^^^^^^ help: try: `clear()` error: `drain` used to clear a `Vec` - --> $DIR/clear_with_drain.rs:36:7 + --> $DIR/clear_with_drain.rs:38:7 | LL | v.drain(usize::MIN..); // Nay | ^^^^^^^^^^^^^^^^^^^ help: try: `clear()` error: `drain` used to clear a `Vec` - --> $DIR/clear_with_drain.rs:50:7 + --> $DIR/clear_with_drain.rs:52:7 | LL | v.drain(..); // Nay | ^^^^^^^^^ help: try: `clear()` error: `drain` used to clear a `Vec` - --> $DIR/clear_with_drain.rs:64:7 + --> $DIR/clear_with_drain.rs:66:7 | LL | v.drain(..v.len()); // Nay | ^^^^^^^^^^^^^^^^ help: try: `clear()`