new lint: unnecessary_reserve
#14114
Conversation
rustbot
added
the
S-waiting-on-review
wowinter13
changed the title
unnecessary_reserve
|
@samueltardieu Samuel, really appreciate your review and points! |
| vec.reserve(array1.len()); | ||
| vec.extend(array2); | ||
|
|
||
| // do not lint |
There was a problem hiding this comment.
Why not? Or it can be done in a later PR.
There was a problem hiding this comment.
#9073 (comment)
@flip1995 is your opinion still the same?
I thought that the main focus of this lint is to catch cases where we're explicitly reserving space for the exact same slice/collection that we're about to extend with, like:
let array = &[1, 2, 3];
vec.reserve(array.len());
vec.extend(array);
There was a problem hiding this comment.
Oh wow, that was a long time.
I think that this pattern will be really rare in real world code. So linting it will probably not have a lot of benefits. And if someone should hard-code those numbers it might be for a reason.
I don't have a strong opinion about this. So if someone reviewing this disagrees, feel free to also lint on this.
There was a problem hiding this comment.
@flip1995 thanks a lot!
Could you also spend a bit of time reviewing this PR when you get a second?
Everything was addressed
There was a problem hiding this comment.
I'm pretty backed up on Clippy tasks right now, sorry 😞 so I'll let Samuel / Jarcho continue the review
| ) -> Option<rustc_span::Span> { | ||
| let mut found_reserve = false; | ||
| let mut read_found = false; | ||
| let mut spanless_eq = SpanlessEq::new(cx); |
There was a problem hiding this comment.
You need to deny side effects by calling .deny_side_effects(), or you will lint
(Vec::<u8>::new()).reserve(arr.len());
(Vec::<u8>::new()).extend(&arr);even though this is not the same receiver.
| if expr.hir_id == args_a.hir_id { | ||
| found_reserve = true; | ||
| } |
There was a problem hiding this comment.
| if expr.hir_id == args_a.hir_id { | |
| found_reserve = true; | |
| } | |
| found_reserve = expr.hir_id == args_a.hir_id; |
| && acceptable_type(cx, struct_calling_on) | ||
| && spanless_eq.eq_expr(struct_calling_on, struct_expr) | ||
| { | ||
| read_found = true; |
There was a problem hiding this comment.
You should stop iterating at this point by returning ControlFlow::Break(block.span) and return the result of for_each_expr(), you don't need read_found at all.
This is assuming that for_each_expr() is the right choice, I'm not sure it is. Maybe you could iterate over the statements of the block (and the expression if present) instead of over any expressions, that can be nested. Here the lint will trigger for:
let _: ((), (), u32)= (vec.reserve(arr.len()), vec.extend(&arr), 42);which is unlikely to be encountered, but still, the fix would be incorrect. The current version of the lint would even propose to remove the whole triplet, saying to remove the line.
You could even look for two successive statements (or expr for the last one), with the extend() immediately following the reserve().
| if let ExprKind::MethodCall(PathSegment { ident: method, .. }, struct_calling_on, args_a, _) = expr.kind | ||
| && method.name.as_str() == "reserve" |
There was a problem hiding this comment.
It is more idiomatic for Clippy code to have method be the PathSegment, and deconstruct the arguments since you know you want only one. Also, the receiver of a method is usually named receiver, or it could be reserve_receiver if you need to manipulate several ones:
| if let ExprKind::MethodCall(PathSegment { ident: method, .. }, struct_calling_on, args_a, _) = expr.kind | |
| && method.name.as_str() == "reserve" | |
| if let ExprKind::MethodCall(method, receiver, [arg], _) = expr.kind | |
| && method.ident.name.as_str() == "reserve" |
Note that you won't have to use args_a[0] later, arg will contain the argument of reserve.
| if let Some(did) = adt_def_id(cx.typeck_results().expr_ty_adjusted(struct_calling_on)) { | ||
| matches!(cx.tcx.get_diagnostic_name(did), Some(sym::Vec | sym::VecDeque)) | ||
| } else { | ||
| false | ||
| } |
There was a problem hiding this comment.
You can be a bit more compact but this is a matter of style:
| if let Some(did) = adt_def_id(cx.typeck_results().expr_ty_adjusted(struct_calling_on)) { | |
| matches!(cx.tcx.get_diagnostic_name(did), Some(sym::Vec | sym::VecDeque)) | |
| } else { | |
| false | |
| } | |
| adt_def_id(cx.typeck_results().expr_ty_adjusted(struct_calling_on)) | |
| .is_some_and(|did| matches!(cx.tcx.get_diagnostic_name(did), Some(sym::Vec | sym::VecDeque))) |
| ) = args_a.kind | ||
| && method_call_a.name == sym::len | ||
| && match_def_path(cx, expr_def_id, &paths::ITER_EXTEND) | ||
| && acceptable_type(cx, struct_calling_on) |
There was a problem hiding this comment.
Why do you need to check for the type if you will compare the expressions anyway? Not doing so would even let you inline the shorter form of acceptable_type() at the only place where it would now be called.
|
|
||
| // do not lint | ||
| vec.reserve(1); | ||
| vec.extend([1]); |
There was a problem hiding this comment.
Btw, error markers are now required in order for tests to pass. If you rebase over master, you will see examples in all the UI test files.
|
Also, is there any reason why this lint is not under the |
resurrection of #10157
fixes #8982
changelog: [
unnecessary_reserve]: add new lint unnecessary_reserve