diff --git a/CHANGELOG.md b/CHANGELOG.md index 751f9fccd88..3a274ae1d9c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -3642,6 +3642,7 @@ Released 2018-09-13 [`range_step_by_zero`]: https://rust-lang.github.io/rust-clippy/master/index.html#range_step_by_zero [`range_zip_with_len`]: https://rust-lang.github.io/rust-clippy/master/index.html#range_zip_with_len [`rc_buffer`]: https://rust-lang.github.io/rust-clippy/master/index.html#rc_buffer +[`rc_clone_in_vec_init`]: https://rust-lang.github.io/rust-clippy/master/index.html#rc_clone_in_vec_init [`rc_mutex`]: https://rust-lang.github.io/rust-clippy/master/index.html#rc_mutex [`recursive_format_impl`]: https://rust-lang.github.io/rust-clippy/master/index.html#recursive_format_impl [`redundant_allocation`]: https://rust-lang.github.io/rust-clippy/master/index.html#redundant_allocation diff --git a/clippy_lints/src/lib.register_all.rs b/clippy_lints/src/lib.register_all.rs index e68718f9fdf..b608ef9c693 100644 --- a/clippy_lints/src/lib.register_all.rs +++ b/clippy_lints/src/lib.register_all.rs @@ -263,6 +263,7 @@ store.register_group(true, "clippy::all", Some("clippy_all"), vec![ LintId::of(ranges::MANUAL_RANGE_CONTAINS), LintId::of(ranges::RANGE_ZIP_WITH_LEN), LintId::of(ranges::REVERSED_EMPTY_RANGES), + LintId::of(rc_clone_in_vec_init::RC_CLONE_IN_VEC_INIT), LintId::of(redundant_clone::REDUNDANT_CLONE), LintId::of(redundant_closure_call::REDUNDANT_CLOSURE_CALL), LintId::of(redundant_field_names::REDUNDANT_FIELD_NAMES), diff --git a/clippy_lints/src/lib.register_lints.rs b/clippy_lints/src/lib.register_lints.rs index 5768edc5018..9224d9d5113 100644 --- a/clippy_lints/src/lib.register_lints.rs +++ b/clippy_lints/src/lib.register_lints.rs @@ -446,6 +446,7 @@ store.register_lints(&[ ranges::RANGE_PLUS_ONE, ranges::RANGE_ZIP_WITH_LEN, ranges::REVERSED_EMPTY_RANGES, + rc_clone_in_vec_init::RC_CLONE_IN_VEC_INIT, redundant_clone::REDUNDANT_CLONE, redundant_closure_call::REDUNDANT_CLOSURE_CALL, redundant_else::REDUNDANT_ELSE, diff --git a/clippy_lints/src/lib.register_suspicious.rs b/clippy_lints/src/lib.register_suspicious.rs index 7a881bfe399..940bf7ace5e 100644 --- a/clippy_lints/src/lib.register_suspicious.rs +++ b/clippy_lints/src/lib.register_suspicious.rs @@ -26,6 +26,7 @@ store.register_group(true, "clippy::suspicious", Some("clippy_suspicious"), vec! LintId::of(methods::SUSPICIOUS_MAP), LintId::of(mut_key::MUTABLE_KEY_TYPE), LintId::of(octal_escapes::OCTAL_ESCAPES), + LintId::of(rc_clone_in_vec_init::RC_CLONE_IN_VEC_INIT), LintId::of(suspicious_trait_impl::SUSPICIOUS_ARITHMETIC_IMPL), LintId::of(suspicious_trait_impl::SUSPICIOUS_OP_ASSIGN_IMPL), ]) diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index 3bb821a1482..3c3cb51c8e3 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -345,6 +345,7 @@ mod ptr_offset_with_cast; mod pub_use; mod question_mark; mod ranges; +mod rc_clone_in_vec_init; mod redundant_clone; mod redundant_closure_call; mod redundant_else; @@ -890,6 +891,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: let max_include_file_size = conf.max_include_file_size; store.register_late_pass(move || Box::new(large_include_file::LargeIncludeFile::new(max_include_file_size))); store.register_late_pass(|| Box::new(strings::TrimSplitWhitespace)); + store.register_late_pass(|| Box::new(rc_clone_in_vec_init::RcCloneInVecInit)); // add lints here, do not remove this comment, it's used in `new_lint` } diff --git a/clippy_lints/src/rc_clone_in_vec_init.rs b/clippy_lints/src/rc_clone_in_vec_init.rs new file mode 100644 index 00000000000..6391890b043 --- /dev/null +++ b/clippy_lints/src/rc_clone_in_vec_init.rs @@ -0,0 +1,90 @@ +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, MacroCall}; +use rustc_hir::{Expr, ExprKind, QPath, TyKind}; +use rustc_lint::{LateContext, LateLintPass}; +use rustc_session::{declare_lint_pass, declare_tool_lint}; +use rustc_span::{sym, Symbol}; + +declare_clippy_lint! { + /// ### What it does + /// Checks for `Arc::new` or `Rc::new` in `vec![elem; len]` + /// + /// ### Why is this bad? + /// This will create `elem` once and clone it `len` times - doing so with `Arc` or `Rc` + /// is a bit misleading, as it will create references to the same pointer, rather + /// than different instances. + /// + /// ### Example + /// ```rust + /// let v = vec![std::sync::Arc::new("some data".to_string()); 100]; + /// // or + /// let v = vec![std::rc::Rc::new("some data".to_string()); 100]; + /// ``` + /// Use instead: + /// ```rust + /// + /// // Initialize each value separately: + /// let mut data = Vec::with_capacity(100); + /// for _ in 0..100 { + /// data.push(std::rc::Rc::new("some data".to_string())); + /// } + /// + /// // Or if you want clones of the same reference, + /// // Create the reference beforehand to clarify that + /// // it should be cloned for each value + /// let data = std::rc::Rc::new("some data".to_string()); + /// let v = vec![data; 100]; + /// ``` + #[clippy::version = "1.62.0"] + pub RC_CLONE_IN_VEC_INIT, + suspicious, + "initializing `Arc` or `Rc` in `vec![elem; len]`" +} +declare_lint_pass!(RcCloneInVecInit => [RC_CLONE_IN_VEC_INIT]); + +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, _)) = VecArgs::hir(cx, expr) else { return; }; + let Some(symbol) = new_reference_call(cx, elem) else { return; }; + + emit_lint(cx, symbol, ¯o_call); + } +} + +fn emit_lint(cx: &LateContext<'_>, symbol: Symbol, macro_call: &MacroCall) { + let symbol_name = symbol.as_str(); + + span_lint_and_then( + cx, + RC_CLONE_IN_VEC_INIT, + macro_call.span, + &format!("calling `{symbol_name}::new` in `vec![elem; len]`"), + |diag| { + diag.note(format!("each element will point to the same `{symbol_name}` instance")); + diag.help(format!( + "if this is intentional, consider extracting the `{symbol_name}` initialization to a variable" + )); + diag.help("or if not, initialize each element individually"); + }, + ); +} + +/// Checks whether the given `expr` is a call to `Arc::new` or `Rc::new` +fn new_reference_call(cx: &LateContext<'_>, expr: &Expr<'_>) -> Option { + 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); + } + } + + None +} diff --git a/tests/ui/rc_clone_in_vec_init/arc.rs b/tests/ui/rc_clone_in_vec_init/arc.rs new file mode 100644 index 00000000000..9f4e27dfa62 --- /dev/null +++ b/tests/ui/rc_clone_in_vec_init/arc.rs @@ -0,0 +1,49 @@ +#![warn(clippy::rc_clone_in_vec_init)] +use std::sync::{Arc, Mutex}; + +fn main() {} + +fn should_warn_simple_case() { + let v = vec![Arc::new("x".to_string()); 2]; +} + +fn should_warn_complex_case() { + let v = vec![ + std::sync::Arc::new(Mutex::new({ + let x = 1; + dbg!(x); + x + })); + 2 + ]; +} + +fn should_not_warn_custom_arc() { + #[derive(Clone)] + struct Arc; + + impl Arc { + fn new() -> Self { + Arc + } + } + + let v = vec![Arc::new(); 2]; +} + +fn should_not_warn_vec_from_elem_but_not_arc() { + let v = vec![String::new(); 2]; + let v1 = vec![1; 2]; + let v2 = vec![ + Box::new(std::sync::Arc::new({ + let y = 3; + dbg!(y); + y + })); + 2 + ]; +} + +fn should_not_warn_vec_macro_but_not_from_elem() { + let v = vec![Arc::new("x".to_string())]; +} diff --git a/tests/ui/rc_clone_in_vec_init/arc.stderr b/tests/ui/rc_clone_in_vec_init/arc.stderr new file mode 100644 index 00000000000..19e4727cb98 --- /dev/null +++ b/tests/ui/rc_clone_in_vec_init/arc.stderr @@ -0,0 +1,30 @@ +error: calling `Arc::new` in `vec![elem; len]` + --> $DIR/arc.rs:7:13 + | +LL | let v = vec![Arc::new("x".to_string()); 2]; + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | + = note: `-D clippy::rc-clone-in-vec-init` implied by `-D warnings` + = note: each element will point to the same `Arc` instance + = help: if this is intentional, consider extracting the `Arc` initialization to a variable + = help: or if not, initialize each element individually + +error: calling `Arc::new` in `vec![elem; len]` + --> $DIR/arc.rs:11:13 + | +LL | let v = vec![ + | _____________^ +LL | | std::sync::Arc::new(Mutex::new({ +LL | | let x = 1; +LL | | dbg!(x); +... | +LL | | 2 +LL | | ]; + | |_____^ + | + = note: each element will point to the same `Arc` instance + = help: if this is intentional, consider extracting the `Arc` initialization to a variable + = help: or if not, initialize each element individually + +error: aborting due to 2 previous errors + diff --git a/tests/ui/rc_clone_in_vec_init/rc.rs b/tests/ui/rc_clone_in_vec_init/rc.rs new file mode 100644 index 00000000000..5e2834aa902 --- /dev/null +++ b/tests/ui/rc_clone_in_vec_init/rc.rs @@ -0,0 +1,50 @@ +#![warn(clippy::rc_clone_in_vec_init)] +use std::rc::Rc; +use std::sync::Mutex; + +fn main() {} + +fn should_warn_simple_case() { + let v = vec![Rc::new("x".to_string()); 2]; +} + +fn should_warn_complex_case() { + let v = vec![ + std::rc::Rc::new(Mutex::new({ + let x = 1; + dbg!(x); + x + })); + 2 + ]; +} + +fn should_not_warn_custom_arc() { + #[derive(Clone)] + struct Rc; + + impl Rc { + fn new() -> Self { + Rc + } + } + + let v = vec![Rc::new(); 2]; +} + +fn should_not_warn_vec_from_elem_but_not_rc() { + let v = vec![String::new(); 2]; + let v1 = vec![1; 2]; + let v2 = vec![ + Box::new(std::rc::Rc::new({ + let y = 3; + dbg!(y); + y + })); + 2 + ]; +} + +fn should_not_warn_vec_macro_but_not_from_elem() { + let v = vec![Rc::new("x".to_string())]; +} diff --git a/tests/ui/rc_clone_in_vec_init/rc.stderr b/tests/ui/rc_clone_in_vec_init/rc.stderr new file mode 100644 index 00000000000..50ffcca9676 --- /dev/null +++ b/tests/ui/rc_clone_in_vec_init/rc.stderr @@ -0,0 +1,30 @@ +error: calling `Rc::new` in `vec![elem; len]` + --> $DIR/rc.rs:8:13 + | +LL | let v = vec![Rc::new("x".to_string()); 2]; + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | + = note: `-D clippy::rc-clone-in-vec-init` implied by `-D warnings` + = note: each element will point to the same `Rc` instance + = help: if this is intentional, consider extracting the `Rc` initialization to a variable + = help: or if not, initialize each element individually + +error: calling `Rc::new` in `vec![elem; len]` + --> $DIR/rc.rs:12:13 + | +LL | let v = vec![ + | _____________^ +LL | | std::rc::Rc::new(Mutex::new({ +LL | | let x = 1; +LL | | dbg!(x); +... | +LL | | 2 +LL | | ]; + | |_____^ + | + = note: each element will point to the same `Rc` instance + = help: if this is intentional, consider extracting the `Rc` initialization to a variable + = help: or if not, initialize each element individually + +error: aborting due to 2 previous errors +