diff --git a/CHANGELOG.md b/CHANGELOG.md index 3710342d8..461adb729 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 51f618da4..c1a25aa13 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 b56413fc2..d239f1433 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 000000000..7466221fb --- /dev/null +++ b/clippy_lints/src/try_err.rs @@ -0,0 +1,115 @@ +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}; +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 + /// fn foo(fail: bool) -> Result { + /// if fail { + /// Err("failed")?; + /// } + /// Ok(0) + /// } + /// ``` + /// Could be written: + /// + /// ```rust + /// 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, &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); + 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_sugg( + cx, + TRY_ERR, + expr.span, + "returning an `Err(_)` with the `?` operator", + "try this", + suggestion, + Applicability::MachineApplicable + ); + } + } + } +} + +// 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().find_map(|ty| find_err_return_type_arm(cx, ty)) + } 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, &paths::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/clippy_lints/src/utils/paths.rs b/clippy_lints/src/utils/paths.rs index 1960618eb..e08ff3e97 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 663b6a5e7..8c49a3b18 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: "return errors explicitly rather than hiding them behind a `?`", + deprecation: None, + module: "try_err", + }, Lint { name: "type_complexity", group: "complexity", diff --git a/tests/ui/try_err.fixed b/tests/ui/try_err.fixed new file mode 100644 index 000000000..117300e84 --- /dev/null +++ b/tests/ui/try_err.fixed @@ -0,0 +1,80 @@ +// run-rustfix + +#![deny(clippy::try_err)] + +// Tests that a simple case works +// Should flag `Err(err)?` +pub fn basic_test() -> Result { + let err: i32 = 1; + // To avoid warnings during rustfix + if true { + return Err(err); + } + Ok(0) +} + +// Tests that `.into()` is added when appropriate +pub fn into_test() -> Result { + let err: u8 = 1; + // To avoid warnings during rustfix + if true { + return Err(err.into()); + } + 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; + // To avoid warnings during rustfix + if true { + return 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; + // To avoid warnings during rustfix + if true { + return Err(err.into()); + } + 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.rs b/tests/ui/try_err.rs new file mode 100644 index 000000000..828cf639a --- /dev/null +++ b/tests/ui/try_err.rs @@ -0,0 +1,80 @@ +// run-rustfix + +#![deny(clippy::try_err)] + +// Tests that a simple case works +// Should flag `Err(err)?` +pub fn basic_test() -> Result { + let err: i32 = 1; + // To avoid warnings during rustfix + if true { + Err(err)?; + } + Ok(0) +} + +// Tests that `.into()` is added when appropriate +pub fn into_test() -> Result { + let err: u8 = 1; + // To avoid warnings during rustfix + if true { + 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; + // To avoid warnings during rustfix + if true { + 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; + // To avoid warnings during rustfix + if true { + 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 000000000..dbe05ce51 --- /dev/null +++ b/tests/ui/try_err.stderr @@ -0,0 +1,32 @@ +error: returning an `Err(_)` with the `?` operator + --> $DIR/try_err.rs:11:9 + | +LL | Err(err)?; + | ^^^^^^^^^ help: try this: `return Err(err)` + | +note: lint level defined here + --> $DIR/try_err.rs:3:9 + | +LL | #![deny(clippy::try_err)] + | ^^^^^^^^^^^^^^^ + +error: returning an `Err(_)` with the `?` operator + --> $DIR/try_err.rs:21:9 + | +LL | Err(err)?; + | ^^^^^^^^^ help: try this: `return Err(err.into())` + +error: returning an `Err(_)` with the `?` operator + --> $DIR/try_err.rs:41:17 + | +LL | Err(err)?; + | ^^^^^^^^^ help: try this: `return Err(err)` + +error: returning an `Err(_)` with the `?` operator + --> $DIR/try_err.rs:60:17 + | +LL | Err(err)?; + | ^^^^^^^^^ help: try this: `return Err(err.into())` + +error: aborting due to 4 previous errors +