Add new lint suspicious_slice_copies#16671
Conversation
rustbot
added
the
S-waiting-on-review
|
r? @Jarcho rustbot has assigned @Jarcho. Use Why was this reviewer chosen?The reviewer was selected based on:
|
| const FEATURE_SLICE_AS_MUT_ARRAY: RustcVersion = RustcVersion { | ||
| major: 1, | ||
| minor: 93, | ||
| patch: 0, | ||
| }; |
There was a problem hiding this comment.
This should be defined in clippy_utils::msrvs.
| } | ||
|
|
||
| pub(super) fn check(cx: &LateContext<'_>, expr: &Expr<'_>, msrv: Msrv) { | ||
| if let ExprKind::MethodCall(_, recv_expr, _, try_into_span) = expr.kind |
There was a problem hiding this comment.
This was already done before calling this. Just pass the arguments you need in.
| fn receiver(cx: &LateContext<'_>, expr: &Expr<'_>) -> Option<TryIntoReceiver> { | ||
| match cx.typeck_results().expr_ty(expr).kind() { | ||
| ty::Slice(_) => Some(TryIntoReceiver::Slice), | ||
| ty::Ref(_, ref_ty, Mutability::Mut) if let ty::Slice(_) = ref_ty.kind() => Some(TryIntoReceiver::SliceMutRef), | ||
| _ => None, | ||
| } | ||
| } |
There was a problem hiding this comment.
This would be better as a method of TryIntoReceiver. Should also take Ty rather than hiding the access to typeck behind a function.
| // try_into is called with slice or slice mut ref | ||
| && let Some(receiver) = receiver(cx, recv_expr) | ||
|
|
||
| // try_into returns Result<[T; N], TryFromSliceError> | ||
| && let ty::Adt(r_def, args) = cx.typeck_results().expr_ty(expr).kind() |
There was a problem hiding this comment.
Once you know you're calling TryInto::try_into You can get the source and destination types using node_args.
&& let [src_ty, dst_ty] = &**cx.typeck_result().node_args(call_expr.hir_id)This will also mean you don't have to deal with Result at all.
| && let Some(parent_call) = get_parent_expr(cx, expr) | ||
| && let ExprKind::MethodCall(path, .., unwrap) = parent_call.kind | ||
| && let sym::unwrap | sym::expect = path.ident.name | ||
|
|
||
| // use mut ref | ||
| && let Some(mut_ref_expr) = get_parent_expr(cx, parent_call) | ||
| && let ExprKind::AddrOf(BorrowKind::Ref, Mutability::Mut, inner_expr) = mut_ref_expr.kind |
There was a problem hiding this comment.
When walking multiple parents you should use TyCtxt::hir_parent_iter.
| ( | ||
| mut_ref_expr.span.with_hi(try_into_span.hi()), | ||
| format!("{recv_snippet}.as_mut_array()"), | ||
| ) | ||
| } else { | ||
| ( | ||
| mut_ref_expr.span.with_hi(recv_expr.span.hi()), | ||
| match receiver { | ||
| TryIntoReceiver::Slice => format!( | ||
| "({mut_ref_snippet} {recv_snippet})", | ||
| mut_ref_snippet = snippet( | ||
| cx, | ||
| mut_ref_expr | ||
| .span | ||
| .until(inner_expr.span.with_leading_whitespace(cx).into_span()), | ||
| ".." | ||
| ) | ||
| ), | ||
| TryIntoReceiver::SliceMutRef => recv_snippet.to_string(), | ||
| }, | ||
| ) |
There was a problem hiding this comment.
These don't work if the expression has extra parenthesis. e.g. &mut (x.try_into().unwrap()). Trying to minimize the suggestion size is quite a bit of work and I don't recommend doing it with the current snippet and span APIs. Just replace the whole expression.
| ) | ||
| }; | ||
|
|
||
| diag.span_suggestion(span, "try", sugg, Applicability::MachineApplicable); |
There was a problem hiding this comment.
This is not a MachineApplicable suggestion since it's a semantic change. The help message should also be something like "to borrow the slice as an array" so it's clear what's being recommended.
A second suggestion for something like <[_; _]>::try_from() when creating a new array is actually the intention would also be nice. It's nicer than having to add an expect to silence the lint.
| mut_ref_expr.span.with_hi(unwrap.hi()), | ||
| "using a mutable reference to temporary array", | ||
| |diag| { | ||
| let recv_snippet = snippet(cx, recv_expr.span, ".."); |
There was a problem hiding this comment.
This should use snippet_with_context.
rustbot
added
S-waiting-on-author
|
Reminder, once the PR becomes ready for a review, use |
|
@rustbot label lint-nominated |
|
This lint has been nominated for inclusion. |
|
@rustbot ready |
rustbot
added
S-waiting-on-review
3 participants
implements lint for checking usage of a
&mutto a temporary array copied from a slicechangelog: [
suspicious_slice_copies].stderrfile)cargo testpasses locallycargo dev update_lintscargo dev fmtFixes #16507. The lint checks only calls
try_intoby[T]or&mut [T]. If any other cases should be covered by this lint, please share your feedback.