Auto merge of #10528 - bluthej:clear-with-drain, r=llogiq

Clear with drain

changelog: [`clear_with_drain`]: Add new lint

Fixes #9339
This commit is contained in:
bors 2023-03-27 11:41:48 +00:00
commit 70db22648b
9 changed files with 346 additions and 25 deletions

View file

@ -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

View file

@ -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,

View file

@ -0,0 +1,28 @@
use clippy_utils::diagnostics::span_lint_and_sugg;
use clippy_utils::is_range_full;
use clippy_utils::ty::is_type_diagnostic_item;
use rustc_errors::Applicability;
use rustc_hir::{Expr, ExprKind, QPath};
use rustc_lint::LateContext;
use rustc_span::symbol::sym;
use rustc_span::Span;
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 ExprKind::Path(QPath::Resolved(None, container_path)) = recv.kind
&& is_range_full(cx, arg, Some(container_path))
{
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,
);
}
}

View file

@ -1,7 +1,5 @@
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_lint::LateContext;
@ -15,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_full_range(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,
@ -29,19 +27,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
}
})
}

View file

@ -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;
@ -110,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;
@ -3190,6 +3191,31 @@ declare_clippy_lint! {
"single command line argument that looks like it should be multiple arguments"
}
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
/// let mut v = vec![1, 2, 3];
/// v.drain(..);
/// ```
/// Use instead:
/// ```rust
/// let mut v = vec![1, 2, 3];
/// v.clear();
/// ```
#[clippy::version = "1.69.0"]
pub CLEAR_WITH_DRAIN,
nursery,
"calling `drain` in order to `clear` a `Vec`"
}
pub struct Methods {
avoid_breaking_exported_api: bool,
msrv: Msrv,
@ -3318,6 +3344,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.
@ -3563,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 {

View file

@ -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;
@ -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,8 @@ 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;
@ -1491,6 +1493,68 @@ pub fn is_else_clause(tcx: TyCtxt<'_>, expr: &Expr<'_>) -> bool {
}
}
/// 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)
{
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.
/// unlike `is_integer_literal`, this version does const folding
pub fn is_integer_const(cx: &LateContext<'_>, e: &Expr<'_>, value: u128) -> bool {

View file

@ -0,0 +1,86 @@
// run-rustfix
#![allow(unused)]
#![warn(clippy::clear_with_drain)]
fn range() {
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];
let iter = v.drain(usize::MIN..v.len()); // Yay
let n = iter.count();
let mut v = vec![1, 2, 3];
v.clear(); // Nay
let mut v = vec![1, 2, 3];
v.clear(); // Nay
}
fn range_from() {
let mut v = vec![1, 2, 3];
let iter = v.drain(0..); // Yay
let mut v = vec![1, 2, 3];
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
let mut v = vec![1, 2, 3];
v.clear(); // Nay
let mut v = vec![1, 2, 3];
v.clear(); // Nay
}
fn range_full() {
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 v = vec![1, 2, 3];
let iter = v.drain(..v.len()); // Yay
let mut v = vec![1, 2, 3];
let iter = v.drain(..v.len()); // Yay
for x in iter {
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<i8> = v.drain(1..v.len() - 1).collect(); // Yay
}
fn main() {}

View file

@ -0,0 +1,86 @@
// run-rustfix
#![allow(unused)]
#![warn(clippy::clear_with_drain)]
fn range() {
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];
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
let mut v = vec![1, 2, 3];
v.drain(usize::MIN..v.len()); // Nay
}
fn range_from() {
let mut v = vec![1, 2, 3];
let iter = v.drain(0..); // Yay
let mut v = vec![1, 2, 3];
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
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() {
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 v = vec![1, 2, 3];
let iter = v.drain(..v.len()); // Yay
let mut v = vec![1, 2, 3];
let iter = v.drain(..v.len()); // Yay
for x in iter {
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<i8> = v.drain(1..v.len() - 1).collect(); // Yay
}
fn main() {}

View file

@ -0,0 +1,40 @@
error: `drain` used to clear a `Vec`
--> $DIR/clear_with_drain.rs:17: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: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:35:7
|
LL | v.drain(0..); // Nay
| ^^^^^^^^^^ help: try: `clear()`
error: `drain` used to clear a `Vec`
--> $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:52:7
|
LL | v.drain(..); // Nay
| ^^^^^^^^^ help: try: `clear()`
error: `drain` used to clear a `Vec`
--> $DIR/clear_with_drain.rs:66:7
|
LL | v.drain(..v.len()); // Nay
| ^^^^^^^^^^^^^^^^ help: try: `clear()`
error: aborting due to 6 previous errors