Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

New lint: slice_indexing_after_deconstruction #7569

Closed
xFrednet opened this issue Aug 16, 2021 · 6 comments · Fixed by #7643
Closed

New lint: slice_indexing_after_deconstruction #7569

xFrednet opened this issue Aug 16, 2021 · 6 comments · Fixed by #7643
Assignees
Labels
A-lint Area: New lints E-medium Call for participation: Medium difficulty level problem and requires some initial experience.

Comments

@xFrednet
Copy link
Member

xFrednet commented Aug 16, 2021

What it does

The lint checks for slices that come from value deconstruction and are only used to access individual slice values.

Categories (optional)

  • Kind: pedantic (This could also be style, but I'm not sure if we want it warn-by-default.)

Advantages

Deconstruction of slices allows direct value access without using indexes on the slice, which can lead to panics. It also enables the user to give slice values a better name and indirectly check the correct length of the slice. See #7566 files as an example of how this can be an improvement.

Clippy already had a few instances where ICEs would have been prevented by using deconstruction of slices.

Example

fn main() {
    let slice: Option<&[u32]> = Some(&[1, 2, 3]);
    
    if let Some(slice) = slice {
        println!("{}", slice[0]);
    }
}

Could be written as:

fn main() {
    let slice: Option<&[u32]> = Some(&[1, 2, 3]);
    
    if let Some([first, ..]) = slice {
        println!("{}", first);
    }
}

Notes:

  • Clippy uses pattern matching extensively, often in if_chain macros with if let statements. I think it would be reasonable for this lint to also check if statements inside if_chain macros.
  • The lint probably shouldn't warn if the slice instance is used for something else besides direct value access.
@xFrednet xFrednet added the A-lint Area: New lints label Aug 16, 2021
@xFrednet

This comment has been minimized.

@rustbot rustbot added the E-medium Call for participation: Medium difficulty level problem and requires some initial experience. label Aug 16, 2021
@camsteffen
Copy link
Contributor

We have to pick some max threshold for indexes to lint. Like we don't want to lint slice[100]. Maybe slice[3] is a reasonable max to lint? In other words, suggest a slice pattern with up to 4 elements.

@xFrednet
Copy link
Member Author

Good suggestion, a threshold of 3 would catch all instance in Clippy that I've seen. We could also have this configurable and have the default be 3 👍.

@xFrednet
Copy link
Member Author

Reviewing a rustup PR I found this awesome refactoring which could also be suggested by this lint as an enhancement later on:

// from
for win in blocks.windows(2) {

// to
let mut iter = blocks.windows(2);
while let Some(&[win0, win1]) = iter.next() {

(Let's just ignore that this refactoring was done in my first bigger lint implementation xD)

@camsteffen
Copy link
Contributor

I'm holding out for for [a, b] in slice.array_windows(2) (currently unstable)

@xFrednet
Copy link
Member Author

That would be even nicer. I want to work on this once rustup is merged. It sounds like a useful and fun lint 🙃

@rustbot claim

bors added a commit that referenced this issue Sep 5, 2021
…hearth

Avoid slice indexing in Clippy (down with the ICEs)

While working on #7569 I got about 23 lint reports where we can avoid slice indexing by destructing the slice early. This is a preparation PR to avoid fixing them in the lint PR. (The implementation already takes about 300 lines without tests 😅). Either way, this should hopefully be easy to review 🙃

---

changelog: none
bors added a commit that referenced this issue Nov 11, 2021
New lint `avoidable_slice_indexing` to avoid slice indexing

A new lint to check for slices that could be deconstructed to avoid indexing. This lint should hopefully prevent some panics in other projects and ICEs for us. See #7569 for an example

The implementation specifically checks for immutable bindings in `if let` statements to slices and arrays. Then it checks if these bindings are only used for value access using indices and that these indices are lower than the configured limit. I did my best to keep the implementation small, however the check was sadly quite complex. Now it's around 300 lines for the implementation and the rest are test.

---

Optional future improvements:
* Check for these instances also in `match` statements
* Check for mutable slice bindings that could also be destructed

---

changelog: New lint [`avoidable_slice_indexing`]

I've already fixed a bunch of lint triggers in #7638 to make this PR smaller

Closes: #7569
@bors bors closed this as completed in 3d4d0cf Nov 11, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-lint Area: New lints E-medium Call for participation: Medium difficulty level problem and requires some initial experience.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants