From b4775435e875e3668ad920d911fabaed58f8860f Mon Sep 17 00:00:00 2001 From: Jake Goulding Date: Wed, 18 Aug 2021 16:33:50 -0400 Subject: [PATCH] Adjust the output of unnecessary_unwrap and provide a suggestion --- clippy_lints/src/unwrap.rs | 129 +++++++++++++++--- clippy_utils/src/usage.rs | 11 +- .../complex_conditionals.stderr | 39 ++++-- .../complex_conditionals_nested.stderr | 4 +- .../checked_unwrap/simple_conditionals.stderr | 36 ++--- 5 files changed, 161 insertions(+), 58 deletions(-) diff --git a/clippy_lints/src/unwrap.rs b/clippy_lints/src/unwrap.rs index 42738bc3e..b2ab300c2 100644 --- a/clippy_lints/src/unwrap.rs +++ b/clippy_lints/src/unwrap.rs @@ -1,10 +1,11 @@ use clippy_utils::diagnostics::span_lint_and_then; use clippy_utils::higher; use clippy_utils::ty::is_type_diagnostic_item; -use clippy_utils::{differing_macro_contexts, usage::is_potentially_mutated}; +use clippy_utils::{differing_macro_contexts, path_to_local, usage::is_potentially_mutated}; use if_chain::if_chain; +use rustc_errors::Applicability; use rustc_hir::intravisit::{walk_expr, walk_fn, FnKind, NestedVisitorMap, Visitor}; -use rustc_hir::{BinOpKind, Body, Expr, ExprKind, FnDecl, HirId, Path, QPath, UnOp}; +use rustc_hir::{BinOpKind, Body, Expr, ExprKind, FnDecl, HirId, PathSegment, UnOp}; use rustc_lint::{LateContext, LateLintPass}; use rustc_middle::hir::map::Map; use rustc_middle::lint::in_external_macro; @@ -74,26 +75,61 @@ struct UnwrappableVariablesVisitor<'a, 'tcx> { unwrappables: Vec>, cx: &'a LateContext<'tcx>, } + +/// What kind of unwrappable this is. +#[derive(Copy, Clone, Debug)] +enum UnwrappableKind { + Option, + Result, +} + +impl UnwrappableKind { + fn success_variant_pattern(self) -> &'static str { + match self { + UnwrappableKind::Option => "Some(..)", + UnwrappableKind::Result => "Ok(..)", + } + } + + fn error_variant_pattern(self) -> &'static str { + match self { + UnwrappableKind::Option => "None", + UnwrappableKind::Result => "Err(..)", + } + } +} + /// Contains information about whether a variable can be unwrapped. #[derive(Copy, Clone, Debug)] struct UnwrapInfo<'tcx> { /// The variable that is checked - ident: &'tcx Path<'tcx>, + local_id: HirId, + /// The if itself + if_expr: &'tcx Expr<'tcx>, /// The check, like `x.is_ok()` check: &'tcx Expr<'tcx>, + /// The check's name, like `is_ok` + check_name: &'tcx PathSegment<'tcx>, /// The branch where the check takes place, like `if x.is_ok() { .. }` branch: &'tcx Expr<'tcx>, /// Whether `is_some()` or `is_ok()` was called (as opposed to `is_err()` or `is_none()`). safe_to_unwrap: bool, + /// What kind of unwrappable this is. + kind: UnwrappableKind, + /// If the check is the entire condition (`if x.is_ok()`) or only a part of it (`foo() && + /// x.is_ok()`) + is_entire_condition: bool, } /// Collects the information about unwrappable variables from an if condition /// The `invert` argument tells us whether the condition is negated. fn collect_unwrap_info<'tcx>( cx: &LateContext<'tcx>, + if_expr: &'tcx Expr<'_>, expr: &'tcx Expr<'_>, branch: &'tcx Expr<'_>, invert: bool, + is_entire_condition: bool, ) -> Vec> { fn is_relevant_option_call(cx: &LateContext<'_>, ty: Ty<'_>, method_name: &str) -> bool { is_type_diagnostic_item(cx, ty, sym::option_type) && ["is_some", "is_none"].contains(&method_name) @@ -106,18 +142,18 @@ fn collect_unwrap_info<'tcx>( if let ExprKind::Binary(op, left, right) = &expr.kind { match (invert, op.node) { (false, BinOpKind::And | BinOpKind::BitAnd) | (true, BinOpKind::Or | BinOpKind::BitOr) => { - let mut unwrap_info = collect_unwrap_info(cx, left, branch, invert); - unwrap_info.append(&mut collect_unwrap_info(cx, right, branch, invert)); + let mut unwrap_info = collect_unwrap_info(cx, if_expr, left, branch, invert, false); + unwrap_info.append(&mut collect_unwrap_info(cx, if_expr, right, branch, invert, false)); return unwrap_info; }, _ => (), } } else if let ExprKind::Unary(UnOp::Not, expr) = &expr.kind { - return collect_unwrap_info(cx, expr, branch, !invert); + return collect_unwrap_info(cx, if_expr, expr, branch, !invert, false); } else { if_chain! { if let ExprKind::MethodCall(method_name, _, args, _) = &expr.kind; - if let ExprKind::Path(QPath::Resolved(None, path)) = &args[0].kind; + if let Some(local_id) = path_to_local(&args[0]); let ty = cx.typeck_results().expr_ty(&args[0]); let name = method_name.ident.as_str(); if is_relevant_option_call(cx, ty, &name) || is_relevant_result_call(cx, ty, &name); @@ -129,7 +165,24 @@ fn collect_unwrap_info<'tcx>( _ => unreachable!(), }; let safe_to_unwrap = unwrappable != invert; - return vec![UnwrapInfo { ident: path, check: expr, branch, safe_to_unwrap }]; + let kind = if is_type_diagnostic_item(cx, ty, sym::option_type) { + UnwrappableKind::Option + } else { + UnwrappableKind::Result + }; + + return vec![ + UnwrapInfo { + local_id, + if_expr, + check: expr, + check_name: method_name, + branch, + safe_to_unwrap, + kind, + is_entire_condition, + } + ] } } } @@ -137,11 +190,17 @@ fn collect_unwrap_info<'tcx>( } impl<'a, 'tcx> UnwrappableVariablesVisitor<'a, 'tcx> { - fn visit_branch(&mut self, cond: &'tcx Expr<'_>, branch: &'tcx Expr<'_>, else_branch: bool) { + fn visit_branch( + &mut self, + if_expr: &'tcx Expr<'_>, + cond: &'tcx Expr<'_>, + branch: &'tcx Expr<'_>, + else_branch: bool, + ) { let prev_len = self.unwrappables.len(); - for unwrap_info in collect_unwrap_info(self.cx, cond, branch, else_branch) { - if is_potentially_mutated(unwrap_info.ident, cond, self.cx) - || is_potentially_mutated(unwrap_info.ident, branch, self.cx) + for unwrap_info in collect_unwrap_info(self.cx, if_expr, cond, branch, else_branch, true) { + if is_potentially_mutated(unwrap_info.local_id, cond, self.cx) + || is_potentially_mutated(unwrap_info.local_id, branch, self.cx) { // if the variable is mutated, we don't know whether it can be unwrapped: continue; @@ -163,32 +222,62 @@ impl<'a, 'tcx> Visitor<'tcx> for UnwrappableVariablesVisitor<'a, 'tcx> { } if let Some(higher::If { cond, then, r#else }) = higher::If::hir(expr) { walk_expr(self, cond); - self.visit_branch(cond, then, false); + self.visit_branch(expr, cond, then, false); if let Some(else_inner) = r#else { - self.visit_branch(cond, else_inner, true); + self.visit_branch(expr, cond, else_inner, true); } } else { // find `unwrap[_err]()` calls: if_chain! { if let ExprKind::MethodCall(method_name, _, args, _) = expr.kind; - if let ExprKind::Path(QPath::Resolved(None, path)) = args[0].kind; + if let Some(id) = path_to_local(&args[0]); 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.ident.res == path.res); + .find(|u| u.local_id == id); // Span contexts should not differ with the conditional branch if !differing_macro_contexts(unwrappable.branch.span, expr.span); if !differing_macro_contexts(unwrappable.branch.span, unwrappable.check.span); 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() + }; + span_lint_and_then( self.cx, UNNECESSARY_UNWRAP, expr.span, - &format!("you checked before that `{}()` cannot fail, \ - instead of checking and unwrapping, it's better to use `if let` or `match`", - method_name.ident.name), - |diag| { diag.span_label(unwrappable.check.span, "the check is happening here"); }, + &format!( + "called `{}` on `{}` after checking its variant with `{}`", + method_name.ident.name, + unwrappable_variable_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, + ); + } else { + diag.span_label(unwrappable.check.span, "the check is happening here"); + diag.help("try using `if let` or `match`"); + } + }, ); } else { span_lint_and_then( diff --git a/clippy_utils/src/usage.rs b/clippy_utils/src/usage.rs index ac885e999..098ec175f 100644 --- a/clippy_utils/src/usage.rs +++ b/clippy_utils/src/usage.rs @@ -1,10 +1,9 @@ use crate as utils; use rustc_hir as hir; -use rustc_hir::def::Res; use rustc_hir::intravisit; use rustc_hir::intravisit::{NestedVisitorMap, Visitor}; use rustc_hir::HirIdSet; -use rustc_hir::{Expr, ExprKind, HirId, Path}; +use rustc_hir::{Expr, ExprKind, HirId}; use rustc_infer::infer::TyCtxtInferExt; use rustc_lint::LateContext; use rustc_middle::hir::map::Map; @@ -35,12 +34,8 @@ pub fn mutated_variables<'tcx>(expr: &'tcx Expr<'_>, cx: &LateContext<'tcx>) -> Some(delegate.used_mutably) } -pub fn is_potentially_mutated<'tcx>(variable: &'tcx Path<'_>, expr: &'tcx Expr<'_>, cx: &LateContext<'tcx>) -> bool { - if let Res::Local(id) = variable.res { - mutated_variables(expr, cx).map_or(true, |mutated| mutated.contains(&id)) - } else { - true - } +pub fn is_potentially_mutated<'tcx>(variable: HirId, expr: &'tcx Expr<'_>, cx: &LateContext<'tcx>) -> bool { + mutated_variables(expr, cx).map_or(true, |mutated| mutated.contains(&variable)) } struct MutVarsDelegate { diff --git a/tests/ui/checked_unwrap/complex_conditionals.stderr b/tests/ui/checked_unwrap/complex_conditionals.stderr index 33bb5136e..46c6f6970 100644 --- a/tests/ui/checked_unwrap/complex_conditionals.stderr +++ b/tests/ui/checked_unwrap/complex_conditionals.stderr @@ -1,4 +1,4 @@ -error: you checked before that `unwrap()` cannot fail, instead of checking and unwrapping, it's better to use `if let` or `match` +error: called `unwrap` on `x` after checking its variant with `is_ok` --> $DIR/complex_conditionals.rs:8:9 | LL | if x.is_ok() && y.is_err() { @@ -11,6 +11,7 @@ note: the lint level is defined here | LL | #![deny(clippy::panicking_unwrap, clippy::unnecessary_unwrap)] | ^^^^^^^^^^^^^^^^^^^^^^^^^^ + = help: try using `if let` or `match` error: this call to `unwrap_err()` will always panic --> $DIR/complex_conditionals.rs:9:9 @@ -36,7 +37,7 @@ LL | if x.is_ok() && y.is_err() { LL | y.unwrap(); // will panic | ^^^^^^^^^^ -error: you checked before that `unwrap_err()` cannot fail, instead of checking and unwrapping, it's better to use `if let` or `match` +error: called `unwrap_err` on `y` after checking its variant with `is_err` --> $DIR/complex_conditionals.rs:11:9 | LL | if x.is_ok() && y.is_err() { @@ -44,6 +45,8 @@ LL | if x.is_ok() && y.is_err() { ... LL | y.unwrap_err(); // unnecessary | ^^^^^^^^^^^^^^ + | + = help: try using `if let` or `match` error: this call to `unwrap()` will always panic --> $DIR/complex_conditionals.rs:25:9 @@ -54,7 +57,7 @@ LL | if x.is_ok() || y.is_ok() { LL | x.unwrap(); // will panic | ^^^^^^^^^^ -error: you checked before that `unwrap_err()` cannot fail, instead of checking and unwrapping, it's better to use `if let` or `match` +error: called `unwrap_err` on `x` after checking its variant with `is_ok` --> $DIR/complex_conditionals.rs:26:9 | LL | if x.is_ok() || y.is_ok() { @@ -62,6 +65,8 @@ LL | if x.is_ok() || y.is_ok() { ... LL | x.unwrap_err(); // unnecessary | ^^^^^^^^^^^^^^ + | + = help: try using `if let` or `match` error: this call to `unwrap()` will always panic --> $DIR/complex_conditionals.rs:27:9 @@ -72,7 +77,7 @@ LL | if x.is_ok() || y.is_ok() { LL | y.unwrap(); // will panic | ^^^^^^^^^^ -error: you checked before that `unwrap_err()` cannot fail, instead of checking and unwrapping, it's better to use `if let` or `match` +error: called `unwrap_err` on `y` after checking its variant with `is_ok` --> $DIR/complex_conditionals.rs:28:9 | LL | if x.is_ok() || y.is_ok() { @@ -80,14 +85,18 @@ LL | if x.is_ok() || y.is_ok() { ... LL | y.unwrap_err(); // unnecessary | ^^^^^^^^^^^^^^ + | + = help: try using `if let` or `match` -error: you checked before that `unwrap()` cannot fail, instead of checking and unwrapping, it's better to use `if let` or `match` +error: called `unwrap` on `x` after checking its variant with `is_ok` --> $DIR/complex_conditionals.rs:32:9 | LL | if x.is_ok() && !(y.is_ok() || z.is_err()) { | --------- the check is happening here LL | x.unwrap(); // unnecessary | ^^^^^^^^^^ + | + = help: try using `if let` or `match` error: this call to `unwrap_err()` will always panic --> $DIR/complex_conditionals.rs:33:9 @@ -107,7 +116,7 @@ LL | if x.is_ok() && !(y.is_ok() || z.is_err()) { LL | y.unwrap(); // will panic | ^^^^^^^^^^ -error: you checked before that `unwrap_err()` cannot fail, instead of checking and unwrapping, it's better to use `if let` or `match` +error: called `unwrap_err` on `y` after checking its variant with `is_ok` --> $DIR/complex_conditionals.rs:35:9 | LL | if x.is_ok() && !(y.is_ok() || z.is_err()) { @@ -115,8 +124,10 @@ LL | if x.is_ok() && !(y.is_ok() || z.is_err()) { ... LL | y.unwrap_err(); // unnecessary | ^^^^^^^^^^^^^^ + | + = help: try using `if let` or `match` -error: you checked before that `unwrap()` cannot fail, instead of checking and unwrapping, it's better to use `if let` or `match` +error: called `unwrap` on `z` after checking its variant with `is_err` --> $DIR/complex_conditionals.rs:36:9 | LL | if x.is_ok() && !(y.is_ok() || z.is_err()) { @@ -124,6 +135,8 @@ LL | if x.is_ok() && !(y.is_ok() || z.is_err()) { ... LL | z.unwrap(); // unnecessary | ^^^^^^^^^^ + | + = help: try using `if let` or `match` error: this call to `unwrap_err()` will always panic --> $DIR/complex_conditionals.rs:37:9 @@ -143,7 +156,7 @@ LL | if x.is_ok() || !(y.is_ok() && z.is_err()) { LL | x.unwrap(); // will panic | ^^^^^^^^^^ -error: you checked before that `unwrap_err()` cannot fail, instead of checking and unwrapping, it's better to use `if let` or `match` +error: called `unwrap_err` on `x` after checking its variant with `is_ok` --> $DIR/complex_conditionals.rs:46:9 | LL | if x.is_ok() || !(y.is_ok() && z.is_err()) { @@ -151,8 +164,10 @@ LL | if x.is_ok() || !(y.is_ok() && z.is_err()) { ... LL | x.unwrap_err(); // unnecessary | ^^^^^^^^^^^^^^ + | + = help: try using `if let` or `match` -error: you checked before that `unwrap()` cannot fail, instead of checking and unwrapping, it's better to use `if let` or `match` +error: called `unwrap` on `y` after checking its variant with `is_ok` --> $DIR/complex_conditionals.rs:47:9 | LL | if x.is_ok() || !(y.is_ok() && z.is_err()) { @@ -160,6 +175,8 @@ LL | if x.is_ok() || !(y.is_ok() && z.is_err()) { ... LL | y.unwrap(); // unnecessary | ^^^^^^^^^^ + | + = help: try using `if let` or `match` error: this call to `unwrap_err()` will always panic --> $DIR/complex_conditionals.rs:48:9 @@ -179,7 +196,7 @@ LL | if x.is_ok() || !(y.is_ok() && z.is_err()) { LL | z.unwrap(); // will panic | ^^^^^^^^^^ -error: you checked before that `unwrap_err()` cannot fail, instead of checking and unwrapping, it's better to use `if let` or `match` +error: called `unwrap_err` on `z` after checking its variant with `is_err` --> $DIR/complex_conditionals.rs:50:9 | LL | if x.is_ok() || !(y.is_ok() && z.is_err()) { @@ -187,6 +204,8 @@ LL | if x.is_ok() || !(y.is_ok() && z.is_err()) { ... LL | z.unwrap_err(); // unnecessary | ^^^^^^^^^^^^^^ + | + = help: try using `if let` or `match` error: aborting due to 20 previous errors diff --git a/tests/ui/checked_unwrap/complex_conditionals_nested.stderr b/tests/ui/checked_unwrap/complex_conditionals_nested.stderr index a01f7f956..542ab5330 100644 --- a/tests/ui/checked_unwrap/complex_conditionals_nested.stderr +++ b/tests/ui/checked_unwrap/complex_conditionals_nested.stderr @@ -1,8 +1,8 @@ -error: you checked before that `unwrap()` cannot fail, instead of checking and unwrapping, it's better to use `if let` or `match` +error: called `unwrap` on `x` after checking its variant with `is_some` --> $DIR/complex_conditionals_nested.rs:8:13 | LL | if x.is_some() { - | ----------- the check is happening here + | -------------- help: try: `if let Some(..) = x` LL | x.unwrap(); // unnecessary | ^^^^^^^^^^ | diff --git a/tests/ui/checked_unwrap/simple_conditionals.stderr b/tests/ui/checked_unwrap/simple_conditionals.stderr index 7127ff89b..82f269543 100644 --- a/tests/ui/checked_unwrap/simple_conditionals.stderr +++ b/tests/ui/checked_unwrap/simple_conditionals.stderr @@ -1,8 +1,8 @@ -error: you checked before that `unwrap()` cannot fail, instead of checking and unwrapping, it's better to use `if let` or `match` +error: called `unwrap` on `x` after checking its variant with `is_some` --> $DIR/simple_conditionals.rs:39:9 | LL | if x.is_some() { - | ----------- the check is happening here + | -------------- help: try: `if let Some(..) = x` LL | x.unwrap(); // unnecessary | ^^^^^^^^^^ | @@ -12,11 +12,11 @@ note: the lint level is defined here LL | #![deny(clippy::panicking_unwrap, clippy::unnecessary_unwrap)] | ^^^^^^^^^^^^^^^^^^^^^^^^^^ -error: you checked before that `expect()` cannot fail, instead of checking and unwrapping, it's better to use `if let` or `match` +error: called `expect` on `x` after checking its variant with `is_some` --> $DIR/simple_conditionals.rs:40:9 | LL | if x.is_some() { - | ----------- the check is happening here + | -------------- help: try: `if let Some(..) = x` LL | x.unwrap(); // unnecessary LL | x.expect("an error message"); // unnecessary | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ @@ -53,20 +53,20 @@ LL | if x.is_none() { LL | x.unwrap(); // will panic | ^^^^^^^^^^ -error: you checked before that `unwrap()` cannot fail, instead of checking and unwrapping, it's better to use `if let` or `match` +error: called `unwrap` on `x` after checking its variant with `is_none` --> $DIR/simple_conditionals.rs:48:9 | LL | if x.is_none() { - | ----------- the check is happening here + | -------------- help: try: `if let Some(..) = x` ... LL | x.unwrap(); // unnecessary | ^^^^^^^^^^ -error: you checked before that `unwrap()` cannot fail, instead of checking and unwrapping, it's better to use `if let` or `match` +error: called `unwrap` on `x` after checking its variant with `is_some` --> $DIR/simple_conditionals.rs:7:13 | LL | if $a.is_some() { - | ------------ the check is happening here + | --------------- help: try: `if let Some(..) = x` LL | $a.unwrap(); // unnecessary | ^^^^^^^^^^^ ... @@ -75,19 +75,19 @@ LL | m!(x); | = note: this error originates in the macro `m` (in Nightly builds, run with -Z macro-backtrace for more info) -error: you checked before that `unwrap()` cannot fail, instead of checking and unwrapping, it's better to use `if let` or `match` +error: called `unwrap` on `x` after checking its variant with `is_ok` --> $DIR/simple_conditionals.rs:56:9 | LL | if x.is_ok() { - | --------- the check is happening here + | ------------ help: try: `if let Ok(..) = x` LL | x.unwrap(); // unnecessary | ^^^^^^^^^^ -error: you checked before that `expect()` cannot fail, instead of checking and unwrapping, it's better to use `if let` or `match` +error: called `expect` on `x` after checking its variant with `is_ok` --> $DIR/simple_conditionals.rs:57:9 | LL | if x.is_ok() { - | --------- the check is happening here + | ------------ help: try: `if let Ok(..) = x` LL | x.unwrap(); // unnecessary LL | x.expect("an error message"); // unnecessary | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ @@ -119,11 +119,11 @@ LL | if x.is_ok() { LL | x.expect("an error message"); // will panic | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ -error: you checked before that `unwrap_err()` cannot fail, instead of checking and unwrapping, it's better to use `if let` or `match` +error: called `unwrap_err` on `x` after checking its variant with `is_ok` --> $DIR/simple_conditionals.rs:62:9 | LL | if x.is_ok() { - | --------- the check is happening here + | ------------ help: try: `if let Err(..) = x` ... LL | x.unwrap_err(); // unnecessary | ^^^^^^^^^^^^^^ @@ -136,20 +136,20 @@ LL | if x.is_err() { LL | x.unwrap(); // will panic | ^^^^^^^^^^ -error: you checked before that `unwrap_err()` cannot fail, instead of checking and unwrapping, it's better to use `if let` or `match` +error: called `unwrap_err` on `x` after checking its variant with `is_err` --> $DIR/simple_conditionals.rs:66:9 | LL | if x.is_err() { - | ---------- the check is happening here + | ------------- help: try: `if let Err(..) = x` LL | x.unwrap(); // will panic LL | x.unwrap_err(); // unnecessary | ^^^^^^^^^^^^^^ -error: you checked before that `unwrap()` cannot fail, instead of checking and unwrapping, it's better to use `if let` or `match` +error: called `unwrap` on `x` after checking its variant with `is_err` --> $DIR/simple_conditionals.rs:68:9 | LL | if x.is_err() { - | ---------- the check is happening here + | ------------- help: try: `if let Ok(..) = x` ... LL | x.unwrap(); // unnecessary | ^^^^^^^^^^