option_if_let_else suggests map_or instead of map_or_else even if else branch contains a function call

[Arnavion]

#[deny(clippy::option_if_let_else)]
pub fn foo(o: Option<i32>, f: impl FnOnce(i32), g: impl FnOnce()) {
    if let Some(i) = o {
        f(i)
    }
    else {
        g()
    }
}

The lint suggests o.map_or(g(), |i| f(i)), which is wrong because the else branch should be evaluated lazily because it's a function call. It ought to have suggested o.map_or_else(|| g(), |i| f(i))

Meta

• cargo clippy -V: clippy 0.0.212 (39d5a61 2020-07-17)
• rustc -Vv:

  rustc 1.47.0-nightly (39d5a61f2 2020-07-17)
  binary: rustc
  commit-hash: 39d5a61f2e4e237123837f5162cc275c2fd7e625
  commit-date: 2020-07-17
  host: x86_64-unknown-linux-gnu
  release: 1.47.0-nightly
  LLVM version: 10.0
  

[Arnavion]

I now see that #5301 (comment) implies this is intentional, and that after the suggestion, the clippy::or_fun_call lint would suggest changing it to map_or_else. However

#[deny(clippy::or_fun_call)]
pub fn foo(o: Option<i32>, f: impl FnOnce(i32), g: impl FnOnce()) {
    o.map_or(g(), |i| f(i))
}

does not raise any errors.

It does cause the clippy::unit_arg lint to fire, but that lint itself has a bad suggestion, for which I've filed #5823

So if we modify the code to remove the possibility of triggering clippy::unit_arg:

#[deny(clippy::or_fun_call)]
pub fn foo(o: Option<i32>, f: impl FnOnce(i32) -> i32, g: impl FnOnce() -> i32) -> i32 {
    o.map_or(g(), |i| f(i))
}

this code still does not raise any errors.

Perhaps this issue can be repurposed to figure out why clippy::or_fun_call did not fire on the code after applying clippy::if_let_else_'s suggestion.

[tnielens]

I think clippy::option_if_let_else should recommend map_or_else by default to preserve the code semantic. If the else block consists in a single identifier (or another pattern without side-effect), then map_or is applicable.

[tnielens]

#[deny(clippy::or_fun_call)]
pub fn foo(o: Option<i32>, f: impl FnOnce(i32) -> i32, g: impl FnOnce() -> i32) -> i32 {
    o.map_or(g(), |i| f(i))
}

doesn't trigger the lint because it only looks for function calls of at least one argument. See in methods/mod.rs.
The reasoning was maybe that functions taking no arguments have probably side-effects.

[xFrednet]

This issue seems to be fixed. Clippy at least suggests the usage of map_or_else for the example code. Playground

[Arnavion]

The change you're seeing is from #5937 , which considered this issue to only be "partially fixed". I'm not sure what the unfixed part is, however.

[J-ZhengLi]

as of today, this does not lint at all, not sure when that happened or whether it was on purpose or not