Rust-clippy: unnecessary_wraps has various bugs

[jyn514]

I tried this code: https://github.com/rust-lang/rust/blob/5be3f9f10e9fd59ea03816840a6051413fbdefae/compiler/rustc_data_structures/src/graph/scc/mod.rs#L268

I expected to see this happen:

1. unnecessary_wraps should not fire, because this is a trait implementation and the trait should not be changed.
2. If unnecessary_wraps does fire, the suggestion should be correct.

Instead, this happened:

error[E0308]: mismatched types
   --> compiler/rustc_data_structures/src/graph/scc/mod.rs:268:49
    |
266 |     fn inspect_node(&mut self, node: G::Node) -> Option<WalkReturn<S>> {
    |                                                  --------------------- expected `std::option::Option<graph::scc::WalkReturn<S>>` because of return type
267 |         match self.find_state(node) {
268 |             NodeState::InCycle { scc_index } => WalkReturn::Complete { scc_index },
    |                                                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    |                                                 |
    |                                                 expected enum `std::option::Option`, found enum `graph::scc::WalkReturn`
    |                                                 help: try using a variant of the expected enum: `Some(WalkReturn::Complete { scc_index })`
    |
    = note: expected enum `std::option::Option<graph::scc::WalkReturn<_>>`
               found enum `graph::scc::WalkReturn<_>`
Original diagnostics will follow.

warning: this function's return value is unnecessarily wrapped by `Option`
   --> compiler/rustc_data_structures/src/graph/scc/mod.rs:266:5
    |
266 | /     fn inspect_node(&mut self, node: G::Node) -> Option<WalkReturn<S>> {
267 | |         Some(match self.find_state(node) {
268 | |             NodeState::InCycle { scc_index } => WalkReturn::Complete { scc_index },
269 | |
...   |
278 | |         })
279 | |     }
    | |_____^
    |
    = 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...
    |
266 |     fn inspect_node(&mut self, node: G::Node) -> graph::scc::WalkReturn<S> {
    |                                                  ^^^^^^^^^^^^^^^^^^^^^^^^^
help: ...and change the returning expressions
    |
267 |         match self.find_state(node) {
268 |             NodeState::InCycle { scc_index } => WalkReturn::Complete { scc_index },
269 | 
270 |             NodeState::BeingVisited { depth: min_depth } => WalkReturn::Cycle { min_depth },
271 | 
272 |             NodeState::NotVisited => return None,

Meta

• cargo clippy -V: clippy 0.0.212 (f4db9ff 2020-12-02)
• rustc -Vv:

rustc 1.50.0-nightly (f4db9ffb2 2020-12-02)
binary: rustc
commit-hash: f4db9ffb22dfcb702dbdb2e0607cb91791866b57
commit-date: 2020-12-02
host: x86_64-unknown-linux-gnu
release: 1.50.0-nightly

[jyn514]

Another similar issue: https://github.com/rust-lang/rust/blob/5be3f9f10e9fd59ea03816840a6051413fbdefae/library/test/src/cli.rs#L400-L404. This can be modified, but is modified incorrectly.

error[E0308]: mismatched types
   --> library/test/src/cli.rs:403:5
    |
400 | fn get_filter(matches: &getopts::Matches) -> OptPartRes<Option<String>> {
    |                                              -------------------------- expected `std::result::Result<std::option::Option<std::string::String>, std::string::String>` b
ecause of return type
...
403 |     filter
    |     ^^^^^^
    |     |
    |     expected enum `std::result::Result`, found enum `std::option::Option`
    |     help: try using a variant of the expected enum: `Ok(filter)`
    |
    = note: expected enum `std::result::Result<std::option::Option<_>, std::string::String>`
               found enum `std::option::Option<_>`

Original diagnostics will follow.

warning: this function's return value is unnecessarily wrapped by `Result`
   --> library/test/src/cli.rs:400:1
    |
400 | / fn get_filter(matches: &getopts::Matches) -> OptPartRes<Option<String>> {
401 | |     let filter = if !matches.free.is_empty() { Some(matches.free[0].clone()) } else { None };
402 | |
403 | |     Ok(filter)
404 | | }
    | |_^
    |
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#unnecessary_wraps
help: remove `Result` from the return type...
    |
400 | fn get_filter(matches: &getopts::Matches) -> std::option::Option<std::string::String> {
    |                                              ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
help: ...and change the returning expressions
    |
403 |     filter

[jyn514]

I think in general this lint shouldn't be marked MachineApplicable, since you need to update all callers as well.

[flip1995]

1. unnecessary_wraps should not fire, because this is a trait implementation and the trait should not be changed.

The code you linked isn't a trait implementation. Did you accidentally link the wrong code? I just verified, that it doesn't trigger on trait impls/defs: Playground. I remember leaving this exact review.

1. If unnecessary_wraps does fire, the suggestion should be correct.

The suggestion looks correct to me. It suggests to remove the Option around the return value and tells you to change all returns.

I think in general this lint shouldn't be marked MachineApplicable, since you need to update all callers as well.

That I agree with. The problem was, that the return value suggestion was MaybeIncorrect but the suggestions for the return sites was MachineApplicable. I'll change that.

[jyn514]

The code you linked isn't a trait implementation.

Oops, you're right, sorry. I got confused by all the where bounds 😅

The suggestion looks correct to me.

The suggestion shown is correct, but the suggestion applied is not. Maybe this is a rustfix bug? It only applied the second suggestion, not the first.

[flip1995]

Maybe this is a rustfix bug? It only applied the second suggestion, not the first.

Nah, it's something I missed during review. The suggestions this produces are split up in multiple calls to diag.span_suggestion(..) which had different applicabilities. #6418 sets the applicability of every suggestion to MaybeIncorrect, which should solve this issue.