unnecessary_wrap doesn't track usage, leading to false positives

[ltratt]

Lint name: unnecessary_wraps

I tried this code:

fn f() -> Option<i32> { Some(0) }
fn g() -> Option<i32> { Some(1) }

fn main() {
    let _: Vec<&(dyn Fn() -> Option<i32>)> = vec![&f, &g];
}

and was given this warning by Clippy:

warning: this function's return value is unnecessarily wrapped by `Option`
 --> src/main.rs:1:1
  |
1 | fn f() -> Option<i32> { Some(0) }
  | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  |
  = note: `#[warn(clippy::unnecessary_wraps)]` on by default
  = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#unnecessary_wraps
help: remove `Option` from the return type...
  |
1 | fn f() -> i32 { Some(0) }
  |           ^^^
help: ...and change the returning expressions
  |
1 | fn f() -> Option<i32> { 0 }
  |                         ^

warning: this function's return value is unnecessarily wrapped by `Option`
 --> src/main.rs:2:1
  |
2 | fn g() -> Option<i32> { Some(1) }
  | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  |
  = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#unnecessary_wraps
help: remove `Option` from the return type...
  |
2 | fn g() -> i32 { Some(1) }
  |           ^^^
help: ...and change the returning expressions
  |
2 | fn g() -> Option<i32> { 1 }
  |                         ^

warning: 2 warnings emitted

even though references are stored to the functions in a Vec where they must necessarily share the same type.

Just to check that Clippy isn't checking control / data flow (because the above example the types really can be simplified), I tried this slightly more involved example:

fn f(_: bool) -> Option<i32> { Some(0) }
fn g(x: bool) -> Option<i32> { if x { Some(1) } else { None } }

fn main() {
    let _: Vec<&(dyn Fn(bool) -> Option<i32>)> = vec![&f, &g];
}

which shows the same issue:

warning: this function's return value is unnecessarily wrapped by `Option`
 --> src/main.rs:1:1
  |
1 | fn f(_: bool) -> Option<i32> { Some(0) }
  | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  |
  = note: `#[warn(clippy::unnecessary_wraps)]` on by default
  = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#unnecessary_wraps
help: remove `Option` from the return type...
  |
1 | fn f(_: bool) -> i32 { Some(0) }
  |                  ^^^
help: ...and change the returning expressions
  |
1 | fn f(_: bool) -> Option<i32> { 0 }
  |                                ^

In both cases, it's impossible to refactor the functions in question, so the lint warning is problematic.

I am a big fan of Clippy, and have learnt to trust it almost implicitly, but unnecessary_wraps is definitely causing me more work than normal, with the above example a case of a definite false positive, but I have other more "questionable" cases too. The basic problem is that authors often know that a function might return an error condition in the future so they use Option/Result even when it is not currently necessary, to avoid future API churn. I fear that the unnecessary_wraps lint will unintentionally discourage people from doing so, thereby causing the community more work in the long run and/or more fragile APIs. [Edit: Yair Halberstadt pointed out that the lint only triggers on private functions, which is good, but I'm seeing the problem on intra-crate APIs too.] I do wonder if it should be opt-in, rather than opt-out. Please don't feel that I'm complaining -- as I've said, I really like Clippy, and it's made large chunks of my Rust code much better!

Meta

• cargo clippy -V: clippy 0.1.51 (4253153 2021-01-17)
• rustc -Vv:

  rustc 1.51.0-nightly (4253153db 2021-01-17)
  binary: rustc
  commit-hash: 4253153db205251f72ea4493687a31e04a2a8ca0
  commit-date: 2021-01-17
  host: x86_64-unknown-linux-gnu
  release: 1.51.0-nightly
  LLVM version: 11.0.1

[camsteffen]

FWIW, here are a couple workarounds. I don't know if it's feasible for clippy to analyze usage in that way. Personally, I think these workarounds are better since you are only creating an Option when you actually need it.

fn main() {
    let _: Vec<&(dyn Fn() -> Option<i32>)> = vec![
        &|| Some(f()), // use a closure
        &then_some(f), // use a helper to "decorate" the function
    ];
}

fn f() -> i32 { 0 }

fn then_some(fun: impl Fn() -> i32 + 'static) -> impl Fn() -> Option<i32> {
    move || Some(fun())
}

[ltratt]

@camsteffen I had not considered that sort of work around -- it's quite cute!

Unfortunately it's not obvious to me that it's feasible in one case I have, which is in code generated by lrpar (https://crates.io/crates/lrpar) when the user specifies productions that return Result. Each production is turned into a function, but some of those might have a Result type which returns a fixed Ok (or Err) e.g. this grammar https://github.com/softdevteam/grmtools/blob/master/lrpar/examples/calc_ast/src/calc.y has this production:

   --> /home/ltratt/grmtools/target/debug/build/calc_ast-e233dad0b0ce54bc/out/calc.y.rs:297:5
    |
297 | /     fn __gt_action_5<'lexer, 'input: 'lexer>(__gt_ridx: ::cfgrammar::RIdx<u32>,
298 | |                      __gt_lexer: &'lexer dyn ::lrpar::NonStreamingLexer<'input, u32>,
299 | |                      __gt_span: ::lrpar::Span,
300 | |                      mut __gt_arg_1: ::std::result::Result<::lrpar::Lexeme<u32>, ::lrpar::Lexeme<u32>>) 
301 | | ->                  Result<Expr, ()> {
302 | | Ok(Expr::Number{ span: __gt_span })
303 | |     }
    | |_____^

Admittedly, in this case I could add #![allow(clippy::unnecessary_wraps)] to each generated function, although I can't (easily) know if it's needed or not. In some other cases I've got, though, I'm not lucky enough to have generated code.

[camsteffen]

@camsteffen I had not considered that sort of work around -- it's quite cute!

Thanks! 😆

Interesting. That does seem like a valid use case, though not a very typical one. Could the generator just allow the lint for the whole file or module?

[ltratt]

The generator could do that, but up until now I've felt that it's presumptuous to suppress warnings when the user didn't ask me to. I think I might need to rethink that!

[camsteffen]

Well is code generated by a library the library's code? I say yes, at least kinda. I don't think anyone will accuse you of being presumptuous anyways 😄.

[bowlofeggs]

Another example of usage mattering is serde with a default value on a field. Consider something like this:

#[derive(Deserialize)]
struct TheBestYouveEverSeen {
    #[serde(default = "TheBestYouveEverSeen::default_cool_member")]
    cool_member: Option<u8>
}

impl TheBestYouveEverSeen {
    fn default_cool_member() -> Option<u8> {
        Some(2)
    }
}

Here, we do want to return a Some always, and changing the return type to a u8 won't work since cool_member does need to be an Option.