From 0d83a3a18b0efb047fdf04d3ed906154b08e58d6 Mon Sep 17 00:00:00 2001 From: J-ZhengLi Date: Wed, 10 Jan 2024 15:31:20 +0800 Subject: [PATCH 1/2] re-orgnize [`useless_vec`]'s code --- clippy_lints/src/vec.rs | 168 ++++++++++++++++++++-------------------- 1 file changed, 86 insertions(+), 82 deletions(-) diff --git a/clippy_lints/src/vec.rs b/clippy_lints/src/vec.rs index b3489142558..a918bd1c824 100644 --- a/clippy_lints/src/vec.rs +++ b/clippy_lints/src/vec.rs @@ -9,7 +9,7 @@ use clippy_utils::ty::is_copy; use clippy_utils::visitors::for_each_local_use_after_expr; use clippy_utils::{get_parent_expr, higher, is_trait_method}; use rustc_errors::Applicability; -use rustc_hir::{BorrowKind, Expr, ExprKind, HirId, Mutability, Node, PatKind}; +use rustc_hir::{BorrowKind, Expr, ExprKind, HirId, Local, Mutability, Node, Pat, PatKind}; use rustc_lint::{LateContext, LateLintPass}; use rustc_middle::ty; use rustc_middle::ty::layout::LayoutOf; @@ -52,35 +52,25 @@ declare_clippy_lint! { impl_lint_pass!(UselessVec => [USELESS_VEC]); -fn adjusts_to_slice(cx: &LateContext<'_>, e: &Expr<'_>) -> bool { - matches!(cx.typeck_results().expr_ty_adjusted(e).kind(), ty::Ref(_, ty, _) if ty.is_slice()) -} - -/// Checks if the given expression is a method call to a `Vec` method -/// that also exists on slices. If this returns true, it means that -/// this expression does not actually require a `Vec` and could just work with an array. -pub fn is_allowed_vec_method(cx: &LateContext<'_>, e: &Expr<'_>) -> bool { - const ALLOWED_METHOD_NAMES: &[&str] = &["len", "as_ptr", "is_empty"]; - - if let ExprKind::MethodCall(path, ..) = e.kind { - ALLOWED_METHOD_NAMES.contains(&path.ident.name.as_str()) - } else { - is_trait_method(cx, e, sym::IntoIterator) - } -} - impl<'tcx> LateLintPass<'tcx> for UselessVec { fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) { - if let Some(vec_args) = higher::VecArgs::hir(cx, expr.peel_borrows()) { + let Some(vec_args) = higher::VecArgs::hir(cx, expr.peel_borrows()) else { + return; + }; + + match cx.tcx.parent_hir_node(expr.hir_id) { // search for `let foo = vec![_]` expressions where all uses of `foo` // adjust to slices or call a method that exist on slices (e.g. len) - if let Node::Local(local) = cx.tcx.parent_hir_node(expr.hir_id) - // for now ignore locals with type annotations. - // this is to avoid compile errors when doing the suggestion here: let _: Vec<_> = vec![..]; - && local.ty.is_none() - && let PatKind::Binding(_, id, ..) = local.pat.kind - { - let only_slice_uses = for_each_local_use_after_expr(cx, id, expr.hir_id, |expr| { + Node::Local(Local { + ty: None, + pat: + Pat { + kind: PatKind::Binding(_, id, ..), + .. + }, + .. + }) => { + let only_slice_uses = for_each_local_use_after_expr(cx, *id, expr.hir_id, |expr| { // allow indexing into a vec and some set of allowed method calls that exist on slices, too if let Some(parent) = get_parent_expr(cx, expr) && (adjusts_to_slice(cx, expr) @@ -100,26 +90,22 @@ impl<'tcx> LateLintPass<'tcx> for UselessVec { } else { self.span_to_lint_map.insert(span, None); } - } + }, // if the local pattern has a specified type, do not lint. - else if let Some(_) = higher::VecArgs::hir(cx, expr) - && let Node::Local(local) = cx.tcx.parent_hir_node(expr.hir_id) - && local.ty.is_some() - { + Node::Local(Local { ty: Some(_), .. }) if higher::VecArgs::hir(cx, expr).is_some() => { let span = expr.span.ctxt().outer_expn_data().call_site; self.span_to_lint_map.insert(span, None); - } + }, // search for `for _ in vec![...]` - else if let Some(parent) = get_parent_expr(cx, expr) - && parent.span.is_desugaring(DesugaringKind::ForLoop) - && self.msrv.meets(msrvs::ARRAY_INTO_ITERATOR) + Node::Expr(Expr { span, .. }) + if span.is_desugaring(DesugaringKind::ForLoop) && self.msrv.meets(msrvs::ARRAY_INTO_ITERATOR) => { // report the error around the `vec!` not inside `:` let span = expr.span.ctxt().outer_expn_data().call_site; self.check_vec_macro(cx, &vec_args, span, expr.hir_id, SuggestedType::Array); - } + }, // search for `&vec![_]` or `vec![_]` expressions where the adjusted type is `&[_]` - else { + _ => { let (suggest_slice, span) = if let ExprKind::AddrOf(BorrowKind::Ref, mutability, _) = expr.kind { // `expr` is `&vec![_]`, so suggest `&[_]` (or `&mut[_]` resp.) (SuggestedType::SliceRef(mutability), expr.span) @@ -134,20 +120,14 @@ impl<'tcx> LateLintPass<'tcx> for UselessVec { } else { self.span_to_lint_map.insert(span, None); } - } + }, } } fn check_crate_post(&mut self, cx: &LateContext<'tcx>) { for (span, lint_opt) in &self.span_to_lint_map { if let Some((hir_id, suggest_slice, snippet, applicability)) = lint_opt { - let help_msg = format!( - "you can use {} directly", - match suggest_slice { - SuggestedType::SliceRef(_) => "a slice", - SuggestedType::Array => "an array", - } - ); + let help_msg = format!("you can use {} directly", suggest_slice.desc(),); span_lint_hir_and_then(cx, USELESS_VEC, *hir_id, *span, "useless use of `vec!`", |diag| { diag.span_suggestion(*span, help_msg, snippet, *applicability); }); @@ -158,14 +138,6 @@ impl<'tcx> LateLintPass<'tcx> for UselessVec { extract_msrv_attr!(LateContext); } -#[derive(Copy, Clone)] -pub(crate) enum SuggestedType { - /// Suggest using a slice `&[..]` / `&mut [..]` - SliceRef(Mutability), - /// Suggest using an array: `[..]` - Array, -} - impl UselessVec { fn check_vec_macro<'tcx>( &mut self, @@ -194,44 +166,21 @@ impl UselessVec { return; } - let elem = snippet_with_applicability(cx, elem.span, "elem", &mut applicability); - let len = snippet_with_applicability(cx, len.span, "len", &mut applicability); - - match suggest_slice { - SuggestedType::SliceRef(Mutability::Mut) => format!("&mut [{elem}; {len}]"), - SuggestedType::SliceRef(Mutability::Not) => format!("&[{elem}; {len}]"), - SuggestedType::Array => format!("[{elem}; {len}]"), - } + suggest_slice.snippet(cx, Some(elem.span), Some(len.span), &mut applicability) } else { return; } }, higher::VecArgs::Vec(args) => { - if let Some(last) = args.iter().last() { + let args_span = if let Some(last) = args.iter().last() { if args.len() as u64 * size_of(cx, last) > self.too_large_for_stack { return; } - let span = args[0].span.source_callsite().to(last.span.source_callsite()); - let args = snippet_with_applicability(cx, span, "..", &mut applicability); - - match suggest_slice { - SuggestedType::SliceRef(Mutability::Mut) => { - format!("&mut [{args}]") - }, - SuggestedType::SliceRef(Mutability::Not) => { - format!("&[{args}]") - }, - SuggestedType::Array => { - format!("[{args}]") - }, - } + Some(args[0].span.source_callsite().to(last.span.source_callsite())) } else { - match suggest_slice { - SuggestedType::SliceRef(Mutability::Mut) => "&mut []".to_owned(), - SuggestedType::SliceRef(Mutability::Not) => "&[]".to_owned(), - SuggestedType::Array => "[]".to_owned(), - } - } + None + }; + suggest_slice.snippet(cx, args_span, None, &mut applicability) }, }; @@ -241,7 +190,62 @@ impl UselessVec { } } +#[derive(Copy, Clone)] +pub(crate) enum SuggestedType { + /// Suggest using a slice `&[..]` / `&mut [..]` + SliceRef(Mutability), + /// Suggest using an array: `[..]` + Array, +} + +impl SuggestedType { + fn desc(self) -> &'static str { + match self { + Self::SliceRef(_) => "a slice", + Self::Array => "an array", + } + } + + fn snippet( + self, + cx: &LateContext<'_>, + args_span: Option, + len_span: Option, + app: &mut Applicability, + ) -> String { + let args = args_span + .map(|sp| snippet_with_applicability(cx, sp, "..", app)) + .unwrap_or_default(); + let maybe_len = len_span + .map(|sp| format!("; {}", snippet_with_applicability(cx, sp, "len", app))) + .unwrap_or_default(); + + match self { + Self::SliceRef(Mutability::Mut) => format!("&mut [{args}{maybe_len}]"), + Self::SliceRef(Mutability::Not) => format!("&[{args}{maybe_len}]"), + Self::Array => format!("[{args}{maybe_len}]"), + } + } +} + fn size_of(cx: &LateContext<'_>, expr: &Expr<'_>) -> u64 { let ty = cx.typeck_results().expr_ty_adjusted(expr); cx.layout_of(ty).map_or(0, |l| l.size.bytes()) } + +fn adjusts_to_slice(cx: &LateContext<'_>, e: &Expr<'_>) -> bool { + matches!(cx.typeck_results().expr_ty_adjusted(e).kind(), ty::Ref(_, ty, _) if ty.is_slice()) +} + +/// Checks if the given expression is a method call to a `Vec` method +/// that also exists on slices. If this returns true, it means that +/// this expression does not actually require a `Vec` and could just work with an array. +pub fn is_allowed_vec_method(cx: &LateContext<'_>, e: &Expr<'_>) -> bool { + const ALLOWED_METHOD_NAMES: &[&str] = &["len", "as_ptr", "is_empty"]; + + if let ExprKind::MethodCall(path, ..) = e.kind { + ALLOWED_METHOD_NAMES.contains(&path.ident.name.as_str()) + } else { + is_trait_method(cx, e, sym::IntoIterator) + } +} From 929f746b5c19f7bf8e52cc32a3bb868fef0461e6 Mon Sep 17 00:00:00 2001 From: J-ZhengLi Date: Sat, 24 Feb 2024 00:35:48 +0800 Subject: [PATCH 2/2] fix the actual bug --- clippy_lints/src/vec.rs | 75 +++++++++++++++++++---------------------- tests/ui/vec.fixed | 4 +++ tests/ui/vec.rs | 4 +++ tests/ui/vec.stderr | 8 ++++- 4 files changed, 50 insertions(+), 41 deletions(-) diff --git a/clippy_lints/src/vec.rs b/clippy_lints/src/vec.rs index a918bd1c824..7cfcf3fe946 100644 --- a/clippy_lints/src/vec.rs +++ b/clippy_lints/src/vec.rs @@ -4,7 +4,7 @@ use std::ops::ControlFlow; use clippy_config::msrvs::{self, Msrv}; use clippy_utils::consts::{constant, Constant}; use clippy_utils::diagnostics::span_lint_hir_and_then; -use clippy_utils::source::snippet_with_applicability; +use clippy_utils::source::snippet_opt; use clippy_utils::ty::is_copy; use clippy_utils::visitors::for_each_local_use_after_expr; use clippy_utils::{get_parent_expr, higher, is_trait_method}; @@ -57,6 +57,8 @@ impl<'tcx> LateLintPass<'tcx> for UselessVec { let Some(vec_args) = higher::VecArgs::hir(cx, expr.peel_borrows()) else { return; }; + // the parent callsite of this `vec!` expression, or span to the borrowed one such as `&vec!` + let callsite = expr.span.parent_callsite().unwrap_or(expr.span); match cx.tcx.parent_hir_node(expr.hir_id) { // search for `let foo = vec![_]` expressions where all uses of `foo` @@ -84,41 +86,31 @@ impl<'tcx> LateLintPass<'tcx> for UselessVec { }) .is_continue(); - let span = expr.span.ctxt().outer_expn_data().call_site; if only_slice_uses { - self.check_vec_macro(cx, &vec_args, span, expr.hir_id, SuggestedType::Array); + self.check_vec_macro(cx, &vec_args, callsite, expr.hir_id, SuggestedType::Array); } else { - self.span_to_lint_map.insert(span, None); + self.span_to_lint_map.insert(callsite, None); } }, // if the local pattern has a specified type, do not lint. Node::Local(Local { ty: Some(_), .. }) if higher::VecArgs::hir(cx, expr).is_some() => { - let span = expr.span.ctxt().outer_expn_data().call_site; - self.span_to_lint_map.insert(span, None); + self.span_to_lint_map.insert(callsite, None); }, // search for `for _ in vec![...]` Node::Expr(Expr { span, .. }) if span.is_desugaring(DesugaringKind::ForLoop) && self.msrv.meets(msrvs::ARRAY_INTO_ITERATOR) => { - // report the error around the `vec!` not inside `:` - let span = expr.span.ctxt().outer_expn_data().call_site; - self.check_vec_macro(cx, &vec_args, span, expr.hir_id, SuggestedType::Array); + let suggest_slice = suggest_type(expr); + self.check_vec_macro(cx, &vec_args, callsite, expr.hir_id, suggest_slice); }, // search for `&vec![_]` or `vec![_]` expressions where the adjusted type is `&[_]` _ => { - let (suggest_slice, span) = if let ExprKind::AddrOf(BorrowKind::Ref, mutability, _) = expr.kind { - // `expr` is `&vec![_]`, so suggest `&[_]` (or `&mut[_]` resp.) - (SuggestedType::SliceRef(mutability), expr.span) - } else { - // `expr` is the `vec![_]` expansion, so suggest `[_]` - // and also use the span of the actual `vec![_]` expression - (SuggestedType::Array, expr.span.ctxt().outer_expn_data().call_site) - }; + let suggest_slice = suggest_type(expr); if adjusts_to_slice(cx, expr) { - self.check_vec_macro(cx, &vec_args, span, expr.hir_id, suggest_slice); + self.check_vec_macro(cx, &vec_args, callsite, expr.hir_id, suggest_slice); } else { - self.span_to_lint_map.insert(span, None); + self.span_to_lint_map.insert(callsite, None); } }, } @@ -151,8 +143,6 @@ impl UselessVec { return; } - let mut applicability = Applicability::MachineApplicable; - let snippet = match *vec_args { higher::VecArgs::Repeat(elem, len) => { if let Some(Constant::Int(len_constant)) = constant(cx, cx.typeck_results(), len) { @@ -166,7 +156,7 @@ impl UselessVec { return; } - suggest_slice.snippet(cx, Some(elem.span), Some(len.span), &mut applicability) + suggest_slice.snippet(cx, Some(elem.span), Some(len.span)) } else { return; } @@ -180,13 +170,16 @@ impl UselessVec { } else { None }; - suggest_slice.snippet(cx, args_span, None, &mut applicability) + suggest_slice.snippet(cx, args_span, None) }, }; - self.span_to_lint_map - .entry(span) - .or_insert(Some((hir_id, suggest_slice, snippet, applicability))); + self.span_to_lint_map.entry(span).or_insert(Some(( + hir_id, + suggest_slice, + snippet, + Applicability::MachineApplicable, + ))); } } @@ -206,24 +199,16 @@ impl SuggestedType { } } - fn snippet( - self, - cx: &LateContext<'_>, - args_span: Option, - len_span: Option, - app: &mut Applicability, - ) -> String { - let args = args_span - .map(|sp| snippet_with_applicability(cx, sp, "..", app)) - .unwrap_or_default(); + fn snippet(self, cx: &LateContext<'_>, args_span: Option, len_span: Option) -> String { + let maybe_args = args_span.and_then(|sp| snippet_opt(cx, sp)).unwrap_or_default(); let maybe_len = len_span - .map(|sp| format!("; {}", snippet_with_applicability(cx, sp, "len", app))) + .and_then(|sp| snippet_opt(cx, sp).map(|s| format!("; {s}"))) .unwrap_or_default(); match self { - Self::SliceRef(Mutability::Mut) => format!("&mut [{args}{maybe_len}]"), - Self::SliceRef(Mutability::Not) => format!("&[{args}{maybe_len}]"), - Self::Array => format!("[{args}{maybe_len}]"), + Self::SliceRef(Mutability::Mut) => format!("&mut [{maybe_args}{maybe_len}]"), + Self::SliceRef(Mutability::Not) => format!("&[{maybe_args}{maybe_len}]"), + Self::Array => format!("[{maybe_args}{maybe_len}]"), } } } @@ -249,3 +234,13 @@ pub fn is_allowed_vec_method(cx: &LateContext<'_>, e: &Expr<'_>) -> bool { is_trait_method(cx, e, sym::IntoIterator) } } + +fn suggest_type(expr: &Expr<'_>) -> SuggestedType { + if let ExprKind::AddrOf(BorrowKind::Ref, mutability, _) = expr.kind { + // `expr` is `&vec![_]`, so suggest `&[_]` (or `&mut[_]` resp.) + SuggestedType::SliceRef(mutability) + } else { + // `expr` is the `vec![_]` expansion, so suggest `[_]` + SuggestedType::Array + } +} diff --git a/tests/ui/vec.fixed b/tests/ui/vec.fixed index b318fd42f7c..9e755a82af4 100644 --- a/tests/ui/vec.fixed +++ b/tests/ui/vec.fixed @@ -217,3 +217,7 @@ fn issue_11958() { // should not lint, `String` is not `Copy` f(&vec!["test".to_owned(); 2]); } + +fn issue_12101() { + for a in &[1, 2] {} +} diff --git a/tests/ui/vec.rs b/tests/ui/vec.rs index 08ad6efa37f..c483271438b 100644 --- a/tests/ui/vec.rs +++ b/tests/ui/vec.rs @@ -217,3 +217,7 @@ fn issue_11958() { // should not lint, `String` is not `Copy` f(&vec!["test".to_owned(); 2]); } + +fn issue_12101() { + for a in &(vec![1, 2]) {} +} diff --git a/tests/ui/vec.stderr b/tests/ui/vec.stderr index 6fd4748fff3..3faea8033fe 100644 --- a/tests/ui/vec.stderr +++ b/tests/ui/vec.stderr @@ -121,5 +121,11 @@ error: useless use of `vec!` LL | this_macro_doesnt_need_vec!(vec![1]); | ^^^^^^^ help: you can use an array directly: `[1]` -error: aborting due to 20 previous errors +error: useless use of `vec!` + --> tests/ui/vec.rs:222:14 + | +LL | for a in &(vec![1, 2]) {} + | ^^^^^^^^^^^^^ help: you can use a slice directly: `&[1, 2]` + +error: aborting due to 21 previous errors