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

feat(fix): Do not lint if the target code is inside a loop #8992

Merged
merged 3 commits into from
Aug 21, 2022

Conversation

kyoto7250
Copy link
Contributor

@kyoto7250 kyoto7250 commented Jun 13, 2022

close #8753

we consider the following code.

fn main() {
    let vec = vec![1];
    let w: Vec<usize> = vec.iter().map(|i| i * i).collect();  // <- once.

    for i in 0..2 {
        let _ = w.contains(&i);
    }
}

and the clippy will issue the following warning.

warning: avoid using `collect()` when not needed
 --> src/main.rs:3:51
  |
3 |     let w: Vec<usize> = vec.iter().map(|i| i * i).collect();
  |                                                   ^^^^^^^
...
6 |         let _ = w.contains(&i);
  |                 -------------- the iterator could be used here instead
  |
  = note: `#[warn(clippy::needless_collect)]` on by default
  = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#needless_collect
help: check if the original Iterator contains an element instead of collecting then checking
  |
3 ~     
4 | 
5 |     for i in 0..2 {
6 ~         let _ = vec.iter().map(|i| i * i).any(|x| x == i);

Rewrite the code as indicated.

fn main() {
    let vec = vec![1];

    for i in 0..2 {
        let _ = vec.iter().map(|i| i * i).any(|x| x == i);  // <- execute `map` every loop.
    }
}

this code is valid in the compiler, but, it is different from the code before the rewrite.
So, we should not lint, If collect is outside of a loop.

Thank you in advance.


changelog: Do not lint if the target code is inside a loop in needless_collect

@rust-highfive
Copy link

r? @flip1995

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

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Jun 13, 2022
@kyoto7250 kyoto7250 changed the title feat(fix): Do not lint if the target code is inside a loop in needless_collect feat(fix): Do not lint if the target code is inside a loop Jun 13, 2022
@kyoto7250 kyoto7250 force-pushed the fix_8753 branch 7 times, most recently from b5d7f5c to 743d41a Compare June 14, 2022 14:37
@kyoto7250 kyoto7250 marked this pull request as ready for review June 14, 2022 14:49
kyoto7250 and others added 2 commits August 21, 2022 10:47
Only check for the kind of loop once instead of re-desugaring it.
@flip1995
Copy link
Member

Thanks and sorry for taking so long! I did a small code cleanup and rebased the branch.

@bors r+

@bors
Copy link
Collaborator

bors commented Aug 21, 2022

📌 Commit 318ed05 has been approved by flip1995

It is now in the queue for this repository.

@bors
Copy link
Collaborator

bors commented Aug 21, 2022

⌛ Testing commit 318ed05 with merge d0a426c...

bors added a commit that referenced this pull request Aug 21, 2022
feat(fix): Do not lint if the target code is inside a loop

close #8753

we consider the following code.

```rust
fn main() {
    let vec = vec![1];
    let w: Vec<usize> = vec.iter().map(|i| i * i).collect();  // <- once.

    for i in 0..2 {
        let _ = w.contains(&i);
    }
}
```

and the clippy will issue the following warning.

```rust
warning: avoid using `collect()` when not needed
 --> src/main.rs:3:51
  |
3 |     let w: Vec<usize> = vec.iter().map(|i| i * i).collect();
  |                                                   ^^^^^^^
...
6 |         let _ = w.contains(&i);
  |                 -------------- the iterator could be used here instead
  |
  = note: `#[warn(clippy::needless_collect)]` on by default
  = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#needless_collect
help: check if the original Iterator contains an element instead of collecting then checking
  |
3 ~
4 |
5 |     for i in 0..2 {
6 ~         let _ = vec.iter().map(|i| i * i).any(|x| x == i);
```

Rewrite the code as indicated.

```rust
fn main() {
    let vec = vec![1];

    for i in 0..2 {
        let _ = vec.iter().map(|i| i * i).any(|x| x == i);  // <- execute `map` every loop.
    }
}
```

this code is valid in the compiler, but, it is different from the code before the rewrite.
So, we should not lint, If `collect` is outside of a loop.

Thank you in advance.

---

changelog: Do not lint if the target code is inside a loop in `needless_collect`
@bors
Copy link
Collaborator

bors commented Aug 21, 2022

💔 Test failed - checks-action_test

@flip1995
Copy link
Member

@bors try

@bors
Copy link
Collaborator

bors commented Aug 21, 2022

⌛ Trying commit 95c4751 with merge dcd68e4...

bors added a commit that referenced this pull request Aug 21, 2022
feat(fix): Do not lint if the target code is inside a loop

close #8753

we consider the following code.

```rust
fn main() {
    let vec = vec![1];
    let w: Vec<usize> = vec.iter().map(|i| i * i).collect();  // <- once.

    for i in 0..2 {
        let _ = w.contains(&i);
    }
}
```

and the clippy will issue the following warning.

```rust
warning: avoid using `collect()` when not needed
 --> src/main.rs:3:51
  |
3 |     let w: Vec<usize> = vec.iter().map(|i| i * i).collect();
  |                                                   ^^^^^^^
...
6 |         let _ = w.contains(&i);
  |                 -------------- the iterator could be used here instead
  |
  = note: `#[warn(clippy::needless_collect)]` on by default
  = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#needless_collect
help: check if the original Iterator contains an element instead of collecting then checking
  |
3 ~
4 |
5 |     for i in 0..2 {
6 ~         let _ = vec.iter().map(|i| i * i).any(|x| x == i);
```

Rewrite the code as indicated.

```rust
fn main() {
    let vec = vec![1];

    for i in 0..2 {
        let _ = vec.iter().map(|i| i * i).any(|x| x == i);  // <- execute `map` every loop.
    }
}
```

this code is valid in the compiler, but, it is different from the code before the rewrite.
So, we should not lint, If `collect` is outside of a loop.

Thank you in advance.

---

changelog: Do not lint if the target code is inside a loop in `needless_collect`
@bors
Copy link
Collaborator

bors commented Aug 21, 2022

💔 Test failed - checks-action_test

@flip1995
Copy link
Member

@bors try

@bors
Copy link
Collaborator

bors commented Aug 21, 2022

⌛ Trying commit a272da1 with merge 1ef9bb9...

bors added a commit that referenced this pull request Aug 21, 2022
feat(fix): Do not lint if the target code is inside a loop

close #8753

we consider the following code.

```rust
fn main() {
    let vec = vec![1];
    let w: Vec<usize> = vec.iter().map(|i| i * i).collect();  // <- once.

    for i in 0..2 {
        let _ = w.contains(&i);
    }
}
```

and the clippy will issue the following warning.

```rust
warning: avoid using `collect()` when not needed
 --> src/main.rs:3:51
  |
3 |     let w: Vec<usize> = vec.iter().map(|i| i * i).collect();
  |                                                   ^^^^^^^
...
6 |         let _ = w.contains(&i);
  |                 -------------- the iterator could be used here instead
  |
  = note: `#[warn(clippy::needless_collect)]` on by default
  = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#needless_collect
help: check if the original Iterator contains an element instead of collecting then checking
  |
3 ~
4 |
5 |     for i in 0..2 {
6 ~         let _ = vec.iter().map(|i| i * i).any(|x| x == i);
```

Rewrite the code as indicated.

```rust
fn main() {
    let vec = vec![1];

    for i in 0..2 {
        let _ = vec.iter().map(|i| i * i).any(|x| x == i);  // <- execute `map` every loop.
    }
}
```

this code is valid in the compiler, but, it is different from the code before the rewrite.
So, we should not lint, If `collect` is outside of a loop.

Thank you in advance.

---

changelog: Do not lint if the target code is inside a loop in `needless_collect`
@flip1995
Copy link
Member

That error was spurious I think, so let's retry

@bors r+

@bors
Copy link
Collaborator

bors commented Aug 21, 2022

📌 Commit 070b035 has been approved by flip1995

It is now in the queue for this repository.

@bors
Copy link
Collaborator

bors commented Aug 21, 2022

⌛ Testing commit 070b035 with merge e19a05c...

@bors
Copy link
Collaborator

bors commented Aug 21, 2022

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: flip1995
Pushing e19a05c to master...

@bors bors merged commit e19a05c into rust-lang:master Aug 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[maybe] false positive on clippy::needless-collect
4 participants