From 60a80849ce20bbfc3bbef741a2be8cdc7225b96d Mon Sep 17 00:00:00 2001 From: Joe Frikker Date: Tue, 18 Jun 2019 23:22:51 -0400 Subject: [PATCH] Adding try_err lint --- CHANGELOG.md | 1 + README.md | 2 +- clippy_lints/src/lib.rs | 4 ++ clippy_lints/src/try_err.rs | 120 ++++++++++++++++++++++++++++++++++++ src/lintlist/mod.rs | 9 ++- tests/ui/try_err.rs | 65 +++++++++++++++++++ tests/ui/try_err.stderr | 32 ++++++++++ 7 files changed, 231 insertions(+), 2 deletions(-) create mode 100644 clippy_lints/src/try_err.rs create mode 100644 tests/ui/try_err.rs create mode 100644 tests/ui/try_err.stderr diff --git a/CHANGELOG.md b/CHANGELOG.md index 3710342d841..461adb729f9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1134,6 +1134,7 @@ Released 2018-09-13 [`transmuting_null`]: https://rust-lang.github.io/rust-clippy/master/index.html#transmuting_null [`trivial_regex`]: https://rust-lang.github.io/rust-clippy/master/index.html#trivial_regex [`trivially_copy_pass_by_ref`]: https://rust-lang.github.io/rust-clippy/master/index.html#trivially_copy_pass_by_ref +[`try_err`]: https://rust-lang.github.io/rust-clippy/master/index.html#try_err [`type_complexity`]: https://rust-lang.github.io/rust-clippy/master/index.html#type_complexity [`unicode_not_nfc`]: https://rust-lang.github.io/rust-clippy/master/index.html#unicode_not_nfc [`unimplemented`]: https://rust-lang.github.io/rust-clippy/master/index.html#unimplemented diff --git a/README.md b/README.md index 51f618da4b7..c1a25aa1300 100644 --- a/README.md +++ b/README.md @@ -6,7 +6,7 @@ A collection of lints to catch common mistakes and improve your [Rust](https://github.com/rust-lang/rust) code. -[There are 305 lints included in this crate!](https://rust-lang.github.io/rust-clippy/master/index.html) +[There are 306 lints included in this crate!](https://rust-lang.github.io/rust-clippy/master/index.html) We have a bunch of lint categories to allow you to choose how much Clippy is supposed to ~~annoy~~ help you: diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index b56413fc2c4..d239f14339f 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -263,6 +263,7 @@ pub mod temporary_assignment; pub mod transmute; pub mod transmuting_null; pub mod trivially_copy_pass_by_ref; +pub mod try_err; pub mod types; pub mod unicode; pub mod unsafe_removed_from_name; @@ -546,6 +547,7 @@ pub fn register_plugins(reg: &mut rustc_plugin::Registry<'_>, conf: &Conf) { reg.register_early_lint_pass(box literal_representation::DecimalLiteralRepresentation::new( conf.literal_representation_threshold )); + reg.register_late_lint_pass(box try_err::TryErr); reg.register_late_lint_pass(box use_self::UseSelf); reg.register_late_lint_pass(box bytecount::ByteCount); reg.register_late_lint_pass(box infinite_iter::InfiniteIter); @@ -861,6 +863,7 @@ pub fn register_plugins(reg: &mut rustc_plugin::Registry<'_>, conf: &Conf) { transmute::WRONG_TRANSMUTE, transmuting_null::TRANSMUTING_NULL, trivially_copy_pass_by_ref::TRIVIALLY_COPY_PASS_BY_REF, + try_err::TRY_ERR, types::ABSURD_EXTREME_COMPARISONS, types::BORROWED_BOX, types::BOX_VEC, @@ -963,6 +966,7 @@ pub fn register_plugins(reg: &mut rustc_plugin::Registry<'_>, conf: &Conf) { returns::NEEDLESS_RETURN, returns::UNUSED_UNIT, strings::STRING_LIT_AS_BYTES, + try_err::TRY_ERR, types::FN_TO_NUMERIC_CAST, types::FN_TO_NUMERIC_CAST_WITH_TRUNCATION, types::IMPLICIT_HASHER, diff --git a/clippy_lints/src/try_err.rs b/clippy_lints/src/try_err.rs new file mode 100644 index 00000000000..ec0b96174da --- /dev/null +++ b/clippy_lints/src/try_err.rs @@ -0,0 +1,120 @@ +use crate::utils::{match_qpath, paths, snippet, span_lint_and_then}; +use if_chain::if_chain; +use rustc::hir::*; +use rustc::lint::{LateContext, LateLintPass, LintArray, LintPass}; +use rustc::ty::Ty; +use rustc::{declare_lint_pass, declare_tool_lint}; +use rustc_errors::Applicability; + +declare_clippy_lint! { + /// **What it does:** Checks for usages of `Err(x)?`. + /// + /// **Why is this bad?** The `?` operator is designed to allow calls that + /// can fail to be easily chained. For example, `foo()?.bar()` or + /// `foo(bar()?)`. Because `Err(x)?` can't be used that way (it will + /// always return), it is more clear to write `return Err(x)`. + /// + /// **Known problems:** None. + /// + /// **Example:** + /// + /// ```rust,ignore + /// // Bad + /// fn foo(fail: bool) -> Result { + /// if fail { + /// Err("failed")?; + /// } + /// Ok(0) + /// } + /// + /// // Good + /// fn foo(fail: bool) -> Result { + /// if fail { + /// return Err("failed".into()); + /// } + /// Ok(0) + /// } + /// ``` + pub TRY_ERR, + style, + "return errors explicitly rather than hiding them behind a `?`" +} + +declare_lint_pass!(TryErr => [TRY_ERR]); + +impl<'a, 'tcx> LateLintPass<'a, 'tcx> for TryErr { + fn check_expr(&mut self, cx: &LateContext<'a, 'tcx>, expr: &'tcx Expr) { + // Looks for a structure like this: + // match ::std::ops::Try::into_result(Err(5)) { + // ::std::result::Result::Err(err) => + // #[allow(unreachable_code)] + // return ::std::ops::Try::from_error(::std::convert::From::from(err)), + // ::std::result::Result::Ok(val) => + // #[allow(unreachable_code)] + // val, + // }; + if_chain! { + 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 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); + if let ExprKind::Path(ref err_fun_path) = err_fun.node; + if match_qpath(err_fun_path, &paths::RESULT_ERR); + if let Some(return_type) = find_err_return_type(cx, &expr.node); + + then { + let err_type = cx.tables.expr_ty(err_arg); + let suggestion = if err_type == return_type { + format!("return Err({})", snippet(cx, err_arg.span, "_")) + } else { + format!("return Err({}.into())", snippet(cx, err_arg.span, "_")) + }; + + span_lint_and_then( + cx, + TRY_ERR, + expr.span, + &format!("confusing error return, consider using `{}`", suggestion), + |db| { + db.span_suggestion( + expr.span, + "try this", + suggestion, + Applicability::MaybeIncorrect + ); + }, + ); + } + } + } +} + +// In order to determine whether to suggest `.into()` or not, we need to find the error type the +// function returns. To do that, we look for the From::from call (see tree above), and capture +// 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) + } else { + None + } +} + +// Check for From::from in one of the match arms. +fn find_err_return_type_arm<'a, 'tcx>(cx: &LateContext<'a, 'tcx>, arm: &'tcx Arm) -> Option> { + if_chain! { + 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 let Some(from_error_arg) = from_error_args.get(0); + then { + Some(cx.tables.expr_ty(from_error_arg)) + } else { + None + } + } +} diff --git a/src/lintlist/mod.rs b/src/lintlist/mod.rs index 663b6a5e709..e8abb198e78 100644 --- a/src/lintlist/mod.rs +++ b/src/lintlist/mod.rs @@ -6,7 +6,7 @@ pub use lint::Lint; pub use lint::LINT_LEVELS; // begin lint list, do not remove this comment, it’s used in `update_lints` -pub const ALL_LINTS: [Lint; 305] = [ +pub const ALL_LINTS: [Lint; 306] = [ Lint { name: "absurd_extreme_comparisons", group: "correctness", @@ -1820,6 +1820,13 @@ pub const ALL_LINTS: [Lint; 305] = [ deprecation: None, module: "trivially_copy_pass_by_ref", }, + Lint { + name: "try_err", + group: "style", + desc: "TODO", + deprecation: None, + module: "try_err", + }, Lint { name: "type_complexity", group: "complexity", diff --git a/tests/ui/try_err.rs b/tests/ui/try_err.rs new file mode 100644 index 00000000000..d8decbbac66 --- /dev/null +++ b/tests/ui/try_err.rs @@ -0,0 +1,65 @@ +#![deny(clippy::try_err)] + +// Tests that a simple case works +// Should flag `Err(err)?` +pub fn basic_test() -> Result { + let err: i32 = 1; + Err(err)?; + Ok(0) +} + +// Tests that `.into()` is added when appropriate +pub fn into_test() -> Result { + let err: u8 = 1; + Err(err)?; + Ok(0) +} + +// Tests that tries in general don't trigger the error +pub fn negative_test() -> Result { + Ok(nested_error()? + 1) +} + + +// Tests that `.into()` isn't added when the error type +// matches the surrounding closure's return type, even +// when it doesn't match the surrounding function's. +pub fn closure_matches_test() -> Result { + let res: Result = Some(1).into_iter() + .map(|i| { + let err: i8 = 1; + Err(err)?; + Ok(i) + }) + .next() + .unwrap(); + + Ok(res?) +} + +// Tests that `.into()` isn't added when the error type +// doesn't match the surrounding closure's return type. +pub fn closure_into_test() -> Result { + let res: Result = Some(1).into_iter() + .map(|i| { + let err: i8 = 1; + Err(err)?; + Ok(i) + }) + .next() + .unwrap(); + + Ok(res?) +} + +fn nested_error() -> Result { + Ok(1) +} + +fn main() { + basic_test().unwrap(); + into_test().unwrap(); + negative_test().unwrap(); + closure_matches_test().unwrap(); + closure_into_test().unwrap(); +} diff --git a/tests/ui/try_err.stderr b/tests/ui/try_err.stderr new file mode 100644 index 00000000000..4e6f273052f --- /dev/null +++ b/tests/ui/try_err.stderr @@ -0,0 +1,32 @@ +error: confusing error return, consider using `return Err(err)` + --> $DIR/try_err.rs:7:5 + | +LL | Err(err)?; + | ^^^^^^^^^ help: try this: `return Err(err)` + | +note: lint level defined here + --> $DIR/try_err.rs:1:9 + | +LL | #![deny(clippy::try_err)] + | ^^^^^^^^^^^^^^^ + +error: confusing error return, consider using `return Err(err.into())` + --> $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)` + --> $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())` + --> $DIR/try_err.rs:46:13 + | +LL | Err(err)?; + | ^^^^^^^^^ help: try this: `return Err(err.into())` + +error: aborting due to 4 previous errors +