new lint: unnecessary_indexing

[Jacherr]

Fixes #11858

Adds a new lint which checks for an if-conditional which checks if a sequence is not empty (!seq.is_empty()) and, in the body of the if statement, accesses the first element of said sequence.

Helps to reduce the complexity of the code and improve readability.

changelog: Add new lint unnecessary_indexing

[rustbot]

r? @Manishearth

rustbot has assigned @Manishearth.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

[Jacherr]

I've given my best shot at making sure that the code is as clean as possible but I haven't done much with visitors etc. before, so my implementation is probably still a bit rough around the edges! It does, however, get the job done in all of the tests I put together (including mixes of locals and expressions, so on and so forth).

The code is made a bit more complicated because it needs to account for the presence of multiple locals, as well as no locals at all, referencing seq[0].

Edit: The new implementation with a visitor should appear here as soon as GitHub decides to work again 😅

[rustbot]

There are merge commits (commits with multiple parents) in your changes. We have a no merge policy so these commits will need to be removed for this pull request to be merged.

You can start a rebase with the following commands:

$ # rebase
$ git rebase -i master
$ # delete any merge commits in the editor that appears
$ git push --force-with-lease

The following commits are merge commits:

• 60f7f06
• a8a7371
• 92ca7c9
• 3a14911
• 99e8000

[Manishearth]

r? clippy

still somewhat busy for a PR this involved

[Jacherr]

Made a slight adjustment to not lint if a mutable reference to the sequence is taken, as this can affect the lint suggestions in a negative way, so this should hopefully eliminate some opportunities for false positives to appear.

[bors]

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

[J-ZhengLi]

👀 It appears that the blocks.expr is being ignored, thus the code below does not lint:

let first = if !a.is_empty() {
    a[0]
};

Could you add a test to confirm that?

[J-ZhengLi]

I just gave it a try, and this case indeed have FN, passing the whole block to the visitor call solves it, since the block.expr will then be visited, can you change it?

and, make sure to skip the lint if the is_empty call and the indexing are from some external macro, there's a rustc_middle::lint::in_external_macro function does the check. We don't want the lint to suggesting user to change something that they have no control of.
it's pretty good as a first implementation (or second)

[Alexendoo]

A regular .from_expansion() would be fine, we don't really want to lint across local macros here either

[J-ZhengLi]

looks like @giraffate haven't been active for quite a while, so, it might make sense to re-assign someone else for now?

r? clippy

[Jacherr]

Apologies, I accidentally managed to close this when attempting to fix the conflicts on this branch.

[Jacherr]

@rustbot ready

[Alexendoo]

Do we need to store index_receiver? It looks like using conditional_receiver above would suffice as they have the same source text

[Jacherr]

index_receiver has some additional bounds on it, for example it is only Some if the index is a primitive constant, and not some kind of dynamic value stored in a variable or constant somewhere else, so I think it can stay. I'll update the comment on the field in the struct to better explain this.

[bors]

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

[Jacherr]

@rustbot ready

Once everything's good to go I can squash everything. The commit history is getting quite messy :p

[bors]

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

[Alexendoo]

If we use if let Some(ident) = first_local.pat.simple_ident() we can use ident.name later instead of getting a snippet from the pat's span

[Alexendoo]

We should push all the suggestions into one vec and use multipart_suggestion at the end for all the changes, multiple span suggestions are presented as alternatives rather than something to combine in e.g. rust-analyzer

[Alexendoo]

Let's be confident that our suggestion won't require dereferencing

Suggested change

	"consider using if..let syntax (variable may need to be dereferenced)",
	"consider using `if let` instead of indexing",

[Alexendoo]

This will false negative for any code that mutates unrelated things

if !x.is_empty() {
    unrelated.push(x[0]);
}

Something like

} else if path_to_local_id(x, conditional_receiver_hid) {
    match cx.typeck_results().expr_ty_adjusted(x).kind() {
        RawPtr(_, Mutability::Mut) | Ref(_, _, Mutability::Mut) => return ControlFlow::Break(());
        Adt(..) => {
            if /* parent expr is not & */ {
                return ControlFlow::Break(())
            }
        }
        _ => {}
    }
}

would narrow it down to only things that mutate our input, it should also let us remove this check and lint when the type is &mut [T]/&mut Vec<T> (assuming it works)

[Alexendoo]

A step further would be to not break immediately but store a bool that we've seen a mutation and break in the if let ExprKind::Index(receiver, index, _) if it's true

This would be since it doesn't matter if the code is mutating the input as long as it comes after all the indexes we're modifying:

// ok
if !input.is_empty() {
    input[0];
    input.push(1);
}

// not ok
if !input.is_empty() {
    input.push(1);
    input[0];
}

[Alexendoo]

It would also need a borrow when x is auto borrowed

fn f(x: &[String]) {
    if !x.is_empty() {
        let _ = x[0].len();
    }
}

[Alexendoo]

As a callback to #12464 (comment) we could make this a bool and check it before returning to ditch the field

[Alexendoo]

@rustbot author