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

manual_unwrap_or_default ignores side effect for None branch #12569

Closed
crumblingstatue opened this issue Mar 26, 2024 · 2 comments · Fixed by #12579
Closed

manual_unwrap_or_default ignores side effect for None branch #12569

crumblingstatue opened this issue Mar 26, 2024 · 2 comments · Fixed by #12579
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

@crumblingstatue
Copy link

crumblingstatue commented Mar 26, 2024

Summary

manual_unwrap_or_default lint's suggestion ignores the side effect(s) present in the None branch.

Reproducer

fn main() {
    let result = match 1u32.checked_div(0) {
        Some(value) => value,
        None => {
            eprintln!("Hello, I'm a side effect");
            0
        },
    };
    eprintln!("{result}");
}
warning: match can be simplified with `.unwrap_or_default()`
 --> src/main.rs:2:18
  |
2 |       let result = match 1u32.checked_div(0) {
  |  __________________^
3 | |         Some(value) => value,
4 | |         None => {
5 | |             eprintln!("Hello, I'm a side effect");
6 | |             0
7 | |         },
8 | |     };
  | |_____^ help: replace it with: `1u32.checked_div(0).unwrap_or_default()`
  |
  = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#manual_unwrap_or_default
  = note: `#[warn(clippy::manual_unwrap_or_default)]` on by default

Clippy makes the side effect disappear with its suggestion.

Version

rustc 1.79.0-nightly (5f2c7d2bf 2024-03-25)
binary: rustc
commit-hash: 5f2c7d2bfd46cad00352ab7cd66242077e2e518c
commit-date: 2024-03-25
host: x86_64-unknown-linux-gnu
release: 1.79.0-nightly
LLVM version: 18.1.2

Additional Labels

No response

@crumblingstatue crumblingstatue added the C-bug Category: Clippy is not doing the correct thing label Mar 26, 2024
@J-ZhengLi
Copy link
Member

J-ZhengLi commented Mar 27, 2024

It will make sense to also make sure the comments are not discarded right? just like what has been done in [question_mark], such as:

let result = match 1u32.checked_div(0) {
      Some(value) => value,
      None => {
          // I'd like to keep this comment
          0
      },
  };

Oh, maybe and this, therefore there shouldn't be any comment inside the match expression's span entirely I guess, as they would all get removed by the suggestion.

let result = match 1u32.checked_div(0) {
      Some(value) => value,
      /// I'd like to keep this comment as well
      None => {
          0
      },
  };

@rustbot claim

@ARandomDev99
Copy link
Contributor

ARandomDev99 commented Mar 28, 2024

Related: #12553, #12580

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.

3 participants