`unwrap_or_else_default` -> `unwrap_or_default` and improve resulting lint #10120

smoelius · 2022-12-26T13:22:33Z

Resolves #10080 (though it doesn't implement exactly what's described there)

This PR does the following:

Merges unwrap_or_else_default.rs's code into or_fun_call.rs
Extracts the code to handle unwrap_or(/* default value */) and similar, and moves it into unwrap_or_else_default
Implements the missing functionality from Suggest Entry::or_default for Entry::or_insert(Default::default()) #9342, e.g.,, to handle or_insert_with(Default::default)
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.

changelog: Move: Renamed unwrap_or_else_default to [unwrap_or_default]
#10120
changelog: Enhancement: [unwrap_or_default]: Now handles more functions, like or_insert_with
#10120

rustbot · 2022-12-26T13:22:37Z

r? @flip1995

(rustbot has picked a reviewer for you, use r? to override)

bors · 2023-01-12T15:15:40Z

☔ The latest upstream changes (presumably #10184) made this pull request unmergeable. Please resolve the merge conflicts.

smoelius · 2023-02-26T22:05:24Z

@flip1995 Would it be helpful if I split this into two PRs: one for the false positive fix (the biggest change), and one for everything else?

"try this" -> "try" Current help messages contain a mix of "try", "try this", and one "try this instead". In the spirit of #10631, this PR adopts the first, as it is the most concise. It also updates the `lint_message_conventions` test to catch cases of "try this". (Aside: #10120 unfairly contained multiple changes in one PR. I am trying to break that PR up into smaller pieces.) changelog: Make help messages more concise ("try this" -> "try").

Fix `unwrap_or_else_default` false positive This PR fixes a false positive in the handling of `unwrap_or_else` with a default value when the value is needed for type inference. An easy example to exhibit the false positive is the following: ```rust let option = None; option.unwrap_or_else(Vec::new).push(1); ``` The following code would not compile, because the fact that the value is a `Vec` has been lost: ```rust let option = None; option.unwrap_or_default().push(1); ``` The fix is to: - implement a heuristic to tell whether an expression's type can be determined purely from its subexpressions, and the arguments and locals they use; - apply the heuristic to `unwrap_or_else`'s receiver. The heuristic returns false when applied to `option` in the above example, but it returns true when applied to `option` in either of the following examples: ```rust let option: Option<Vec<u64>> = None; option.unwrap_or_else(Vec::new).push(1); ``` ```rust let option = None::<Vec<u64>>; option.unwrap_or_else(Vec::new).push(1); ``` (Aside: #10120 unfairly contained multiple changes in one PR. I am trying to break that PR up into smaller pieces.) --- changelog: FP: [`unwrap_or_else_default`]: No longer lints if the default value is needed for type inference

smoelius · 2023-07-20T01:08:03Z

@flip1995 Is this PR more reasonable now?

flip1995 · 2023-07-20T07:19:43Z

Sorry, I don't have time to review currently. It would take be too long, even though the PR size now looks better.

r? @blyxyas do you mind taking over?

rustbot · 2023-07-20T07:19:46Z

Failed to set assignee to blyxyas: invalid assignee

Note: Only org members, users with write permissions, or people who have commented on the PR may be assigned.

blyxyas · 2023-07-20T07:31:12Z

I don't mind taking over. Even better, I will take over. Yeehaw 🤠

flip1995 · 2023-07-20T07:32:36Z

@bors delegate=blyxyas

In case you're quicker with the review, than getting your bors rights.

Thanks for taking over! :)

bors · 2023-07-20T07:32:38Z

✌️ @blyxyas, you can now approve this pull request!

If @flip1995 told you to "r=me" after making some further change, please make that change, then do @bors r=@flip1995

blyxyas · 2023-07-20T07:45:56Z

Oh, rust-lang/team#1031 just got merged 🎉

flip1995 · 2023-07-20T07:52:04Z

Nice, now you have all the rights and with #11194 you'll be picked as a reviewer automatically in the future.

blyxyas · 2023-07-20T21:02:15Z

@bors r? blyxyas

flip1995 · 2023-07-21T09:15:22Z

The r? "command" is not a bors command, but something that rustbot will pick up. So you don't have to bother bors with this 🙃 r? @blyxyas would've been enough.

blyxyas · 2023-07-20T07:54:43Z

clippy_lints/src/methods/mod.rs

-    /// Checks for usage of `_.unwrap_or_else(Default::default)` on `Option` and
-    /// `Result` values.
+    /// Checks for usages of the following functions with an argument that constructs a default value
+    /// (e.g., `Default::default` or `String::new`):


Suggested change

/// (e.g., `Default::default` or `String::new`):

/// (e.g. `Default::default` or `String::new`):

Both are correct and used a similar amount of times in rustc (512 without the comma vs 680 with the comma). The preferred spelling differs from place to place though.

Just a personal preference, but I feel like it's weird to have 2 punctuation signs right next to each other and it kinda worsens readability.

I personally find it easier to read as it keeps the punctuation mark only being used at the end of a sentence. That's subjective though ^^

I feel like it's weird to have 2 punctuation signs right next to each other

@blyxyas Personally, I agree with you 100%.

But literally every English writing guide I have consulted says to put a ',' after "e.g.".

So I think the question comes down to: should Clippy comments follow English writing guides?

(And, to state the obvious, this is a total bike shed. 😀)

I'm fine with either. It's bikeshedding and won't cause confusion anyway.

In American English, a comma is always placed after “i.e.” or “e.g.” In fact, this is a punctuation rule on which both the AP Style Book and the Chicago Manual of Style agree. However, in British English, a comma is not used after “i.e.” or “e.g.”

Mmmm... Guess it doesn't matter. It just looks weird, I don't know and honestly, it isn't that big of a problem.
We can continue designing the power plant, do what you consider the best 🤠

blyxyas · 2023-07-22T13:32:55Z

clippy_lints/src/methods/or_fun_call.rs

+            if let Some(sugg) = match (name, call_expr.is_some()) {
+                ("unwrap_or", true) | ("unwrap_or_else", false) => Some("unwrap_or_default"),
+                ("or_insert", true) | ("or_insert_with", false) => Some("or_default"),
                _ => None,
            };


This if statement could be replaced by an early return (the compiler probably optimizes this, but just to be sure)

Is the change I made the one you had in mind?

blyxyas · 2023-07-22T13:36:16Z

clippy_lints/src/methods/or_fun_call.rs

+        if body.params.is_empty()
+            && let hir::Expr{ kind, .. } = &body.value
+            && let hir::ExprKind::MethodCall(hir::PathSegment {ident, ..}, self_arg, _, _) = kind
+            && ident == &symbol::Ident::from_str("to_string")


Unnecessary allocation here 🐄

Suggested change

&& ident == &symbol::Ident::from_str("to_string")

&& ident.name == sym::to_string

tests/ui/or_fun_call.fixed

blyxyas · 2023-07-23T11:56:44Z

I think this is pretty much ready. Just a final question, @xFrednet, what do you mean by "Not handles more functions" 😅

xFrednet · 2023-07-23T20:21:21Z

@blyxyas this confused me for a solid minute ^^. It's just the usual amount of typos in my artfully crafted changelog entries. You noticed it and passed the test 😉. Here have a Cookie: 🍪

blyxyas · 2023-07-23T20:28:02Z

Okis ^w^
@bors r+

bors · 2023-07-23T20:28:03Z

📌 Commit 99202a0 has been approved by blyxyas

It is now in the queue for this repository.

bors · 2023-07-23T20:28:10Z

⌛ Testing commit 99202a0 with merge e4923c2...

bors · 2023-07-23T20:43:05Z

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: blyxyas
Pushing e4923c2 to master...

smoelius · 2023-07-23T22:40:30Z

Thanks very much for the review, @blyxyas.

rustbot assigned flip1995 Dec 26, 2022

rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Dec 26, 2022

smoelius force-pushed the or_insert_with branch from 5864200 to 8b1c3e2 Compare December 26, 2022 13:30

smoelius mentioned this pull request Jan 9, 2023

or_fun_call isn't always a performance lint #8574

Open

smoelius mentioned this pull request Jul 1, 2023

"try this" -> "try" #11055

Merged

smoelius mentioned this pull request Jul 9, 2023

Fix unwrap_or_else_default false positive #11135

Merged

smoelius marked this pull request as draft July 9, 2023 23:30

smoelius added 2 commits July 19, 2023 20:36

Merge unwrap_or_else_default.rs into or_fun_call.rs

84c4112

Rename unwrap_or_else_default to unwrap_or_default

e27977b

smoelius force-pushed the or_insert_with branch from 8b1c3e2 to e27977b Compare July 20, 2023 00:49

smoelius marked this pull request as ready for review July 20, 2023 01:04

rustbot assigned blyxyas and unassigned flip1995 Jul 20, 2023

blyxyas reviewed Jul 22, 2023

View reviewed changes

Start addressing review comments

99202a0

bors merged commit e4923c2 into rust-lang:master Jul 23, 2023
8 checks passed

smoelius deleted the or_insert_with branch July 23, 2023 22:40

Centri3 mentioned this pull request Jul 25, 2023

[needless_pass_by_ref_mut]: Do not lint if passed as a fn-like argument #11207

Merged

blyxyas mentioned this pull request Oct 3, 2023

Changelog for Rust 1.73 🖊️ #11605

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

`unwrap_or_else_default` -> `unwrap_or_default` and improve resulting lint #10120

`unwrap_or_else_default` -> `unwrap_or_default` and improve resulting lint #10120

smoelius commented Dec 26, 2022 •

edited by xFrednet

Loading

rustbot commented Dec 26, 2022

bors commented Jan 12, 2023

smoelius commented Feb 26, 2023

smoelius commented Jul 20, 2023

flip1995 commented Jul 20, 2023

rustbot commented Jul 20, 2023

blyxyas commented Jul 20, 2023

flip1995 commented Jul 20, 2023

bors commented Jul 20, 2023

blyxyas commented Jul 20, 2023 •

edited

Loading

flip1995 commented Jul 20, 2023

blyxyas commented Jul 20, 2023

flip1995 commented Jul 21, 2023

blyxyas Jul 20, 2023

Centri3 Jul 22, 2023

blyxyas Jul 22, 2023

Centri3 Jul 22, 2023

smoelius Jul 22, 2023

Centri3 Jul 22, 2023

blyxyas Jul 22, 2023 •

edited

Loading

blyxyas Jul 22, 2023

smoelius Jul 22, 2023

blyxyas Jul 22, 2023

blyxyas Jul 22, 2023

blyxyas commented Jul 23, 2023

xFrednet commented Jul 23, 2023

blyxyas commented Jul 23, 2023

bors commented Jul 23, 2023

bors commented Jul 23, 2023

bors commented Jul 23, 2023

smoelius commented Jul 23, 2023

	/// (e.g., `Default::default` or `String::new`):
	/// (e.g. `Default::default` or `String::new`):

	&& ident == &symbol::Ident::from_str("to_string")
	&& ident.name == sym::to_string

unwrap_or_else_default -> unwrap_or_default and improve resulting lint #10120

unwrap_or_else_default -> unwrap_or_default and improve resulting lint #10120

Conversation

smoelius commented Dec 26, 2022 • edited by xFrednet Loading

rustbot commented Dec 26, 2022

bors commented Jan 12, 2023

smoelius commented Feb 26, 2023

smoelius commented Jul 20, 2023

flip1995 commented Jul 20, 2023

rustbot commented Jul 20, 2023

blyxyas commented Jul 20, 2023

flip1995 commented Jul 20, 2023

bors commented Jul 20, 2023

blyxyas commented Jul 20, 2023 • edited Loading

flip1995 commented Jul 20, 2023

blyxyas commented Jul 20, 2023

flip1995 commented Jul 21, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

blyxyas Jul 22, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

blyxyas commented Jul 23, 2023

xFrednet commented Jul 23, 2023

blyxyas commented Jul 23, 2023

bors commented Jul 23, 2023

bors commented Jul 23, 2023

bors commented Jul 23, 2023

smoelius commented Jul 23, 2023

`unwrap_or_else_default` -> `unwrap_or_default` and improve resulting lint #10120

`unwrap_or_else_default` -> `unwrap_or_default` and improve resulting lint #10120

smoelius commented Dec 26, 2022 •

edited by xFrednet

Loading

blyxyas commented Jul 20, 2023 •

edited

Loading

blyxyas Jul 22, 2023 •

edited

Loading