False positive for or_fun_call

[tbu-]

I got the following message:

warning: use of `ok_or` followed by a function call
   --> src/file.rs:220:5
    |
220 |     o.ok_or(io::Error::new(io::ErrorKind::InvalidData, SeekOverflow(())))
    |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    |
    = note: #[warn(or_fun_call)] on by default
help: try this
    |     o.ok_or_else(|| io::Error::new(io::ErrorKind::InvalidData, SeekOverflow(())))
    = help: for further information visit https://github.com/Manishearth/rust-clippy/wiki#or_fun_call

AIUI, this does not invoke allocations (SeekOverflow is a zero-sized struct (ZST) and ZSTs aren't actually allocated), and can be const-folded by the compiler. Thus, this ok_or_else shouldn't provide benefit over ok_or.

To make it a bit more obvious, here's an example that definitely doesn't invoke any allocations:

warning: use of `ok_or` followed by a function call
 --> src/lib.rs:4:38
  |
4 |     let x: Result<(), AtomicUsize> = Some(()).ok_or(AtomicUsize::new(0));
  |                                      ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  |
  = note: #[warn(or_fun_call)] on by default
help: try this
  |     let x: Result<(), AtomicUsize> = Some(()).ok_or_else(|| AtomicUsize::new(0));
  = help: for further information visit https://github.com/Manishearth/rust-clippy/wiki#or_fun_call

[oli-obk]

I thought we fixed this...

[Vlad-Shcherbina]

The problem is still there.

struct Zst {}

fn z(_: i32) -> Zst { Zst {} }

fn f(_: i32) -> i32 { 42 }

fn main() {
    let _ = Some(Zst {}).unwrap_or(z(1));
    let _ = Some(42).unwrap_or(f(1));
}

Both produce Clippy warnings:

warning: use of `unwrap_or` followed by a function call
 --> src\main.rs:8:26
  |
8 |     let _ = Some(Zst {}).unwrap_or(z(1));
  |                          ^^^^^^^^^^^^^^^ help: try this: `unwrap_or_else(|| z(1))`
  |
  = note: #[warn(clippy::or_fun_call)] on by default
  = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#or_fun_call

warning: use of `unwrap_or` followed by a function call
 --> src\main.rs:9:22
  |
9 |     let _ = Some(42).unwrap_or(f(1));
  |                      ^^^^^^^^^^^^^^^ help: try this: `unwrap_or_else(|| f(1))`
  |
  = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#or_fun_call

I think this warning should be suppressed (or moved to pedantic) for types that impl Copy.

---

Version info:

> cargo --version --verbose
cargo 1.37.0-nightly (545f35425 2019-05-23)
release: 1.37.0
commit-hash: 545f354259be4e9745ea00a524c0e4c51df01aa6
commit-date: 2019-05-23

> cargo clippy --version
clippy 0.0.212 (20da8f45 2019-06-03)

[upsuper]

The return type is Copy doesn't mean the function call itself is trivial. For example you can totally do

fn request_default_value() -> i32 {
    // do some network request to get the value
}
let _ = option.unwrap_or(request_default_value());

and in this case Clippy should definitely issue this warning.

But I think this situation is indeed improvable. I suggest that Clippy should not issue this warning if it's a const expression, i.e. its value can be computed at compile time.

AtomicUsize::new is already a const fn, thus this issue would be resolved. In @Vlad-Shcherbina's comment, apparently the two functions can be marked const as well (when const fn is stabilized).

[Ekleog]

Seeing the reasoning for closing #5658 ; should this not be closed too?

(Disclaimer: I came here wanting to report this exact same issue, and I do believe that my .or_else(Duration::from_secs(0)) would be clearer than adding the const clippy just made me add for most likely no performance loss, but anyway)

[Allen-Webb]

I hit this with clippy 0.0.212:
.unwrap_or(Duration::from_secs(0))

warning: use of `unwrap_or` followed by a function call
   --> src/linux.rs:105:14
    |
105 |             .unwrap_or(Duration::from_secs(0));
    |              ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `unwrap_or_else(|_| Duration::from_secs(0))`
    |
    = note: `#[warn(clippy::or_fun_call)]` on by default
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#or_fun_call

warning: 1 warning emitted

If this wasn't a constant function, it wouldn't be allowed for initialization of statics (e.g. static TIMEOUT: Duration = Duration::from_secs(15);). In my opinion any function that is allowed for the initialization of a static variable shouldn't trigger this warning.

[camsteffen]

Seeing the reasoning for closing #5658 ; should this not be closed too?

@Ekleog I agree. Closing.

For the original example, Clippy has no way of knowing how expensive io::Error::new will be.

In my opinion any function that is allowed for the initialization of a static variable shouldn't trigger this warning.

@Allen-Webb Building off the discussion in #5658, you could have static TIMEOUT: Duration = Duration::from_secs(compute_nth_prime(42)).