Make it clear that or_fun_call can be a false-positive
#9829
Conversation
|
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @llogiq (or someone else) soon. Please see the contribution instructions for more information. |
rust-highfive
added
the
S-waiting-on-review
|
I must admit that I don't understand why this lint should be in We lint this because of perf (because the function may allocate or perform work), not style. Alas, there are false positives (we are not likely to fix any time soon), but that doesn't mean the reason for linting has changed. We could reduce the false positive by creating an allow or deny list for this lint. That way, people would be able to specify which functions they want to avoid calling needlessly. |
|
As-is, the lint has many false positives, which probably fails to meet the bar for performance lints? I don't know the bar for these in clippy. A list of functions not to warn for doesn't seem feasible to me. A list of functions to warn for seems feasible, but will likely always be incomplete. |
|
|
Removed the recategorization, now it's only a documentation update. |
hrxi
force-pushed
the
pr_or_fun_call
branch
2 times, most recently
from
54f8ce2 to
f3036da
Compare
|
For the record, I'd be OK with recategorizing into |
|
I think you need to add an explicit |
Also move it to nursery so that the false-positives can be dealt with. CC rust-lang#8574
Thanks, I wasn't really aware of nursery being a thing. I'm new to clippy.
Thanks, done. :) |
|
Thank you, and sorry it took so much back-and-forth. @bors r+ |
|
☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test |
`unwrap_or_else_default` -> `unwrap_or_default` and improve resulting lint Resolves #10080 (though it doesn't implement exactly what's described there) This PR does the following: 1. Merges `unwrap_or_else_default.rs`'s code into `or_fun_call.rs` 2. Extracts the code to handle `unwrap_or(/* default value */)` and similar, and moves it into `unwrap_or_else_default` 3. Implements the missing functionality from #9342, e.g.,, to handle `or_insert_with(Default::default)` 4. Renames `unwrap_or_else_default` to `unwrap_or_default` (since the "new" lint handles both `unwrap_or` and `unwrap_or_else`, it seemed sensible to use the shortened name) This PR is currently two commits. The first implements 1-3, the second implements 4. A word about 2: the `or_fun_call` lint currently produces warnings like the following: ``` error: use of `unwrap_or` followed by a call to `new` --> $DIR/or_fun_call.rs:56:14 | LL | with_new.unwrap_or(Vec::new()); | ^^^^^^^^^^^^^^^^^^^^^ help: try: `unwrap_or_default()` ``` To me, such warnings look like they should come from `unwrap_or_else_default`, not `or_fun_call`, especially since `or_fun_call` is [in the nursery](#9829). --- changelog: Move: Renamed `unwrap_or_else_default` to [`unwrap_or_default`] [#10120](#10120) changelog: Enhancement: [`unwrap_or_default`]: Now handles more functions, like `or_insert_with` [#10120](#10120) <!-- changelog_checked-->
This cl adds clippy::or_fun_call warning to the bssl crate to opt in this check. The check is disabled by default since [1]. This helps us avoid work as in aosp/3008114. [1] rust-lang/rust-clippy#9829 Test: add an ok_or usage and check if there's warning Change-Id: I0519eaddc467f91f676eb0af24e68fb69fcf0586
Also move it to nursery so that the false-positives can be dealt with.
CC #8574
changelog: [
or_fun_call]: Mention false-positives, move to nursery.