Extend `indexing_slicing` lint #2790

shnewto · 2018-05-23T03:35:47Z

Hey there clippy team! I've made some assumptions in this PR and I'm not at all certain they'll look like the right approach to you. I'm looking forward to any feedback or revision requests you have, thanks!

Prior to this commit the indexing_slicing lint was limited to indexing/slicing operations on arrays. This meant that the scope of a really useful lint didn't include vectors. In order to include vectors in the indexing_slicing lint a few steps were taken.

The array_indexing.rs source file in clippy_lints was renamed to indexing_slicing.rs to more accurately reflect the lint's new scope. The OUT_OF_BOUNDS_INDEXING lint persists through these changes so if we can know that a constant index or slice on an array is in bounds no lint is triggered.

The array_indexing tests in the tests/ui directory were also extended and moved to indexing_slicing.rs and indexing_slicing.stderr.

The indexing_slicing lint was moved to the clippy_pedantic lint group.

A specific "Consider using" string was added to each of the indexing_slicing lint reports.

At least one of the test scenarios might look peculiar and I'll leave it up to y'all to decide if it's palatable. It's the result of indexing the array x after let x = [1, 2, 3, 4];

error: slicing may panic. Consider using `.get(..n)`or `.get_mut(..n)`instead
  --> $DIR/indexing_slicing.rs:23:6
   |
23 |     &x[0..][..3];
   |      ^^^^^^^^^^^

The error string reports only on the second half's range-to, because the range-from is in bounds!

Again, thanks for taking a look.

Closes #2536

Manishearth · 2018-05-23T04:30:55Z

Could you split up the commit so that the file being renamed is a separate commit from the other changes? It's hard to review otherwise, github just shows it as a file being deleted and a different one being created.

Though it might be worth reviewing from scratch anyway 🤷‍♀️

shnewto · 2018-05-23T04:41:00Z

@Manishearth I'll take a stab at it, forgive me if things end up accidentally messy for a bit 😬

shnewto · 2018-05-23T05:16:31Z

Okay, it looks like we've got two commits now. Thanks again!

mati865 · 2018-06-06T17:39:17Z

Ping @Manishearth, I assume you want to complete the review?

shnewto · 2018-06-12T20:10:12Z

@Manishearth or any other reviewers, I'm trying to keep up and resolve conflicts with the master branch as they arise! If I get behind though and you're waiting for conflicts to be resolved before reviewing let me know and I'll address them as soon as I can!

flip1995 · 2018-06-12T23:21:10Z

Sorry for the long wait time, I'll get to this tomorrow!

flip1995

Thanks for the contribution and welcome to clippy! A little bit of nitpicking and some things you need to change will follow: 😉

Sorry for the long wait time again!

flip1995 · 2018-06-13T21:45:02Z

clippy_lints/src/indexing_slicing.rs

+}
+
+#[derive(Copy, Clone)]
+pub struct IndexingSlicingPass;


I would prefer just IndexingSlicing as the struct name, to keep it consistent with the rest of the code base.

flip1995 · 2018-06-13T21:45:21Z

clippy_lints/src/indexing_slicing.rs

+                // on the `range` returned by `higher::range(cx, b)`.
+                // ExprStruct handles &x[n..m], &x[n..] and &x[..n].
+                // ExprPath handles &x[..] and x[var]
+                ExprStruct(_, _, _) | ExprPath(_) => {


You can just use ExprStruct(..) (and ExprPath(..)) here.

flip1995 · 2018-06-13T21:45:39Z

clippy_lints/src/indexing_slicing.rs

+
+impl<'a, 'tcx> LateLintPass<'a, 'tcx> for IndexingSlicingPass {
+    fn check_expr(&mut self, cx: &LateContext<'a, 'tcx>, expr: &'tcx Expr) {
+        if let ExprIndex(ref a, ref b) = &expr.node {


Why change the names of the variables from telling names array + index to just a + b?

🤦‍♂️

flip1995 · 2018-06-13T21:46:30Z

clippy_lints/src/indexing_slicing.rs

+                        }
+                        match (range.start, range.end) {
+                            (None, Some(_)) => {
+                                cx.span_lint(


Please use the utils::span_lint() methods here. These methods add the link to the clippy documentation to the warning message.

There are also methods for help messages in the utils module. You could use utils::span_help_and_lint() for the Consider using... part of the lint.
Bonus points if you use utils::span_lint_and_sugg() (Can be automatically fixed by rustfix) and generate the right code suggestion. For example: For x[..3] the sugg argument should be x.get(..3). To do this you have to first figure out, whether get() or get_mut() is the right function to use and after that extract the span of the range to insert it as the argument for get[_mut](). Something like this: format!("{}.{}({})", arr/vec_name, "get[_mut]", range.span.as_str()).

But this is definitely not necessary and can be done in a later PR. For now utils::span_help_and_lint() is enough.

This is great to know. Thank you for pointing me in the right direction.

The bonus points suggestion sounds like an awesome value. If it's okay I'll plan on working toward that end in a follow up PR. I'd be happy to raise an issue for that.

Sounds great, thanks!

flip1995 · 2018-06-13T21:47:05Z

clippy_lints/src/indexing_slicing.rs

+                                    expr.span,
+                                    "slicing may panic. Consider using \
+                                     `.get(n..m)` or `.get_mut(n..m)` instead",
+                                );


These calls to span[_help_and]_lint() only differ in the help message. Could you refactor these?

flip1995 · 2018-06-13T21:47:32Z

clippy_lints/src/indexing_slicing.rs

+                            // Else index is in bounds, ok.
+                        }
+                    } else {
+                        cx.span_lint(


Also utils::span_help_and_lint()

flip1995 · 2018-06-13T21:47:39Z

clippy_lints/src/indexing_slicing.rs

+                            (None, None) => (),
+                        }
+                    } else {
+                        cx.span_lint(


Also utils::span_help_and_lint()

flip1995 · 2018-06-13T21:50:46Z

clippy_lints/src/lib.rs

@@ -461,6 +459,7 @@ pub fn register_plugins(reg: &mut rustc_plugin::Registry) {
        enum_variants::PUB_ENUM_VARIANT_NAMES,
        enum_variants::STUTTER,
        if_not_else::IF_NOT_ELSE,
+        indexing_slicing::INDEXING_SLICING,


I think this should be in clippy_restriction. It is at least defined as a restriction lint in indexing_slicing.rs:62.

I'd be happy to correctly make it a restriction but the request in issue #2536 was that this would be a pedantic lint so for now I'll just fix the declaration.

flip1995 · 2018-06-13T21:51:55Z

tests/ui/indexing_slicing.stderr

+  --> $DIR/indexing_slicing.rs:23:6
+   |
+23 |     &x[0..][..3];
+   |      ^^^^^^^^^^^


What happens if you add &x[0..].get(..3) as another test case. Does the lint get triggered on x[0..]?

The lint does not get triggered! It can be added to the test cases.

shnewto · 2018-06-13T23:31:26Z

@flip1995 thanks for the awesome feedback! I've taken a stab at addressing your requests in the last commit.

flip1995 · 2018-06-14T13:53:05Z

clippy_lints/src/indexing_slicing.rs

+                            (Some(_), Some(_)) => {
+                                help_msg =
+                                    "Consider using `.get(n..m)` or `.get_mut(n..m)` instead";
+                            }


You could prettify this a little bit more: see playground

flip1995 · 2018-06-14T14:10:08Z

clippy_lints/src/indexing_slicing.rs

+                                        OUT_OF_BOUNDS_INDEXING,
+                                        expr.span,
+                                        "range is out of bounds",
+                                    );


I would suggest to return after linting for an out of bounds range. On the one hand saying that the range is out of bounds here and then that it might panic with the INDEXING_SLICING lint is kind of redundant. On the other hand linting only once would be more consistent with the code below in the indexing [n] case.

flip1995 · 2018-06-14T14:18:29Z

clippy_lints/src/indexing_slicing.rs

+                // ExprPath handles &x[..] and x[var]
+                ExprStruct(..) | ExprPath(..) => {
+                    if let Some(range) = higher::range(cx, index) {
+                        let ty = cx.tables.expr_ty(array);


You need the ty in both match cases and could move this in front of the match block.

flip1995 · 2018-06-14T14:27:35Z

tests/ui/indexing_slicing.rs

    &x[0..3];
+    &x[0..][..3];
+    &x[0..].get(..3); // Ok


Wait this should trigger the INDEXING_SLICING lint too (on x[0..]). Or am I missing something? What happens if you do something like &x[5..].iter().map(|x| 2 * x).collect::<Vec<i32>>()? This should definitely trigger the lint. Could you add this test with a range 5.. (panic) and a range 2.. (no panic). The former case got linted before the change. playground

Oh sorry for the confusion! &x[0..].get(..3); does not trigger the lint and was put in this test file to ensure that stderr was not generated for it. And I will add the examples you mentioned 👍

Ahhh I misunderstood this then, thanks for the clarification!

flip1995 · 2018-06-14T14:37:46Z

clippy_lints/src/indexing_slicing.rs

+
+/// **What it does:** Checks for usage of indexing or slicing. Does not report
+/// if we can tell that the indexing or slicing operations on an array are in
+/// bounds.


This is not really true (yet). In the examples below x[2] is listed as a bad example, but we can definitely tell, that 2 is in bounds. Maybe rewrite the second sentence as a problem and reformulate the "What it does"-part so it states, that get[_mut]() should be used instead of indexing/slicing with [a..b]

There should definitely be an example that shows an array so this statement is clearer. It should be true though that this lint does not report on an array if we can tell that the operation is in bounds.

shnewto · 2018-06-14T16:47:38Z

@flip1995 thanks for another round of thoughtful feedback. I've made a new pass and have hopefully incorporated or addressed each of this round's changes.

flip1995 · 2018-06-14T16:50:53Z

Thanks for the quick fixes, I'm currently on the train and have no internet on my laptop, I'll try to take another look at this, this evening or tomorrow!

shnewto · 2018-06-14T17:28:49Z

tests/ui/indexing_slicing.stderr

   |      ^^^^^^^^
   |
   = help: Consider using `.get(..n)`or `.get_mut(..n)` instead

-error: aborting due to 36 previous errors
+error: aborting due to 28 previous errors


The errors should have gone to 37... Looks like something got bumped in the return refactoring. I'm investigating now.

37? That would be 1 more error message. I think 28 could be right. Before the last commit multiple lines triggered both the INDEXING_SLICING and the OUT_OF_BOUNDS_INDEXING Lint. Now there should only be the OUT_OF_BOUNDS_INDEXING Lint in this cases.

But I didn't really look at the .stderr file yet.

🤔

Ah totally. You're right about the number going down.

I'm not certain it's still catching everything it should though. Minimally I'm going to push a change that makes that test file a little clearer about what should and shouldn't produce stderr so it's not so easy to get lost.

shnewto · 2018-06-14T20:18:02Z

@flip1995 Turned out that some cases had slipped through the cracks and were being handled incorrectly. I've pushed a commit that contains a minor linting re-factor and added some comments that will hopefully clear up what output can be expected from this lint's tests.

Thanks again for taking the time with me on this.

flip1995 · 2018-06-15T13:57:26Z

The last commit simplified the lint logic pretty much - nice. Number of errors and error messages seem to be right. LGTM now.

Could you add one more test for indexing with a const instead of a literal:

const N: usize = 15;
x[N];

I'm pretty sure this works just fine, but better save than sorry.

Thanks again for taking the time with me on this.

Thanks for contributing, we're happy about every new and future contributor!

shnewto · 2018-06-15T15:57:46Z

Always happy to extend tests!

shnewto · 2018-06-15T18:08:44Z

@flip1995 It looks like there's a CONST_ERR lint in librustc (declared here https://github.com/rust-lang/rust/blob/5205ae8bdc2991eecf3bfbb58ed8f56c0673e738/src/librustc/lint/builtin.rs#L29) that seems to already handle out of bound indexing (https://github.com/rust-lang/rust/blob/5205ae8bdc2991eecf3bfbb58ed8f56c0673e738/src/librustc_mir/transform/const_prop.rs#L545) and the OUT_OF_BOUNDS_INDEXING lint that lives on in this PR steps on it.

If it sounds reasonable to you I'd like to verify that the CONST_ERR lint does catch each of the appropriate cases in this PR's tests then remove the OUT_OF_BOUNDS_INDEXING lint so in order to defer to rustc.

shnewto · 2018-06-15T18:41:36Z

🤔

rustc's const_err lint does not warn on out of bounds ranged indexing operations on arrays the way this project's OUT_OF_BOUNDS_INDEXING lint does. I'm up for suggestions here, maybe maintain this PR's ranged OUT_OF_BOUNDS_INDEXING lint and remove the other here https://github.com/shnewto/rust-clippy/blob/ce5076b7b852b2d908ef705aee135e09ef10cc95/clippy_lints/src/indexing_slicing.rs#L140?

flip1995 · 2018-06-18T11:00:15Z

Retriggering travis

mati865 · 2018-06-18T11:09:53Z

It does need rebase to build with latest nightly.

flip1995 · 2018-06-18T12:13:12Z

If it sounds reasonable to you I'd like to verify that the CONST_ERR lint does catch each of the appropriate cases in this PR's tests then remove the OUT_OF_BOUNDS_INDEXING lint so in order to defer to rustc.

I think keeping the OUT_OF_BOUNDS_INDEXING lint as it is, is ok for now. At least as long as we are not on the level of #2536 (comment). If this lint achieves this level, we can think about uplifting it to rustc / integrating it with the const propagator. But since I'm not 100% sure: What do you think about this @oli-obk? Also const_err gets triggered only on nightly.

Maybe related #2846, but we are far from including this lint into this list.

@mati865 it got successfully build with the latest nightly. travis:

$ rustc --version
rustc 1.28.0-nightly (86a8f1a63 2018-06-17)
$ rustup --version
rustup 1.11.0 (e751ff9f8 2018-02-13)
$ cargo --version
cargo 1.28.0-nightly (e2348c2db 2018-06-07)

Everything LGTM now and I would merge this after the CONST_ERR vs. OUT_OF_BOUNDS_INDEXING case is clear.

oli-obk · 2018-06-18T12:21:06Z

I think it's ok to have overlap with the const_err lint for now.

The const_err lint will only lint usize indexing on arrays for quite some time. This PRs lint also triggers on slicing via ranges, maybe it should just not lint for usize indexing on arrays?

mati865 · 2018-06-18T12:39:34Z

@flip1995 looks like it got auto-rebased:

    Checking clippy_lints v0.0.208 (file:///home/travis/build/rust-lang-nursery/rust-clippy/clippy_lints)
    Checking clippy v0.0.208 (file:///home/travis/build/rust-lang-nursery/rust-clippy)
    Finished dev [unoptimized + debuginfo] target(s) in 1m 05s

Hey there clippy team! I've made some assumptions in this PR and I'm not at all certain they'll look like the right approach to you. I'm looking forward to any feedback or revision requests you have, thanks! Prior to this commit the `indexing_slicing` lint was limited to indexing/slicing operations on arrays. This meant that the scope of a really useful lint didn't include vectors. In order to include vectors in the `indexing_slicing` lint a few steps were taken. The `array_indexing.rs` source file in `clippy_lints` was renamed to `indexing_slicing.rs` to more accurately reflect the lint's new scope. The `OUT_OF_BOUNDS_INDEXING` lint persists through these changes so if we can know that a constant index or slice on an array is in bounds no lint is triggered. The `array_indexing` tests in the `tests/ui` directory were also extended and moved to `indexing_slicing.rs` and `indexing_slicing.stderr`. The `indexing_slicing` lint was moved to the `clippy_pedantic` lint group. A specific "Consider using" string was added to each of the `indexing_slicing` lint reports. At least one of the test scenarios might look peculiar and I'll leave it up to y'all to decide if it's palatable. It's the result of indexing the array `x` after `let x = [1, 2, 3, 4];` ``` error: slicing may panic. Consider using `.get(..n)`or `.get_mut(..n)`instead --> $DIR/indexing_slicing.rs:23:6 | 23 | &x[0..][..3]; | ^^^^^^^^^^^ ``` The error string reports only on the second half's range-to, because the range-from is in bounds! Again, thanks for taking a look. Closes #2536

This commit renames instances of `array_indexing` to `indexing_slicing` and moves the `indexing_slicing` lint to the `clippy_pedantic` group. The justification for this commit's changes are detailed in the previous commit's message.

… process of reviewing PR #2790. The changes reflected in this commit are as follows: - Revised `IndexingSlicingPass` struct name to IndexingSlicing for consistency with the rest of the code base. - Revised match arm condition to use `(..)` shorthand in favor of `(_, _, _)`. - Restored a couple telling variable names. - Calls to `cx.span_lint` were revised to use `utils::span_help_and_lint`. - Took a stab at refactoring some generalizable calls to `utils::span_help_and_lint` to minimize duplicate code. - Revised INDEXING_SLICING declaration to pedantic rather than restriction. - Added `&x[0..].get(..3)` to the test cases.

The changes reflected in this commit (requested in PR #2790) are as follows: - Extended `INDEXING_SLICING` documentation to include the array type so that it is clearer when indexing operations are allowed. - Variable `ty` defined identically in multiple scopes was moved to an outer scope so it's only defined once. - Added a missing return statement to ensure only one lint is triggered by a scenario. - Prettified match statement with a `let` clause. (I learned something new!) - Added `&x[5..].iter().map(|x| 2 * x).collect::<Vec<i32>>()` and `&x[2..].iter().map(|x| 2 * x).collect::<Vec<i32>>()` to the test cases. The first _should trigger the lint/stderr_ and the second _should not_.

This commit contains a few changes. In an attempt to clarify which test cases should and should not produce stderr it became clear that some cases were being handled incorrectly. In order to address these test cases, a minor re-factor was made to the linting logic itself. The re-factor was driven by edge case handling including a need for additional match conditions for `ExprCall` (`&x[0..=4]`) and `ExprBinary` (`x[1 << 3]`). Rather than attempt to account for each potential `Expr*` the code was re-factored into simply "if ranged index" and an "otherwise" conditions.

In this commit tests were added to ensure that tests with a `const` index behaved as expected. In order to minimize the changes to the test's corresponding `stderr`, the tests were appended to the end of the file.

shnewto · 2018-06-19T17:16:23Z

Just checking in. Is anyone currently expecting action on my end?

flip1995 · 2018-06-19T20:49:46Z

maybe it should just not lint for usize indexing on arrays?

It would be great if you could address this.

This commit removes the logic in this PR that linted out-of-bounds constant `usize` indexing on arrays. That case is already handled by rustc's `const_err` lint. Beyond removing the linting logic, the test file and its associated stderr were updated to verify that const `usize` indexing operations on arrays are no longer handled by this `indexing_slicing` lint.

shnewto · 2018-06-20T22:51:33Z

I went ahead and removed the lint for constant usize indexing on arrays. The test code that uses constant usize indexing was left in (and the stderr modified) to ensure that the indexing_slicing lint is no longer triggered in those cases.

oli-obk · 2018-06-21T08:31:34Z

Thanks for bearing with us!

flip1995 requested changes Jun 13, 2018

View reviewed changes

flip1995 reviewed Jun 14, 2018

View reviewed changes

shnewto commented Jun 14, 2018

View reviewed changes

flip1995 closed this Jun 18, 2018

flip1995 reopened this Jun 18, 2018

flip1995 approved these changes Jun 18, 2018

View reviewed changes

shnewto added 5 commits June 19, 2018 16:27

Rename instances of array_indexing

5b759ef

This commit renames instances of `array_indexing` to `indexing_slicing` and moves the `indexing_slicing` lint to the `clippy_pedantic` group. The justification for this commit's changes are detailed in the previous commit's message.

Add tests that index with a const value.

e63f5df

In this commit tests were added to ensure that tests with a `const` index behaved as expected. In order to minimize the changes to the test's corresponding `stderr`, the tests were appended to the end of the file.

oli-obk merged commit 25510cf into rust-lang:master Jun 21, 2018

shnewto deleted the vectors-to-indexing-slicing-lint branch June 21, 2018 18:10

flip1995 mentioned this pull request Jul 19, 2018

Why isn't indexing_slicing a restriction ? #2933

Closed

Extend indexing_slicing lint #2790

Extend indexing_slicing lint #2790

Conversation

shnewto commented May 23, 2018

Manishearth commented May 23, 2018

shnewto commented May 23, 2018

shnewto commented May 23, 2018

mati865 commented Jun 6, 2018

shnewto commented Jun 12, 2018 • edited Loading

flip1995 commented Jun 12, 2018

flip1995 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shnewto commented Jun 13, 2018

flip1995 Jun 14, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

flip1995 Jun 14, 2018 • edited Loading

Choose a reason for hiding this comment

shnewto Jun 14, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shnewto commented Jun 14, 2018

flip1995 commented Jun 14, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shnewto commented Jun 14, 2018

flip1995 commented Jun 15, 2018 • edited Loading

shnewto commented Jun 15, 2018

shnewto commented Jun 15, 2018

shnewto commented Jun 15, 2018

flip1995 commented Jun 18, 2018

mati865 commented Jun 18, 2018

flip1995 commented Jun 18, 2018

oli-obk commented Jun 18, 2018

mati865 commented Jun 18, 2018

shnewto commented Jun 19, 2018

flip1995 commented Jun 19, 2018

shnewto commented Jun 20, 2018

oli-obk commented Jun 21, 2018

Extend `indexing_slicing` lint #2790

Extend `indexing_slicing` lint #2790

shnewto commented Jun 12, 2018 •

edited

Loading

flip1995 Jun 14, 2018 •

edited

Loading

flip1995 Jun 14, 2018 •

edited

Loading

shnewto Jun 14, 2018 •

edited

Loading

flip1995 commented Jun 15, 2018 •

edited

Loading