Skip to content

Conversation

J-ZhengLi
Copy link
Member

@J-ZhengLi J-ZhengLi commented Mar 28, 2024

fixes: #12569
closes: #12580

change applicability to MaybeIncorrect when the expression contains a comment base on suggestion in this zulip discussion


changelog: fix [manual_unwrap_or_default] suggestion ignoring side-effects, and adjust its applicability.

@rustbot
Copy link
Collaborator

rustbot commented Mar 28, 2024

r? @Alexendoo

rustbot has assigned @Alexendoo.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Mar 28, 2024
@J-ZhengLi J-ZhengLi marked this pull request as draft March 28, 2024 10:03
@ARandomDev99
Copy link
Contributor

Related: #12553, #12580

@J-ZhengLi J-ZhengLi marked this pull request as ready for review March 29, 2024 01:54
@J-ZhengLi J-ZhengLi force-pushed the issue12569 branch 2 times, most recently from 497d2e2 to 9cafdbd Compare March 29, 2024 02:13
Applicability::MachineApplicable,
Applicability::MaybeIncorrect,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would be a shame to lose --fix in cases where it is appropriate, could set the applicability based on https://doc.rust-lang.org/nightly/nightly-rustc/clippy_utils/fn.span_contains_comment.html

Copy link
Member Author

@J-ZhengLi J-ZhengLi Mar 30, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks, forgot that I can do that

(kinda want to do some refractoring, having to put same code in different place everytime (same as #12566) is a little bit annoying lol)

@Alexendoo
Copy link
Member

Thanks!

@bors r+

@bors
Copy link
Contributor

bors commented Mar 30, 2024

📌 Commit 5750e46 has been approved by Alexendoo

It is now in the queue for this repository.

@bors
Copy link
Contributor

bors commented Mar 30, 2024

⌛ Testing commit 5750e46 with merge 88d842e...

@bors
Copy link
Contributor

bors commented Mar 30, 2024

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: Alexendoo
Pushing 88d842e to master...

@jqnatividad
Copy link
Contributor

I'm still getting a false positive with this lint with rustc 1.79.0-nightly (88c2f4f 2024-04-02):

cargo +nightly clippy -F all_features -- -W clippy::perf
...
warning: if let can be simplified with `.unwrap_or_default()`
   --> src/cmd/excel.rs:603:23
    |
603 |       let date_format = if let Some(df) = args.flag_date_format {
    |  _______________________^
604 | |         no_date_format = false;
605 | |         df
606 | |     } else {
607 | |         no_date_format = true;
608 | |         String::new()
609 | |     };
    | |_____^ help: replace it with: `args.flag_date_format.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

warning: `qsv` (bin "qsv") generated 1 warning (run `cargo clippy --fix --bin "qsv"` to apply 1 suggestion)
    Finished `dev` profile [unoptimized + debuginfo] target(s) in 58.70s

https://github.com/jqnatividad/qsv/blob/b542903c1da15280883d21b0616b46d411e238e9/src/cmd/excel.rs#L602-L612

@y21
Copy link
Member

y21 commented Apr 3, 2024

@jqnatividad This fix is not on nightly yet, since the clippy submodule in the rust-lang/rust repo (where the clippy rustup component is built from) is only updated every 2 weeks.
The next sync should happen tomorrow if memory serves me right?

You can also clone the clippy repo locally and run cargo dev lint /path/to/crate to see if it's fixed on master.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties
Projects
None yet
Development

Successfully merging this pull request may close these issues.

manual_unwrap_or_default does not take into account mutable code in the else branch manual_unwrap_or_default ignores side effect for None branch
7 participants