Rust: must_use Option::map*

[Mark-Simulacrum]

We were discussing https://github.com/rust-lang/rust/issues/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:

https://github.com/rust-lang/rust/blob/3360cc3a0ea33c84d0b0b1163107b1c1acbf2a69/src/liballoc/tests/slice.rs#L1311

https://github.com/rust-lang/rust/blob/3360cc3a0ea33c84d0b0b1163107b1c1acbf2a69/src/tools/tidy/src/error_codes_check.rs#L43