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

needless_collect doesn't understand local borrows #6066

Open
jpdoyle opened this issue Sep 18, 2020 · 7 comments
Open

needless_collect doesn't understand local borrows #6066

jpdoyle opened this issue Sep 18, 2020 · 7 comments
Labels
C-bug Category: Clippy is not doing the correct thing E-medium Call for participation: Medium difficulty level problem and requires some initial experience. L-nursery Lint: Currently in the nursery group

Comments

@jpdoyle
Copy link

jpdoyle commented Sep 18, 2020

When running clippy (0.0.212 (2020-09-17 f3c923a)) on https://play.rust-lang.org/?version=nightly&mode=debug&edition=2018&gist=5b4bbd5f3b653c7d9d8a84d2a63903fa I get the suggestion:

    Checking playground v0.0.1 (/playground)
warning: avoid using `collect()` when not needed
 --> src/main.rs:4:5
  |
4 | /     let a_filt = a.into_iter().filter(|x| !b.contains(x)).collect::<Vec<_>>();
5 | |     
6 | |     for v in b.into_iter().chain(a_filt.into_iter()) {
  | |_________________________________^
  |
  = note: `#[warn(clippy::needless_collect)]` on by default
help: Use the original Iterator instead of collecting it and then producing a new one
  |
4 |     
5 |     
6 |     for v in b.into_iter().chain(a.into_iter().filter(|x| !b.contains(x))) {
  |

warning: 1 warning emitted

    Finished dev [unoptimized + debuginfo] target(s) in 0.33s

However, the original Iterator cannot be used, as demonstrated by https://play.rust-lang.org/?version=nightly&mode=debug&edition=2018&gist=65b782dbf6cd1731a6906afcd9704e99

   Compiling playground v0.0.1 (/playground)
error[E0505]: cannot move out of `b` because it is borrowed
 --> src/main.rs:6:14
  |
4 |     let a_filt = a.into_iter().filter(|x| !b.contains(x));
  |                                       ---  - borrow occurs due to use in closure
  |                                       |
  |                                       borrow of `b` occurs here
5 |     
6 |     for v in b.into_iter().chain(a_filt) {
  |              ^                   ------ borrow later used here
  |              |
  |              move out of `b` occurs here

error: aborting due to previous error

For more information about this error, try `rustc --explain E0505`.
error: could not compile `playground`

To learn more, run the command again with --verbose.
@jpdoyle jpdoyle added the C-bug Category: Clippy is not doing the correct thing label Sep 18, 2020
@flip1995 flip1995 added the E-medium Call for participation: Medium difficulty level problem and requires some initial experience. label Oct 5, 2020
@flip1995
Copy link
Member

flip1995 commented Oct 5, 2020

So to generalize the problem: If something is borrowed in the collect expression, but later moved in the Iterator expression, this lint should not trigger. When attempting to fix this, a look at the let_and_return lint might help to find out how to check for borrows in an expression (potentially the "borrow check" can be shared).

@SKyletoft
Copy link

Might be the same issue, might not be:

fn variations(n: u64, mask: &str) -> Vec<u64> {
	let positions = mask
		.char_indices()
		.filter(|&(_, d)| d == 'X')
		.map(|(idx, _)| 35 - idx as u64)
		.collect::<Vec<u64>>();
	(0..2u64.pow(positions.len() as u32))
		.rev()
		.map(|m| {
			positions.iter().enumerate().fold(n, |acc, (i, &curr)| {
				let to_set = (((1 << i) & m) << curr) >> i;
				let with_hole = (!(1 << curr)) & acc;
				with_hole | to_set
			})
		})
		.collect()
}
warning: avoid using `collect()` when not needed
  --> src/main.rs:94:2
   |
94 |       let positions = mask
   |  _____^
95 | |         .char_indices()
96 | |         .filter(|&(_, d)| d == 'X')
97 | |         .map(|(idx, _)| 35 - idx as u64)
98 | |         .collect::<Vec<u64>>();
99 | |     (0..2u64.pow(positions.len() as u32))
   | |_________________^
   |
   = note: `#[warn(clippy::needless_collect)]` on by default
help: Take the original Iterator's count instead of collecting it and finding the length
   |
94 |     
95 |     (0..2u64.pow(mask
96 |         .char_indices()
97 |         .filter(|&(_, d)| d == 'X')
98 |         .map(|(idx, _)| 35 - idx as u64).count() as u32))
   |

warning: 1 warning emitted

Here the first statement can be replaced with the .count() version, but it doesn't recognise that the collected version is used again later in the statement

@jpdoyle
Copy link
Author

jpdoyle commented Dec 16, 2020

something is wrong with tracking where iterators are used, for sure. A small variation on @SKyletoft's example passes without warning: https://play.rust-lang.org/?version=nightly&mode=debug&edition=2018&gist=f9a04a7d9f7d4409cb91caa73deba23f

@jpdoyle
Copy link
Author

jpdoyle commented Dec 16, 2020

@djc

This comment has been minimized.

@giraffate

This comment has been minimized.

@djc

This comment has been minimized.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-bug Category: Clippy is not doing the correct thing E-medium Call for participation: Medium difficulty level problem and requires some initial experience. L-nursery Lint: Currently in the nursery group
Projects
None yet
Development

No branches or pull requests

6 participants