Fix: fixed multipart_suggestion in index_refutable_slice uitest

[scottgerring]

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--fixable 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

[rustbot]

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @y21 (or someone else) some time within the next two weeks.

Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (S-waiting-on-review and S-waiting-on-author) stays updated, invoking these commands when appropriate:

• @rustbot author: the review is finished, PR author should check the comments and take action accordingly
• @rustbot review: the author is ready for a review, this PR will be queued again in the reviewer's queue

[scottgerring]

I changed this as the --fix works now for the included test cases

[flip1995]

FYI: This level is not about if --fix is possible or not, but rather whether the suggestion is always correct. I don't know the reason, why it was set to MaybeIncorrect and this might actually be MachineApplicable. As you looked into this lint: Can you think of cases where applying the suggestions by those lints might break (or change semantics) of the code?

[y21]

You could argue that the suggestion is always a semantic change as it's removing a panic possibly intended to act as an assertion (for when an in-bounds index is an invariant), e.g. when a slice being non-empty is an invariant of a function then changing x[0] to if let [x_0, ..] = x doesn't make much sense as it only delays the bug or makes the bug go unnoticed.

I'm also not sure about the introduced names. slice_7 seems like a meaningless name to introduce and IMO it would make more sense to just have the user fix it manually and give the bindings proper names

[flip1995]

Then I would keep it as MaybeIncorrect at least, maybe as HasPlaceholders even. Leaning towards the former.

[scottgerring]

Hey both - thanks for the detailed feedback!

Reading up about the applicability, it does seem like MaybeIncorrect is where it should stay - it produces valid code, but may not be what the user intended - either because the placeholder names aren't really immediately useful, or because the user is intentionally using this as a mechanism to panic early as @y21 suggests.

HasPlaceholders seems off as it does produce valid code - although the dummy field names do otherwise seem quite placeholder-ish!

     /// The suggestion contains placeholders like `(...)` or `{ /* fields */ }`. The suggestion
     /// cannot be applied automatically because it will not result in valid Rust code. The user
    /// will need to fill in the placeholders.

I'll fix this up and check the other files this test touches too.

[scottgerring]

@rustbot review

[flip1995]

This also touches

• tests/ui/index_refutable_slice/if_let_slice_binding.stderr
• tests/ui/index_refutable_slice/slice_indexing_in_macro.stderr

Which are also part of the list in the tracking issue. Can you try to remove the no-rustfix comment there too and if that also Just Work ™️ with your fixes in this PR?

[scottgerring]

This also touches

* `tests/ui/index_refutable_slice/if_let_slice_binding.stderr`

* `tests/ui/index_refutable_slice/slice_indexing_in_macro.stderr`a

Which are also part of the list in the tracking issue. Can you try to remove the no-rustfix comment there too and if that also Just Work ™️ with your fixes in this PR?

Good catch - These are both good ✅
I've rolled the applicability back down to MaybeIncorrect and removed rust-nofix from these; I think it should be good to go.

[scottgerring]

@flip1995 i've rebased this one onto master and cleaned it up into a single commit. Should be good to go!

[y21]

Sorry, took me a bit to get to this PR. This looks good to me, thank you.