Skip to content

Commit 2a28347

Browse files
authored
Fix: fixed multipart_suggestion in index_refutable_slice uitest (#13727)
This should address #13099 for the derivable_impls test. As this combines everything into a single multipart_suggestion, the feedback message is a little less "targeted" than it was before, but now it provides a complete`--fix`able suggestion - e.g.: ``` error: this binding can be a slice pattern to avoid indexing --> tests/ui-toml/max_suggested_slice_pattern_length/index_refutable_slice.rs:5:17 | LL | if let Some(slice) = slice { | ^^^^^ | note: the lint level is defined here --> tests/ui-toml/max_suggested_slice_pattern_length/index_refutable_slice.rs:1:9 | LL | #![deny(clippy::index_refutable_slice)] | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace the binding and indexed access with a slice pattern | LL ~ if let Some([_, _, _, _, _, _, _, slice_7, ..]) = slice { LL | LL | // This would usually not be linted but is included now due to the LL | // index limit in the config file LL ~ println!("{}", slice_7); | ``` changelog: [index_refutable_slice]: Fixed multipart_suggestions to provide correct rustfix-able lint
2 parents 6a3ef93 + e493664 commit 2a28347

10 files changed

+311
-110
lines changed

clippy_lints/src/index_refutable_slice.rs

+15-21
Original file line numberDiff line numberDiff line change
@@ -135,7 +135,7 @@ fn lint_slice(cx: &LateContext<'_>, slice: &SliceLintInformation) {
135135
.map(|(index, _)| *index)
136136
.collect::<FxIndexSet<_>>();
137137

138-
let value_name = |index| format!("{}_{index}", slice.ident.name);
138+
let value_name = |index| format!("{}_{}", slice.ident.name, index);
139139

140140
if let Some(max_index) = used_indices.iter().max() {
141141
let opt_ref = if slice.needs_ref { "ref " } else { "" };
@@ -150,35 +150,29 @@ fn lint_slice(cx: &LateContext<'_>, slice: &SliceLintInformation) {
150150
.collect::<Vec<_>>();
151151
let pat_sugg = format!("[{}, ..]", pat_sugg_idents.join(", "));
152152

153+
let mut suggestions = Vec::new();
154+
155+
// Add the binding pattern suggestion
156+
if !slice.pattern_spans.is_empty() {
157+
suggestions.extend(slice.pattern_spans.iter().map(|span| (*span, pat_sugg.clone())));
158+
}
159+
160+
// Add the index replacement suggestions
161+
if !slice.index_use.is_empty() {
162+
suggestions.extend(slice.index_use.iter().map(|(index, span)| (*span, value_name(*index))));
163+
}
164+
153165
span_lint_and_then(
154166
cx,
155167
INDEX_REFUTABLE_SLICE,
156168
slice.ident.span,
157169
"this binding can be a slice pattern to avoid indexing",
158170
|diag| {
159171
diag.multipart_suggestion(
160-
"try using a slice pattern here",
161-
slice
162-
.pattern_spans
163-
.iter()
164-
.map(|span| (*span, pat_sugg.clone()))
165-
.collect(),
172+
"replace the binding and indexed access with a slice pattern",
173+
suggestions,
166174
Applicability::MaybeIncorrect,
167175
);
168-
169-
diag.multipart_suggestion(
170-
"and replace the index expressions here",
171-
slice
172-
.index_use
173-
.iter()
174-
.map(|(index, span)| (*span, value_name(*index)))
175-
.collect(),
176-
Applicability::MaybeIncorrect,
177-
);
178-
179-
// The lint message doesn't contain a warning about the removed index expression,
180-
// since `filter_lintable_slices` will only return slices where all access indices
181-
// are known at compile time. Therefore, they can be removed without side effects.
182176
},
183177
);
184178
}
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,24 @@
1+
#![deny(clippy::index_refutable_slice)]
2+
3+
fn below_limit() {
4+
let slice: Option<&[u32]> = Some(&[1, 2, 3]);
5+
if let Some([_, _, _, _, _, _, _, slice_7, ..]) = slice {
6+
//~^ ERROR: binding can be a slice pattern
7+
// This would usually not be linted but is included now due to the
8+
// index limit in the config file
9+
println!("{}", slice_7);
10+
}
11+
}
12+
13+
fn above_limit() {
14+
let slice: Option<&[u32]> = Some(&[1, 2, 3]);
15+
if let Some(slice) = slice {
16+
// This will not be linted as 8 is above the limit
17+
println!("{}", slice[8]);
18+
}
19+
}
20+
21+
fn main() {
22+
below_limit();
23+
above_limit();
24+
}

tests/ui-toml/max_suggested_slice_pattern_length/index_refutable_slice.rs

-2
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,5 @@
11
#![deny(clippy::index_refutable_slice)]
22

3-
//@no-rustfix: need to change the suggestion to a multipart suggestion
4-
53
fn below_limit() {
64
let slice: Option<&[u32]> = Some(&[1, 2, 3]);
75
if let Some(slice) = slice {
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
error: this binding can be a slice pattern to avoid indexing
2-
--> tests/ui-toml/max_suggested_slice_pattern_length/index_refutable_slice.rs:7:17
2+
--> tests/ui-toml/max_suggested_slice_pattern_length/index_refutable_slice.rs:5:17
33
|
44
LL | if let Some(slice) = slice {
55
| ^^^^^
@@ -9,14 +9,14 @@ note: the lint level is defined here
99
|
1010
LL | #![deny(clippy::index_refutable_slice)]
1111
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
12-
help: try using a slice pattern here
12+
help: replace the binding and indexed access with a slice pattern
1313
|
14-
LL | if let Some([_, _, _, _, _, _, _, slice_7, ..]) = slice {
15-
| ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
16-
help: and replace the index expressions here
14+
LL ~ if let Some([_, _, _, _, _, _, _, slice_7, ..]) = slice {
15+
LL |
16+
LL | // This would usually not be linted but is included now due to the
17+
LL | // index limit in the config file
18+
LL ~ println!("{}", slice_7);
1719
|
18-
LL | println!("{}", slice_7);
19-
| ~~~~~~~
2020

2121
error: aborting due to 1 previous error
2222

Original file line numberDiff line numberDiff line change
@@ -0,0 +1,177 @@
1+
#![deny(clippy::index_refutable_slice)]
2+
#![allow(clippy::uninlined_format_args, clippy::needless_lifetimes)]
3+
4+
enum SomeEnum<T> {
5+
One(T),
6+
Two(T),
7+
Three(T),
8+
Four(T),
9+
}
10+
11+
fn lintable_examples() {
12+
// Try with reference
13+
let slice: Option<&[u32]> = Some(&[1, 2, 3]);
14+
if let Some([slice_0, ..]) = slice {
15+
//~^ ERROR: this binding can be a slice pattern to avoid indexing
16+
println!("{}", slice_0);
17+
}
18+
19+
// Try with copy
20+
let slice: Option<[u32; 3]> = Some([1, 2, 3]);
21+
if let Some([slice_0, ..]) = slice {
22+
//~^ ERROR: this binding can be a slice pattern to avoid indexing
23+
println!("{}", slice_0);
24+
}
25+
26+
// Try with long slice and small indices
27+
let slice: Option<[u32; 9]> = Some([1, 2, 3, 4, 5, 6, 7, 8, 9]);
28+
if let Some([slice_0, _, slice_2, ..]) = slice {
29+
//~^ ERROR: this binding can be a slice pattern to avoid indexing
30+
println!("{}", slice_2);
31+
println!("{}", slice_0);
32+
}
33+
34+
// Multiple bindings
35+
let slice_wrapped: SomeEnum<[u32; 3]> = SomeEnum::One([5, 6, 7]);
36+
if let SomeEnum::One([slice_0, ..]) | SomeEnum::Three([slice_0, ..]) = slice_wrapped {
37+
//~^ ERROR: this binding can be a slice pattern to avoid indexing
38+
println!("{}", slice_0);
39+
}
40+
41+
// Two lintable slices in one if let
42+
let a_wrapped: SomeEnum<[u32; 3]> = SomeEnum::One([9, 5, 1]);
43+
let b_wrapped: Option<[u32; 2]> = Some([4, 6]);
44+
if let (SomeEnum::Three([_, _, a_2, ..]), Some([_, b_1, ..])) = (a_wrapped, b_wrapped) {
45+
//~^ ERROR: this binding can be a slice pattern to avoid indexing
46+
//~| ERROR: this binding can be a slice pattern to avoid indexing
47+
println!("{} -> {}", a_2, b_1);
48+
}
49+
50+
// This requires the slice values to be borrowed as the slice values can only be
51+
// borrowed and `String` doesn't implement copy
52+
let slice: Option<[String; 2]> = Some([String::from("1"), String::from("2")]);
53+
if let Some([_, ref slice_1, ..]) = slice {
54+
//~^ ERROR: this binding can be a slice pattern to avoid indexing
55+
println!("{:?}", slice_1);
56+
}
57+
println!("{:?}", slice);
58+
59+
// This should not suggest using the `ref` keyword as the scrutinee is already
60+
// a reference
61+
let slice: Option<[String; 2]> = Some([String::from("1"), String::from("2")]);
62+
if let Some([slice_0, ..]) = &slice {
63+
//~^ ERROR: this binding can be a slice pattern to avoid indexing
64+
println!("{:?}", slice_0);
65+
}
66+
println!("{:?}", slice);
67+
}
68+
69+
fn slice_index_above_limit() {
70+
let slice: Option<&[u32]> = Some(&[1, 2, 3]);
71+
72+
if let Some(slice) = slice {
73+
// Would cause a panic, IDK
74+
println!("{}", slice[7]);
75+
}
76+
}
77+
78+
fn slice_is_used() {
79+
let slice: Option<&[u32]> = Some(&[1, 2, 3]);
80+
if let Some(slice) = slice {
81+
println!("{:?}", slice.len());
82+
}
83+
84+
let slice: Option<&[u32]> = Some(&[1, 2, 3]);
85+
if let Some(slice) = slice {
86+
println!("{:?}", slice.to_vec());
87+
}
88+
89+
let opt: Option<[String; 2]> = Some([String::from("Hello"), String::from("world")]);
90+
if let Some(slice) = opt {
91+
if !slice.is_empty() {
92+
println!("first: {}", slice[0]);
93+
}
94+
}
95+
}
96+
97+
/// The slice is used by an external function and should therefore not be linted
98+
fn check_slice_as_arg() {
99+
fn is_interesting<T>(slice: &[T; 2]) -> bool {
100+
!slice.is_empty()
101+
}
102+
103+
let slice_wrapped: Option<[String; 2]> = Some([String::from("Hello"), String::from("world")]);
104+
if let Some(slice) = &slice_wrapped {
105+
if is_interesting(slice) {
106+
println!("This is interesting {}", slice[0]);
107+
}
108+
}
109+
println!("{:?}", slice_wrapped);
110+
}
111+
112+
fn check_slice_in_struct() {
113+
#[derive(Debug)]
114+
struct Wrapper<'a> {
115+
inner: Option<&'a [String]>,
116+
is_awesome: bool,
117+
}
118+
119+
impl<'a> Wrapper<'a> {
120+
fn is_super_awesome(&self) -> bool {
121+
self.is_awesome
122+
}
123+
}
124+
125+
let inner = &[String::from("New"), String::from("World")];
126+
let wrap = Wrapper {
127+
inner: Some(inner),
128+
is_awesome: true,
129+
};
130+
131+
// Test 1: Field access
132+
if let Some([slice_0, ..]) = wrap.inner {
133+
//~^ ERROR: this binding can be a slice pattern to avoid indexing
134+
if wrap.is_awesome {
135+
println!("This is awesome! {}", slice_0);
136+
}
137+
}
138+
139+
// Test 2: function access
140+
if let Some([slice_0, ..]) = wrap.inner {
141+
//~^ ERROR: this binding can be a slice pattern to avoid indexing
142+
if wrap.is_super_awesome() {
143+
println!("This is super awesome! {}", slice_0);
144+
}
145+
}
146+
println!("Complete wrap: {:?}", wrap);
147+
}
148+
149+
/// This would be a nice additional feature to have in the future, but adding it
150+
/// now would make the PR too large. This is therefore only a test that we don't
151+
/// lint cases we can't make a reasonable suggestion for
152+
fn mutable_slice_index() {
153+
// Mut access
154+
let mut slice: Option<[String; 1]> = Some([String::from("Penguin")]);
155+
if let Some(ref mut slice) = slice {
156+
slice[0] = String::from("Mr. Penguin");
157+
}
158+
println!("Use after modification: {:?}", slice);
159+
160+
// Mut access on reference
161+
let mut slice: Option<[String; 1]> = Some([String::from("Cat")]);
162+
if let Some(slice) = &mut slice {
163+
slice[0] = String::from("Lord Meow Meow");
164+
}
165+
println!("Use after modification: {:?}", slice);
166+
}
167+
168+
/// The lint will ignore bindings with sub patterns as it would be hard
169+
/// to build correct suggestions for these instances :)
170+
fn binding_with_sub_pattern() {
171+
let slice: Option<&[u32]> = Some(&[1, 2, 3]);
172+
if let Some(slice @ [_, _, _]) = slice {
173+
println!("{:?}", slice[2]);
174+
}
175+
}
176+
177+
fn main() {}

tests/ui/index_refutable_slice/if_let_slice_binding.rs

-2
Original file line numberDiff line numberDiff line change
@@ -1,8 +1,6 @@
11
#![deny(clippy::index_refutable_slice)]
22
#![allow(clippy::uninlined_format_args, clippy::needless_lifetimes)]
33

4-
//@no-rustfix: need to change the suggestion to a multipart suggestion
5-
64
enum SomeEnum<T> {
75
One(T),
86
Two(T),

0 commit comments

Comments
 (0)