Skip to content

Commit d901079

Browse files
borsbors
committed
Auto merge of #8814 - yonip23:add-suggestion-to-rc-clone-in-vec-init, r=xFrednet
add suggestions to rc_clone_in_vec_init A followup to #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`]
2 parents 219d702 + ed3744b commit d901079

File tree

5 files changed

+269
-22
lines changed

5 files changed

+269
-22
lines changed

‎clippy_lints/src/rc_clone_in_vec_init.rs‎

Lines changed: 61 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -1,11 +1,13 @@
11
use clippy_utils::diagnostics::span_lint_and_then;
22
use clippy_utils::higher::VecArgs;
33
use clippy_utils::last_path_segment;
4-
use clippy_utils::macros::{root_macro_call_first_node, MacroCall};
4+
use clippy_utils::macros::root_macro_call_first_node;
5+
use clippy_utils::source::{indent_of, snippet};
6+
use rustc_errors::Applicability;
57
use rustc_hir::{Expr, ExprKind, QPath, TyKind};
68
use rustc_lint::{LateContext, LateLintPass};
79
use rustc_session::{declare_lint_pass, declare_tool_lint};
8-
use rustc_span::{sym, Symbol};
10+
use rustc_span::{sym, Span, Symbol};
911

1012
declare_clippy_lint! {
1113
/// ### What it does
@@ -47,27 +49,76 @@ declare_lint_pass!(RcCloneInVecInit => [RC_CLONE_IN_VEC_INIT]);
4749
impl LateLintPass<'_> for RcCloneInVecInit {
4850
fn check_expr(&mut self, cx: &LateContext<'_>, expr: &Expr<'_>) {
4951
let Some(macro_call) = root_macro_call_first_node(cx, expr) else { return; };
50-
let Some(VecArgs::Repeat(elem, _)) = VecArgs::hir(cx, expr) else { return; };
52+
let Some(VecArgs::Repeat(elem, len)) = VecArgs::hir(cx, expr) else { return; };
5153
let Some(symbol) = new_reference_call(cx, elem) else { return; };
5254

53-
emit_lint(cx, symbol, &macro_call);
55+
emit_lint(cx, symbol, macro_call.span, elem, len);
5456
}
5557
}
5658

57-
fn emit_lint(cx: &LateContext<'_>, symbol: Symbol, macro_call: &MacroCall) {
59+
fn elem_snippet(cx: &LateContext<'_>, elem: &Expr<'_>, symbol_name: &str) -> String {
60+
let elem_snippet = snippet(cx, elem.span, "..").to_string();
61+
if elem_snippet.contains('\n') {
62+
// This string must be found in `elem_snippet`, otherwise we won't be constructing
63+
// the snippet in the first place.
64+
let reference_creation = format!("{symbol_name}::new");
65+
let (code_until_reference_creation, _right) = elem_snippet.split_once(&reference_creation).unwrap();
66+
67+
return format!("{code_until_reference_creation}{reference_creation}(..)");
68+
}
69+
70+
elem_snippet
71+
}
72+
73+
fn loop_init_suggestion(elem: &str, len: &str, indent: &str) -> String {
74+
format!(
75+
r#"{{
76+
{indent} let mut v = Vec::with_capacity({len});
77+
{indent} (0..{len}).for_each(|_| v.push({elem}));
78+
{indent} v
79+
{indent}}}"#
80+
)
81+
}
82+
83+
fn extract_suggestion(elem: &str, len: &str, indent: &str) -> String {
84+
format!(
85+
"{{
86+
{indent} let data = {elem};
87+
{indent} vec![data; {len}]
88+
{indent}}}"
89+
)
90+
}
91+
92+
fn emit_lint(cx: &LateContext<'_>, symbol: Symbol, lint_span: Span, elem: &Expr<'_>, len: &Expr<'_>) {
5893
let symbol_name = symbol.as_str();
5994

6095
span_lint_and_then(
6196
cx,
6297
RC_CLONE_IN_VEC_INIT,
63-
macro_call.span,
98+
lint_span,
6499
&format!("calling `{symbol_name}::new` in `vec![elem; len]`"),
65100
|diag| {
101+
let len_snippet = snippet(cx, len.span, "..");
102+
let elem_snippet = elem_snippet(cx, elem, symbol_name);
103+
let indentation = " ".repeat(indent_of(cx, lint_span).unwrap_or(0));
104+
let loop_init_suggestion = loop_init_suggestion(&elem_snippet, len_snippet.as_ref(), &indentation);
105+
let extract_suggestion = extract_suggestion(&elem_snippet, len_snippet.as_ref(), &indentation);
106+
66107
diag.note(format!("each element will point to the same `{symbol_name}` instance"));
67-
diag.help(format!(
68-
"if this is intentional, consider extracting the `{symbol_name}` initialization to a variable"
69-
));
70-
diag.help("or if not, initialize each element individually");
108+
diag.span_suggestion(
109+
lint_span,
110+
format!("consider initializing each `{symbol_name}` element individually"),
111+
loop_init_suggestion,
112+
Applicability::Unspecified,
113+
);
114+
diag.span_suggestion(
115+
lint_span,
116+
format!(
117+
"or if this is intentional, consider extracting the `{symbol_name}` initialization to a variable"
118+
),
119+
extract_suggestion,
120+
Applicability::Unspecified,
121+
);
71122
},
72123
);
73124
}

‎tests/ui/rc_clone_in_vec_init/arc.rs‎

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,16 @@ fn should_warn_simple_case() {
77
let v = vec![Arc::new("x".to_string()); 2];
88
}
99

10+
fn should_warn_simple_case_with_big_indentation() {
11+
if true {
12+
let k = 1;
13+
dbg!(k);
14+
if true {
15+
let v = vec![Arc::new("x".to_string()); 2];
16+
}
17+
}
18+
}
19+
1020
fn should_warn_complex_case() {
1121
let v = vec![
1222
std::sync::Arc::new(Mutex::new({
@@ -16,6 +26,15 @@ fn should_warn_complex_case() {
1626
}));
1727
2
1828
];
29+
30+
let v1 = vec![
31+
Arc::new(Mutex::new({
32+
let x = 1;
33+
dbg!(x);
34+
x
35+
}));
36+
2
37+
];
1938
}
2039

2140
fn should_not_warn_custom_arc() {

‎tests/ui/rc_clone_in_vec_init/arc.stderr‎

Lines changed: 85 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -6,11 +6,47 @@ LL | let v = vec![Arc::new("x".to_string()); 2];
66
|
77
= note: `-D clippy::rc-clone-in-vec-init` implied by `-D warnings`
88
= note: each element will point to the same `Arc` instance
9-
= help: if this is intentional, consider extracting the `Arc` initialization to a variable
10-
= help: or if not, initialize each element individually
9+
help: consider initializing each `Arc` element individually
10+
|
11+
LL ~ let v = {
12+
LL + let mut v = Vec::with_capacity(2);
13+
LL + (0..2).for_each(|_| v.push(Arc::new("x".to_string())));
14+
LL + v
15+
LL ~ };
16+
|
17+
help: or if this is intentional, consider extracting the `Arc` initialization to a variable
18+
|
19+
LL ~ let v = {
20+
LL + let data = Arc::new("x".to_string());
21+
LL + vec![data; 2]
22+
LL ~ };
23+
|
24+
25+
error: calling `Arc::new` in `vec![elem; len]`
26+
--> $DIR/arc.rs:15:21
27+
|
28+
LL | let v = vec![Arc::new("x".to_string()); 2];
29+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
30+
|
31+
= note: each element will point to the same `Arc` instance
32+
help: consider initializing each `Arc` element individually
33+
|
34+
LL ~ let v = {
35+
LL + let mut v = Vec::with_capacity(2);
36+
LL + (0..2).for_each(|_| v.push(Arc::new("x".to_string())));
37+
LL + v
38+
LL ~ };
39+
|
40+
help: or if this is intentional, consider extracting the `Arc` initialization to a variable
41+
|
42+
LL ~ let v = {
43+
LL + let data = Arc::new("x".to_string());
44+
LL + vec![data; 2]
45+
LL ~ };
46+
|
1147

1248
error: calling `Arc::new` in `vec![elem; len]`
13-
--> $DIR/arc.rs:11:13
49+
--> $DIR/arc.rs:21:13
1450
|
1551
LL | let v = vec![
1652
| _____________^
@@ -23,8 +59,51 @@ LL | | ];
2359
| |_____^
2460
|
2561
= note: each element will point to the same `Arc` instance
26-
= help: if this is intentional, consider extracting the `Arc` initialization to a variable
27-
= help: or if not, initialize each element individually
62+
help: consider initializing each `Arc` element individually
63+
|
64+
LL ~ let v = {
65+
LL + let mut v = Vec::with_capacity(2);
66+
LL + (0..2).for_each(|_| v.push(std::sync::Arc::new(..)));
67+
LL + v
68+
LL ~ };
69+
|
70+
help: or if this is intentional, consider extracting the `Arc` initialization to a variable
71+
|
72+
LL ~ let v = {
73+
LL + let data = std::sync::Arc::new(..);
74+
LL + vec![data; 2]
75+
LL ~ };
76+
|
77+
78+
error: calling `Arc::new` in `vec![elem; len]`
79+
--> $DIR/arc.rs:30:14
80+
|
81+
LL | let v1 = vec![
82+
| ______________^
83+
LL | | Arc::new(Mutex::new({
84+
LL | | let x = 1;
85+
LL | | dbg!(x);
86+
... |
87+
LL | | 2
88+
LL | | ];
89+
| |_____^
90+
|
91+
= note: each element will point to the same `Arc` instance
92+
help: consider initializing each `Arc` element individually
93+
|
94+
LL ~ let v1 = {
95+
LL + let mut v = Vec::with_capacity(2);
96+
LL + (0..2).for_each(|_| v.push(Arc::new(..)));
97+
LL + v
98+
LL ~ };
99+
|
100+
help: or if this is intentional, consider extracting the `Arc` initialization to a variable
101+
|
102+
LL ~ let v1 = {
103+
LL + let data = Arc::new(..);
104+
LL + vec![data; 2]
105+
LL ~ };
106+
|
28107

29-
error: aborting due to 2 previous errors
108+
error: aborting due to 4 previous errors
30109

‎tests/ui/rc_clone_in_vec_init/rc.rs‎

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,16 @@ fn should_warn_simple_case() {
88
let v = vec![Rc::new("x".to_string()); 2];
99
}
1010

11+
fn should_warn_simple_case_with_big_indentation() {
12+
if true {
13+
let k = 1;
14+
dbg!(k);
15+
if true {
16+
let v = vec![Rc::new("x".to_string()); 2];
17+
}
18+
}
19+
}
20+
1121
fn should_warn_complex_case() {
1222
let v = vec![
1323
std::rc::Rc::new(Mutex::new({
@@ -17,6 +27,15 @@ fn should_warn_complex_case() {
1727
}));
1828
2
1929
];
30+
31+
let v1 = vec![
32+
Rc::new(Mutex::new({
33+
let x = 1;
34+
dbg!(x);
35+
x
36+
}));
37+
2
38+
];
2039
}
2140

2241
fn should_not_warn_custom_arc() {

‎tests/ui/rc_clone_in_vec_init/rc.stderr‎

Lines changed: 85 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -6,11 +6,47 @@ LL | let v = vec![Rc::new("x".to_string()); 2];
66
|
77
= note: `-D clippy::rc-clone-in-vec-init` implied by `-D warnings`
88
= note: each element will point to the same `Rc` instance
9-
= help: if this is intentional, consider extracting the `Rc` initialization to a variable
10-
= help: or if not, initialize each element individually
9+
help: consider initializing each `Rc` element individually
10+
|
11+
LL ~ let v = {
12+
LL + let mut v = Vec::with_capacity(2);
13+
LL + (0..2).for_each(|_| v.push(Rc::new("x".to_string())));
14+
LL + v
15+
LL ~ };
16+
|
17+
help: or if this is intentional, consider extracting the `Rc` initialization to a variable
18+
|
19+
LL ~ let v = {
20+
LL + let data = Rc::new("x".to_string());
21+
LL + vec![data; 2]
22+
LL ~ };
23+
|
24+
25+
error: calling `Rc::new` in `vec![elem; len]`
26+
--> $DIR/rc.rs:16:21
27+
|
28+
LL | let v = vec![Rc::new("x".to_string()); 2];
29+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
30+
|
31+
= note: each element will point to the same `Rc` instance
32+
help: consider initializing each `Rc` element individually
33+
|
34+
LL ~ let v = {
35+
LL + let mut v = Vec::with_capacity(2);
36+
LL + (0..2).for_each(|_| v.push(Rc::new("x".to_string())));
37+
LL + v
38+
LL ~ };
39+
|
40+
help: or if this is intentional, consider extracting the `Rc` initialization to a variable
41+
|
42+
LL ~ let v = {
43+
LL + let data = Rc::new("x".to_string());
44+
LL + vec![data; 2]
45+
LL ~ };
46+
|
1147

1248
error: calling `Rc::new` in `vec![elem; len]`
13-
--> $DIR/rc.rs:12:13
49+
--> $DIR/rc.rs:22:13
1450
|
1551
LL | let v = vec![
1652
| _____________^
@@ -23,8 +59,51 @@ LL | | ];
2359
| |_____^
2460
|
2561
= note: each element will point to the same `Rc` instance
26-
= help: if this is intentional, consider extracting the `Rc` initialization to a variable
27-
= help: or if not, initialize each element individually
62+
help: consider initializing each `Rc` element individually
63+
|
64+
LL ~ let v = {
65+
LL + let mut v = Vec::with_capacity(2);
66+
LL + (0..2).for_each(|_| v.push(std::rc::Rc::new(..)));
67+
LL + v
68+
LL ~ };
69+
|
70+
help: or if this is intentional, consider extracting the `Rc` initialization to a variable
71+
|
72+
LL ~ let v = {
73+
LL + let data = std::rc::Rc::new(..);
74+
LL + vec![data; 2]
75+
LL ~ };
76+
|
77+
78+
error: calling `Rc::new` in `vec![elem; len]`
79+
--> $DIR/rc.rs:31:14
80+
|
81+
LL | let v1 = vec![
82+
| ______________^
83+
LL | | Rc::new(Mutex::new({
84+
LL | | let x = 1;
85+
LL | | dbg!(x);
86+
... |
87+
LL | | 2
88+
LL | | ];
89+
| |_____^
90+
|
91+
= note: each element will point to the same `Rc` instance
92+
help: consider initializing each `Rc` element individually
93+
|
94+
LL ~ let v1 = {
95+
LL + let mut v = Vec::with_capacity(2);
96+
LL + (0..2).for_each(|_| v.push(Rc::new(..)));
97+
LL + v
98+
LL ~ };
99+
|
100+
help: or if this is intentional, consider extracting the `Rc` initialization to a variable
101+
|
102+
LL ~ let v1 = {
103+
LL + let data = Rc::new(..);
104+
LL + vec![data; 2]
105+
LL ~ };
106+
|
28107

29-
error: aborting due to 2 previous errors
108+
error: aborting due to 4 previous errors
30109

0 commit comments

Comments
 (0)