Auto merge of #8885 - Serial-ATA:rc-clone-in-vec-init-weak, r=llogiq

bors · bors · commit a5ece8132fef · 2022-05-27T04:02:53.000Z
Support `Weak` in [`rc_clone_in_vec_init`]

changelog: Support `Weak` in [`rc_clone_in_vec_init`]
diff --git 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,34 +44,20 @@ 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]);
 
 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,31 +97,41 @@ 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,
                 format!(
                     "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
@@ -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
@@ -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
@@ -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

Original file line number	Diff line number	Diff line change
`@@ -1,4 +1,4 @@`
`1`		-error: calling `Arc::new` in `vec![elem; len]`
	`1`	+error: initializing a reference-counted pointer in `vec![elem; len]`
`2`	`2`	`--> $DIR/arc.rs:7:13`
`3`	`3`	`\|`
`4`	`4`	`LL \| let v = vec![Arc::new("x".to_string()); 2];`
@@ -10,19 +10,19 @@ help: consider initializing each `Arc` element individually
`10`	`10`	`\|`
`11`	`11`	`LL ~ let v = {`
`12`	`12`	`LL + let mut v = Vec::with_capacity(2);`
`13`		`-LL + (0..2).for_each(\|_\| v.push(Arc::new("x".to_string())));`
	`13`	`+LL + (0..2).for_each(\|_\| v.push(Arc::new(..)));`
`14`	`14`	`LL + v`
`15`	`15`	`LL ~ };`
`16`	`16`	`\|`
`17`	`17`	help: or if this is intentional, consider extracting the `Arc` initialization to a variable
`18`	`18`	`\|`
`19`	`19`	`LL ~ let v = {`
`20`		`-LL + let data = Arc::new("x".to_string());`
	`20`	`+LL + let data = Arc::new(..);`
`21`	`21`	`LL + vec![data; 2]`
`22`	`22`	`LL ~ };`
`23`	`23`	`\|`
`24`	`24`
`25`		-error: calling `Arc::new` in `vec![elem; len]`
	`25`	+error: initializing a reference-counted pointer in `vec![elem; len]`
`26`	`26`	`--> $DIR/arc.rs:15:21`
`27`	`27`	`\|`
`28`	`28`	`LL \| let v = vec![Arc::new("x".to_string()); 2];`
@@ -33,19 +33,19 @@ help: consider initializing each `Arc` element individually
`33`	`33`	`\|`
`34`	`34`	`LL ~ let v = {`
`35`	`35`	`LL + let mut v = Vec::with_capacity(2);`
`36`		`-LL + (0..2).for_each(\|_\| v.push(Arc::new("x".to_string())));`
	`36`	`+LL + (0..2).for_each(\|_\| v.push(Arc::new(..)));`
`37`	`37`	`LL + v`
`38`	`38`	`LL ~ };`
`39`	`39`	`\|`
`40`	`40`	help: or if this is intentional, consider extracting the `Arc` initialization to a variable
`41`	`41`	`\|`
`42`	`42`	`LL ~ let v = {`
`43`		`-LL + let data = Arc::new("x".to_string());`
	`43`	`+LL + let data = Arc::new(..);`
`44`	`44`	`LL + vec![data; 2]`
`45`	`45`	`LL ~ };`
`46`	`46`	`\|`
`47`	`47`
`48`		-error: calling `Arc::new` in `vec![elem; len]`
	`48`	+error: initializing a reference-counted pointer in `vec![elem; len]`
`49`	`49`	`--> $DIR/arc.rs:21:13`
`50`	`50`	`\|`
`51`	`51`	`LL \| let v = vec![`
`@@ -75,7 +75,7 @@ LL + vec![data; 2]`
`75`	`75`	`LL ~ };`
`76`	`76`	`\|`
`77`	`77`
`78`		-error: calling `Arc::new` in `vec![elem; len]`
	`78`	+error: initializing a reference-counted pointer in `vec![elem; len]`
`79`	`79`	`--> $DIR/arc.rs:30:14`
`80`	`80`	`\|`
`81`	`81`	`LL \| let v1 = vec![`
Original file line number	Diff line number	Diff line change
`@@ -1,4 +1,4 @@`
`1`		-error: calling `Rc::new` in `vec![elem; len]`
	`1`	+error: initializing a reference-counted pointer in `vec![elem; len]`
`2`	`2`	`--> $DIR/rc.rs:8:13`
`3`	`3`	`\|`
`4`	`4`	`LL \| let v = vec![Rc::new("x".to_string()); 2];`
@@ -10,19 +10,19 @@ help: consider initializing each `Rc` element individually
`10`	`10`	`\|`
`11`	`11`	`LL ~ let v = {`
`12`	`12`	`LL + let mut v = Vec::with_capacity(2);`
`13`		`-LL + (0..2).for_each(\|_\| v.push(Rc::new("x".to_string())));`
	`13`	`+LL + (0..2).for_each(\|_\| v.push(Rc::new(..)));`
`14`	`14`	`LL + v`
`15`	`15`	`LL ~ };`
`16`	`16`	`\|`
`17`	`17`	help: or if this is intentional, consider extracting the `Rc` initialization to a variable
`18`	`18`	`\|`
`19`	`19`	`LL ~ let v = {`
`20`		`-LL + let data = Rc::new("x".to_string());`
	`20`	`+LL + let data = Rc::new(..);`
`21`	`21`	`LL + vec![data; 2]`
`22`	`22`	`LL ~ };`
`23`	`23`	`\|`
`24`	`24`
`25`		-error: calling `Rc::new` in `vec![elem; len]`
	`25`	+error: initializing a reference-counted pointer in `vec![elem; len]`
`26`	`26`	`--> $DIR/rc.rs:16:21`
`27`	`27`	`\|`
`28`	`28`	`LL \| let v = vec![Rc::new("x".to_string()); 2];`
@@ -33,19 +33,19 @@ help: consider initializing each `Rc` element individually
`33`	`33`	`\|`
`34`	`34`	`LL ~ let v = {`
`35`	`35`	`LL + let mut v = Vec::with_capacity(2);`
`36`		`-LL + (0..2).for_each(\|_\| v.push(Rc::new("x".to_string())));`
	`36`	`+LL + (0..2).for_each(\|_\| v.push(Rc::new(..)));`
`37`	`37`	`LL + v`
`38`	`38`	`LL ~ };`
`39`	`39`	`\|`
`40`	`40`	help: or if this is intentional, consider extracting the `Rc` initialization to a variable
`41`	`41`	`\|`
`42`	`42`	`LL ~ let v = {`
`43`		`-LL + let data = Rc::new("x".to_string());`
	`43`	`+LL + let data = Rc::new(..);`
`44`	`44`	`LL + vec![data; 2]`
`45`	`45`	`LL ~ };`
`46`	`46`	`\|`
`47`	`47`
`48`		-error: calling `Rc::new` in `vec![elem; len]`
	`48`	+error: initializing a reference-counted pointer in `vec![elem; len]`
`49`	`49`	`--> $DIR/rc.rs:22:13`
`50`	`50`	`\|`
`51`	`51`	`LL \| let v = vec![`
`@@ -75,7 +75,7 @@ LL + vec![data; 2]`
`75`	`75`	`LL ~ };`
`76`	`76`	`\|`
`77`	`77`
`78`		-error: calling `Rc::new` in `vec![elem; len]`
	`78`	+error: initializing a reference-counted pointer in `vec![elem; len]`
`79`	`79`	`--> $DIR/rc.rs:31:14`
`80`	`80`	`\|`
`81`	`81`	`LL \| let v1 = vec![`