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

suspicious_map ignores side effects #7767

Closed
zvavybir opened this issue Oct 4, 2021 · 3 comments · Fixed by #7770
Closed

suspicious_map ignores side effects #7767

zvavybir opened this issue Oct 4, 2021 · 3 comments · Fixed by #7770
Labels
C-bug Category: Clippy is not doing the correct thing I-false-positive Issue: The lint was triggered on code it shouldn't have

Comments

@zvavybir
Copy link
Contributor

zvavybir commented Oct 4, 2021

suspicious_map ignores possible side effects of the map. That was already mentioned #5253 and fixed, but only if it mutates something and not other kinds of side effects.

Lint name:
suspicious_map

I tried this code:

use std::sync::mpsc::Sender;

fn send_iter_and_get_len<I: Iterator>(iter: I, tx: Sender<I::Item>) -> usize
{
    iter.map(|x| tx.send(x)).count()
}

I expected to see this happen: No clippy warnings.

Instead, this happened: I got the following warning and wrong suggestions:

warning: this call to `map()` won't have an effect on the call to `count()`
 --> src/main.rs:5:5
  |
5 |     iter.map(|x| tx.send(x)).count()
  |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  |
  = note: `#[warn(clippy::suspicious_map)]` on by default
  = help: make sure you did not confuse `map` with `filter` or `for_each`
  = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#suspicious_map

Meta

Rust version (rustc -Vv):

rustc 1.57.0-nightly (9dbb26efe 2021-10-03)
binary: rustc
commit-hash: 9dbb26efe8a886f7bba94c2b996c9535ce07f917
commit-date: 2021-10-03
host: x86_64-unknown-linux-gnu
release: 1.57.0-nightly
LLVM version: 13.0.0
@zvavybir zvavybir added C-bug Category: Clippy is not doing the correct thing I-false-positive Issue: The lint was triggered on code it shouldn't have labels Oct 4, 2021
@xFrednet
Copy link
Member

xFrednet commented Oct 5, 2021

I sadly don't see an easy way to fix this without creating a bunch of false negatives. I would also consider this lint to be correct here. Is there a reason why you used map? If you're only trying to interact/inspect each element, then it might be better to use inspect instead. It might be good to reference inspect() in the lint description. 🙃

@zvavybir
Copy link
Contributor Author

zvavybir commented Oct 5, 2021

I already thought that I might be missing a more appropriate method, but I figured it's still an issue since it should be mentioned. So I agree that it can be just an documentation edit. I'll open a pull request, or?

@xFrednet
Copy link
Member

xFrednet commented Oct 5, 2021

It would be awesome if you could create a PR. 🙃 The description can be found here:

declare_clippy_lint! {
/// ### What it does
/// Checks for calls to `map` followed by a `count`.
///
/// ### Why is this bad?
/// It looks suspicious. 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.
///
/// ### Example
/// ```rust
/// let _ = (0..3).map(|x| x + 2).count();
/// ```
pub SUSPICIOUS_MAP,
suspicious,
"suspicious usage of map"
}

bors added a commit that referenced this issue Oct 5, 2021
improved help message for `suspicious_map`

`suspicious_map`'s help message assumes that the literal behavior is never the intended one, although it's sometimes.  This PR adds a mention of `inspect`, offering a idiomatic alternative.

fixes #7767

---

* Improved help message of [`suspicious_map`].

changelog:
@bors bors closed this as completed in b9dedf3 Oct 5, 2021
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 I-false-positive Issue: The lint was triggered on code it shouldn't have
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants