Clear collection reassignment#16549
Conversation
|
r? @dswij rustbot has assigned @dswij. Use Why was this reviewer chosen?The reviewer was selected based on:
|
This comment has been minimized.
This comment has been minimized.
|
Lintcheck changes for 47b2444
This comment will be updated if you push new changes |
There was a problem hiding this comment.
This is a good first step, but this requires changes:
- Since this lint only triggers on call to
something::new(), it should probably be part of theclippy_lints::methodsmodule. - You should add tests showing that the lint will not trigger if the code belongs to a macro (this will probably be taken care of automatically once integrated into the
methodsmodule). - You should also add more complex tests such as
*v = Vec::new();, as it should be replaced by(*v).clear();or, even better,v.clear();instead.
We usually avoid comparing strings in Clippy. Method names are compared using symbols (sym::new()), and types names usually use diagnostic items.
r? samueltardieu
@rustbot author
| fn is_collection_new_call(cx: &LateContext<'_>, func: &Expr<'_>) -> bool { | ||
| if let ExprKind::Path(QPath::TypeRelative(ty, method)) = func.kind | ||
| && method.ident.name.as_str() == "new" | ||
| { | ||
| let ty = cx.typeck_results().node_type(ty.hir_id); | ||
| let type_name = ty.to_string(); | ||
|
|
||
| // Check if it's one of the standard collections | ||
| // Type names come in the format "std::vec::Vec<T>" or "std::collections::HashMap<K, V>" | ||
| type_name.contains("::Vec<") | ||
| || type_name.contains("::HashMap<") | ||
| || type_name.contains("::HashSet<") | ||
| || type_name.contains("::VecDeque<") | ||
| || type_name.contains("::BTreeMap<") | ||
| || type_name.contains("::BTreeSet<") | ||
| || type_name.contains("::BinaryHeap<") | ||
| || type_name.contains("::LinkedList<") | ||
| } else { | ||
| false | ||
| } | ||
| } |
There was a problem hiding this comment.
This is not the proper way of checking for the type, we don't use string matching in Clippy as this is not robust. You should look at the diagnostic items page in the compiler dev guide, and use functions from clippy_utils to check for them.
| /// vec.push(1); | ||
| /// vec.clear(); | ||
| /// ``` | ||
| #[clippy::version = "1.XX.0"] |
There was a problem hiding this comment.
| #[clippy::version = "1.XX.0"] | |
| #[clippy::version = "1.95.0"] |
| /// ```no_run | ||
| /// let mut vec = Vec::new(); | ||
| /// vec.push(1); | ||
| /// vec = Vec::new(); |
There was a problem hiding this comment.
You should insert a f(&vec) in the middle, otherwise it makes little sense to push an element and erase the collection afterwards without using it.
|
Reminder, once the PR becomes ready for a review, use |
|
Note that you can run the tests including the internal ones by using |
|
@samueltardieu Thanks for the review. I will work on these changes:
I will need a bit of time to study the methods module structure |
- Use diagnostic items instead of string matching for type checking - Move lint to methods module as requested - Add macro handling via in_external_macro check - Support dereference patterns (*v = Vec::new() → v.clear()) - Change applicability to MaybeIncorrect - Add comprehensive tests for all collection types - Fix version to 1.95.0 Addresses all review comments from samueltardieu on PR rust-lang#16549
This comment has been minimized.
This comment has been minimized.
|
|
||
| # Remove jujutsu directory from search tools | ||
| .jj | ||
| .DS_Store |
There was a problem hiding this comment.
This doesn't belong to this PR.
There was a problem hiding this comment.
This file should not have been added
| @@ -0,0 +1,152 @@ | |||
| #![warn(clippy::clear_instead_of_new)] | |||
| #![allow(unused)] | |||
There was a problem hiding this comment.
This should not be needed:
| #![allow(unused)] |
| #![allow(clippy::clear_instead_of_new)] | ||
| #![allow(unused, clippy::useless_vec)] |
There was a problem hiding this comment.
| #![allow(clippy::clear_instead_of_new)] | |
| #![allow(unused, clippy::useless_vec)] | |
| #![allow(clippy::clear_instead_of_new, clippy::useless_vec)] |
| @@ -1,3 +1,4 @@ | |||
| #![allow(clippy::clear_instead_of_new)] | |||
There was a problem hiding this comment.
Move it after the #![warn(…)]
| #![allow(clippy::clear_instead_of_new)] | ||
| #![allow(clippy::useless_vec, clippy::manual_repeat_n)] |
| } | ||
| } | ||
|
|
||
| #[allow(clippy::clear_instead_of_new)] |
There was a problem hiding this comment.
Please use #[expect] instead.
| let is_new_call = if let ExprKind::Call(func, args) = value.kind | ||
| && args.is_empty() | ||
| && let ExprKind::Path(QPath::TypeRelative(ty_path, method)) = func.kind | ||
| && method.ident.name == sym::new |
There was a problem hiding this comment.
This will have been checked in the methods module check_expr() function already.
| // Get the type definition, peeling any references | ||
| let ty = ty.peel_refs(); | ||
|
|
||
| // Check if it's one of the standard library collections | ||
| // We check for types that have a .clear() method | ||
| if let ty::Adt(adt, _) = ty.kind() { | ||
| let def_id = adt.did(); | ||
|
|
||
| // Use diagnostic items to identify the type | ||
| // This is the proper way to check types in Clippy, not string matching | ||
| cx.tcx.is_diagnostic_item(sym::Vec, def_id) | ||
| || cx.tcx.is_diagnostic_item(sym::HashMap, def_id) | ||
| || cx.tcx.is_diagnostic_item(sym::HashSet, def_id) | ||
| || cx.tcx.is_diagnostic_item(sym::VecDeque, def_id) | ||
| || cx.tcx.is_diagnostic_item(sym::BTreeMap, def_id) | ||
| || cx.tcx.is_diagnostic_item(sym::BTreeSet, def_id) | ||
| || cx.tcx.is_diagnostic_item(sym::BinaryHeap, def_id) | ||
| || cx.tcx.is_diagnostic_item(sym::LinkedList, def_id) | ||
| } else { | ||
| false | ||
| } |
There was a problem hiding this comment.
You can do this more efficiently with something like:
| // Get the type definition, peeling any references | |
| let ty = ty.peel_refs(); | |
| // Check if it's one of the standard library collections | |
| // We check for types that have a .clear() method | |
| if let ty::Adt(adt, _) = ty.kind() { | |
| let def_id = adt.did(); | |
| // Use diagnostic items to identify the type | |
| // This is the proper way to check types in Clippy, not string matching | |
| cx.tcx.is_diagnostic_item(sym::Vec, def_id) | |
| || cx.tcx.is_diagnostic_item(sym::HashMap, def_id) | |
| || cx.tcx.is_diagnostic_item(sym::HashSet, def_id) | |
| || cx.tcx.is_diagnostic_item(sym::VecDeque, def_id) | |
| || cx.tcx.is_diagnostic_item(sym::BTreeMap, def_id) | |
| || cx.tcx.is_diagnostic_item(sym::BTreeSet, def_id) | |
| || cx.tcx.is_diagnostic_item(sym::BinaryHeap, def_id) | |
| || cx.tcx.is_diagnostic_item(sym::LinkedList, def_id) | |
| } else { | |
| false | |
| } | |
| matches!(ty.peel_refs().opt_diag_name(), Some(sym::HashMap | sym::HashSet | …)) |
There was a problem hiding this comment.
Changes to this file don't seem to belong to this PR.
| @@ -1,3 +1,4 @@ | |||
| #![allow(clippy::clear_instead_of_new)] | |||
- Use diagnostic items instead of string matching for type checking - Move lint to methods module as requested - Add macro handling via in_external_macro check - Support dereference patterns (*v = Vec::new() → v.clear()) - Use get_diagnostic_name for efficient compiler query - Change applicability to MaybeIncorrect - Add comprehensive tests for all collection types Addresses all review comments from samueltardieu on PR rust-lang#16549
This comment has been minimized.
This comment has been minimized.
|
All the requested changes have been addressed:
@rustbot ready |
|
@samueltardieu Thank you for the review. I'll make the changes accordingly |
In which case can the applicability be incorrect? |
The applicability can be incorrect in uninitialized variables, different capacity semantics For eg:- Lint suggests: v.clear() Vec::new() drops old vec, creates new one with capacity 0 |
It would be best for the lint not to trigger at all in this case (see
This is a real one. Maybe an extra note could be added to the diagnostic saying that using |
1227981 to
3ba4e8f
Compare
|
@rustbot ready |
| fn peel_derefs<'tcx>(expr: &'tcx Expr<'tcx>) -> &'tcx Expr<'tcx> { | ||
| let mut e = expr; | ||
| while let ExprKind::Unary(UnOp::Deref, inner) = e.kind { | ||
| e = inner; | ||
| } | ||
| e | ||
| } |
There was a problem hiding this comment.
You don't need to use an extra variable:
| fn peel_derefs<'tcx>(expr: &'tcx Expr<'tcx>) -> &'tcx Expr<'tcx> { | |
| let mut e = expr; | |
| while let ExprKind::Unary(UnOp::Deref, inner) = e.kind { | |
| e = inner; | |
| } | |
| e | |
| } | |
| fn peel_derefs<'tcx>(mut expr: &'tcx Expr<'tcx>) -> &'tcx Expr<'tcx> { | |
| while let ExprKind::Unary(UnOp::Deref, inner) = expr.kind { | |
| expr = inner; | |
| } | |
| expr | |
| } |
| false | ||
| } | ||
|
|
||
| fn local_snippet<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'tcx>) -> Option<String> { |
There was a problem hiding this comment.
This function doesn't do what its name implies, as it also checks that the local is initialized. At least, it should include a /// docstring.
| // turbofish on the type (e.g. `Vec::<i32>::new()`) changes type inference, | ||
| // so replacing with `.clear()` could be wrong |
There was a problem hiding this comment.
This comment doesn't explain what the function does. It should be placed at the calling place.
| } | ||
| } | ||
|
|
||
| fn emit_lint<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'tcx>, sugg: &str) { |
There was a problem hiding this comment.
| fn emit_lint<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'tcx>, sugg: &str) { | |
| fn emit_lint(cx: &LateContext<'_>, expr: &Expr<'_>, sugg: &str) { |
| false | ||
| } | ||
|
|
||
| fn local_snippet<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'tcx>) -> Option<String> { |
There was a problem hiding this comment.
| fn local_snippet<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'tcx>) -> Option<String> { | |
| fn local_snippet(cx: &LateContext<'_>, expr: &Expr<'_>) -> Option<String> { |
| } | ||
| } | ||
|
|
||
| fn check_vec_macro<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'tcx>, lhs: &'tcx Expr<'tcx>, rhs: &'tcx Expr<'tcx>) { |
There was a problem hiding this comment.
| fn check_vec_macro<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'tcx>, lhs: &'tcx Expr<'tcx>, rhs: &'tcx Expr<'tcx>) { | |
| fn check_vec_macro(cx: &LateContext<'_>, expr: &Expr<'_>, lhs: &Expr<'_>, rhs: &Expr<'_>) { |
| fn check_collection_new<'tcx>( | ||
| cx: &LateContext<'tcx>, | ||
| expr: &'tcx Expr<'tcx>, | ||
| lhs: &'tcx Expr<'tcx>, | ||
| rhs: &'tcx Expr<'tcx>, | ||
| ) { |
There was a problem hiding this comment.
| fn check_collection_new<'tcx>( | |
| cx: &LateContext<'tcx>, | |
| expr: &'tcx Expr<'tcx>, | |
| lhs: &'tcx Expr<'tcx>, | |
| rhs: &'tcx Expr<'tcx>, | |
| ) { | |
| fn check_collection_new(cx: &LateContext<'_>, expr: &Expr<'_>, lhs: &Expr<'_>, rhs: &Expr<'_>) { |
|
|
||
| use super::NEW_INSTEAD_OF_CLEAR; | ||
|
|
||
| pub(super) fn check<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'tcx>) { |
There was a problem hiding this comment.
| pub(super) fn check<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'tcx>) { | |
| pub(super) fn check(cx: &LateContext<'_>, expr: &Expr<'_>) { |
| #[clippy::version = "1.96.0"] | ||
| pub NEW_INSTEAD_OF_CLEAR, | ||
| perf, | ||
| "assigning `Collection::new()` or `vec![]` instead of calling `.clear()`" |
There was a problem hiding this comment.
| "assigning `Collection::new()` or `vec![]` instead of calling `.clear()`" | |
| "creating a new collection instead of calling `.clear()`" |
3ba4e8f to
c6da352
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
47f42c0 to
a0264f3
Compare
6f08e27 to
4e4f9db
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
56feac5 to
ffbd0e2
Compare
|
This PR was rebased onto a different master commit. Here's a range-diff highlighting what actually changed. Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers. |
ffbd0e2 to
78bfc5c
Compare
|
@samueltardieu Thanks for taking the time to go through this so thoroughly 🙏 I ended up reworking a good chunk of it. The lint's now called new_instead_of_clear and lives in operators/ — you were right, it's matching on an assignment of a type-relative ::new(), not an actual method call, so that's where it belongs. I also cleaned out all the stuff that shouldn't have been in here in the first place — the .gitignore change, that stray .swp file I accidentally committed, and the write/mod.rs / only_used_in_recursion.rs edits. On the lint itself: it now checks the type via opt_diag_name instead of comparing strings, I pulled the duplicated emit code into one place, simplified the lifetimes to '_, added a doc comment to local_snippet, and moved the turbofish explanation up to where it's actually used. Tests use the plain //~^ new_instead_of_clear markers now, and the doc example has a f(&v) in the middle like you suggested. Two small places I went a different way than the inline comments, just so it's not a surprise: I set the version to 1.98.0 instead of 1.96.0 — master moved on to the 1.98 cycle in the meantime. |
| { | ||
| let rhs_ty = cx.typeck_results().node_type(ty.hir_id); | ||
| if matches!( | ||
| rhs_ty.peel_refs().opt_diag_name(cx), |
There was a problem hiding this comment.
What ref could need peeling here?
| rhs_ty.peel_refs().opt_diag_name(cx), | |
| rhs_ty.opt_diag_name(cx), |
| fn local_snippet(cx: &LateContext<'_>, expr: &Expr<'_>) -> Option<String> { | ||
| let inner = peel_derefs(expr); | ||
| if let ExprKind::Path(QPath::Resolved(None, path)) = inner.kind | ||
| && let Res::Local(hir_id) = path.res | ||
| && local_is_initialized(cx, hir_id) | ||
| { | ||
| Some(snippet(cx, inner.span, "..").into_owned()) | ||
| } else { | ||
| None | ||
| } | ||
| } |
There was a problem hiding this comment.
You should manipulate a Symbol instead of building an owned string.
| fn local_snippet(cx: &LateContext<'_>, expr: &Expr<'_>) -> Option<String> { | |
| let inner = peel_derefs(expr); | |
| if let ExprKind::Path(QPath::Resolved(None, path)) = inner.kind | |
| && let Res::Local(hir_id) = path.res | |
| && local_is_initialized(cx, hir_id) | |
| { | |
| Some(snippet(cx, inner.span, "..").into_owned()) | |
| } else { | |
| None | |
| } | |
| } | |
| fn local_snippet(cx: &LateContext<'_>, expr: &Expr<'_>) -> Option<Symbol> { | |
| peel_ref_operators(cx, expr) | |
| .res_local_id_and_ident() | |
| .filter(|(hir_id, _)| local_is_initialized(cx, *hir_id)) | |
| .map(|(_, ident)| ident.name) | |
| } |
Note that peel_ref_operators() is more conservative than your peel_derefs(), and that's a good thing: even if we will not lint the Box<Vec<_>> case, we will not risk having another smart pointer misbehave. What if the smartpointer S in S<Vec<_>> has its own clear() method which does something completely different? That one would be called instead.
| format!("{sugg}.clear()"), | ||
| Applicability::MaybeIncorrect, | ||
| ); | ||
| diag.note("`.clear()` retains the allocated memory for reuse"); |
There was a problem hiding this comment.
This is not true for BTreeMap, BTreeSet, or LinkedList. The note should not be issued for those types, as they don't preallocate memory.
Maybe you could distinguish types that will clear from others, and detect String as well, by doing something like (note that emit_lint() has not two extra parameters, will_clear and may_have_side_effect for the vec![init; 0] case):
fn check_collection_new(cx: &LateContext<'_>, expr: &Expr<'_>, lhs: &Expr<'_>, rhs: &Expr<'_>) {
// skip macro expansions (e.g. vec![] is handled separately)
if !rhs.span.from_expansion()
&& let ExprKind::Call(func, []) = rhs.kind
&& let ExprKind::Path(QPath::TypeRelative(ty, method)) = func.kind
&& method.ident.name == sym::new
// turbofish on the type (e.g. `Vec::<i32>::new()`) changes type inference,
// so replacing with `.clear()` could be wrong
&& !has_type_args(ty)
&& let rhs_ty = cx.typeck_results().node_type(ty.hir_id)
&& let will_clear = match rhs_ty.opt_diag_name(cx) {
Some(sym::Vec | sym::HashMap | sym::HashSet | sym::VecDeque | sym::BinaryHeap) => true,
Some(sym::BTreeMap | sym::BTreeSet | sym::LinkedList) => false,
None if rhs_ty.opt_def_id().is_some_and(|def_id| cx.tcx.is_lang_item(def_id, LangItem::String)) => true,
_ => return,
}
&& let Some(sugg) = local_snippet(cx, lhs)
{
emit_lint(cx, expr, sugg, will_clear, None);
}
}| | sym::BTreeSet | ||
| | sym::BinaryHeap | ||
| | sym::LinkedList | ||
| ) |
There was a problem hiding this comment.
Shouldn't String be also considered? (it is a lang item, not a diagnostic item)
| if matching_root_macro_call(cx, rhs.span, sym::vec_macro).is_some() | ||
| && matches!(VecArgs::hir(cx, rhs), Some(VecArgs::Vec([]))) | ||
| && let Some(sugg) = local_snippet(cx, lhs) | ||
| { | ||
| emit_lint(cx, expr, &sugg); | ||
| } |
There was a problem hiding this comment.
Doesn't VecArgs::hir() also check that the vec![] macro is used?
You could also check for vec![init; 0], and pass the initialization function to emit_lint():
| if matching_root_macro_call(cx, rhs.span, sym::vec_macro).is_some() | |
| && matches!(VecArgs::hir(cx, rhs), Some(VecArgs::Vec([]))) | |
| && let Some(sugg) = local_snippet(cx, lhs) | |
| { | |
| emit_lint(cx, expr, &sugg); | |
| } | |
| if let may_have_side_effect = match VecArgs::hir(cx, rhs) { | |
| Some(VecArgs::Vec([])) => None, | |
| Some(VecArgs::Repeat(init, count)) if is_integer_literal(count, 0) => Some(init), | |
| _ => return, | |
| } | |
| && let Some(sugg) = local_snippet(cx, lhs) | |
| { | |
| emit_lint(cx, expr, sugg, may_have_side_effect); | |
| } |
This way, emit_lint() could add a note if there is a risk of behavior change because side effects would no longer be executed, with something like:
if let Some(init) = may_have_side_effect
&& !eq_expr_value(cx, expr.span.ctxt(), init, init)
{
diag.span_note(
init.span,
"side effects of this expression will no longer be executed",
);
}| { | ||
| let rhs_ty = cx.typeck_results().node_type(ty.hir_id); |
There was a problem hiding this comment.
No need to interrupt the if let chain here:
| { | |
| let rhs_ty = cx.typeck_results().node_type(ty.hir_id); | |
| && let rhs_ty = cx.typeck_results().node_type(ty.hir_id) |
Add a `perf` lint that detects reassigning an already-initialized collection with `Collection::new()` or `vec![]` instead of calling `.clear()`, which needlessly discards the existing allocation. Covers `Vec`, `VecDeque`, `HashMap`, `HashSet`, `BinaryHeap`, `String`, `BTreeMap`, `BTreeSet` and `LinkedList`. The memory-reuse note is only shown for types that retain their allocation on `.clear()`. The lint fires only when the variable is already initialized, peels reference operators but not smart-pointer derefs, and skips macro expansions and turbofish types where switching to `.clear()` could change behavior.
The new lint fires on collection reassignments present in unrelated test files that run with -D warnings. Add #![allow(clippy::new_instead_of_clear)] to keep each test focused on its own lint, and re-bless the affected snapshots.
78bfc5c to
47b2444
Compare
View all comments
Add clear_instead_of_new lint
What does this PR do?
Adds a new perf lint that detects when a collection is reassigned to a new empty collection instead of calling .clear().
Why?
Reassigning a collection with collection = CollectionType::new() is inefficient because it:-
Advantage
Using .clear() is more efficient as it:-
What collections are supported?
Example
Before:
After:
Fixes #16520
changelog: [
clear_instead_of_new]: new lint to suggest using .clear() instead of reassigning empty collections