diff --git a/clippy_lints/src/index_refutable_slice.rs b/clippy_lints/src/index_refutable_slice.rs index c2030a5ab090..15650c4f7329 100644 --- a/clippy_lints/src/index_refutable_slice.rs +++ b/clippy_lints/src/index_refutable_slice.rs @@ -135,7 +135,7 @@ fn lint_slice(cx: &LateContext<'_>, slice: &SliceLintInformation) { .map(|(index, _)| *index) .collect::>(); - let value_name = |index| format!("{}_{index}", slice.ident.name); + let value_name = |index| format!("{}_{}", slice.ident.name, index); if let Some(max_index) = used_indices.iter().max() { let opt_ref = if slice.needs_ref { "ref " } else { "" }; @@ -150,6 +150,18 @@ fn lint_slice(cx: &LateContext<'_>, slice: &SliceLintInformation) { .collect::>(); let pat_sugg = format!("[{}, ..]", pat_sugg_idents.join(", ")); + let mut suggestions = Vec::new(); + + // Add the binding pattern suggestion + if !slice.pattern_spans.is_empty() { + suggestions.extend(slice.pattern_spans.iter().map(|span| (*span, pat_sugg.clone()))); + } + + // Add the index replacement suggestions + if !slice.index_use.is_empty() { + suggestions.extend(slice.index_use.iter().map(|(index, span)| (*span, value_name(*index)))); + } + span_lint_and_then( cx, INDEX_REFUTABLE_SLICE, @@ -157,28 +169,10 @@ fn lint_slice(cx: &LateContext<'_>, slice: &SliceLintInformation) { "this binding can be a slice pattern to avoid indexing", |diag| { diag.multipart_suggestion( - "try using a slice pattern here", - slice - .pattern_spans - .iter() - .map(|span| (*span, pat_sugg.clone())) - .collect(), + "replace the binding and indexed access with a slice pattern", + suggestions, Applicability::MaybeIncorrect, ); - - diag.multipart_suggestion( - "and replace the index expressions here", - slice - .index_use - .iter() - .map(|(index, span)| (*span, value_name(*index))) - .collect(), - Applicability::MaybeIncorrect, - ); - - // The lint message doesn't contain a warning about the removed index expression, - // since `filter_lintable_slices` will only return slices where all access indices - // are known at compile time. Therefore, they can be removed without side effects. }, ); } diff --git a/tests/ui-toml/max_suggested_slice_pattern_length/index_refutable_slice.fixed b/tests/ui-toml/max_suggested_slice_pattern_length/index_refutable_slice.fixed new file mode 100644 index 000000000000..36540bf1dcf7 --- /dev/null +++ b/tests/ui-toml/max_suggested_slice_pattern_length/index_refutable_slice.fixed @@ -0,0 +1,24 @@ +#![deny(clippy::index_refutable_slice)] + +fn below_limit() { + let slice: Option<&[u32]> = Some(&[1, 2, 3]); + if let Some([_, _, _, _, _, _, _, slice_7, ..]) = slice { + //~^ ERROR: binding can be a slice pattern + // This would usually not be linted but is included now due to the + // index limit in the config file + println!("{}", slice_7); + } +} + +fn above_limit() { + let slice: Option<&[u32]> = Some(&[1, 2, 3]); + if let Some(slice) = slice { + // This will not be linted as 8 is above the limit + println!("{}", slice[8]); + } +} + +fn main() { + below_limit(); + above_limit(); +} diff --git a/tests/ui-toml/max_suggested_slice_pattern_length/index_refutable_slice.rs b/tests/ui-toml/max_suggested_slice_pattern_length/index_refutable_slice.rs index e64c8ff32900..da76bb20fd96 100644 --- a/tests/ui-toml/max_suggested_slice_pattern_length/index_refutable_slice.rs +++ b/tests/ui-toml/max_suggested_slice_pattern_length/index_refutable_slice.rs @@ -1,7 +1,5 @@ #![deny(clippy::index_refutable_slice)] -//@no-rustfix: need to change the suggestion to a multipart suggestion - fn below_limit() { let slice: Option<&[u32]> = Some(&[1, 2, 3]); if let Some(slice) = slice { diff --git a/tests/ui-toml/max_suggested_slice_pattern_length/index_refutable_slice.stderr b/tests/ui-toml/max_suggested_slice_pattern_length/index_refutable_slice.stderr index 3ea600c7d7b0..022deb330e6e 100644 --- a/tests/ui-toml/max_suggested_slice_pattern_length/index_refutable_slice.stderr +++ b/tests/ui-toml/max_suggested_slice_pattern_length/index_refutable_slice.stderr @@ -1,5 +1,5 @@ error: this binding can be a slice pattern to avoid indexing - --> tests/ui-toml/max_suggested_slice_pattern_length/index_refutable_slice.rs:7:17 + --> tests/ui-toml/max_suggested_slice_pattern_length/index_refutable_slice.rs:5:17 | LL | if let Some(slice) = slice { | ^^^^^ @@ -9,14 +9,14 @@ note: the lint level is defined here | LL | #![deny(clippy::index_refutable_slice)] | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ -help: try using a slice pattern here +help: replace the binding and indexed access with a slice pattern | -LL | if let Some([_, _, _, _, _, _, _, slice_7, ..]) = slice { - | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ -help: and replace the index expressions here +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); | -LL | println!("{}", slice_7); - | ~~~~~~~ error: aborting due to 1 previous error diff --git a/tests/ui/index_refutable_slice/if_let_slice_binding.fixed b/tests/ui/index_refutable_slice/if_let_slice_binding.fixed new file mode 100644 index 000000000000..ea8e56e18b08 --- /dev/null +++ b/tests/ui/index_refutable_slice/if_let_slice_binding.fixed @@ -0,0 +1,177 @@ +#![deny(clippy::index_refutable_slice)] +#![allow(clippy::uninlined_format_args, clippy::needless_lifetimes)] + +enum SomeEnum { + One(T), + Two(T), + Three(T), + Four(T), +} + +fn lintable_examples() { + // Try with reference + let slice: Option<&[u32]> = Some(&[1, 2, 3]); + if let Some([slice_0, ..]) = slice { + //~^ ERROR: this binding can be a slice pattern to avoid indexing + println!("{}", slice_0); + } + + // Try with copy + let slice: Option<[u32; 3]> = Some([1, 2, 3]); + if let Some([slice_0, ..]) = slice { + //~^ ERROR: this binding can be a slice pattern to avoid indexing + println!("{}", slice_0); + } + + // Try with long slice and small indices + let slice: Option<[u32; 9]> = Some([1, 2, 3, 4, 5, 6, 7, 8, 9]); + if let Some([slice_0, _, slice_2, ..]) = slice { + //~^ ERROR: this binding can be a slice pattern to avoid indexing + println!("{}", slice_2); + println!("{}", slice_0); + } + + // Multiple bindings + let slice_wrapped: SomeEnum<[u32; 3]> = SomeEnum::One([5, 6, 7]); + if let SomeEnum::One([slice_0, ..]) | SomeEnum::Three([slice_0, ..]) = slice_wrapped { + //~^ ERROR: this binding can be a slice pattern to avoid indexing + println!("{}", slice_0); + } + + // Two lintable slices in one if let + let a_wrapped: SomeEnum<[u32; 3]> = SomeEnum::One([9, 5, 1]); + let b_wrapped: Option<[u32; 2]> = Some([4, 6]); + if let (SomeEnum::Three([_, _, a_2, ..]), Some([_, b_1, ..])) = (a_wrapped, b_wrapped) { + //~^ ERROR: this binding can be a slice pattern to avoid indexing + //~| ERROR: this binding can be a slice pattern to avoid indexing + println!("{} -> {}", a_2, b_1); + } + + // This requires the slice values to be borrowed as the slice values can only be + // borrowed and `String` doesn't implement copy + let slice: Option<[String; 2]> = Some([String::from("1"), String::from("2")]); + if let Some([_, ref slice_1, ..]) = slice { + //~^ ERROR: this binding can be a slice pattern to avoid indexing + println!("{:?}", slice_1); + } + println!("{:?}", slice); + + // This should not suggest using the `ref` keyword as the scrutinee is already + // a reference + let slice: Option<[String; 2]> = Some([String::from("1"), String::from("2")]); + if let Some([slice_0, ..]) = &slice { + //~^ ERROR: this binding can be a slice pattern to avoid indexing + println!("{:?}", slice_0); + } + println!("{:?}", slice); +} + +fn slice_index_above_limit() { + let slice: Option<&[u32]> = Some(&[1, 2, 3]); + + if let Some(slice) = slice { + // Would cause a panic, IDK + println!("{}", slice[7]); + } +} + +fn slice_is_used() { + let slice: Option<&[u32]> = Some(&[1, 2, 3]); + if let Some(slice) = slice { + println!("{:?}", slice.len()); + } + + let slice: Option<&[u32]> = Some(&[1, 2, 3]); + if let Some(slice) = slice { + println!("{:?}", slice.to_vec()); + } + + let opt: Option<[String; 2]> = Some([String::from("Hello"), String::from("world")]); + if let Some(slice) = opt { + if !slice.is_empty() { + println!("first: {}", slice[0]); + } + } +} + +/// The slice is used by an external function and should therefore not be linted +fn check_slice_as_arg() { + fn is_interesting(slice: &[T; 2]) -> bool { + !slice.is_empty() + } + + let slice_wrapped: Option<[String; 2]> = Some([String::from("Hello"), String::from("world")]); + if let Some(slice) = &slice_wrapped { + if is_interesting(slice) { + println!("This is interesting {}", slice[0]); + } + } + println!("{:?}", slice_wrapped); +} + +fn check_slice_in_struct() { + #[derive(Debug)] + struct Wrapper<'a> { + inner: Option<&'a [String]>, + is_awesome: bool, + } + + impl<'a> Wrapper<'a> { + fn is_super_awesome(&self) -> bool { + self.is_awesome + } + } + + let inner = &[String::from("New"), String::from("World")]; + let wrap = Wrapper { + inner: Some(inner), + is_awesome: true, + }; + + // Test 1: Field access + if let Some([slice_0, ..]) = wrap.inner { + //~^ ERROR: this binding can be a slice pattern to avoid indexing + if wrap.is_awesome { + println!("This is awesome! {}", slice_0); + } + } + + // Test 2: function access + if let Some([slice_0, ..]) = wrap.inner { + //~^ ERROR: this binding can be a slice pattern to avoid indexing + if wrap.is_super_awesome() { + println!("This is super awesome! {}", slice_0); + } + } + println!("Complete wrap: {:?}", wrap); +} + +/// This would be a nice additional feature to have in the future, but adding it +/// now would make the PR too large. This is therefore only a test that we don't +/// lint cases we can't make a reasonable suggestion for +fn mutable_slice_index() { + // Mut access + let mut slice: Option<[String; 1]> = Some([String::from("Penguin")]); + if let Some(ref mut slice) = slice { + slice[0] = String::from("Mr. Penguin"); + } + println!("Use after modification: {:?}", slice); + + // Mut access on reference + let mut slice: Option<[String; 1]> = Some([String::from("Cat")]); + if let Some(slice) = &mut slice { + slice[0] = String::from("Lord Meow Meow"); + } + println!("Use after modification: {:?}", slice); +} + +/// The lint will ignore bindings with sub patterns as it would be hard +/// to build correct suggestions for these instances :) +fn binding_with_sub_pattern() { + let slice: Option<&[u32]> = Some(&[1, 2, 3]); + if let Some(slice @ [_, _, _]) = slice { + println!("{:?}", slice[2]); + } +} + +fn main() {} diff --git a/tests/ui/index_refutable_slice/if_let_slice_binding.rs b/tests/ui/index_refutable_slice/if_let_slice_binding.rs index a4cb50bd6822..1c1d1c4cbe46 100644 --- a/tests/ui/index_refutable_slice/if_let_slice_binding.rs +++ b/tests/ui/index_refutable_slice/if_let_slice_binding.rs @@ -1,8 +1,6 @@ #![deny(clippy::index_refutable_slice)] #![allow(clippy::uninlined_format_args, clippy::needless_lifetimes)] -//@no-rustfix: need to change the suggestion to a multipart suggestion - enum SomeEnum { One(T), Two(T), diff --git a/tests/ui/index_refutable_slice/if_let_slice_binding.stderr b/tests/ui/index_refutable_slice/if_let_slice_binding.stderr index 8819cb0e28bf..14ee2e54cab1 100644 --- a/tests/ui/index_refutable_slice/if_let_slice_binding.stderr +++ b/tests/ui/index_refutable_slice/if_let_slice_binding.stderr @@ -1,5 +1,5 @@ error: this binding can be a slice pattern to avoid indexing - --> tests/ui/index_refutable_slice/if_let_slice_binding.rs:16:17 + --> tests/ui/index_refutable_slice/if_let_slice_binding.rs:14:17 | LL | if let Some(slice) = slice { | ^^^^^ @@ -9,150 +9,134 @@ note: the lint level is defined here | LL | #![deny(clippy::index_refutable_slice)] | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ -help: try using a slice pattern here +help: replace the binding and indexed access with a slice pattern | -LL | if let Some([slice_0, ..]) = slice { - | ~~~~~~~~~~~~~ -help: and replace the index expressions here +LL ~ if let Some([slice_0, ..]) = slice { +LL | +LL ~ println!("{}", slice_0); | -LL | println!("{}", slice_0); - | ~~~~~~~ error: this binding can be a slice pattern to avoid indexing - --> tests/ui/index_refutable_slice/if_let_slice_binding.rs:23:17 + --> tests/ui/index_refutable_slice/if_let_slice_binding.rs:21:17 | LL | if let Some(slice) = slice { | ^^^^^ | -help: try using a slice pattern here +help: replace the binding and indexed access with a slice pattern | -LL | if let Some([slice_0, ..]) = slice { - | ~~~~~~~~~~~~~ -help: and replace the index expressions here +LL ~ if let Some([slice_0, ..]) = slice { +LL | +LL ~ println!("{}", slice_0); | -LL | println!("{}", slice_0); - | ~~~~~~~ error: this binding can be a slice pattern to avoid indexing - --> tests/ui/index_refutable_slice/if_let_slice_binding.rs:30:17 + --> tests/ui/index_refutable_slice/if_let_slice_binding.rs:28:17 | LL | if let Some(slice) = slice { | ^^^^^ | -help: try using a slice pattern here - | -LL | if let Some([slice_0, _, slice_2, ..]) = slice { - | ~~~~~~~~~~~~~~~~~~~~~~~~~ -help: and replace the index expressions here +help: replace the binding and indexed access with a slice pattern | +LL ~ if let Some([slice_0, _, slice_2, ..]) = slice { +LL | LL ~ println!("{}", slice_2); LL ~ println!("{}", slice_0); | error: this binding can be a slice pattern to avoid indexing - --> tests/ui/index_refutable_slice/if_let_slice_binding.rs:38:26 + --> tests/ui/index_refutable_slice/if_let_slice_binding.rs:36:26 | LL | if let SomeEnum::One(slice) | SomeEnum::Three(slice) = slice_wrapped { | ^^^^^ | -help: try using a slice pattern here +help: replace the binding and indexed access with a slice pattern | -LL | if let SomeEnum::One([slice_0, ..]) | SomeEnum::Three([slice_0, ..]) = slice_wrapped { - | ~~~~~~~~~~~~~ ~~~~~~~~~~~~~ -help: and replace the index expressions here +LL ~ if let SomeEnum::One([slice_0, ..]) | SomeEnum::Three([slice_0, ..]) = slice_wrapped { +LL | +LL ~ println!("{}", slice_0); | -LL | println!("{}", slice_0); - | ~~~~~~~ error: this binding can be a slice pattern to avoid indexing - --> tests/ui/index_refutable_slice/if_let_slice_binding.rs:46:29 + --> tests/ui/index_refutable_slice/if_let_slice_binding.rs:44:29 | LL | if let (SomeEnum::Three(a), Some(b)) = (a_wrapped, b_wrapped) { | ^ | -help: try using a slice pattern here +help: replace the binding and indexed access with a slice pattern | -LL | if let (SomeEnum::Three([_, _, a_2, ..]), Some(b)) = (a_wrapped, b_wrapped) { - | ~~~~~~~~~~~~~~~ -help: and replace the index expressions here +LL ~ if let (SomeEnum::Three([_, _, a_2, ..]), Some(b)) = (a_wrapped, b_wrapped) { +LL | +LL | +LL ~ println!("{} -> {}", a_2, b[1]); | -LL | println!("{} -> {}", a_2, b[1]); - | ~~~ error: this binding can be a slice pattern to avoid indexing - --> tests/ui/index_refutable_slice/if_let_slice_binding.rs:46:38 + --> tests/ui/index_refutable_slice/if_let_slice_binding.rs:44:38 | LL | if let (SomeEnum::Three(a), Some(b)) = (a_wrapped, b_wrapped) { | ^ | -help: try using a slice pattern here +help: replace the binding and indexed access with a slice pattern | -LL | if let (SomeEnum::Three(a), Some([_, b_1, ..])) = (a_wrapped, b_wrapped) { - | ~~~~~~~~~~~~ -help: and replace the index expressions here +LL ~ if let (SomeEnum::Three(a), Some([_, b_1, ..])) = (a_wrapped, b_wrapped) { +LL | +LL | +LL ~ println!("{} -> {}", a[2], b_1); | -LL | println!("{} -> {}", a[2], b_1); - | ~~~ error: this binding can be a slice pattern to avoid indexing - --> tests/ui/index_refutable_slice/if_let_slice_binding.rs:55:21 + --> tests/ui/index_refutable_slice/if_let_slice_binding.rs:53:21 | LL | if let Some(ref slice) = slice { | ^^^^^ | -help: try using a slice pattern here +help: replace the binding and indexed access with a slice pattern | -LL | if let Some([_, ref slice_1, ..]) = slice { - | ~~~~~~~~~~~~~~~~~~~~ -help: and replace the index expressions here +LL ~ if let Some([_, ref slice_1, ..]) = slice { +LL | +LL ~ println!("{:?}", slice_1); | -LL | println!("{:?}", slice_1); - | ~~~~~~~ error: this binding can be a slice pattern to avoid indexing - --> tests/ui/index_refutable_slice/if_let_slice_binding.rs:64:17 + --> tests/ui/index_refutable_slice/if_let_slice_binding.rs:62:17 | LL | if let Some(slice) = &slice { | ^^^^^ | -help: try using a slice pattern here +help: replace the binding and indexed access with a slice pattern | -LL | if let Some([slice_0, ..]) = &slice { - | ~~~~~~~~~~~~~ -help: and replace the index expressions here +LL ~ if let Some([slice_0, ..]) = &slice { +LL | +LL ~ println!("{:?}", slice_0); | -LL | println!("{:?}", slice_0); - | ~~~~~~~ error: this binding can be a slice pattern to avoid indexing - --> tests/ui/index_refutable_slice/if_let_slice_binding.rs:134:17 + --> tests/ui/index_refutable_slice/if_let_slice_binding.rs:132:17 | LL | if let Some(slice) = wrap.inner { | ^^^^^ | -help: try using a slice pattern here +help: replace the binding and indexed access with a slice pattern | -LL | if let Some([slice_0, ..]) = wrap.inner { - | ~~~~~~~~~~~~~ -help: and replace the index expressions here +LL ~ if let Some([slice_0, ..]) = wrap.inner { +LL | +LL | if wrap.is_awesome { +LL ~ println!("This is awesome! {}", slice_0); | -LL | println!("This is awesome! {}", slice_0); - | ~~~~~~~ error: this binding can be a slice pattern to avoid indexing - --> tests/ui/index_refutable_slice/if_let_slice_binding.rs:142:17 + --> tests/ui/index_refutable_slice/if_let_slice_binding.rs:140:17 | LL | if let Some(slice) = wrap.inner { | ^^^^^ | -help: try using a slice pattern here +help: replace the binding and indexed access with a slice pattern | -LL | if let Some([slice_0, ..]) = wrap.inner { - | ~~~~~~~~~~~~~ -help: and replace the index expressions here +LL ~ if let Some([slice_0, ..]) = wrap.inner { +LL | +LL | if wrap.is_super_awesome() { +LL ~ println!("This is super awesome! {}", slice_0); | -LL | println!("This is super awesome! {}", slice_0); - | ~~~~~~~ error: aborting due to 10 previous errors diff --git a/tests/ui/index_refutable_slice/slice_indexing_in_macro.fixed b/tests/ui/index_refutable_slice/slice_indexing_in_macro.fixed new file mode 100644 index 000000000000..72edc539f043 --- /dev/null +++ b/tests/ui/index_refutable_slice/slice_indexing_in_macro.fixed @@ -0,0 +1,29 @@ +#![deny(clippy::index_refutable_slice)] + +extern crate if_chain; +use if_chain::if_chain; + +macro_rules! if_let_slice_macro { + () => { + // This would normally be linted + let slice: Option<&[u32]> = Some(&[1, 2, 3]); + if let Some(slice) = slice { + println!("{}", slice[0]); + } + }; +} + +fn main() { + // Don't lint this + if_let_slice_macro!(); + + // Do lint this + if_chain! { + let slice: Option<&[u32]> = Some(&[1, 2, 3]); + if let Some([slice_0, ..]) = slice; + //~^ ERROR: this binding can be a slice pattern to avoid indexing + then { + println!("{}", slice_0); + } + } +} diff --git a/tests/ui/index_refutable_slice/slice_indexing_in_macro.rs b/tests/ui/index_refutable_slice/slice_indexing_in_macro.rs index 5d9fad488899..7b474ba423b9 100644 --- a/tests/ui/index_refutable_slice/slice_indexing_in_macro.rs +++ b/tests/ui/index_refutable_slice/slice_indexing_in_macro.rs @@ -1,7 +1,5 @@ #![deny(clippy::index_refutable_slice)] -//@no-rustfix: need to change the suggestion to a multipart suggestion - extern crate if_chain; use if_chain::if_chain; diff --git a/tests/ui/index_refutable_slice/slice_indexing_in_macro.stderr b/tests/ui/index_refutable_slice/slice_indexing_in_macro.stderr index 69f0aaa97778..64741abb9114 100644 --- a/tests/ui/index_refutable_slice/slice_indexing_in_macro.stderr +++ b/tests/ui/index_refutable_slice/slice_indexing_in_macro.stderr @@ -1,5 +1,5 @@ error: this binding can be a slice pattern to avoid indexing - --> tests/ui/index_refutable_slice/slice_indexing_in_macro.rs:25:21 + --> tests/ui/index_refutable_slice/slice_indexing_in_macro.rs:23:21 | LL | if let Some(slice) = slice; | ^^^^^ @@ -9,14 +9,13 @@ note: the lint level is defined here | LL | #![deny(clippy::index_refutable_slice)] | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ -help: try using a slice pattern here +help: replace the binding and indexed access with a slice pattern | -LL | if let Some([slice_0, ..]) = slice; - | ~~~~~~~~~~~~~ -help: and replace the index expressions here +LL ~ if let Some([slice_0, ..]) = slice; +LL | +LL | then { +LL ~ println!("{}", slice_0); | -LL | println!("{}", slice_0); - | ~~~~~~~ error: aborting due to 1 previous error