New lint `index_refutable_slice` to avoid slice indexing #7643

xFrednet · 2021-09-07T13:42:00Z

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 [index_refutable_slice]

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

Closes: #7569

rust-highfive · 2021-09-07T13:42:04Z

r? @camsteffen

(rust-highfive has picked a reviewer for you, use r? to override)

camsteffen · 2021-09-07T20:30:45Z

I'm afraid of false positives as this lint changes semantics. Maybe this is acceptable for a pedantic lint, but I'm not sure. It's like suspicious-pedantic...

if let Some(slice) = opt {
    if !slice.is_empty() {
        println!("first: {}", slice[0]);
    }
}
if let Some(slice) = opt {
    if interesting_stuff(slice) {
        // interesting_stuff never returns true for empty slices
        println!("first: {}", slice[0]);
    }
}
if let Some(slice) = foo.bar {
    if foo.is_interesting {
        // foo.is_interesting is only true if the slice is not empty
        println!("first: {}", slice[0]);
    } else {
        println!("not interesting :(");
    }
}

xFrednet · 2021-09-07T21:13:34Z

I was also a bit afraid of FPs. The implementation is therefore quite cautious, it at least covers all cases that I could think of and should only lint if the slice is used for indexing. I remove it from the list if it's used in any other way, but FPs could maybe still happen. The first two cases should definitely be covered, the third one should work as well. I'll add these tests tomorrow and also try to run lintcheck with it 🙃

I would definitely not make this allow-by-default at the start. And even later I think it should be an opt-in like it is in the pedantic group. We could also set it to nursery for the start. I mainly wrote it with Clippy in mind xD

camsteffen · 2021-09-08T19:51:03Z

Yeah I'm wary of lints where we can never really be sure that we're giving a false positive. You can add heuristics to be cautious, but ultimately we can't know from static analysis why the coder might have certain assumptions about the length of the slice, and how those assumptions change at different points in the code. The heuristics remove false positives and add false negatives at the same time. I think the pedantic group allows for picky lints, but not necessarily more false positives. Maybe this can just go in nursery or restriction. @rust-lang/clippy thoughts?

camsteffen · 2021-09-08T19:52:58Z

I mainly wrote it with Clippy in mind xD

That's fine, but I also think the FP rate in Clippy will be lower than in most other domains. For example, we often have a slice of arguments and we simply expect the number of arguments to be one static value.

flip1995 · 2021-09-09T09:47:26Z

I like the idea of the lint. We could put it in nursery for now and try with the lintcheck tool how often it triggers in the wild.

I haven't read the code, so I don't know what the scope of patterns is this lint triggers on. The tests seem to only address cases where the slice is immediately indexed after the if let. So I think the scope of this lint is an open question.

xFrednet · 2021-09-09T16:02:40Z

Okay, then I'll change the group to nursery.

The tests seem to only address cases where the slice is immediately indexed after the if let

The lint uses a UseExprVisitor to move through the entire block. The tests are rather simple, as you've noticed. I've already fixed the lint triggers inside Clippy in #7638 which displays some more complex example. I'll put it on the to-do list to add some more complex examples.

(Sorry, for not adding them sooner, my semester is currently taking up a lot of time. It should be better in the upcoming week 🙃 )

camsteffen · 2021-09-10T15:06:43Z

Alright, maybe I'm being overly pessimistic. :)

I have an idea for naming the lint/config if you like it. index_refutable_slice as in "index a slice binding from a refutable pattern". This conveys that a let statement would never be linted. For the config, max_suggested_slice_pattern_length as I think this is more "to the point" of what we want to configure. It could be reused by another lint, but if not that's fine.

clippy_lints/src/avoidable_slice_indexing.rs

tests/ui/avoidable_slice_indexing/if_let_slice_destruction.rs

xFrednet · 2021-09-13T17:40:20Z

I've updated the code now, this should hopefully address all comments and make the pipeline green ^^.

@rustbot label -S-waiting-on-author +S-waiting-on-review

xFrednet · 2021-09-13T18:01:09Z

I have an idea for naming the lint/config if you like it. index_refutable_slice as in "index a slice binding from a refutable pattern". This conveys that a let statement would never be linted. For the config, max_suggested_slice_pattern_length as I think this is more "to the point" of what we want to configure.

I missed this review comment earlier. I like the suggested names. Do you want to review the latest changes beforehand? As this refactoring will most likely break the review comment links from GitHub.

camsteffen · 2021-09-13T18:08:45Z

Nah go ahead.

xFrednet · 2021-09-13T19:17:25Z

I've also rebased on upstream/master now 🙃

camsteffen

FWIW it's okay by me to squash in changes when it's a significant refactor.

clippy_lints/src/index_refutable_slice.rs

clippy_lints/src/utils/conf.rs

clippy_lints/src/index_refutable_slice.rs

tests/ui-toml/max_suggested_slice_pattern_length/avoidable_slice_indexing_limit.rs

tests/ui/index_refutable_slice/if_let_slice_binding.stderr

clippy_lints/src/index_refutable_slice.rs

bors · 2021-09-17T22:17:32Z

☔ The latest upstream changes (presumably #7653) made this pull request unmergeable. Please resolve the merge conflicts.

xFrednet · 2021-09-24T11:04:16Z

Just a small note for triage: I'm currently quite busy, but I still plan to continue this PR. Thank you for the review 🙃

giraffate · 2021-10-12T00:22:52Z

ping from triage @xFrednet. This is just a reminder.

xFrednet · 2021-11-03T14:23:25Z

I'm starting to revive this branch. I'll ping you and switch the labels once I'm done 🙃

xFrednet · 2021-11-03T15:52:08Z

Thank you for waiting on this review! This should now be ready for review again @camsteffen 🙃

FWIW it's okay by me to squash in changes when it's a significant refactor.

Good to know, I've squashed the changes from your review into the commit as well.

camsteffen

Just some minor stuff

clippy_lints/src/index_refutable_slice.rs

clippy_lints/src/utils/conf.rs

tests/ui-toml/max_suggested_slice_pattern_length/avoidable_slice_indexing_limit.rs

tests/ui-toml/min_rust_version/min_rust_version.rs

bors · 2021-11-09T01:13:26Z

☔ The latest upstream changes (presumably #7944) made this pull request unmergeable. Please resolve the merge conflicts.

camsteffen

LGTM, thanks! Please squash.

* Finding pattern slices for `avoidable_slice_indexing` * `avoidable_slice_indexing` analysing slice usage * Add configuration to `avoidable_slice_indexing` * Emitting `avoidable_slice_indexing` with suggestions * Dogfooding and fixing bugs * Add ui-toml test for `avoidable_slice_indexing` * Correctly suggest `ref` keywords for `avoidable_slice_indexing` * Test and document `mut` for `avoid_slice_indexing` * Handle macros with `avoidable_slice_indexing` lint * Ignore slices with sub patterns in `avoidable_slice_indexing` * Update lint description for `avoidable_slice_indexing` * Move `avoidable_slice_indexing` to nursery * Added more tests for `avoidable_slice_indexing` * Update documentation and message for `avoidable_slice_indexing` * Teach `avoidable_slice_indexing` about `HirId`s and `Visitors` * Rename lint to `index_refutable_slice` and connected config

xFrednet · 2021-11-11T16:40:11Z

Done! Thanks for the review!

camsteffen · 2021-11-11T16:41:04Z

@bors r+

bors · 2021-11-11T16:41:05Z

📌 Commit e444cbe has been approved by camsteffen

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 · 2021-11-11T16:41:11Z

⌛ Testing commit e444cbe with merge 7c52841...

camsteffen · 2021-11-11T16:41:45Z

@bors r- (let's fix the title and changelog with the changed name)

camsteffen · 2021-11-11T16:42:53Z

Er maybe it's too late to r- 🤷 @bors r+

bors · 2021-11-11T16:42:54Z

📌 Commit e444cbe has been approved by camsteffen

bors · 2021-11-11T16:43:00Z

⌛ Testing commit e444cbe with merge 3d4d0cf...

bors · 2021-11-11T16:56:14Z

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: camsteffen
Pushing 3d4d0cf to master...

rust-highfive assigned camsteffen Sep 7, 2021

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Sep 7, 2021

xFrednet force-pushed the 7569-splits-for-slices branch from 255f0e5 to 83559b8 Compare September 7, 2021 13:47

camsteffen reviewed Sep 10, 2021

View reviewed changes

camsteffen added S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties labels Sep 12, 2021

xFrednet force-pushed the 7569-splits-for-slices branch 2 times, most recently from 9075c9e to 5cd1765 Compare September 13, 2021 13:45

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties and removed S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) labels Sep 13, 2021

xFrednet force-pushed the 7569-splits-for-slices branch from 1ed88fa to cc982bd Compare September 13, 2021 19:17

xFrednet force-pushed the 7569-splits-for-slices branch from cc982bd to 6162ec8 Compare September 13, 2021 19:22

camsteffen suggested changes Sep 13, 2021

View reviewed changes

xFrednet added S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties labels Sep 14, 2021

xFrednet force-pushed the 7569-splits-for-slices branch from cf7e0bc to 776c868 Compare November 3, 2021 15:50

xFrednet added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties and removed S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) labels Nov 3, 2021

camsteffen reviewed Nov 5, 2021

View reviewed changes

camsteffen added S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties labels Nov 10, 2021

xFrednet force-pushed the 7569-splits-for-slices branch from d2a232f to 820f242 Compare November 11, 2021 14:34

xFrednet added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties and removed S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) labels Nov 11, 2021

xFrednet force-pushed the 7569-splits-for-slices branch 2 times, most recently from ae84f3e to 9f612b0 Compare November 11, 2021 15:52

camsteffen approved these changes Nov 11, 2021

View reviewed changes

xFrednet force-pushed the 7569-splits-for-slices branch from 9f612b0 to e444cbe Compare November 11, 2021 16:34

xFrednet changed the title ~~New lint avoidable_slice_indexing to avoid slice indexing~~ New lint index_refutable_slice to avoid slice indexing Nov 11, 2021

bors merged commit 3d4d0cf into rust-lang:master Nov 11, 2021

xFrednet deleted the 7569-splits-for-slices branch November 11, 2021 16:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New lint `index_refutable_slice` to avoid slice indexing #7643

New lint `index_refutable_slice` to avoid slice indexing #7643

xFrednet commented Sep 7, 2021 •

edited

Loading

rust-highfive commented Sep 7, 2021

camsteffen commented Sep 7, 2021 •

edited

Loading

xFrednet commented Sep 7, 2021 •

edited

Loading

camsteffen commented Sep 8, 2021

camsteffen commented Sep 8, 2021

flip1995 commented Sep 9, 2021

xFrednet commented Sep 9, 2021 •

edited

Loading

camsteffen commented Sep 10, 2021

xFrednet commented Sep 13, 2021

xFrednet commented Sep 13, 2021 •

edited

Loading

camsteffen commented Sep 13, 2021

xFrednet commented Sep 13, 2021

camsteffen left a comment

bors commented Sep 17, 2021

xFrednet commented Sep 24, 2021

giraffate commented Oct 12, 2021

xFrednet commented Nov 3, 2021

xFrednet commented Nov 3, 2021

camsteffen left a comment

bors commented Nov 9, 2021

camsteffen left a comment

xFrednet commented Nov 11, 2021

camsteffen commented Nov 11, 2021

bors commented Nov 11, 2021

bors commented Nov 11, 2021

camsteffen commented Nov 11, 2021

camsteffen commented Nov 11, 2021

bors commented Nov 11, 2021

bors commented Nov 11, 2021

bors commented Nov 11, 2021

New lint index_refutable_slice to avoid slice indexing #7643

New lint index_refutable_slice to avoid slice indexing #7643

Conversation

xFrednet commented Sep 7, 2021 • edited Loading

rust-highfive commented Sep 7, 2021

camsteffen commented Sep 7, 2021 • edited Loading

xFrednet commented Sep 7, 2021 • edited Loading

camsteffen commented Sep 8, 2021

camsteffen commented Sep 8, 2021

flip1995 commented Sep 9, 2021

xFrednet commented Sep 9, 2021 • edited Loading

camsteffen commented Sep 10, 2021

xFrednet commented Sep 13, 2021

xFrednet commented Sep 13, 2021 • edited Loading

camsteffen commented Sep 13, 2021

xFrednet commented Sep 13, 2021

camsteffen left a comment

Choose a reason for hiding this comment

bors commented Sep 17, 2021

xFrednet commented Sep 24, 2021

giraffate commented Oct 12, 2021

xFrednet commented Nov 3, 2021

xFrednet commented Nov 3, 2021

camsteffen left a comment

Choose a reason for hiding this comment

bors commented Nov 9, 2021

camsteffen left a comment

Choose a reason for hiding this comment

xFrednet commented Nov 11, 2021

camsteffen commented Nov 11, 2021

bors commented Nov 11, 2021

bors commented Nov 11, 2021

camsteffen commented Nov 11, 2021

camsteffen commented Nov 11, 2021

bors commented Nov 11, 2021

bors commented Nov 11, 2021

bors commented Nov 11, 2021

New lint `index_refutable_slice` to avoid slice indexing #7643

New lint `index_refutable_slice` to avoid slice indexing #7643

xFrednet commented Sep 7, 2021 •

edited

Loading

camsteffen commented Sep 7, 2021 •

edited

Loading

xFrednet commented Sep 7, 2021 •

edited

Loading

xFrednet commented Sep 9, 2021 •

edited

Loading

xFrednet commented Sep 13, 2021 •

edited

Loading