diff --git a/clippy_lints/src/methods/err_expect.rs b/clippy_lints/src/methods/err_expect.rs new file mode 100644 index 000000000000..be9d4ad94fb8 --- /dev/null +++ b/clippy_lints/src/methods/err_expect.rs @@ -0,0 +1,60 @@ +use super::ERR_EXPECT; +use clippy_utils::diagnostics::span_lint_and_sugg; +use clippy_utils::ty::implements_trait; +use clippy_utils::{meets_msrv, msrvs, ty::is_type_diagnostic_item}; +use rustc_errors::Applicability; +use rustc_lint::LateContext; +use rustc_middle::ty; +use rustc_middle::ty::Ty; +use rustc_semver::RustcVersion; +use rustc_span::{sym, Span}; + +pub(super) fn check( + cx: &LateContext<'_>, + _expr: &rustc_hir::Expr<'_>, + recv: &rustc_hir::Expr<'_>, + msrv: Option<&RustcVersion>, + expect_span: Span, + err_span: Span, +) { + if_chain! { + if is_type_diagnostic_item(cx, cx.typeck_results().expr_ty(recv), sym::Result); + // Test the version to make sure the lint can be showed (expect_err has been + // introduced in rust 1.17.0 : https://github.com/rust-lang/rust/pull/38982) + if meets_msrv(msrv, &msrvs::EXPECT_ERR); + + // Grabs the `Result` type + let result_type = cx.typeck_results().expr_ty(recv); + // Tests if the T type in a `Result` is not None + if let Some(data_type) = get_data_type(cx, result_type); + // Tests if the T type in a `Result` implements debug + if has_debug_impl(data_type, cx); + + then { + span_lint_and_sugg( + cx, + ERR_EXPECT, + err_span.to(expect_span), + "called `.err().expect()` on a `Result` value", + "try", + "expect_err".to_string(), + Applicability::MachineApplicable + ); + } + }; +} + +/// Given a `Result` type, return its data (`T`). +fn get_data_type<'a>(cx: &LateContext<'_>, ty: Ty<'a>) -> Option> { + match ty.kind() { + ty::Adt(_, substs) if is_type_diagnostic_item(cx, ty, sym::Result) => substs.types().next(), + _ => None, + } +} + +/// Given a type, very if the Debug trait has been impl'd +fn has_debug_impl<'tcx>(ty: Ty<'tcx>, cx: &LateContext<'tcx>) -> bool { + cx.tcx + .get_diagnostic_item(sym::Debug) + .map_or(false, |debug| implements_trait(cx, ty, debug, &[])) +} diff --git a/clippy_lints/src/methods/mod.rs b/clippy_lints/src/methods/mod.rs index b5a9af8c3bb7..c5c871c9d291 100644 --- a/clippy_lints/src/methods/mod.rs +++ b/clippy_lints/src/methods/mod.rs @@ -9,6 +9,7 @@ mod chars_next_cmp_with_unwrap; mod clone_on_copy; mod clone_on_ref_ptr; mod cloned_instead_of_copied; +mod err_expect; mod expect_fun_call; mod expect_used; mod extend_with_drain; @@ -362,6 +363,29 @@ declare_clippy_lint! { "using `ok().expect()`, which gives worse error messages than calling `expect` directly on the Result" } +declare_clippy_lint! { + /// ### What it does + /// Checks for `.err().expect()` calls on the `Result` type. + /// + /// ### Why is this bad? + /// `.expect_err()` can be called directly to avoid the extra type conversion from `err()`. + /// + /// ### Example + /// ```should_panic + /// let x: Result = Ok(10); + /// x.err().expect("Testing err().expect()"); + /// ``` + /// Use instead: + /// ```should_panic + /// let x: Result = Ok(10); + /// x.expect_err("Testing expect_err"); + /// ``` + #[clippy::version = "1.61.0"] + pub ERR_EXPECT, + style, + r#"using `.err().expect("")` when `.expect_err("")` can be used"# +} + declare_clippy_lint! { /// ### What it does /// Checks for usages of `_.unwrap_or_else(Default::default)` on `Option` and @@ -2168,6 +2192,7 @@ impl_lint_pass!(Methods => [ NEEDLESS_SPLITN, UNNECESSARY_TO_OWNED, UNNECESSARY_JOIN, + ERR_EXPECT, ]); /// Extracts a method call name, args, and `Span` of the method name. @@ -2431,8 +2456,10 @@ fn check_methods<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>, msrv: Optio }, ("expect", [_]) => match method_call(recv) { Some(("ok", [recv], _)) => ok_expect::check(cx, expr, recv), + Some(("err", [recv], err_span)) => err_expect::check(cx, expr, recv, msrv, span, err_span), _ => expect_used::check(cx, expr, recv), }, + ("extend", [arg]) => { string_extend_chars::check(cx, expr, recv, arg); extend_with_drain::check(cx, expr, recv, arg); @@ -2574,6 +2601,7 @@ fn check_methods<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>, msrv: Optio unnecessary_lazy_eval::check(cx, expr, recv, u_arg, "unwrap_or"); }, }, + _ => {}, } } diff --git a/clippy_lints/src/utils/conf.rs b/clippy_lints/src/utils/conf.rs index 680b2eb1da72..0bb818e20790 100644 --- a/clippy_lints/src/utils/conf.rs +++ b/clippy_lints/src/utils/conf.rs @@ -156,7 +156,7 @@ define_Conf! { /// /// Suppress lints whenever the suggested change would cause breakage for other crates. (avoid_breaking_exported_api: bool = true), - /// Lint: MANUAL_SPLIT_ONCE, MANUAL_STR_REPEAT, CLONED_INSTEAD_OF_COPIED, REDUNDANT_FIELD_NAMES, REDUNDANT_STATIC_LIFETIMES, FILTER_MAP_NEXT, CHECKED_CONVERSIONS, MANUAL_RANGE_CONTAINS, USE_SELF, MEM_REPLACE_WITH_DEFAULT, MANUAL_NON_EXHAUSTIVE, OPTION_AS_REF_DEREF, MAP_UNWRAP_OR, MATCH_LIKE_MATCHES_MACRO, MANUAL_STRIP, MISSING_CONST_FOR_FN, UNNESTED_OR_PATTERNS, FROM_OVER_INTO, PTR_AS_PTR, IF_THEN_SOME_ELSE_NONE, APPROX_CONSTANT, DEPRECATED_CFG_ATTR, INDEX_REFUTABLE_SLICE, MAP_CLONE, BORROW_AS_PTR, MANUAL_BITS. + /// Lint: MANUAL_SPLIT_ONCE, MANUAL_STR_REPEAT, CLONED_INSTEAD_OF_COPIED, REDUNDANT_FIELD_NAMES, REDUNDANT_STATIC_LIFETIMES, FILTER_MAP_NEXT, CHECKED_CONVERSIONS, MANUAL_RANGE_CONTAINS, USE_SELF, MEM_REPLACE_WITH_DEFAULT, MANUAL_NON_EXHAUSTIVE, OPTION_AS_REF_DEREF, MAP_UNWRAP_OR, MATCH_LIKE_MATCHES_MACRO, MANUAL_STRIP, MISSING_CONST_FOR_FN, UNNESTED_OR_PATTERNS, FROM_OVER_INTO, PTR_AS_PTR, IF_THEN_SOME_ELSE_NONE, APPROX_CONSTANT, DEPRECATED_CFG_ATTR, INDEX_REFUTABLE_SLICE, MAP_CLONE, BORROW_AS_PTR, MANUAL_BITS, EXPECT_ERR. /// /// The minimum rust version that the project supports (msrv: Option = None), diff --git a/clippy_utils/src/msrvs.rs b/clippy_utils/src/msrvs.rs index fce93153d96e..12191109b8c9 100644 --- a/clippy_utils/src/msrvs.rs +++ b/clippy_utils/src/msrvs.rs @@ -30,6 +30,6 @@ msrv_aliases! { 1,34,0 { TRY_FROM } 1,30,0 { ITERATOR_FIND_MAP, TOOL_ATTRIBUTES } 1,28,0 { FROM_BOOL } - 1,17,0 { FIELD_INIT_SHORTHAND, STATIC_IN_CONST } + 1,17,0 { FIELD_INIT_SHORTHAND, STATIC_IN_CONST, EXPECT_ERR } 1,16,0 { STR_REPEAT } } diff --git a/tests/ui/err_expect.fixed b/tests/ui/err_expect.fixed new file mode 100644 index 000000000000..7e18d70bae40 --- /dev/null +++ b/tests/ui/err_expect.fixed @@ -0,0 +1,14 @@ +// run-rustfix + +struct MyTypeNonDebug; + +#[derive(Debug)] +struct MyTypeDebug; + +fn main() { + let test_debug: Result = Ok(MyTypeDebug); + test_debug.expect_err("Testing debug type"); + + let test_non_debug: Result = Ok(MyTypeNonDebug); + test_non_debug.err().expect("Testing non debug type"); +} diff --git a/tests/ui/err_expect.rs b/tests/ui/err_expect.rs new file mode 100644 index 000000000000..bf8c3c9fb8c9 --- /dev/null +++ b/tests/ui/err_expect.rs @@ -0,0 +1,14 @@ +// run-rustfix + +struct MyTypeNonDebug; + +#[derive(Debug)] +struct MyTypeDebug; + +fn main() { + let test_debug: Result = Ok(MyTypeDebug); + test_debug.err().expect("Testing debug type"); + + let test_non_debug: Result = Ok(MyTypeNonDebug); + test_non_debug.err().expect("Testing non debug type"); +} diff --git a/tests/ui/err_expect.stderr b/tests/ui/err_expect.stderr new file mode 100644 index 000000000000..ffd97e00a5c0 --- /dev/null +++ b/tests/ui/err_expect.stderr @@ -0,0 +1,10 @@ +error: called `.err().expect()` on a `Result` value + --> $DIR/err_expect.rs:10:16 + | +LL | test_debug.err().expect("Testing debug type"); + | ^^^^^^^^^^^^ help: try: `expect_err` + | + = note: `-D clippy::err-expect` implied by `-D warnings` + +error: aborting due to previous error + diff --git a/tests/ui/min_rust_version_attr.rs b/tests/ui/min_rust_version_attr.rs index 72e9bf9eb368..7666d01ffe12 100644 --- a/tests/ui/min_rust_version_attr.rs +++ b/tests/ui/min_rust_version_attr.rs @@ -145,6 +145,11 @@ fn int_from_bool() -> u8 { true as u8 } +fn err_expect() { + let x: Result = Ok(10); + x.err().expect("Testing expect_err"); +} + fn main() { filter_map_next(); checked_conversion(); @@ -162,6 +167,7 @@ fn main() { missing_const_for_fn(); unnest_or_patterns(); int_from_bool(); + err_expect(); } mod just_under_msrv { diff --git a/tests/ui/min_rust_version_attr.stderr b/tests/ui/min_rust_version_attr.stderr index 6b3fdb0844b4..9ed6308f1153 100644 --- a/tests/ui/min_rust_version_attr.stderr +++ b/tests/ui/min_rust_version_attr.stderr @@ -1,12 +1,12 @@ error: stripping a prefix manually - --> $DIR/min_rust_version_attr.rs:186:24 + --> $DIR/min_rust_version_attr.rs:192:24 | LL | assert_eq!(s["hello, ".len()..].to_uppercase(), "WORLD!"); | ^^^^^^^^^^^^^^^^^^^^ | = note: `-D clippy::manual-strip` implied by `-D warnings` note: the prefix was tested here - --> $DIR/min_rust_version_attr.rs:185:9 + --> $DIR/min_rust_version_attr.rs:191:9 | LL | if s.starts_with("hello, ") { | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ @@ -17,13 +17,13 @@ LL ~ assert_eq!(.to_uppercase(), "WORLD!"); | error: stripping a prefix manually - --> $DIR/min_rust_version_attr.rs:198:24 + --> $DIR/min_rust_version_attr.rs:204:24 | LL | assert_eq!(s["hello, ".len()..].to_uppercase(), "WORLD!"); | ^^^^^^^^^^^^^^^^^^^^ | note: the prefix was tested here - --> $DIR/min_rust_version_attr.rs:197:9 + --> $DIR/min_rust_version_attr.rs:203:9 | LL | if s.starts_with("hello, ") { | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^