From 1e6c6976dd12406d2b57de17f1f667527d7977c6 Mon Sep 17 00:00:00 2001 From: Joe Frikker Date: Sat, 22 Jun 2019 16:34:07 -0400 Subject: [PATCH] PR comments --- clippy_lints/src/try_err.rs | 31 +++++++++++++------------------ clippy_lints/src/utils/paths.rs | 1 + src/lintlist/mod.rs | 2 +- tests/ui/try_err.stderr | 8 ++++---- 4 files changed, 19 insertions(+), 23 deletions(-) diff --git a/clippy_lints/src/try_err.rs b/clippy_lints/src/try_err.rs index ec0b96174da4..eec13545956d 100644 --- a/clippy_lints/src/try_err.rs +++ b/clippy_lints/src/try_err.rs @@ -1,4 +1,4 @@ -use crate::utils::{match_qpath, paths, snippet, span_lint_and_then}; +use crate::utils::{match_qpath, paths, snippet, span_lint_and_sugg}; use if_chain::if_chain; use rustc::hir::*; use rustc::lint::{LateContext, LateLintPass, LintArray, LintPass}; @@ -17,17 +17,17 @@ declare_clippy_lint! { /// **Known problems:** None. /// /// **Example:** - /// - /// ```rust,ignore - /// // Bad + /// ```rust /// fn foo(fail: bool) -> Result { /// if fail { /// Err("failed")?; /// } /// Ok(0) /// } + /// ``` + /// Could be written: /// - /// // Good + /// ```rust /// fn foo(fail: bool) -> Result { /// if fail { /// return Err("failed".into()); @@ -57,7 +57,7 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for TryErr { if let ExprKind::Match(ref match_arg, _, MatchSource::TryDesugar) = expr.node; if let ExprKind::Call(ref match_fun, ref try_args) = match_arg.node; if let ExprKind::Path(ref match_fun_path) = match_fun.node; - if match_qpath(match_fun_path, &["std", "ops", "Try", "into_result"]); + if match_qpath(match_fun_path, &paths::TRY_INTO_RESULT); if let Some(ref try_arg) = try_args.get(0); if let ExprKind::Call(ref err_fun, ref err_args) = try_arg.node; if let Some(ref err_arg) = err_args.get(0); @@ -73,19 +73,14 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for TryErr { format!("return Err({}.into())", snippet(cx, err_arg.span, "_")) }; - span_lint_and_then( + span_lint_and_sugg( cx, TRY_ERR, expr.span, - &format!("confusing error return, consider using `{}`", suggestion), - |db| { - db.span_suggestion( - expr.span, - "try this", - suggestion, - Applicability::MaybeIncorrect - ); - }, + "returning an `Err(_)` with the `?` operator", + "try this", + suggestion, + Applicability::MaybeIncorrect ); } } @@ -97,7 +92,7 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for TryErr { // its output type. fn find_err_return_type<'a, 'tcx>(cx: &LateContext<'a, 'tcx>, expr: &'tcx ExprKind) -> Option> { if let ExprKind::Match(_, ref arms, MatchSource::TryDesugar) = expr { - arms.iter().filter_map(|ty| find_err_return_type_arm(cx, ty)).nth(0) + arms.iter().find_map(|ty| find_err_return_type_arm(cx, ty)) } else { None } @@ -109,7 +104,7 @@ fn find_err_return_type_arm<'a, 'tcx>(cx: &LateContext<'a, 'tcx>, arm: &'tcx Arm if let ExprKind::Ret(Some(ref err_ret)) = arm.body.node; if let ExprKind::Call(ref from_error_path, ref from_error_args) = err_ret.node; if let ExprKind::Path(ref from_error_fn) = from_error_path.node; - if match_qpath(from_error_fn, &["std", "ops", "Try", "from_error"]); + if match_qpath(from_error_fn, &paths::TRY_FROM_ERROR); if let Some(from_error_arg) = from_error_args.get(0); then { Some(cx.tables.expr_ty(from_error_arg)) diff --git a/clippy_lints/src/utils/paths.rs b/clippy_lints/src/utils/paths.rs index 1960618ebfaa..e08ff3e9705b 100644 --- a/clippy_lints/src/utils/paths.rs +++ b/clippy_lints/src/utils/paths.rs @@ -107,6 +107,7 @@ pub const TO_OWNED_METHOD: [&str; 4] = ["alloc", "borrow", "ToOwned", "to_owned" pub const TO_STRING: [&str; 3] = ["alloc", "string", "ToString"]; pub const TO_STRING_METHOD: [&str; 4] = ["alloc", "string", "ToString", "to_string"]; pub const TRANSMUTE: [&str; 4] = ["core", "intrinsics", "", "transmute"]; +pub const TRY_FROM_ERROR: [&str; 4] = ["std", "ops", "Try", "from_error"]; pub const TRY_INTO_RESULT: [&str; 4] = ["std", "ops", "Try", "into_result"]; pub const UNINIT: [&str; 4] = ["core", "intrinsics", "", "uninit"]; pub const VEC: [&str; 3] = ["alloc", "vec", "Vec"]; diff --git a/src/lintlist/mod.rs b/src/lintlist/mod.rs index e8abb198e78b..8c49a3b18380 100644 --- a/src/lintlist/mod.rs +++ b/src/lintlist/mod.rs @@ -1823,7 +1823,7 @@ pub const ALL_LINTS: [Lint; 306] = [ Lint { name: "try_err", group: "style", - desc: "TODO", + desc: "return errors explicitly rather than hiding them behind a `?`", deprecation: None, module: "try_err", }, diff --git a/tests/ui/try_err.stderr b/tests/ui/try_err.stderr index 4e6f273052fe..b2fb35ffb515 100644 --- a/tests/ui/try_err.stderr +++ b/tests/ui/try_err.stderr @@ -1,4 +1,4 @@ -error: confusing error return, consider using `return Err(err)` +error: returning an `Err(_)` with the `?` operator --> $DIR/try_err.rs:7:5 | LL | Err(err)?; @@ -10,19 +10,19 @@ note: lint level defined here LL | #![deny(clippy::try_err)] | ^^^^^^^^^^^^^^^ -error: confusing error return, consider using `return Err(err.into())` +error: returning an `Err(_)` with the `?` operator --> $DIR/try_err.rs:14:5 | LL | Err(err)?; | ^^^^^^^^^ help: try this: `return Err(err.into())` -error: confusing error return, consider using `return Err(err)` +error: returning an `Err(_)` with the `?` operator --> $DIR/try_err.rs:31:13 | LL | Err(err)?; | ^^^^^^^^^ help: try this: `return Err(err)` -error: confusing error return, consider using `return Err(err.into())` +error: returning an `Err(_)` with the `?` operator --> $DIR/try_err.rs:46:13 | LL | Err(err)?;