diff options
| -rw-r--r-- | clippy_lints/src/rc_clone_in_vec_init.rs | 56 | ||||
| -rw-r--r-- | tests/ui/rc_clone_in_vec_init/arc.stderr | 16 | ||||
| -rw-r--r-- | tests/ui/rc_clone_in_vec_init/rc.stderr | 16 | ||||
| -rw-r--r-- | tests/ui/rc_clone_in_vec_init/weak.rs | 83 | ||||
| -rw-r--r-- | tests/ui/rc_clone_in_vec_init/weak.stderr | 201 |
5 files changed, 327 insertions, 45 deletions
diff --git a/clippy_lints/src/rc_clone_in_vec_init.rs b/clippy_lints/src/rc_clone_in_vec_init.rs index 110f58f3734..8db8c4e9b78 100644 --- a/clippy_lints/src/rc_clone_in_vec_init.rs +++ b/clippy_lints/src/rc_clone_in_vec_init.rs @@ -2,7 +2,9 @@ use clippy_utils::diagnostics::span_lint_and_then; use clippy_utils::higher::VecArgs; use clippy_utils::last_path_segment; use clippy_utils::macros::root_macro_call_first_node; +use clippy_utils::paths; use clippy_utils::source::{indent_of, snippet}; +use clippy_utils::ty::match_type; use rustc_errors::Applicability; use rustc_hir::{Expr, ExprKind, QPath, TyKind}; use rustc_lint::{LateContext, LateLintPass}; @@ -11,10 +13,11 @@ use rustc_span::{sym, Span, Symbol}; declare_clippy_lint! { /// ### What it does - /// Checks for `Arc::new` or `Rc::new` in `vec![elem; len]` + /// Checks for reference-counted pointers (`Arc`, `Rc`, `rc::Weak`, and `sync::Weak`) + /// in `vec![elem; len]` /// /// ### Why is this bad? - /// This will create `elem` once and clone it `len` times - doing so with `Arc` or `Rc` + /// This will create `elem` once and clone it `len` times - doing so with `Arc`/`Rc`/`Weak` /// is a bit misleading, as it will create references to the same pointer, rather /// than different instances. /// @@ -26,7 +29,6 @@ declare_clippy_lint! { /// ``` /// Use instead: /// ```rust - /// /// // Initialize each value separately: /// let mut data = Vec::with_capacity(100); /// for _ in 0..100 { @@ -42,7 +44,7 @@ declare_clippy_lint! { #[clippy::version = "1.62.0"] pub RC_CLONE_IN_VEC_INIT, suspicious, - "initializing `Arc` or `Rc` in `vec![elem; len]`" + "initializing reference-counted pointer in `vec![elem; len]`" } declare_lint_pass!(RcCloneInVecInit => [RC_CLONE_IN_VEC_INIT]); @@ -50,26 +52,12 @@ impl LateLintPass<'_> for RcCloneInVecInit { fn check_expr(&mut self, cx: &LateContext<'_>, expr: &Expr<'_>) { let Some(macro_call) = root_macro_call_first_node(cx, expr) else { return; }; let Some(VecArgs::Repeat(elem, len)) = VecArgs::hir(cx, expr) else { return; }; - let Some(symbol) = new_reference_call(cx, elem) else { return; }; + let Some((symbol, func_span)) = ref_init(cx, elem) else { return; }; - emit_lint(cx, symbol, macro_call.span, elem, len); + emit_lint(cx, symbol, macro_call.span, elem, len, func_span); } } -fn elem_snippet(cx: &LateContext<'_>, elem: &Expr<'_>, symbol_name: &str) -> String { - let elem_snippet = snippet(cx, elem.span, "..").to_string(); - if elem_snippet.contains('\n') { - // This string must be found in `elem_snippet`, otherwise we won't be constructing - // the snippet in the first place. - let reference_creation = format!("{symbol_name}::new"); - let (code_until_reference_creation, _right) = elem_snippet.split_once(&reference_creation).unwrap(); - - return format!("{code_until_reference_creation}{reference_creation}(..)"); - } - - elem_snippet -} - fn loop_init_suggestion(elem: &str, len: &str, indent: &str) -> String { format!( r#"{{ @@ -89,17 +77,17 @@ fn extract_suggestion(elem: &str, len: &str, indent: &str) -> String { ) } -fn emit_lint(cx: &LateContext<'_>, symbol: Symbol, lint_span: Span, elem: &Expr<'_>, len: &Expr<'_>) { +fn emit_lint(cx: &LateContext<'_>, symbol: Symbol, lint_span: Span, elem: &Expr<'_>, len: &Expr<'_>, func_span: Span) { let symbol_name = symbol.as_str(); span_lint_and_then( cx, RC_CLONE_IN_VEC_INIT, lint_span, - &format!("calling `{symbol_name}::new` in `vec![elem; len]`"), + "initializing a reference-counted pointer in `vec![elem; len]`", |diag| { let len_snippet = snippet(cx, len.span, ".."); - let elem_snippet = elem_snippet(cx, elem, symbol_name); + let elem_snippet = format!("{}(..)", snippet(cx, elem.span.with_hi(func_span.hi()), "..")); let indentation = " ".repeat(indent_of(cx, lint_span).unwrap_or(0)); let loop_init_suggestion = loop_init_suggestion(&elem_snippet, len_snippet.as_ref(), &indentation); let extract_suggestion = extract_suggestion(&elem_snippet, len_snippet.as_ref(), &indentation); @@ -109,7 +97,7 @@ fn emit_lint(cx: &LateContext<'_>, symbol: Symbol, lint_span: Span, elem: &Expr< lint_span, format!("consider initializing each `{symbol_name}` element individually"), loop_init_suggestion, - Applicability::Unspecified, + Applicability::HasPlaceholders, ); diag.span_suggestion( lint_span, @@ -117,23 +105,33 @@ fn emit_lint(cx: &LateContext<'_>, symbol: Symbol, lint_span: Span, elem: &Expr< "or if this is intentional, consider extracting the `{symbol_name}` initialization to a variable" ), extract_suggestion, - Applicability::Unspecified, + Applicability::HasPlaceholders, ); }, ); } -/// Checks whether the given `expr` is a call to `Arc::new` or `Rc::new` -fn new_reference_call(cx: &LateContext<'_>, expr: &Expr<'_>) -> Option<Symbol> { +/// Checks whether the given `expr` is a call to `Arc::new`, `Rc::new`, or evaluates to a `Weak` +fn ref_init(cx: &LateContext<'_>, expr: &Expr<'_>) -> Option<(Symbol, Span)> { if_chain! { if let ExprKind::Call(func, _args) = expr.kind; if let ExprKind::Path(ref func_path @ QPath::TypeRelative(ty, _)) = func.kind; if let TyKind::Path(ref ty_path) = ty.kind; if let Some(def_id) = cx.qpath_res(ty_path, ty.hir_id).opt_def_id(); - if last_path_segment(func_path).ident.name == sym::new; then { - return cx.tcx.get_diagnostic_name(def_id).filter(|symbol| symbol == &sym::Arc || symbol == &sym::Rc); + if last_path_segment(func_path).ident.name == sym::new + && let Some(symbol) = cx + .tcx + .get_diagnostic_name(def_id) + .filter(|symbol| symbol == &sym::Arc || symbol == &sym::Rc) { + return Some((symbol, func.span)); + } + + let ty_path = cx.typeck_results().expr_ty(expr); + if match_type(cx, ty_path, &paths::WEAK_RC) || match_type(cx, ty_path, &paths::WEAK_ARC) { + return Some((Symbol::intern("Weak"), func.span)); + } } } diff --git a/tests/ui/rc_clone_in_vec_init/arc.stderr b/tests/ui/rc_clone_in_vec_init/arc.stderr index ce84186c8e3..cd7d91e1206 100644 --- a/tests/ui/rc_clone_in_vec_init/arc.stderr +++ b/tests/ui/rc_clone_in_vec_init/arc.stderr @@ -1,4 +1,4 @@ -error: calling `Arc::new` in `vec![elem; len]` +error: initializing a reference-counted pointer in `vec![elem; len]` --> $DIR/arc.rs:7:13 | LL | let v = vec![Arc::new("x".to_string()); 2]; @@ -10,19 +10,19 @@ help: consider initializing each `Arc` element individually | LL ~ let v = { LL + let mut v = Vec::with_capacity(2); -LL + (0..2).for_each(|_| v.push(Arc::new("x".to_string()))); +LL + (0..2).for_each(|_| v.push(Arc::new(..))); LL + v LL ~ }; | help: or if this is intentional, consider extracting the `Arc` initialization to a variable | LL ~ let v = { -LL + let data = Arc::new("x".to_string()); +LL + let data = Arc::new(..); LL + vec![data; 2] LL ~ }; | -error: calling `Arc::new` in `vec![elem; len]` +error: initializing a reference-counted pointer in `vec![elem; len]` --> $DIR/arc.rs:15:21 | LL | let v = vec![Arc::new("x".to_string()); 2]; @@ -33,19 +33,19 @@ help: consider initializing each `Arc` element individually | LL ~ let v = { LL + let mut v = Vec::with_capacity(2); -LL + (0..2).for_each(|_| v.push(Arc::new("x".to_string()))); +LL + (0..2).for_each(|_| v.push(Arc::new(..))); LL + v LL ~ }; | help: or if this is intentional, consider extracting the `Arc` initialization to a variable | LL ~ let v = { -LL + let data = Arc::new("x".to_string()); +LL + let data = Arc::new(..); LL + vec![data; 2] LL ~ }; | -error: calling `Arc::new` in `vec![elem; len]` +error: initializing a reference-counted pointer in `vec![elem; len]` --> $DIR/arc.rs:21:13 | LL | let v = vec![ @@ -75,7 +75,7 @@ LL + vec![data; 2] LL ~ }; | -error: calling `Arc::new` in `vec![elem; len]` +error: initializing a reference-counted pointer in `vec![elem; len]` --> $DIR/arc.rs:30:14 | LL | let v1 = vec![ diff --git a/tests/ui/rc_clone_in_vec_init/rc.stderr b/tests/ui/rc_clone_in_vec_init/rc.stderr index 0f5cc0cf98f..fe861afe054 100644 --- a/tests/ui/rc_clone_in_vec_init/rc.stderr +++ b/tests/ui/rc_clone_in_vec_init/rc.stderr @@ -1,4 +1,4 @@ -error: calling `Rc::new` in `vec![elem; len]` +error: initializing a reference-counted pointer in `vec![elem; len]` --> $DIR/rc.rs:8:13 | LL | let v = vec![Rc::new("x".to_string()); 2]; @@ -10,19 +10,19 @@ help: consider initializing each `Rc` element individually | LL ~ let v = { LL + let mut v = Vec::with_capacity(2); -LL + (0..2).for_each(|_| v.push(Rc::new("x".to_string()))); +LL + (0..2).for_each(|_| v.push(Rc::new(..))); LL + v LL ~ }; | help: or if this is intentional, consider extracting the `Rc` initialization to a variable | LL ~ let v = { -LL + let data = Rc::new("x".to_string()); +LL + let data = Rc::new(..); LL + vec![data; 2] LL ~ }; | -error: calling `Rc::new` in `vec![elem; len]` +error: initializing a reference-counted pointer in `vec![elem; len]` --> $DIR/rc.rs:16:21 | LL | let v = vec![Rc::new("x".to_string()); 2]; @@ -33,19 +33,19 @@ help: consider initializing each `Rc` element individually | LL ~ let v = { LL + let mut v = Vec::with_capacity(2); -LL + (0..2).for_each(|_| v.push(Rc::new("x".to_string()))); +LL + (0..2).for_each(|_| v.push(Rc::new(..))); LL + v LL ~ }; | help: or if this is intentional, consider extracting the `Rc` initialization to a variable | LL ~ let v = { -LL + let data = Rc::new("x".to_string()); +LL + let data = Rc::new(..); LL + vec![data; 2] LL ~ }; | -error: calling `Rc::new` in `vec![elem; len]` +error: initializing a reference-counted pointer in `vec![elem; len]` --> $DIR/rc.rs:22:13 | LL | let v = vec![ @@ -75,7 +75,7 @@ LL + vec![data; 2] LL ~ }; | -error: calling `Rc::new` in `vec![elem; len]` +error: initializing a reference-counted pointer in `vec![elem; len]` --> $DIR/rc.rs:31:14 | LL | let v1 = vec![ diff --git a/tests/ui/rc_clone_in_vec_init/weak.rs b/tests/ui/rc_clone_in_vec_init/weak.rs new file mode 100644 index 00000000000..693c9b553c5 --- /dev/null +++ b/tests/ui/rc_clone_in_vec_init/weak.rs @@ -0,0 +1,83 @@ +#![warn(clippy::rc_clone_in_vec_init)] +use std::rc::{Rc, Weak as UnSyncWeak}; +use std::sync::{Arc, Mutex, Weak as SyncWeak}; + +fn main() {} + +fn should_warn_simple_case() { + let v = vec![SyncWeak::<u32>::new(); 2]; + let v2 = vec![UnSyncWeak::<u32>::new(); 2]; + + let v = vec![Rc::downgrade(&Rc::new("x".to_string())); 2]; + let v = vec![Arc::downgrade(&Arc::new("x".to_string())); 2]; +} + +fn should_warn_simple_case_with_big_indentation() { + if true { + let k = 1; + dbg!(k); + if true { + let v = vec![Arc::downgrade(&Arc::new("x".to_string())); 2]; + let v2 = vec![Rc::downgrade(&Rc::new("x".to_string())); 2]; + } + } +} + +fn should_warn_complex_case() { + let v = vec![ + Arc::downgrade(&Arc::new(Mutex::new({ + let x = 1; + dbg!(x); + x + }))); + 2 + ]; + + let v1 = vec![ + Rc::downgrade(&Rc::new(Mutex::new({ + let x = 1; + dbg!(x); + x + }))); + 2 + ]; +} + +fn should_not_warn_custom_weak() { + #[derive(Clone)] + struct Weak; + + impl Weak { + fn new() -> Self { + Weak + } + } + + let v = vec![Weak::new(); 2]; +} + +fn should_not_warn_vec_from_elem_but_not_weak() { + let v = vec![String::new(); 2]; + let v1 = vec![1; 2]; + let v2 = vec![ + Box::new(Arc::downgrade(&Arc::new({ + let y = 3; + dbg!(y); + y + }))); + 2 + ]; + let v3 = vec![ + Box::new(Rc::downgrade(&Rc::new({ + let y = 3; + dbg!(y); + y + }))); + 2 + ]; +} + +fn should_not_warn_vec_macro_but_not_from_elem() { + let v = vec![Arc::downgrade(&Arc::new("x".to_string()))]; + let v = vec![Rc::downgrade(&Rc::new("x".to_string()))]; +} diff --git a/tests/ui/rc_clone_in_vec_init/weak.stderr b/tests/ui/rc_clone_in_vec_init/weak.stderr new file mode 100644 index 00000000000..4a21946ccdf --- /dev/null +++ b/tests/ui/rc_clone_in_vec_init/weak.stderr @@ -0,0 +1,201 @@ +error: initializing a reference-counted pointer in `vec![elem; len]` + --> $DIR/weak.rs:8:13 + | +LL | let v = vec![SyncWeak::<u32>::new(); 2]; + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | + = note: `-D clippy::rc-clone-in-vec-init` implied by `-D warnings` + = note: each element will point to the same `Weak` instance +help: consider initializing each `Weak` element individually + | +LL ~ let v = { +LL + let mut v = Vec::with_capacity(2); +LL + (0..2).for_each(|_| v.push(SyncWeak::<u32>::new(..))); +LL + v +LL ~ }; + | +help: or if this is intentional, consider extracting the `Weak` initialization to a variable + | +LL ~ let v = { +LL + let data = SyncWeak::<u32>::new(..); +LL + vec![data; 2] +LL ~ }; + | + +error: initializing a reference-counted pointer in `vec![elem; len]` + --> $DIR/weak.rs:9:14 + | +LL | let v2 = vec![UnSyncWeak::<u32>::new(); 2]; + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | + = note: each element will point to the same `Weak` instance +help: consider initializing each `Weak` element individually + | +LL ~ let v2 = { +LL + let mut v = Vec::with_capacity(2); +LL + (0..2).for_each(|_| v.push(UnSyncWeak::<u32>::new(..))); +LL + v +LL ~ }; + | +help: or if this is intentional, consider extracting the `Weak` initialization to a variable + | +LL ~ let v2 = { +LL + let data = UnSyncWeak::<u32>::new(..); +LL + vec![data; 2] +LL ~ }; + | + +error: initializing a reference-counted pointer in `vec![elem; len]` + --> $DIR/weak.rs:11:13 + | +LL | let v = vec![Rc::downgrade(&Rc::new("x".to_string())); 2]; + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | + = note: each element will point to the same `Weak` instance +help: consider initializing each `Weak` element individually + | +LL ~ let v = { +LL + let mut v = Vec::with_capacity(2); +LL + (0..2).for_each(|_| v.push(Rc::downgrade(..))); +LL + v +LL ~ }; + | +help: or if this is intentional, consider extracting the `Weak` initialization to a variable + | +LL ~ let v = { +LL + let data = Rc::downgrade(..); +LL + vec![data; 2] +LL ~ }; + | + +error: initializing a reference-counted pointer in `vec![elem; len]` + --> $DIR/weak.rs:12:13 + | +LL | let v = vec![Arc::downgrade(&Arc::new("x".to_string())); 2]; + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | + = note: each element will point to the same `Weak` instance +help: consider initializing each `Weak` element individually + | +LL ~ let v = { +LL + let mut v = Vec::with_capacity(2); +LL + (0..2).for_each(|_| v.push(Arc::downgrade(..))); +LL + v +LL ~ }; + | +help: or if this is intentional, consider extracting the `Weak` initialization to a variable + | +LL ~ let v = { +LL + let data = Arc::downgrade(..); +LL + vec![data; 2] +LL ~ }; + | + +error: initializing a reference-counted pointer in `vec![elem; len]` + --> $DIR/weak.rs:20:21 + | +LL | let v = vec![Arc::downgrade(&Arc::new("x".to_string())); 2]; + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | + = note: each element will point to the same `Weak` instance +help: consider initializing each `Weak` element individually + | +LL ~ let v = { +LL + let mut v = Vec::with_capacity(2); +LL + (0..2).for_each(|_| v.push(Arc::downgrade(..))); +LL + v +LL ~ }; + | +help: or if this is intentional, consider extracting the `Weak` initialization to a variable + | +LL ~ let v = { +LL + let data = Arc::downgrade(..); +LL + vec![data; 2] +LL ~ }; + | + +error: initializing a reference-counted pointer in `vec![elem; len]` + --> $DIR/weak.rs:21:22 + | +LL | let v2 = vec![Rc::downgrade(&Rc::new("x".to_string())); 2]; + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | + = note: each element will point to the same `Weak` instance +help: consider initializing each `Weak` element individually + | +LL ~ let v2 = { +LL + let mut v = Vec::with_capacity(2); +LL + (0..2).for_each(|_| v.push(Rc::downgrade(..))); +LL + v +LL ~ }; + | +help: or if this is intentional, consider extracting the `Weak` initialization to a variable + | +LL ~ let v2 = { +LL + let data = Rc::downgrade(..); +LL + vec![data; 2] +LL ~ }; + | + +error: initializing a reference-counted pointer in `vec![elem; len]` + --> $DIR/weak.rs:27:13 + | +LL | let v = vec![ + | _____________^ +LL | | Arc::downgrade(&Arc::new(Mutex::new({ +LL | | let x = 1; +LL | | dbg!(x); +... | +LL | | 2 +LL | | ]; + | |_____^ + | + = note: each element will point to the same `Weak` instance +help: consider initializing each `Weak` element individually + | +LL ~ let v = { +LL + let mut v = Vec::with_capacity(2); +LL + (0..2).for_each(|_| v.push(Arc::downgrade(..))); +LL + v +LL ~ }; + | +help: or if this is intentional, consider extracting the `Weak` initialization to a variable + | +LL ~ let v = { +LL + let data = Arc::downgrade(..); +LL + vec![data; 2] +LL ~ }; + | + +error: initializing a reference-counted pointer in `vec![elem; len]` + --> $DIR/weak.rs:36:14 + | +LL | let v1 = vec![ + | ______________^ +LL | | Rc::downgrade(&Rc::new(Mutex::new({ +LL | | let x = 1; +LL | | dbg!(x); +... | +LL | | 2 +LL | | ]; + | |_____^ + | + = note: each element will point to the same `Weak` instance +help: consider initializing each `Weak` element individually + | +LL ~ let v1 = { +LL + let mut v = Vec::with_capacity(2); +LL + (0..2).for_each(|_| v.push(Rc::downgrade(..))); +LL + v +LL ~ }; + | +help: or if this is intentional, consider extracting the `Weak` initialization to a variable + | +LL ~ let v1 = { +LL + let data = Rc::downgrade(..); +LL + vec![data; 2] +LL ~ }; + | + +error: aborting due to 8 previous errors + |