Auto merge of rust-lang#8814 - yonip23:add-suggestion-to-rc-clone-in-vec-init, r=xFrednet

bors · bors · commit d901079ca117 · 2022-05-17T05:46:00.000Z
add suggestions to rc_clone_in_vec_init A followup to rust-lang/rust-clippy#8769 I also switch the order of the 2 suggestions, since the loop initialization one is probably the common case. `@xFrednet` I'm not letting you guys rest for a minute 😅 changelog: add suggestions to [`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
@@ -1,11 +1,13 @@
 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 clippy_utils::macros::root_macro_call_first_node;
+use clippy_utils::source::{indent_of, snippet};
+use rustc_errors::Applicability;
 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};
+use rustc_span::{sym, Span, Symbol};
 
 declare_clippy_lint! {
     /// ### What it does
@@ -47,27 +49,76 @@ 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(VecArgs::Repeat(elem, len)) = VecArgs::hir(cx, expr) else { return; };
         let Some(symbol) = new_reference_call(cx, elem) else { return; };
 
-        emit_lint(cx, symbol, &macro_call);
+        emit_lint(cx, symbol, macro_call.span, elem, len);
     }
 }
 
-fn emit_lint(cx: &LateContext<'_>, symbol: Symbol, macro_call: &MacroCall) {
+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#"{{
+{indent}    let mut v = Vec::with_capacity({len});
+{indent}    (0..{len}).for_each(|_| v.push({elem}));
+{indent}    v
+{indent}}}"#
+    )
+}
+
+fn extract_suggestion(elem: &str, len: &str, indent: &str) -> String {
+    format!(
+        "{{
+{indent}    let data = {elem};
+{indent}    vec![data; {len}]
+{indent}}}"
+    )
+}
+
+fn emit_lint(cx: &LateContext<'_>, symbol: Symbol, lint_span: Span, elem: &Expr<'_>, len: &Expr<'_>) {
     let symbol_name = symbol.as_str();
 
     span_lint_and_then(
         cx,
         RC_CLONE_IN_VEC_INIT,
-        macro_call.span,
+        lint_span,
         &format!("calling `{symbol_name}::new` in `vec![elem; len]`"),
         |diag| {
+            let len_snippet = snippet(cx, len.span, "..");
+            let elem_snippet = elem_snippet(cx, elem, symbol_name);
+            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);
+
             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");
+            diag.span_suggestion(
+                lint_span,
+                format!("consider initializing each `{symbol_name}` element individually"),
+                loop_init_suggestion,
+                Applicability::Unspecified,
+            );
+            diag.span_suggestion(
+                lint_span,
+                format!(
+                    "or if this is intentional, consider extracting the `{symbol_name}` initialization to a variable"
+                ),
+                extract_suggestion,
+                Applicability::Unspecified,
+            );
         },
     );
 }
diff --git a/tests/ui/rc_clone_in_vec_init/arc.rs b/tests/ui/rc_clone_in_vec_init/arc.rs
@@ -7,6 +7,16 @@ fn should_warn_simple_case() {
     let v = vec![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::new("x".to_string()); 2];
+        }
+    }
+}
+
 fn should_warn_complex_case() {
     let v = vec![
         std::sync::Arc::new(Mutex::new({
@@ -16,6 +26,15 @@ fn should_warn_complex_case() {
         }));
         2
     ];
+
+    let v1 = vec![
+        Arc::new(Mutex::new({
+            let x = 1;
+            dbg!(x);
+            x
+        }));
+        2
+    ];
 }
 
 fn should_not_warn_custom_arc() {
diff --git a/tests/ui/rc_clone_in_vec_init/arc.stderr b/tests/ui/rc_clone_in_vec_init/arc.stderr
@@ -6,11 +6,47 @@ 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
+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 +         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 +         vec![data; 2]
+LL ~     };
+   |
+
+error: calling `Arc::new` in `vec![elem; len]`
+  --> $DIR/arc.rs:15:21
+   |
+LL |             let v = vec![Arc::new("x".to_string()); 2];
+   |                     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+   |
+   = note: each element will point to the same `Arc` instance
+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 +                 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 +                 vec![data; 2]
+LL ~             };
+   |
 
 error: calling `Arc::new` in `vec![elem; len]`
-  --> $DIR/arc.rs:11:13
+  --> $DIR/arc.rs:21:13
    |
 LL |       let v = vec![
    |  _____________^
@@ -23,8 +59,51 @@ 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
+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(std::sync::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 = std::sync::Arc::new(..);
+LL +         vec![data; 2]
+LL ~     };
+   |
+
+error: calling `Arc::new` in `vec![elem; len]`
+  --> $DIR/arc.rs:30:14
+   |
+LL |       let v1 = vec![
+   |  ______________^
+LL | |         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: consider initializing each `Arc` element individually
+   |
+LL ~     let v1 = {
+LL +         let mut v = Vec::with_capacity(2);
+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 v1 = {
+LL +         let data = Arc::new(..);
+LL +         vec![data; 2]
+LL ~     };
+   |
 
-error: aborting due to 2 previous errors
+error: aborting due to 4 previous errors
 
diff --git a/tests/ui/rc_clone_in_vec_init/rc.rs b/tests/ui/rc_clone_in_vec_init/rc.rs
@@ -8,6 +8,16 @@ fn should_warn_simple_case() {
     let v = vec![Rc::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![Rc::new("x".to_string()); 2];
+        }
+    }
+}
+
 fn should_warn_complex_case() {
     let v = vec![
         std::rc::Rc::new(Mutex::new({
@@ -17,6 +27,15 @@ fn should_warn_complex_case() {
         }));
         2
     ];
+
+    let v1 = vec![
+        Rc::new(Mutex::new({
+            let x = 1;
+            dbg!(x);
+            x
+        }));
+        2
+    ];
 }
 
 fn should_not_warn_custom_arc() {
diff --git a/tests/ui/rc_clone_in_vec_init/rc.stderr b/tests/ui/rc_clone_in_vec_init/rc.stderr
@@ -6,11 +6,47 @@ 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
+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 +         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 +         vec![data; 2]
+LL ~     };
+   |
+
+error: calling `Rc::new` in `vec![elem; len]`
+  --> $DIR/rc.rs:16:21
+   |
+LL |             let v = vec![Rc::new("x".to_string()); 2];
+   |                     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+   |
+   = note: each element will point to the same `Rc` instance
+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 +                 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 +                 vec![data; 2]
+LL ~             };
+   |
 
 error: calling `Rc::new` in `vec![elem; len]`
-  --> $DIR/rc.rs:12:13
+  --> $DIR/rc.rs:22:13
    |
 LL |       let v = vec![
    |  _____________^
@@ -23,8 +59,51 @@ 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
+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(std::rc::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 = std::rc::Rc::new(..);
+LL +         vec![data; 2]
+LL ~     };
+   |
+
+error: calling `Rc::new` in `vec![elem; len]`
+  --> $DIR/rc.rs:31:14
+   |
+LL |       let v1 = vec![
+   |  ______________^
+LL | |         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: consider initializing each `Rc` element individually
+   |
+LL ~     let v1 = {
+LL +         let mut v = Vec::with_capacity(2);
+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 v1 = {
+LL +         let data = Rc::new(..);
+LL +         vec![data; 2]
+LL ~     };
+   |
 
-error: aborting due to 2 previous errors
+error: aborting due to 4 previous errors
 
