must_use Option::map*

[Mark-Simulacrum]

We were discussing #71368 in the language team meeting today, and it came up that Option::map should probably be marked must_use. There are technically valid use cases which do not require using the result (i.e. side-effecting uses), but these are so rare that it seems unlikely that we need to worry about them for the purposes of such a lint.

(Probably other combinators on Option should also be checked).

[matklad]

but these are so rare that it seems unlikely that we need to worry about them for the purposes of such a lint.

Not sure about this. I've definitely seen such code in the wild. I think it's important that build-in rustc lints do not have false-positives. Even small amount of false-positives would massively diminish the value of the lint system, as you would need to think "is this a false positive?" about every other lint.

Now, it might be a good idea to lint against side-effectful operations in Option::map, on the grounds that this is a bad style, and I can get behind it. I just want to push back against "there are valid uses ... but they are rare" kind of reasoning. (of course, there's nothing absolute here, and if the uses are indeed exceedingly rare it might be ok, but I think I've seen such code in real life (probably even in rust-analyzer), which makes me think that they are not negligibly rare).

[Mark-Simulacrum]

Yeah, I agree that's very valid push back. However in this case I think if let is a much more readable way to accomplish what map does on Option. But maybe a crater run would help gauge the prevalence of spurious warnings - and the common reasons why. Maybe there's a pattern we can identify and not lint on.

[matklad]

s/but these are so rare that it seems unlikely that we need to worry about them/but they are a bad style and should be linted against anyway/ and I will be happy :-)

[cuviper]

Here's an example I just noticed in the wild:
https://github.com/lu-zero/cargo-c/blob/151744ab35e85baa5bf1d56419c3dc2e41bb6510/src/build.rs#L140-L142

    let mut paths = vec![&build_targets.include];
    build_targets.static_lib.as_ref().map(|p| paths.push(p));
    build_targets.shared_lib.as_ref().map(|p| paths.push(p));

Using if let might be seen as line-count bloat. Another option is extend, which looks nice:

    let mut paths = vec![&build_targets.include];
    paths.extend(&build_targets.static_lib);
    paths.extend(&build_targets.shared_lib);

(not to pick on this author/crate -- it's just useful to consider a real example)

[cuviper]

Here are a couple examples in rust-lang/rust itself:

rust/src/liballoc/tests/slice.rs

Line 1311 in 3360cc3

v.get_mut(1).map(|e| *e = 7);

rust/src/tools/tidy/src/error_codes_check.rs

Line 43 in 3360cc3

error_codes.get_mut(&err_code).map(|x| *x = true);

[Cypher1]

I believe the .for_each function provides a side effecting alternative to .map (which avoids issues where .map is lazy and side effects are dropped.
A few questions:
a) is this equivalence real or is it more complicated than that?
b) if it is real can we use that (to avoid larger changes to code like using 'if's instead of map for option)
c) how would we add this suggestion to the must_use warning to help programmers in future?

Thanks

[JelteF]

I ran into this not being a lint today. Because the thing inside the .map call was fallible. So I accidentally forgot to handle the error, because the call that returned the error was "being used" by turning it into Option<Result<_, _> then the resulting option was not. I feel like map should at least be must_use when Option<Result<_, _>> is returned by it.