Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

False positive branches_sharing_code: trailing expression in else of if-let #7054

Closed
dtolnay opened this issue Apr 9, 2021 · 1 comment · Fixed by #7075
Closed

False positive branches_sharing_code: trailing expression in else of if-let #7054

dtolnay opened this issue Apr 9, 2021 · 1 comment · Fixed by #7075
Assignees
Labels
C-bug Category: Clippy is not doing the correct thing I-false-positive Issue: The lint was triggered on code it shouldn't have

Comments

@dtolnay
Copy link
Member

dtolnay commented Apr 9, 2021

fn main() {
    let string;
    let _x = if let true = true {
        ""
    } else if true {
        string = "x".to_owned();
        &string
    } else {
        string = "y".to_owned();
        &string
    };
}
$ cargo clippy
warning: all if blocks contain the same code at the end
  --> src/main.rs:10:5
   |
10 | /         &string
11 | |     };
   | |_____^
   |
   = note: `#[warn(clippy::branches_sharing_code)]` on by default
   = note: The end suggestion probably needs some adjustments to use the expression result correctly
   = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#branches_sharing_code
help: consider moving the end statements out like this
   |
10 |     }
11 |     &string;
   |

Clippy's preferred code is the following, which does not compile.

fn main() {
    let string;
    let _x = if let true = true {
        ""
    } else if true {
        string = "x".to_owned();
    } else {
        string = "y".to_owned();
    }
    &string;
}
error[E0308]: `if` and `else` have incompatible types
 --> src/main.rs:5:12
  |
3 |        let _x = if let true = true {
  |   ______________-
4 |  |         ""
  |  |         -- expected because of this
5 |  |     } else if true {
  |  |____________^
6 | ||         string = "x".to_owned();
7 | ||     } else {
8 | ||         string = "y".to_owned();
9 | ||     }
  | ||     ^
  | ||_____|
  | |______`if` and `else` have incompatible types
  |        expected `&str`, found `()`

It's possible that clippy means the following, but I still consider this a false positive because I find the original code clearer than this.

fn main() {
    let string;
    let _x = if let true = true {
        ""
    } else {
        if true {
            string = "x".to_owned();
        } else {
            string = "y".to_owned();
        }
        &string
    };
}

Meta

  • cargo clippy -V: clippy 0.1.53 (2e495d2 2021-04-08)
  • rustc -Vv:
    rustc 1.53.0-nightly (2e495d2e8 2021-04-08)
    binary: rustc
    commit-hash: 2e495d2e845cf27740e3665f718acfd3aa17253e
    commit-date: 2021-04-08
    host: x86_64-unknown-linux-gnu
    release: 1.53.0-nightly
    LLVM version: 12.0.0
    

Mentioning @xFrednet @phansch #6463

@dtolnay dtolnay added C-bug Category: Clippy is not doing the correct thing I-false-positive Issue: The lint was triggered on code it shouldn't have labels Apr 9, 2021
dtolnay added a commit to serde-rs/serde that referenced this issue Apr 9, 2021
rust-lang/rust-clippy#7054

    error: all if blocks contain the same code at the end
        --> serde_derive/src/de.rs:2160:5
         |
    2160 | /         &fallthrough_arm_tokens
    2161 | |     };
         | |_____^
         |
    note: the lint level is defined here
        --> serde_derive/src/lib.rs:18:9
         |
    18   | #![deny(clippy::all, clippy::pedantic)]
         |         ^^^^^^^^^^^
         = note: `#[deny(clippy::branches_sharing_code)]` implied by `#[deny(clippy::all)]`
         = note: The end suggestion probably needs some adjustments to use the expression result correctly
         = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#branches_sharing_code
    help: consider moving the end statements out like this
         |
    2160 |     }
    2161 |     &fallthrough_arm_tokens;
         |
@xFrednet
Copy link
Member

xFrednet commented Apr 9, 2021

This is also a FP. We've decided in the PR that only whole if blocks should be linted (not single else ifs). My guess is that it's caused by the same problem like your other issue but I'll have to dig into it. :)

@rustbot claim

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-bug Category: Clippy is not doing the correct thing I-false-positive Issue: The lint was triggered on code it shouldn't have
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants