-
Notifications
You must be signed in to change notification settings - Fork 1.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
New lint truncate_with_drain #13603
base: master
Are you sure you want to change the base?
New lint truncate_with_drain #13603
Conversation
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 (
|
tests/ui/truncate_with_drain.rs
Outdated
let mut v = vec![1, 2, 3]; | ||
let n = v.drain(1..v.len()).count(); | ||
|
||
// Do not lint because iterator is assigned and used |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't that already fully covered by the "Do not lint because iterator is assigned" test?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
tests/ui/truncate_with_drain.rs
Outdated
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ditto, looks already covered by the first test in this function.
tests/ui/truncate_with_drain.rs
Outdated
let mut s = String::from("Hello, world!"); | ||
let n = s.drain(1..s.len()).count(); | ||
|
||
// Do not lint because iterator is assigned and used |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ditto.
tests/ui/truncate_with_drain.rs
Outdated
let mut s = String::from("Hello, world!"); | ||
let iter = s.drain(1..); | ||
|
||
// Do not lint because iterator is assigned and used |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ditto.
@@ -4634,6 +4661,7 @@ impl Methods { | |||
&& matches!(kind, StmtKind::Semi(_)) | |||
&& args.len() <= 1 | |||
{ | |||
truncate_with_drain::check(cx, expr, recv, span, args.first()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't clear_with_drain
get a chance to run first?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the order is not important because truncate_with_drain
don't lint on fully-opened range?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought it would check a 0..
as well but it doesn't. Agreed, the order is unimportant in this case.
|
||
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) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
} | ||
} | ||
|
||
fn match_acceptable_type(cx: &LateContext<'_>, expr: &Expr<'_>, types: &[rustc_span::Symbol]) -> bool { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe a less "general" name would be better. is_handled_collection_type()
maybe?
|
||
// Do lint | ||
let mut v = vec![1, 2, 3]; | ||
v.truncate(1); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wouldn't be better to include error markers to ensure a regression can be identified automatically?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure I know what are error markers, did you mean adding new tests or something?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you, I'll look into that
false | ||
} | ||
}); | ||
let end_is_none_or_max = end.map_or(true, |end| match limits { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe use end.is_none_or(|end| …)
, as a new lint will probably flag this hand-made construct.
TRUNCATE_WITH_DRAIN, | ||
span.with_hi(expr.span.hi()), | ||
format!("`drain` used to truncate a `{ty_name}`"), | ||
"try", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this is supposed to be machine-applicable, you could probably use "use" here instead of "try".
This is basically the same as an existing lint |
So do you think that we should merge two lints into one? I think we can possibly add a lint that replace |
If you can come up with a good name I have no objection. I would have personally named these something like |
How about |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good first patch, some changes to be made =^w^=
Thanks for the contribution! ❤️
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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>
|
||
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) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
// 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) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
@@ -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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/// 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. |
#[clippy::version = "1.84.0"] | ||
pub TRUNCATE_WITH_DRAIN, | ||
nursery, | ||
"calling `drain` in order to `truncate` a `Vec`" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The use of truncate here is as a verb, not as a function name, unless I misunderstood something 👀
"calling `drain` in order to `truncate` a `Vec`" | |
"calling `drain` in order to truncate a `Vec`" |
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}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
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) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this block needed?
AFAIK there isn't any equivalence on ..n
= i64::MIN..n
, in fact, for signed integers the first one will give a syntax error.
}); | ||
let end_is_none_or_max = end.is_none_or(|end| match limits { | ||
RangeLimits::Closed => { | ||
if let rustc_ty::Adt(_, subst) = ty.kind() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The same question here.
☔ The latest upstream changes (presumably #13437) made this pull request unmergeable. Please resolve the merge conflicts. |
I add new lint that replace
vec.drain(x..)
withvec.truncate(x)
. See #13580changelog: new lint: [
truncate_with_drain
]