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

Invalid suggestion of option_if_let_else causes a compilation error error[E0382]: Borrow of moved value #6737

Closed
xFrednet opened this issue Feb 13, 2021 · 1 comment · Fixed by #7573
Labels
C-bug Category: Clippy is not doing the correct thing E-medium Call for participation: Medium difficulty level problem and requires some initial experience. I-suggestion-causes-error Issue: The suggestions provided by this Lint cause an ICE/error when applied L-pedantic Lint: Belongs in the pedantic lint group

Comments

@xFrednet
Copy link
Member

I tried this code:

#[allow(dead_code, clippy::unnecessary_wraps)]
#[deny(clippy::option_if_let_else)]
fn test_fn(acc: Option<String>) -> Option<String> {
    // Computation of mv_value
    let mv_value = "Hello clippy".to_string();
    
    if let Some(mut x) = acc {
        x.push_str(&mv_value);
        Some(x)
    } else {
        Some(mv_value)
    }
}

and got the following lint trigger:

error: use Option::map_or instead of an if let/else
  --> src/main.rs:8:5
   |
8  | /     if let Some(mut x) = acc {
9  | |         x.push_str(&mv_value);
10 | |         Some(x)
11 | |     } else {
12 | |         Some(mv_value)
13 | |     }
   | |_____^
   |
note: the lint level is defined here
  --> src/main.rs:3:8
   |
3  | #[deny(clippy::option_if_let_else)]
   |        ^^^^^^^^^^^^^^^^^^^^^^^^^^
   = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#option_if_let_else
help: try
   |
8  |     acc.map_or(Some(mv_value), |mut x| {
9  |         x.push_str(&mv_value);
10 |         Some(x)
11 |     })
   |

Using the suggested code leads to:

error[E0382]: borrow of moved value: `mv_value`
  --> src/main.rs:16:32
   |
6  |     let mv_value = "Hello clippy".to_string();
   |         -------- move occurs because `mv_value` has type `String`, which does not implement the `Copy` trait
...
16 |     acc.map_or(Some(mv_value), |mut x| {
   |                     --------   ^^^^^^^ value borrowed here after move
   |                     |
   |                     value moved here
17 |         x.push_str(&mv_value);
   |                     -------- borrow occurs due to use in closure

Playground

The if let is used in this case to use mv_value in both branches. This is a limitation of the map_or suggestion.

Meta

  • cargo clippy -V: clippy 0.1.51 (3682750 2021-02-02)
  • rustc -Vv:
    rustc 1.51.0-nightly (368275062 2021-02-02)
    binary: rustc
    commit-hash: 368275062fb655c1f36e0398f88b15379a1f3c93
    commit-date: 2021-02-02
    host: x86_64-unknown-linux-gnu
    release: 1.51.0-nightly
    LLVM version: 11.0.1
    

Keep up the good work 📎 🐧

@xFrednet
Copy link
Member Author

@rustbot label +C-bug +I-suggestion-causes-error +L-pedantic

@rustbot rustbot added C-bug Category: Clippy is not doing the correct thing I-suggestion-causes-error Issue: The suggestions provided by this Lint cause an ICE/error when applied L-pedantic Lint: Belongs in the pedantic lint group labels Feb 13, 2021
@camsteffen camsteffen added the E-medium Call for participation: Medium difficulty level problem and requires some initial experience. label Feb 16, 2021
bors added a commit that referenced this issue Aug 15, 2021
Downgrade option_if_let_else to nursery

I believe that this lint's loose understanding of ownership (#5822, #6737) makes it unsuitable to be enabled by default in its current state, even as a pedantic lint.

Additionally the lint has known problems with type inference (#6137), though I may be willing to consider this a non-blocker in isolation if it weren't for the ownership false positives.

A fourth false positive involving const fn: #7567.

But on top of these, for me the biggest issue is I basically fully agree with #6137 (comment). In my experience this lint universally makes code worse even when the resulting code does compile.

---

changelog: remove [`option_if_let_else`] from default set of enabled lints
@bors bors closed this as completed in fd30241 Aug 30, 2021
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 E-medium Call for participation: Medium difficulty level problem and requires some initial experience. I-suggestion-causes-error Issue: The suggestions provided by this Lint cause an ICE/error when applied L-pedantic Lint: Belongs in the pedantic lint group
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants