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.

Sign up for GitHub

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

Jump to bottom

[unnecessary_unwrap]: Suggest reversing then/else if necessary #11010

Closed
wants to merge 1 commit into from
Closed

[unnecessary_unwrap]: Suggest reversing then/else if necessary #11010

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
131 changes: 74 additions & 57 deletions clippy_lints/src/unwrap.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
use clippy_utils::diagnostics::span_lint_hir_and_then;
use clippy_utils::higher;
use clippy_utils::source::snippet;
use clippy_utils::ty::is_type_diagnostic_item;
use clippy_utils::{path_to_local, usage::is_potentially_mutated};
use if_chain::if_chain;
Expand Down Expand Up @@ -231,67 +232,83 @@ impl<'a, 'tcx> Visitor<'tcx> for UnwrappableVariablesVisitor<'a, 'tcx> {
}
} else {
// find `unwrap[_err]()` calls:
if_chain! {
if let ExprKind::MethodCall(method_name, self_arg, ..) = expr.kind;
if let Some(id) = path_to_local(self_arg);
if [sym::unwrap, sym::expect, sym!(unwrap_err)].contains(&method_name.ident.name);
let call_to_unwrap = [sym::unwrap, sym::expect].contains(&method_name.ident.name);
if let Some(unwrappable) = self.unwrappables.iter()
.find(|u| u.local_id == id);
// Span contexts should not differ with the conditional branch
let span_ctxt = expr.span.ctxt();
if unwrappable.branch.span.ctxt() == span_ctxt;
if unwrappable.check.span.ctxt() == span_ctxt;
then {
if call_to_unwrap == unwrappable.safe_to_unwrap {
let is_entire_condition = unwrappable.is_entire_condition;
let unwrappable_variable_name = self.cx.tcx.hir().name(unwrappable.local_id);
let suggested_pattern = if call_to_unwrap {
unwrappable.kind.success_variant_pattern()
} else {
unwrappable.kind.error_variant_pattern()
};
if let ExprKind::MethodCall(method_name, self_arg, ..) = expr.kind
&& let Some(id) = path_to_local(self_arg)
&& [sym::unwrap, sym::expect, sym!(unwrap_err)].contains(&method_name.ident.name)
&& let call_to_unwrap = [sym::unwrap, sym::expect].contains(&method_name.ident.name)
&& let Some(unwrappable) = self.unwrappables.iter().find(|u| u.local_id == id)
&& let span_ctxt = expr.span.ctxt()
&& unwrappable.branch.span.ctxt() == span_ctxt
&& unwrappable.check.span.ctxt() == span_ctxt
{
if call_to_unwrap == unwrappable.safe_to_unwrap {
let is_entire_condition = unwrappable.is_entire_condition;
let unwrappable_variable_name = self.cx.tcx.hir().name(unwrappable.local_id);
let suggested_pattern = if call_to_unwrap {
unwrappable.kind.success_variant_pattern()
} else {
unwrappable.kind.error_variant_pattern()
};

span_lint_hir_and_then(
self.cx,
UNNECESSARY_UNWRAP,
expr.hir_id,
expr.span,
&format!(
"called `{}` on `{unwrappable_variable_name}` after checking its variant with `{}`",
method_name.ident.name,
unwrappable.check_name.ident.as_str(),
),
|diag| {
if is_entire_condition {
diag.span_suggestion(
unwrappable.check.span.with_lo(unwrappable.if_expr.span.lo()),
"try",
format!(
"if let {suggested_pattern} = {unwrappable_variable_name}",
),
// We don't track how the unwrapped value is used inside the
// block or suggest deleting the unwrap, so we can't offer a
// fixable solution.
Applicability::Unspecified,
);
// Reverse the blocks if it's `is_none`/`is_err`
if let Some(higher::If {
cond: _,
then,
r#else: Some(r#else),
}) = higher::If::hir(unwrappable.if_expr)
&& ["is_none", "is_err"].contains(&unwrappable.check_name.ident.name.as_str())
{
let then_snip = snippet(self.cx, then.span, "<then>");
let else_snip = snippet(self.cx, r#else.span, "<else>");

span_lint_hir_and_then(
self.cx,
UNNECESSARY_UNWRAP,
expr.hir_id,
expr.span,
&format!(
"called `{}` on `{unwrappable_variable_name}` after checking its variant with `{}`",
method_name.ident.name,
unwrappable.check_name.ident.as_str(),
),
|diag| {
if is_entire_condition {
diag.span_suggestion(
unwrappable.check.span.with_lo(unwrappable.if_expr.span.lo()),
"try",
format!(
"if let {suggested_pattern} = {unwrappable_variable_name}",
),
// We don't track how the unwrapped value is used inside the
// block or suggest deleting the unwrap, so we can't offer a
// fixable solution.
diag.multipart_suggestion(
"this will require flipping the blocks",
vec![
(then.span, else_snip.into_owned()),
(r#else.span, then_snip.into_owned()),
],
Applicability::Unspecified,
);
} else {
diag.span_label(unwrappable.check.span, "the check is happening here");
diag.help("try using `if let` or `match`");
}
},
);
} else {
span_lint_hir_and_then(
self.cx,
PANICKING_UNWRAP,
expr.hir_id,
expr.span,
&format!("this call to `{}()` will always panic",
method_name.ident.name),
|diag| { diag.span_label(unwrappable.check.span, "because of this check"); },
);
}
} else {
diag.span_label(unwrappable.check.span, "the check is happening here");
diag.help("try using `if let` or `match`");
}
},
);
} else {
span_lint_hir_and_then(
self.cx,
PANICKING_UNWRAP,
expr.hir_id,
expr.span,
&format!("this call to `{}()` will always panic",
method_name.ident.name),
|diag| { diag.span_label(unwrappable.check.span, "because of this check"); },
);
}
}
walk_expr(self, expr);
Expand Down
13 changes: 13 additions & 0 deletions tests/ui/checked_unwrap/simple_conditionals.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,7 +86,20 @@ fn main() {
// only checks if conditions).
x.unwrap_err();
}
// #11006: Reverse blocks if `is_err`/`is_none` then `unwrap` in else
if x.is_err() {
println!("lolol");
} else {
x.unwrap();
}
let x = Some(());
if x.is_none() {
println!("lolol");
} else {
x.unwrap();
}

let mut x: Result<(), ()> = Ok(());
assert!(x.is_ok(), "{:?}", x.unwrap_err()); // ok, it's a common test pattern
}

Expand Down
92 changes: 82 additions & 10 deletions tests/ui/checked_unwrap/simple_conditionals.stderr
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,11 +56,21 @@ LL | x.unwrap(); // will panic
error: called `unwrap` on `x` after checking its variant with `is_none`
--> $DIR/simple_conditionals.rs:53:9
|
LL | if x.is_none() {
| -------------- help: try: `if let Some(..) = x`
...
LL | x.unwrap(); // unnecessary
| ^^^^^^^^^^
|
help: try
|
LL | if let Some(..) = x {
| ~~~~~~~~~~~~~~~~~~~
help: this will require flipping the blocks
|
LL ~ if x.is_none() {
LL + x.unwrap(); // unnecessary
LL ~ } else {
LL + x.unwrap(); // will panic
LL + }
|

error: called `unwrap` on `x` after checking its variant with `is_some`
--> $DIR/simple_conditionals.rs:12:13
Expand Down Expand Up @@ -139,20 +149,44 @@ LL | x.unwrap(); // will panic
error: called `unwrap_err` on `x` after checking its variant with `is_err`
--> $DIR/simple_conditionals.rs:71:9
|
LL | if x.is_err() {
| ------------- help: try: `if let Err(..) = x`
LL | x.unwrap(); // will panic
LL | x.unwrap_err(); // unnecessary
| ^^^^^^^^^^^^^^
|
help: try
|
LL | if let Err(..) = x {
| ~~~~~~~~~~~~~~~~~~
help: this will require flipping the blocks
|
LL ~ if x.is_err() {
LL + x.unwrap(); // unnecessary
LL + x.unwrap_err(); // will panic
LL ~ } else {
LL + x.unwrap(); // will panic
LL + x.unwrap_err(); // unnecessary
LL + }
|

error: called `unwrap` on `x` after checking its variant with `is_err`
--> $DIR/simple_conditionals.rs:73:9
|
LL | if x.is_err() {
| ------------- help: try: `if let Ok(..) = x`
...
LL | x.unwrap(); // unnecessary
| ^^^^^^^^^^
|
help: try
|
LL | if let Ok(..) = x {
| ~~~~~~~~~~~~~~~~~
help: this will require flipping the blocks
|
LL ~ if x.is_err() {
LL + x.unwrap(); // unnecessary
LL + x.unwrap_err(); // will panic
LL ~ } else {
LL + x.unwrap(); // will panic
LL + x.unwrap_err(); // unnecessary
LL + }
|

error: this call to `unwrap_err()` will always panic
--> $DIR/simple_conditionals.rs:74:9
Expand All @@ -163,5 +197,43 @@ LL | if x.is_err() {
LL | x.unwrap_err(); // will panic
| ^^^^^^^^^^^^^^

error: aborting due to 17 previous errors
error: called `unwrap` on `x` after checking its variant with `is_err`
--> $DIR/simple_conditionals.rs:93:9
|
LL | x.unwrap();
| ^^^^^^^^^^
|
help: try
|
LL | if let Ok(..) = x {
| ~~~~~~~~~~~~~~~~~
help: this will require flipping the blocks
|
LL ~ if x.is_err() {
LL + x.unwrap();
LL ~ } else {
LL + println!("lolol");
LL + }
|

error: called `unwrap` on `x` after checking its variant with `is_none`
--> $DIR/simple_conditionals.rs:99:9
|
LL | x.unwrap();
| ^^^^^^^^^^
|
help: try
|
LL | if let Some(..) = x {
| ~~~~~~~~~~~~~~~~~~~
help: this will require flipping the blocks
|
LL ~ if x.is_none() {
LL + x.unwrap();
LL ~ } else {
LL + println!("lolol");
LL + }
|

error: aborting due to 19 previous errors