From 405d719d1fdffb85930989fc473f32784d56de86 Mon Sep 17 00:00:00 2001 From: Catherine <114838443+Centri3@users.noreply.github.com> Date: Thu, 22 Jun 2023 23:09:43 -0500 Subject: [PATCH] Suggest reversing blocks if necessary --- clippy_lints/src/unwrap.rs | 131 ++++++++++-------- .../ui/checked_unwrap/simple_conditionals.rs | 13 ++ .../checked_unwrap/simple_conditionals.stderr | 92 ++++++++++-- 3 files changed, 169 insertions(+), 67 deletions(-) diff --git a/clippy_lints/src/unwrap.rs b/clippy_lints/src/unwrap.rs index 377d3fb6f4e1..140e8ac0cf8d 100644 --- a/clippy_lints/src/unwrap.rs +++ b/clippy_lints/src/unwrap.rs @@ -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; @@ -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, ""); + let else_snip = snippet(self.cx, r#else.span, ""); - 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); diff --git a/tests/ui/checked_unwrap/simple_conditionals.rs b/tests/ui/checked_unwrap/simple_conditionals.rs index 61042bb90d27..7a1ee8187a63 100644 --- a/tests/ui/checked_unwrap/simple_conditionals.rs +++ b/tests/ui/checked_unwrap/simple_conditionals.rs @@ -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 } diff --git a/tests/ui/checked_unwrap/simple_conditionals.stderr b/tests/ui/checked_unwrap/simple_conditionals.stderr index 93809f6551ad..6a9cb7370355 100644 --- a/tests/ui/checked_unwrap/simple_conditionals.stderr +++ b/tests/ui/checked_unwrap/simple_conditionals.stderr @@ -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 @@ -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 @@ -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