diff --git a/CHANGELOG.md b/CHANGELOG.md index 816147076179..ac16ee0faf28 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7404,6 +7404,7 @@ Released 2018-09-13 [`vec_box`]: https://rust-lang.github.io/rust-clippy/master/index.html#vec_box [`vec_init_then_push`]: https://rust-lang.github.io/rust-clippy/master/index.html#vec_init_then_push [`vec_resize_to_zero`]: https://rust-lang.github.io/rust-clippy/master/index.html#vec_resize_to_zero +[`vec_to_rc_slice`]: https://rust-lang.github.io/rust-clippy/master/index.html#vec_to_rc_slice [`verbose_bit_mask`]: https://rust-lang.github.io/rust-clippy/master/index.html#verbose_bit_mask [`verbose_file_reads`]: https://rust-lang.github.io/rust-clippy/master/index.html#verbose_file_reads [`volatile_composites`]: https://rust-lang.github.io/rust-clippy/master/index.html#volatile_composites diff --git a/clippy_lints/src/declared_lints.rs b/clippy_lints/src/declared_lints.rs index 837bf29f5f39..f0f37472eef7 100644 --- a/clippy_lints/src/declared_lints.rs +++ b/clippy_lints/src/declared_lints.rs @@ -794,6 +794,7 @@ pub static LINTS: &[&::declare_clippy_lint::LintInfo] = &[ crate::useless_conversion::USELESS_CONVERSION_INFO, crate::useless_vec::USELESS_VEC_INFO, crate::vec_init_then_push::VEC_INIT_THEN_PUSH_INFO, + crate::vec_to_rc_slice::VEC_TO_RC_SLICE_INFO, crate::visibility::NEEDLESS_PUB_SELF_INFO, crate::visibility::PUB_WITH_SHORTHAND_INFO, crate::visibility::PUB_WITHOUT_SHORTHAND_INFO, diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index 72ee5cca0397..6b4dd25246d9 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -395,6 +395,7 @@ mod useless_concat; mod useless_conversion; mod useless_vec; mod vec_init_then_push; +mod vec_to_rc_slice; mod visibility; mod volatile_composites; mod wildcard_imports; @@ -867,6 +868,7 @@ pub fn register_lint_passes(store: &mut rustc_lint::LintStore, conf: &'static Co Box::new(move |tcx| Box::new(manual_pop_if::ManualPopIf::new(tcx, conf))), Box::new(move |_| Box::new(manual_noop_waker::ManualNoopWaker::new(conf))), Box::new(|_| Box::new(byte_char_slices::ByteCharSlice)), + Box::new(|_| Box::new(vec_to_rc_slice::VecToRcSlice)), // add late passes here, used by `cargo dev new_lint` ]; store.late_passes.extend(late_lints); diff --git a/clippy_lints/src/vec_to_rc_slice.rs b/clippy_lints/src/vec_to_rc_slice.rs new file mode 100644 index 000000000000..061e440d1f15 --- /dev/null +++ b/clippy_lints/src/vec_to_rc_slice.rs @@ -0,0 +1,157 @@ +use clippy_utils::diagnostics::span_lint_and_then; +use clippy_utils::res::{MaybeDef, MaybeResPath, MaybeTypeckRes}; +use clippy_utils::source::snippet_with_context; +use clippy_utils::{qpath_generic_tys, sym}; +use rustc_errors::Applicability; +use rustc_hir::{Expr, ExprKind, Node, TyKind}; +use rustc_lint::{LateContext, LateLintPass}; +use rustc_middle::ty::{self, Ty}; +use rustc_session::declare_lint_pass; +use rustc_span::Span; + +declare_clippy_lint! { + /// ### What it does + /// Detects conversions from `Vec` to `Arc<[T]>` or `Rc<[T]>` via `.into()`, + /// `Arc::from()`, `Rc::from()`, or `From::from()`. + /// + /// ### Why is this bad? + /// `Arc<[T]>` and `Rc<[T]>` store the reference count and slice data in a single + /// contiguous allocation. Because `Vec` uses a separate heap allocation with a + /// different layout, converting `Vec` into `Arc<[T]>` or `Rc<[T]>` must allocate + /// a new block and **copy** all elements. For large vectors this copy can be expensive. + /// + /// Using `Arc>` (or `Rc>`) avoids the copy: + /// `Vec::into_boxed_slice()` can reuse the existing allocation (shrinking if needed), + /// and wrapping the resulting `Box<[T]>` in an `Arc`/`Rc` is a cheap pointer-sized + /// allocation. + /// + /// The trade-off is one extra level of indirection when accessing the data. + /// + /// ### Example + /// ```no_run + /// # use std::sync::Arc; + /// let v: Vec = vec![1, 2, 3]; + /// let a: Arc<[u8]> = v.into(); + /// ``` + /// Use instead: + /// ```no_run + /// # use std::sync::Arc; + /// let v: Vec = vec![1, 2, 3]; + /// let a: Arc> = Arc::new(v.into_boxed_slice()); + /// ``` + #[clippy::version = "1.97.0"] + pub VEC_TO_RC_SLICE, + perf, + "converting `Vec` to `Arc<[T]>` or `Rc<[T]>` copies all elements to a new allocation" +} + +declare_lint_pass!(VecToRcSlice => [VEC_TO_RC_SLICE]); + +/// Checks if `ty` is `Vec` for some `T`. +fn is_vec(cx: &LateContext<'_>, ty: Ty<'_>) -> bool { + if let ty::Adt(adt, _) = ty.kind() { + cx.tcx.is_diagnostic_item(sym::Vec, adt.did()) + } else { + false + } +} + +/// If `ty` is `Arc<[T]>` or `Rc<[T]>`, returns the wrapper name (`"Arc"` or `"Rc"`). +fn rc_slice_wrapper(cx: &LateContext<'_>, ty: Ty<'_>) -> Option<&'static str> { + if let ty::Adt(adt, args) = ty.kind() + && let Some(inner) = args.types().next() + && inner.is_slice() + { + match cx.tcx.get_diagnostic_name(adt.did()) { + Some(sym::Arc) => Some("Arc"), + Some(sym::Rc) => Some("Rc"), + _ => None, + } + } else { + None + } +} + +/// If the expression is the init of a `let` statement with a type annotation like +/// `Arc<[T]>` or `Rc<[T]>`, returns the span of the inner slice type (e.g. `[T]`). +fn let_ty_inner_slice_span(cx: &LateContext<'_>, expr: &Expr<'_>) -> Option { + if let Node::LetStmt(let_stmt) = cx.tcx.parent_hir_node(expr.hir_id) + && let Some(hir_ty) = let_stmt.ty + && let TyKind::Path(ref qpath) = hir_ty.kind + && let Some(def_id) = hir_ty.basic_res().opt_def_id() + && matches!(cx.tcx.get_diagnostic_name(def_id), Some(sym::Arc | sym::Rc)) + && let Some(inner_ty) = qpath_generic_tys(qpath).next() + && matches!(inner_ty.kind, TyKind::Slice(_)) + { + Some(inner_ty.span) + } else { + None + } +} + +impl<'tcx> LateLintPass<'tcx> for VecToRcSlice { + fn check_expr(&mut self, cx: &LateContext<'tcx>, e: &'tcx Expr<'_>) { + if e.span.from_expansion() { + return; + } + + match e.kind { + // `vec_expr.into()` + ExprKind::MethodCall(name, recv, [], _) => { + if name.ident.name == sym::into + && cx.ty_based_def(e).opt_parent(cx).is_diag_item(cx, sym::Into) + && is_vec(cx, cx.typeck_results().expr_ty(recv)) + && let Some(wrapper) = rc_slice_wrapper(cx, cx.typeck_results().expr_ty(e)) + { + emit_lint(cx, e, recv, wrapper); + } + }, + + // `Arc::from(vec)` / `Rc::from(vec)` / `From::from(vec)` + ExprKind::Call(path, [arg]) => { + if let ExprKind::Path(ref qpath) = path.kind + && let Some(def_id) = cx.qpath_res(qpath, path.hir_id).opt_def_id() + && cx.tcx.is_diagnostic_item(sym::from_fn, def_id) + && is_vec(cx, cx.typeck_results().expr_ty(arg)) + && let Some(wrapper) = rc_slice_wrapper(cx, cx.typeck_results().expr_ty(e)) + { + emit_lint(cx, e, arg, wrapper); + } + }, + + _ => {}, + } + } +} + +fn emit_lint(cx: &LateContext<'_>, expr: &Expr<'_>, vec_expr: &Expr<'_>, wrapper: &str) { + span_lint_and_then( + cx, + VEC_TO_RC_SLICE, + expr.span, + format!("converting a `Vec` to `{wrapper}<[T]>` copies all elements to a new allocation"), + |diag| { + let mut app = Applicability::MaybeIncorrect; + let ctxt = expr.span.ctxt(); + + // Common case: we have a `Vec` that we can call `into_boxed_slice()` on. Generate the snippet + // which does so with the appropriate span to identify the Vec and constructs the target (wrapper) + // type from it too. + // + // e.g. `Rc::from(vec)` -> `Rc::new(vec.into_boxed_slice())` + let vec_snippet = snippet_with_context(cx, vec_expr.span, ctxt, "", &mut app).0; + let expr_sugg = format!("{wrapper}::new({vec_snippet}.into_boxed_slice())"); + let mut sugg = vec![(expr.span, expr_sugg)]; + + // if `expr` is part of a let stmt with a type ascription, also fix the latter as part of the + // suggestion. + let inner_slice_span = let_ty_inner_slice_span(cx, expr); + if let Some(ty_span) = inner_slice_span { + let slice_snippet = snippet_with_context(cx, ty_span, ctxt, "_", &mut app).0; + sugg.push((ty_span, format!("Box<{slice_snippet}>"))); + } + + diag.multipart_suggestion(format!("convert target type to `{wrapper}>`"), sugg, app); + }, + ); +} diff --git a/tests/ui/vec_to_rc_slice.fixed b/tests/ui/vec_to_rc_slice.fixed new file mode 100644 index 000000000000..4acd36367811 --- /dev/null +++ b/tests/ui/vec_to_rc_slice.fixed @@ -0,0 +1,65 @@ +#![warn(clippy::vec_to_rc_slice)] + +use std::rc::Rc; +use std::sync::Arc; + +fn main() { + // Should lint: Vec.into() -> Arc<[T]> + let v: Vec = vec![1, 2, 3]; + let _a: Arc> = Arc::new(v.into_boxed_slice()); + //~^ vec_to_rc_slice + + // Should lint: Arc::from(vec) + let v: Vec = vec![1, 2, 3]; + let _a: Arc> = Arc::new(v.into_boxed_slice()); + //~^ vec_to_rc_slice + + // Should lint: From::from(vec) when target is Arc<[T]> + let v: Vec = vec![1, 2, 3]; + let _a: Arc> = Arc::new(v.into_boxed_slice()); + //~^ vec_to_rc_slice + + // Should lint: fully qualified Arc from + let v: Vec = vec![1, 2, 3]; + let _a = Arc::new(v.into_boxed_slice()); + //~^ vec_to_rc_slice + + // Should lint: Vec.into() -> Rc<[T]> + let v: Vec = vec![1, 2, 3]; + let _a: Rc> = Rc::new(v.into_boxed_slice()); + //~^ vec_to_rc_slice + + // Should lint: Rc::from(vec) + let v: Vec = vec![1, 2, 3]; + let _a: Rc> = Rc::new(v.into_boxed_slice()); + //~^ vec_to_rc_slice + + // Should lint: From::from(vec) when target is Rc<[T]> + let v: Vec = vec![1, 2, 3]; + let _a: Rc> = Rc::new(v.into_boxed_slice()); + //~^ vec_to_rc_slice + + // Should lint: fully qualified Rc from + let v: Vec = vec![1, 2, 3]; + let _a = Rc::new(v.into_boxed_slice()); + //~^ vec_to_rc_slice + + // Should NOT lint: Vec.into() -> something else + let v: Vec = vec![1, 2, 3]; + let _b: Box<[u8]> = v.into(); + + // Should NOT lint: non-Vec into Arc<[T]> + let _c: Arc<[u8]> = Arc::from([1u8, 2, 3].as_slice()); + + // Should NOT lint: Vec into Arc (not Arc<[T]>) + let v: Vec = vec![1, 2, 3]; + let _d: Arc> = Arc::new(v); + + // Should NOT lint: the recommended pattern + let v: Vec = vec![1, 2, 3]; + let _e: Arc> = Arc::new(v.into_boxed_slice()); + + // Should NOT lint: Rc recommended pattern + let v: Vec = vec![1, 2, 3]; + let _f: Rc> = Rc::new(v.into_boxed_slice()); +} diff --git a/tests/ui/vec_to_rc_slice.rs b/tests/ui/vec_to_rc_slice.rs new file mode 100644 index 000000000000..aae52be11dcc --- /dev/null +++ b/tests/ui/vec_to_rc_slice.rs @@ -0,0 +1,65 @@ +#![warn(clippy::vec_to_rc_slice)] + +use std::rc::Rc; +use std::sync::Arc; + +fn main() { + // Should lint: Vec.into() -> Arc<[T]> + let v: Vec = vec![1, 2, 3]; + let _a: Arc<[u8]> = v.into(); + //~^ vec_to_rc_slice + + // Should lint: Arc::from(vec) + let v: Vec = vec![1, 2, 3]; + let _a: Arc<[u8]> = Arc::from(v); + //~^ vec_to_rc_slice + + // Should lint: From::from(vec) when target is Arc<[T]> + let v: Vec = vec![1, 2, 3]; + let _a: Arc<[u8]> = From::from(v); + //~^ vec_to_rc_slice + + // Should lint: fully qualified Arc from + let v: Vec = vec![1, 2, 3]; + let _a = as From>>::from(v); + //~^ vec_to_rc_slice + + // Should lint: Vec.into() -> Rc<[T]> + let v: Vec = vec![1, 2, 3]; + let _a: Rc<[u8]> = v.into(); + //~^ vec_to_rc_slice + + // Should lint: Rc::from(vec) + let v: Vec = vec![1, 2, 3]; + let _a: Rc<[u8]> = Rc::from(v); + //~^ vec_to_rc_slice + + // Should lint: From::from(vec) when target is Rc<[T]> + let v: Vec = vec![1, 2, 3]; + let _a: Rc<[u8]> = From::from(v); + //~^ vec_to_rc_slice + + // Should lint: fully qualified Rc from + let v: Vec = vec![1, 2, 3]; + let _a = as From>>::from(v); + //~^ vec_to_rc_slice + + // Should NOT lint: Vec.into() -> something else + let v: Vec = vec![1, 2, 3]; + let _b: Box<[u8]> = v.into(); + + // Should NOT lint: non-Vec into Arc<[T]> + let _c: Arc<[u8]> = Arc::from([1u8, 2, 3].as_slice()); + + // Should NOT lint: Vec into Arc (not Arc<[T]>) + let v: Vec = vec![1, 2, 3]; + let _d: Arc> = Arc::new(v); + + // Should NOT lint: the recommended pattern + let v: Vec = vec![1, 2, 3]; + let _e: Arc> = Arc::new(v.into_boxed_slice()); + + // Should NOT lint: Rc recommended pattern + let v: Vec = vec![1, 2, 3]; + let _f: Rc> = Rc::new(v.into_boxed_slice()); +} diff --git a/tests/ui/vec_to_rc_slice.stderr b/tests/ui/vec_to_rc_slice.stderr new file mode 100644 index 000000000000..cc664f1a059b --- /dev/null +++ b/tests/ui/vec_to_rc_slice.stderr @@ -0,0 +1,100 @@ +error: converting a `Vec` to `Arc<[T]>` copies all elements to a new allocation + --> tests/ui/vec_to_rc_slice.rs:9:25 + | +LL | let _a: Arc<[u8]> = v.into(); + | ^^^^^^^^ + | + = note: `-D clippy::vec-to-rc-slice` implied by `-D warnings` + = help: to override `-D warnings` add `#[allow(clippy::vec_to_rc_slice)]` +help: convert target type to `Arc>` + | +LL - let _a: Arc<[u8]> = v.into(); +LL + let _a: Arc> = Arc::new(v.into_boxed_slice()); + | + +error: converting a `Vec` to `Arc<[T]>` copies all elements to a new allocation + --> tests/ui/vec_to_rc_slice.rs:14:25 + | +LL | let _a: Arc<[u8]> = Arc::from(v); + | ^^^^^^^^^^^^ + | +help: convert target type to `Arc>` + | +LL - let _a: Arc<[u8]> = Arc::from(v); +LL + let _a: Arc> = Arc::new(v.into_boxed_slice()); + | + +error: converting a `Vec` to `Arc<[T]>` copies all elements to a new allocation + --> tests/ui/vec_to_rc_slice.rs:19:25 + | +LL | let _a: Arc<[u8]> = From::from(v); + | ^^^^^^^^^^^^^ + | +help: convert target type to `Arc>` + | +LL - let _a: Arc<[u8]> = From::from(v); +LL + let _a: Arc> = Arc::new(v.into_boxed_slice()); + | + +error: converting a `Vec` to `Arc<[T]>` copies all elements to a new allocation + --> tests/ui/vec_to_rc_slice.rs:24:14 + | +LL | let _a = as From>>::from(v); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | +help: convert target type to `Arc>` + | +LL - let _a = as From>>::from(v); +LL + let _a = Arc::new(v.into_boxed_slice()); + | + +error: converting a `Vec` to `Rc<[T]>` copies all elements to a new allocation + --> tests/ui/vec_to_rc_slice.rs:29:24 + | +LL | let _a: Rc<[u8]> = v.into(); + | ^^^^^^^^ + | +help: convert target type to `Rc>` + | +LL - let _a: Rc<[u8]> = v.into(); +LL + let _a: Rc> = Rc::new(v.into_boxed_slice()); + | + +error: converting a `Vec` to `Rc<[T]>` copies all elements to a new allocation + --> tests/ui/vec_to_rc_slice.rs:34:24 + | +LL | let _a: Rc<[u8]> = Rc::from(v); + | ^^^^^^^^^^^ + | +help: convert target type to `Rc>` + | +LL - let _a: Rc<[u8]> = Rc::from(v); +LL + let _a: Rc> = Rc::new(v.into_boxed_slice()); + | + +error: converting a `Vec` to `Rc<[T]>` copies all elements to a new allocation + --> tests/ui/vec_to_rc_slice.rs:39:24 + | +LL | let _a: Rc<[u8]> = From::from(v); + | ^^^^^^^^^^^^^ + | +help: convert target type to `Rc>` + | +LL - let _a: Rc<[u8]> = From::from(v); +LL + let _a: Rc> = Rc::new(v.into_boxed_slice()); + | + +error: converting a `Vec` to `Rc<[T]>` copies all elements to a new allocation + --> tests/ui/vec_to_rc_slice.rs:44:14 + | +LL | let _a = as From>>::from(v); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | +help: convert target type to `Rc>` + | +LL - let _a = as From>>::from(v); +LL + let _a = Rc::new(v.into_boxed_slice()); + | + +error: aborting due to 8 previous errors + diff --git a/tests/ui/vec_to_rc_slice_unfixable.rs b/tests/ui/vec_to_rc_slice_unfixable.rs new file mode 100644 index 000000000000..cebc19af55b4 --- /dev/null +++ b/tests/ui/vec_to_rc_slice_unfixable.rs @@ -0,0 +1,20 @@ +//@no-rustfix +#![warn(clippy::vec_to_rc_slice)] + +use std::rc::Rc; +use std::sync::Arc; + +fn accept_arc_slice(_: Arc<[u8]>) {} +fn accept_rc_slice(_: Rc<[u8]>) {} + +fn main() { + // Should lint: result passed to a function expecting Arc<[T]> (fix requires downstream changes) + let v: Vec = vec![1, 2, 3]; + accept_arc_slice(v.into()); + //~^ vec_to_rc_slice + + // Should lint: result passed to a function expecting Rc<[T]> (fix requires downstream changes) + let v: Vec = vec![1, 2, 3]; + accept_rc_slice(Rc::from(v)); + //~^ vec_to_rc_slice +} diff --git a/tests/ui/vec_to_rc_slice_unfixable.stderr b/tests/ui/vec_to_rc_slice_unfixable.stderr new file mode 100644 index 000000000000..943caf2c2397 --- /dev/null +++ b/tests/ui/vec_to_rc_slice_unfixable.stderr @@ -0,0 +1,28 @@ +error: converting a `Vec` to `Arc<[T]>` copies all elements to a new allocation + --> tests/ui/vec_to_rc_slice_unfixable.rs:13:22 + | +LL | accept_arc_slice(v.into()); + | ^^^^^^^^ + | + = note: `-D clippy::vec-to-rc-slice` implied by `-D warnings` + = help: to override `-D warnings` add `#[allow(clippy::vec_to_rc_slice)]` +help: convert target type to `Arc>` + | +LL - accept_arc_slice(v.into()); +LL + accept_arc_slice(Arc::new(v.into_boxed_slice())); + | + +error: converting a `Vec` to `Rc<[T]>` copies all elements to a new allocation + --> tests/ui/vec_to_rc_slice_unfixable.rs:18:21 + | +LL | accept_rc_slice(Rc::from(v)); + | ^^^^^^^^^^^ + | +help: convert target type to `Rc>` + | +LL - accept_rc_slice(Rc::from(v)); +LL + accept_rc_slice(Rc::new(v.into_boxed_slice())); + | + +error: aborting due to 2 previous errors +