New lint truncate_with_drain #13603

Kither12 · 2024-10-25T08:12:07Z

I add new lint that replace vec.drain(x..) with vec.truncate(x). See #13580
changelog: new lint: [truncate_with_drain]

rustbot · 2024-10-25T08:12:13Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @blyxyas (or someone else) some time within the next two weeks.

Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (S-waiting-on-review and S-waiting-on-author) stays updated, invoking these commands when appropriate:

@rustbot author: the review is finished, PR author should check the comments and take action accordingly
@rustbot review: the author is ready for a review, this PR will be queued again in the reviewer's queue

samueltardieu · 2024-10-25T13:19:46Z

tests/ui/truncate_with_drain.rs

+    let mut v = vec![1, 2, 3];
+    let n = v.drain(1..v.len()).count();
+
+    // Do not lint because iterator is assigned and used


Isn't that already fully covered by the "Do not lint because iterator is assigned" test?

I think used and assigned is different in this case and both need to have a test? Maybe I will remove the assigned_and_used test.

samueltardieu · 2024-10-25T13:21:22Z

tests/ui/truncate_with_drain.rs

+    let mut deque = VecDeque::from([1, 2, 3]);
+    let n = deque.drain(1..deque.len()).count();
+
+    // Do not lint because iterator is assigned and used


Ditto, looks already covered by the first test in this function.

samueltardieu · 2024-10-25T13:22:15Z

tests/ui/truncate_with_drain.rs

+    let mut s = String::from("Hello, world!");
+    let n = s.drain(1..s.len()).count();
+
+    // Do not lint because iterator is assigned and used


samueltardieu · 2024-10-25T13:22:40Z

tests/ui/truncate_with_drain.rs

+    let mut s = String::from("Hello, world!");
+    let iter = s.drain(1..);
+
+    // Do not lint because iterator is assigned and used


samueltardieu · 2024-10-25T13:24:05Z

clippy_lints/src/methods/mod.rs

@@ -4634,6 +4661,7 @@ impl Methods {
                        && matches!(kind, StmtKind::Semi(_))
                        && args.len() <= 1
                    {
+                        truncate_with_drain::check(cx, expr, recv, span, args.first());


Shouldn't clear_with_drain get a chance to run first?

I think the order is not important because truncate_with_drain don't lint on fully-opened range?

I thought it would check a 0.. as well but it doesn't. Agreed, the order is unimportant in this case.

samueltardieu · 2024-10-25T13:27:01Z

clippy_lints/src/methods/truncate_with_drain.rs

+
+pub(super) fn check(cx: &LateContext<'_>, expr: &Expr<'_>, recv: &Expr<'_>, span: Span, arg: Option<&Expr<'_>>) {
+    if let Some(arg) = arg {
+        if match_acceptable_type(cx, recv, &ACCEPTABLE_TYPES_WITH_ARG)


It feels unbalanced to pass ACCEPTABLE_TYPES_WITH_ARG while having String builtin into the function. Why not remove this third argument and have ACCEPTABLE_TYPES_WITH_ARG built inside match_acceptable_type() as well?

samueltardieu · 2024-10-25T13:28:14Z

clippy_lints/src/methods/truncate_with_drain.rs

+    }
+}
+
+fn match_acceptable_type(cx: &LateContext<'_>, expr: &Expr<'_>, types: &[rustc_span::Symbol]) -> bool {


Maybe a less "general" name would be better. is_handled_collection_type() maybe?

samueltardieu · 2024-10-25T13:30:24Z

tests/ui/truncate_with_drain.fixed

+
+    // Do lint
+    let mut v = vec![1, 2, 3];
+    v.truncate(1);


Wouldn't be better to include error markers to ensure a regression can be identified automatically?

I'm not sure I know what are error markers, did you mean adding new tests or something?

They explain what error should be raised, and where. See e.g. https://github.com/rust-lang/rust-clippy/blob/master/tests/ui/into_iter_without_iter.rs#L10

Thank you, I'll look into that

samueltardieu · 2024-10-25T13:33:13Z

clippy_lints/src/methods/truncate_with_drain.rs

+                false
+            }
+        });
+        let end_is_none_or_max = end.map_or(true, |end| match limits {


Maybe use end.is_none_or(|end| …), as a new lint will probably flag this hand-made construct.

samueltardieu · 2024-10-25T13:35:53Z

clippy_lints/src/methods/truncate_with_drain.rs

+                TRUNCATE_WITH_DRAIN,
+                span.with_hi(expr.span.hi()),
+                format!("`drain` used to truncate a `{ty_name}`"),
+                "try",


Since this is supposed to be machine-applicable, you could probably use "use" here instead of "try".

Jarcho · 2024-10-26T13:10:38Z

This is basically the same as an existing lint clear_with_drain except it allows a lower bound. The two lints should share an implementation.

Kither12 · 2024-10-26T16:44:19Z

This is basically the same as an existing lint clear_with_drain except it allows a lower bound. The two lints should share an implementation.

So do you think that we should merge two lints into one? I think we can possibly add a lint that replace vec.truncate(0) to vec.clear() as well.

Jarcho · 2024-10-27T13:06:05Z

If you can come up with a good name I have no objection. I would have personally named these something like manual_collection_clear just to open up the possibility of detecting more cases.

Kither12 · 2024-10-27T15:08:39Z

If you can come up with a good name I have no objection. I would have personally named these something like manual_collection_clear just to open up the possibility of detecting more cases.

How about manual_collection_truncate, and then check for truncate(0) and replace to clear()? With that I think we can lint more cases like split_off(x) too.

blyxyas

Good first patch, some changes to be made =^w^=
Thanks for the contribution! ❤️

blyxyas · 2024-10-28T10:49:33Z

tests/ui/truncate_with_drain.rs

Could you add error anotations?
While not enforced by CI yet, we'd like to have them enforced, at least manually.

let mut v = vec![1, 2, 3]; //~^ ERROR: <error msg>

blyxyas · 2024-10-28T10:55:15Z

clippy_lints/src/methods/truncate_with_drain.rs

+
+pub(super) fn check(cx: &LateContext<'_>, expr: &Expr<'_>, recv: &Expr<'_>, span: Span, arg: Option<&Expr<'_>>) {
+    if let Some(arg) = arg {
+        if is_handled_collection_type(cx, recv)


In these all the three functions you call expr_ty on the typecheck results. While this is cached, it's still not ideal. Could you refactor it so that it's only called once in the main check function?

blyxyas · 2024-10-28T14:39:08Z

clippy_lints/src/methods/truncate_with_drain.rs

+    // Use `opt_item_name` while `String` is not a diagnostic item
+        && let Some(ty_name) = cx.tcx.opt_item_name(adt.did())
+    {
+        if let Some(higher::Range { start: Some(start), .. }) = higher::Range::hir(arg) {


Instead of re-parsing the Range, could you pass it from is_range_open_ended or refactor it so that the higher::Range comes from the main check function?

blyxyas · 2024-10-28T14:42:33Z

clippy_lints/src/methods/mod.rs

@@ -4166,6 +4167,31 @@ declare_clippy_lint! {
    "calling `.first().is_some()` or `.first().is_none()` instead of `.is_empty()`"
 }

+declare_clippy_lint! {
+    /// ### What it does
+    /// Checks for usage of `.drain(x..)` for the sole purpose of truncate a container.


Suggested change

/// Checks for usage of `.drain(x..)` for the sole purpose of truncate a container.

/// Checks for usage of `.drain(x..)` for the sole purpose of truncating a container.

blyxyas · 2024-10-28T14:42:56Z

clippy_lints/src/methods/mod.rs

+    #[clippy::version = "1.84.0"]
+    pub TRUNCATE_WITH_DRAIN,
+    nursery,
+    "calling `drain` in order to `truncate` a `Vec`"


The use of truncate here is as a verb, not as a function name, unless I misunderstood something 👀

Suggested change

"calling `drain` in order to `truncate` a `Vec`"

"calling `drain` in order to truncate a `Vec`"

blyxyas · 2024-10-28T14:58:20Z

clippy_lints/src/methods/truncate_with_drain.rs

+use rustc_hir::{Expr, ExprKind, LangItem, Path, QPath};
+use rustc_lint::LateContext;
+use rustc_middle::mir::Const;
+use rustc_middle::ty::{self as rustc_ty};


Is there are reason you alias this? I just spent 3 minutes looking for any reference of it in the documentation and the IDE wasn't helping 😅

I think just importing as ty (the default) would be less confusing

blyxyas · 2024-10-28T15:27:44Z

clippy_lints/src/methods/truncate_with_drain.rs

+            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 Some(min_const) = mir_to_const(cx.tcx, Const::from_ty_const(min_val, bnd_ty, cx.tcx))
+                && let Some(start_const) = ConstEvalCtxt::new(cx).eval(start)


Why is this block needed?

AFAIK there isn't any equivalence on ..n = i64::MIN..n, in fact, for signed integers the second one will give a syntax error.

It checks for usize::MIN..n if I'm not wrong

blyxyas · 2024-10-28T15:27:54Z

clippy_lints/src/methods/truncate_with_drain.rs

+        });
+        let end_is_none_or_max = end.is_none_or(|end| match limits {
+            RangeLimits::Closed => {
+                if let rustc_ty::Adt(_, subst) = ty.kind()


The same question here.

bors · 2024-10-29T08:18:55Z

☔ The latest upstream changes (presumably #13437) made this pull request unmergeable. Please resolve the merge conflicts.

samueltardieu · 2024-11-05T08:37:18Z

Btw, the list of commits brought in this PR doesn't look right.

blyxyas · 2024-11-13T18:55:37Z

@Kither12 if you're having troubles with removing those commits from this PR, you can close it and re-open one. (In the worst case scenario, copy pasting the changes)
Remember to assign me using r? @blyxyas at the end of the PR description if you're going that route.

Kither12 · 2024-11-16T11:24:27Z

@blyxyas I have fixed the commit history, sorry for those messes.

blyxyas · 2024-11-25T19:16:00Z

clippy_lints/src/methods/truncate_with_drain.rs

+    let start_is_none_or_min = start.map_or(true, |start| {
+        if let Adt(_, subst) = ty.kind()
+            && let bnd_ty = subst.type_at(0)
+            && let Some(min_val) = bnd_ty.numeric_min_val(cx.tcx)
+            && let Some(min_const) = mir_to_const(cx.tcx, Const::from_ty_const(min_val, bnd_ty, cx.tcx))
+            && let Some(start_const) = ConstEvalCtxt::new(cx).eval(start)
+        {
+            start_const == min_const
+        } else {
+            false
+        }
+    });
+    let end_is_none_or_max = end.map_or(true, |end| match limits {
+        RangeLimits::Closed => {
+            if let Adt(_, subst) = ty.kind()
+                && let bnd_ty = subst.type_at(0)
+                && let Some(max_val) = bnd_ty.numeric_max_val(cx.tcx)
+                && let Some(max_const) = mir_to_const(cx.tcx, Const::from_ty_const(max_val, bnd_ty, cx.tcx))
+                && let Some(end_const) = ConstEvalCtxt::new(cx).eval(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
+            }
+        },
+    });
+    !start_is_none_or_min && end_is_none_or_max


What about a little refactor?

Suggested change

let start_is_none_or_min = start.map_or(true, |start| {

if let Adt(_, subst) = ty.kind()

&& let bnd_ty = subst.type_at(0)

&& let Some(min_val) = bnd_ty.numeric_min_val(cx.tcx)

&& let Some(min_const) = mir_to_const(cx.tcx, Const::from_ty_const(min_val, bnd_ty, cx.tcx))

&& let Some(start_const) = ConstEvalCtxt::new(cx).eval(start)

{

start_const == min_const

} else {

false

}

});

let end_is_none_or_max = end.map_or(true, |end| match limits {

RangeLimits::Closed => {

if let Adt(_, subst) = ty.kind()

&& let bnd_ty = subst.type_at(0)

&& let Some(max_val) = bnd_ty.numeric_max_val(cx.tcx)

&& let Some(max_const) = mir_to_const(cx.tcx, Const::from_ty_const(max_val, bnd_ty, cx.tcx))

&& let Some(end_const) = ConstEvalCtxt::new(cx).eval(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

}

},

});

!start_is_none_or_min && end_is_none_or_max

let start_is_none_or_min = start.map_or(true, |start| {

if let Adt(_, subst) = ty.kind()

&& let bnd_ty = subst.type_at(0)

&& let Some(min_val) = bnd_ty.numeric_min_val(cx.tcx)

&& let Some(min_const) = mir_to_const(cx.tcx, Const::from_ty_const(min_val, bnd_ty, cx.tcx))

&& let Some(start_const) = ConstEvalCtxt::new(cx).eval(start)

{

start_const == min_const

} else {

false

}

});

if !start_is_none_or_min {

// Is the end none or max?

return end.map_or(true, |end| match limits {

RangeLimits::Closed => {

if let Adt(_, subst) = ty.kind()

&& let bnd_ty = subst.type_at(0)

&& let Some(max_val) = bnd_ty.numeric_max_val(cx.tcx)

&& let Some(max_const) = mir_to_const(cx.tcx, Const::from_ty_const(max_val, bnd_ty, cx.tcx))

&& let Some(end_const) = ConstEvalCtxt::new(cx).eval(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

}

},

});

}

blyxyas · 2024-11-25T19:40:11Z

clippy_lints/src/methods/truncate_with_drain.rs

+        let typeck_results = cx.typeck_results();
+        if match_acceptable_type(cx, recv, typeck_results, &ACCEPTABLE_TYPES_WITH_ARG)
+            && let ExprKind::Path(QPath::Resolved(None, container_path)) = recv.kind
+            && let Some(range) = higher::Range::hir(arg)
+            && let higher::Range { start: Some(start), .. } = range


I think it would be better if we, instead of querying the typeck results for the current body for every body, we check for something simpler? It would be much more performant, as querying typeck results is very expensive.

Suggested change

let typeck_results = cx.typeck_results();

if match_acceptable_type(cx, recv, typeck_results, &ACCEPTABLE_TYPES_WITH_ARG)

&& let ExprKind::Path(QPath::Resolved(None, container_path)) = recv.kind

&& let Some(range) = higher::Range::hir(arg)

&& let higher::Range { start: Some(start), .. } = range

if let ExprKind::Path(QPath::Resolved(None, container_path)) = recv.kind

&& let Some(range) = higher::Range::hir(arg)

&& let higher::Range { start: Some(start), .. } = range

&& let typeck_results = cx.typeck_results()

&& match_acceptable_type(cx, recv, typeck_results, &ACCEPTABLE_TYPES_WITH_ARG)

blyxyas · 2024-11-25T19:42:40Z

clippy_lints/src/methods/truncate_with_drain.rs

+                span.with_hi(expr.span.hi()),
+                format!("`drain` used to truncate a `{ty_name}`"),
+                "try",
+                format!("truncate({})", snippet(cx, start.span, "0")),


Could you use snippet_opt instead of snippet? A false negative is better than an ICE.

blyxyas · 2024-11-25T19:45:17Z

clippy_lints/src/methods/truncate_with_drain.rs

+            && let Some(adt) = typeck_results.expr_ty(recv).ty_adt_def()
+            // Use `opt_item_name` while `String` is not a diagnostic item
+            && let Some(ty_name) = cx.tcx.opt_item_name(adt.did())


I'm not sure if querying for Expr type and item name is worth it for the diagnostic message.

blyxyas · 2024-11-25T19:46:26Z

This patch seems pretty good! Just some comments, refactors and some performance-headed comments.

rustbot assigned blyxyas Oct 25, 2024

rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Oct 25, 2024

samueltardieu reviewed Oct 25, 2024

View reviewed changes

blyxyas requested changes Oct 28, 2024

View reviewed changes

Kither12 force-pushed the ft/truncate_with_drain branch from 13af889 to 6f1b342 Compare November 16, 2024 10:40

truncate_with_drain

6f1b342

blyxyas reviewed Nov 25, 2024

View reviewed changes

	/// Checks for usage of `.drain(x..)` for the sole purpose of truncate a container.
	/// Checks for usage of `.drain(x..)` for the sole purpose of truncating a container.

	"calling `drain` in order to `truncate` a `Vec`"
	"calling `drain` in order to truncate a `Vec`"

New lint truncate_with_drain #13603

Are you sure you want to change the base?

New lint truncate_with_drain #13603

Conversation

Kither12 commented Oct 25, 2024 • edited Loading

rustbot commented Oct 25, 2024

Choose a reason for hiding this comment

Kither12 Oct 25, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Jarcho commented Oct 26, 2024

Kither12 commented Oct 26, 2024 • edited Loading

Jarcho commented Oct 27, 2024

Kither12 commented Oct 27, 2024

blyxyas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

blyxyas Oct 28, 2024 • edited Loading

Choose a reason for hiding this comment

Kither12 Nov 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bors commented Oct 29, 2024

samueltardieu commented Nov 5, 2024 • edited Loading

blyxyas commented Nov 13, 2024 • edited Loading

Kither12 commented Nov 16, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

blyxyas commented Nov 25, 2024

Kither12 commented Oct 25, 2024 •

edited

Loading

Kither12 Oct 25, 2024 •

edited

Loading

Kither12 commented Oct 26, 2024 •

edited

Loading

blyxyas Oct 28, 2024 •

edited

Loading

Kither12 Nov 4, 2024 •

edited

Loading

samueltardieu commented Nov 5, 2024 •

edited

Loading

blyxyas commented Nov 13, 2024 •

edited

Loading