explicit_iter_loop: x.iter_mut() vs &mut *x

[dtolnay]

Summary

I have a hard time being confident that Clippy's suggestion in the following case constitutes an improvement to the code:

-     for xi in x.iter_mut() {
+     for xi in &mut *x {

Could we revisit whether it is worth suggesting a change in this case?

In cases where the suggestion is for xi in x {, I am fully supportive of the lint. Obviously for xi in x is a clear improvement over for xi in x.iter_mut() (modulo the restriction from #8585 being turned on).

I am also generally supportive of for xi in &mut x.

But the case of for xi in &mut *x starts to be over the line for me in symbol soup.

Lint Name

explicit_iter_loop

Reproducer

#![warn(clippy::pedantic)]

pub fn imul(x: &mut Vec<u64>, y: u64) {
    // Multiply iteratively over all elements, adding the carry each time.
    let mut carry = 0;
    for xi in x.iter_mut() {
        carry = scalar::imul(xi, y, carry);
    }

    // Overflow of value, add to end.
    if carry != 0 {
        x.push(carry);
    }
}

mod scalar {
    pub fn imul(_x: &mut u64, _y: u64, _carry: u64) -> u64 {
        todo!()
    }
}

$ cargo clippy

warning: it is more concise to loop over references to containers instead of using explicit iteration methods
 --> src/main.rs:6:15
  |
6 |     for xi in x.iter_mut() {
  |               ^^^^^^^^^^^^ help: to write this more concisely, try: `&mut *x`

This is a case where neither for xi in x { nor for xi in &mut x { compiles.

error[E0382]: borrow of moved value: `x`
   --> src/main.rs:12:9
    |
3   | pub fn imul(x: &mut Vec<u64>, y: u64) {
    |             - move occurs because `x` has type `&mut Vec<u64>`, which does not implement the `Copy` trait
...
6   |     for xi in x {
    |               - `x` moved due to this implicit call to `.into_iter()`
...
12  |         x.push(carry);
    |         ^^^^^^^^^^^^^ value borrowed here after move
    |
note: `into_iter` takes ownership of the receiver `self`, which moves `x`

error[E0277]: `Vec<u64>` is not an iterator
 --> src/main.rs:6:15
  |
6 |     for xi in &mut x {
  |               ^^^^^^ `Vec<u64>` is not an iterator; try calling `.into_iter()` or `.iter()`
  |
  = help: the trait `Iterator` is not implemented for `Vec<u64>`
  = note: required for `&mut Vec<u64>` to implement `Iterator`
  = note: 1 redundant requirement hidden
  = note: required for `&mut &mut Vec<u64>` to implement `Iterator`
  = note: required for `&mut &mut Vec<u64>` to implement `IntoIterator`
help: consider removing the leading `&`-reference
  |
6 -     for xi in &mut x {
6 +     for xi in x {
  |

Version

rustc 1.72.0-nightly (839e9a6e1 2023-07-02)
binary: rustc
commit-hash: 839e9a6e1210934fd24b15548b811a97c77138fc
commit-date: 2023-07-02
host: x86_64-unknown-linux-gnu
release: 1.72.0-nightly
LLVM version: 16.0.5

Additional Labels

No response

[tertsdiepraam]

Another variant of this problem is with an immutable borrow where clippy suggests this:

- for xi in x.iter() {}
+ for xi in &*x {}

For us, in uutils (see uutils/coreutils#5204), this has popped up since Rust 1.72. We think the suggested code is less readable and harder to understand (especially for beginners). I think a good compromise would be to only suggest these changes if no * need to be inserted.

[Benjscho]

Another compromise would be to split this into two lints explicit_iter_loop_deref and explicit_iter_loop_std, where the deref lint contains Deref *, Reborrow &*, and ReborrowMut &mut * syntax. explicit_iter_loop_std can then be pedantic, while explicit_iter_loop_deref can move back to the style group.

[Benjscho]

Created a PR (#11406) that would split these and resolve this issue if that's the desired behaviour.

[tertsdiepraam]

Though I like the idea, one problem with splitting it is that it would require that everyone's allow attributes would need to be changed. I think it's worth exploring how many people actually want the version with *. My guess is that it isn't many people, but I might be wrong.

[Benjscho]

That's true, and one option for that is to then change explicit_iter_loop_std back to explicit_iter_loop. This has the drawback that the behaviour of explicit_iter_loop has then changed. I think needing to readjust the allow attributes makes most sense

[Benjscho]

I misunderstood the pedantic/style designation before. Updated my PR so both lints are active as pedantic, but can be separately allowed/disallowed. This would leave users having to update their allow attributes, but gives better granularity in the linting.

[Centri3]

You could add explicit_iter_loop -> explicit_iter_loop_std in renamed_lints.

[Benjscho]

Awesome thanks, will do that!