suspicious_map oddity

[matthiaskrgr]

fn count_and_sum(numbers: &Vec<u32>) -> (usize, u32)   {
	let mut sum: u32 = 0;
	let count = numbers.iter().map(|n| sum += n).count();

	(count, sum)
}

In this example, we use map to process all values and do something with them that involves another mutable variable or function, in the end, we use count() to get the total number of items processed.

The lint explanation says

Maybe map was confused with filter. If the map call is intentional, this should be rewritten. Or, if you intend to drive the iterator to completion, you can just use for_each instead.

But a filter is not useful here. Using for_each is out of option since for_each().count() does not work since for_each() does not return anything.

I'm not sure if inspect() is the right choice here, at least it is not noted in the lints description.

[flip1995]

I guess we should bail out, if some expr in the map closure is an assignment to an UpVar.

[jpospychala]

Similar issue is not only with assignment but also a function, like

fn count_and_sum(numbers: &Vec<u32>) -> (usize, u32)   {
  let mut sum: u32 = 0;
  let mut sum_mut = |n| sum += n;
  let count = numbers.iter().map(|n| sum_mut(*n)).count();
  (count, sum)
}

Another case is filter.map.count, where count actually makes some sense.

let count = numbers.iter().filter(|n| **n > 2).map(|n| sum +n).count();

Overall, I do like the current lint message, calling count on map is at least confusing and worth reconsidering.

[mathstuf]

Can clippy just check if the argument to map is FnMut as a heuristic?

[mathstuf]

I'm going to take a stab at it.

[mathstuf]

I think I've made progress. Writing a test now.

[mathstuf]

You know, maybe inspect() is the right thing to do here. In fact, any map() argument returning () is probably better served via inspect().

Meh. Brain fart. inspect() isn't strictly better.

[mathstuf]

See #5375. Still a hole, but the "common" case of a literal closure should no longer be a false positive.