fix: `manual_unwrap_or_default` suggests error when expression is a None variant #12688

t-webber · 2024-04-17T23:10:24Z

Suggesting error when expression is none : None.unwrap_or_default() raises an error, as explained in the following issue:

fixes #12670

changelog: [manual_unwrap_or_default]: the lint is ignored when matching None, as the unwrap_or_default method can't infer the type of a None variant.

rustbot · 2024-04-17T23:10:30Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @Jarcho (or someone else) some time within the next two weeks.

Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (S-waiting-on-review and S-waiting-on-author) stays updated, invoking these commands when appropriate:

@rustbot author: the review is finished, PR author should check the comments and take action accordingly
@rustbot review: the author is ready for a review, this PR will be queued again in the reviewer's queue

clippy_lints/src/manual_unwrap_or_default.rs

rustbot · 2024-04-18T10:32:33Z

There are merge commits (commits with multiple parents) in your changes. We have a no merge policy so these commits will need to be removed for this pull request to be merged.

You can start a rebase with the following commands:

$ # rebase
$ git rebase -i master
$ # delete any merge commits in the editor that appears
$ git push --force-with-lease

The following commits are merge commits:

f97d38e

rustbot · 2024-04-18T10:40:12Z

There are merge commits (commits with multiple parents) in your changes. We have a no merge policy so these commits will need to be removed for this pull request to be merged.

You can start a rebase with the following commands:

$ # rebase
$ git rebase -i master
$ # delete any merge commits in the editor that appears
$ git push --force-with-lease

The following commits are merge commits:

clippy_lints/src/manual_unwrap_or_default.rs

bors · 2024-06-07T10:12:52Z

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

tests/ui/manual_unwrap_or_default.rs

Jarcho · 2024-06-08T00:33:11Z

clippy_lints/src/manual_unwrap_or_default.rs

+        if let Some(none_def_id) = cx.tcx.lang_items().option_none_variant() {
+            if let ExprKind::Path(QPath::Resolved(_, path)) = &condition.kind {
+                if let Some(def_id) = path.res.opt_def_id() {
+                    if cx.tcx.parent(def_id) == none_def_id {


This is equivalent to path_res(cx, condition).opt_def_id().is_some_and(|id| Some(cx.tcx.parent(id)) == cx.tcx.lang_items().option_none_variant())

Jarcho · 2024-06-08T00:57:33Z

clippy_lints/src/manual_unwrap_or_default.rs

+        // We check if the expression is not a method or function with a unspecified return type
+        if let ExprKind::MethodCall(_, expr, _, _) = &condition.kind {
+            if let ty::Adt(_, substs) = cx.typeck_results().expr_ty(expr).kind() {
+                if let Some(ok_type) = substs.first() {
+                    return span_lint_and_sugg(
+                        cx,
+                        MANUAL_UNWRAP_OR_DEFAULT,
+                        expr.span,
+                        format!("{expr_name} can be simplified with `.unwrap_or_default()`"),
+                        format!("explicit the type {ok_type} and replace your expression with"),
+                        format!("{receiver}.unwrap_or_default()"),
+                        Applicability::Unspecified,
+                    );
+                }
+            }
+        }


What are you trying to catch with this? This looks like it will just catch any scrutinee that happens to be a method call.

I catch all methods and functions that return a generic type that can't be inferred from it's arguments (e.g. "1".parse()) ?

We have clippy_utils::ty::expr_type_is_certain for that.

Jarcho · 2024-06-08T01:03:34Z

clippy_lints/src/manual_unwrap_or_default.rs

+                            expr.span,
+                            format!("{expr_name} can be simplified with `.unwrap_or_default()`"),
+                            "replace it with",
+                            format!("{receiver}::</* Type */>.unwrap_or_default()"),


You can print any Ty which will print the type's full name. The following should work:

if let ty::Adt(_, args) = cx.typeck_results().expr_ty(expr).kind() && let Some(arg) = args.first() && let Some(arg_ty) = arg.as_type() { format!("{receiver}::<{}>.unwrap_or_default()", arg_ty) }

Jarcho · 2024-07-03T19:19:52Z

@rustbot author

xFrednet · 2024-08-03T08:26:47Z

Hey this is triage, I'm closing this due to inactivity. If you want to continue this implementation, you're welcome to create a new PR. Thank you for the time, you already put into this!

Interested parties are welcome to pick this implementation up as well :)

@rustbot label +S-inactive-closed -S-waiting-on-author -S-waiting-on-review

profetia · 2024-12-28T12:43:18Z

Hi forks! I would like to pick up this implementation, looks like there aren't much left.

@Jarcho

…is uncertain (#13889) fixes #12670 Continuation of #12688. r? @Jarcho if you don't mind? changelog: [`manual_unwrap_or_default`] fix wrong suggestions when condition type is uncertain

rustbot assigned Jarcho Apr 17, 2024

rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Apr 17, 2024

J-ZhengLi reviewed Apr 18, 2024

View reviewed changes

clippy_lints/src/manual_unwrap_or_default.rs Outdated Show resolved Hide resolved

rustbot added has-merge-commits PR has merge commits, merge with caution. S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) labels Apr 18, 2024

t-webber force-pushed the fix_manual_unwrap_or_default branch from dcaf974 to 1fd8241 Compare April 18, 2024 10:38

y21 reviewed Apr 18, 2024

View reviewed changes

clippy_lints/src/manual_unwrap_or_default.rs Outdated Show resolved Hide resolved

clippy_lints/src/manual_unwrap_or_default.rs Outdated Show resolved Hide resolved

t-webber force-pushed the fix_manual_unwrap_or_default branch 2 times, most recently from 988b08b to c6167c3 Compare April 18, 2024 19:06

fix: manual_unwrap_or_default

c9f0374

t-webber force-pushed the fix_manual_unwrap_or_default branch from c6167c3 to c9f0374 Compare April 18, 2024 21:22

rustbot removed has-merge-commits PR has merge commits, merge with caution. S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) labels Apr 18, 2024

update tests

7316845

Jarcho reviewed Jun 8, 2024

View reviewed changes

add: tests configuration in separate file

d7572b8

rustbot added S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties labels Jul 3, 2024

xFrednet closed this Aug 3, 2024

rustbot added the S-inactive-closed Status: Closed due to inactivity label Aug 3, 2024

rustbot removed the S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) label Aug 3, 2024

profetia mentioned this pull request Dec 28, 2024

fix: manual_unwrap_or_default suggests falsely when condition type is uncertain #13889

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

fix: `manual_unwrap_or_default` suggests error when expression is a None variant #12688

fix: `manual_unwrap_or_default` suggests error when expression is a None variant #12688

Uh oh!

t-webber commented Apr 17, 2024

Uh oh!

rustbot commented Apr 17, 2024

Uh oh!

Uh oh!

rustbot commented Apr 18, 2024

Uh oh!

rustbot commented Apr 18, 2024

Uh oh!

Uh oh!

Uh oh!

bors commented Jun 7, 2024

Uh oh!

Uh oh!

Jarcho Jun 8, 2024

Uh oh!

Jarcho Jun 8, 2024

Uh oh!

t-webber Jun 17, 2024

Uh oh!

Jarcho Jun 23, 2024

Uh oh!

Jarcho Jun 8, 2024

Uh oh!

Jarcho commented Jul 3, 2024

Uh oh!

xFrednet commented Aug 3, 2024

Uh oh!

profetia commented Dec 28, 2024

Uh oh!

Uh oh!

fix: manual_unwrap_or_default suggests error when expression is a None variant #12688

fix: manual_unwrap_or_default suggests error when expression is a None variant #12688

Uh oh!

Conversation

t-webber commented Apr 17, 2024

Uh oh!

rustbot commented Apr 17, 2024

Uh oh!

Uh oh!

rustbot commented Apr 18, 2024

Uh oh!

rustbot commented Apr 18, 2024

Uh oh!

Uh oh!

Uh oh!

bors commented Jun 7, 2024

Uh oh!

Uh oh!

Jarcho Jun 8, 2024

Choose a reason for hiding this comment

Uh oh!

Jarcho Jun 8, 2024

Choose a reason for hiding this comment

Uh oh!

t-webber Jun 17, 2024

Choose a reason for hiding this comment

Uh oh!

Jarcho Jun 23, 2024

Choose a reason for hiding this comment

Uh oh!

Jarcho Jun 8, 2024

Choose a reason for hiding this comment

Uh oh!

Jarcho commented Jul 3, 2024

Uh oh!

xFrednet commented Aug 3, 2024

Uh oh!

profetia commented Dec 28, 2024

Uh oh!

Uh oh!

fix: `manual_unwrap_or_default` suggests error when expression is a None variant #12688

fix: `manual_unwrap_or_default` suggests error when expression is a None variant #12688