Auto merge of #8769 - yonip23:8719, r=xFrednet

introduce rc_clone_in_vec_init lint

Closes #8719

changelog: Introduce [`rc_clone_in_vec_init`] lint

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

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),

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,

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),
 ])

clippy_lints/src/lib.rs

@@ -346,6 +346,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;
@@ -900,6 +901,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`
 }
 

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, &macro_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<Symbol> {
+    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
+}

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())];
+}

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
+

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())];
+}

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
+