Auto merge of rust-lang#9225 - tabokie:assert_ok, r=Jarcho

add `[assertions_on_result_states]` lint

Close rust-lang#9162

changelog: add `[assertions_on_result_states]` lint

Signed-off-by: tabokie <xy.tao@outlook.com>

CHANGELOG.md

@@ -3440,6 +3440,7 @@ Released 2018-09-13
 [`as_conversions`]: https://rust-lang.github.io/rust-clippy/master/index.html#as_conversions
 [`as_underscore`]: https://rust-lang.github.io/rust-clippy/master/index.html#as_underscore
 [`assertions_on_constants`]: https://rust-lang.github.io/rust-clippy/master/index.html#assertions_on_constants
+[`assertions_on_result_states`]: https://rust-lang.github.io/rust-clippy/master/index.html#assertions_on_result_states
 [`assign_op_pattern`]: https://rust-lang.github.io/rust-clippy/master/index.html#assign_op_pattern
 [`assign_ops`]: https://rust-lang.github.io/rust-clippy/master/index.html#assign_ops
 [`async_yields_async`]: https://rust-lang.github.io/rust-clippy/master/index.html#async_yields_async

clippy_lints/src/assertions_on_result_states.rs

@@ -0,0 +1,98 @@
+use clippy_utils::diagnostics::span_lint_and_sugg;
+use clippy_utils::macros::{find_assert_args, root_macro_call_first_node, PanicExpn};
+use clippy_utils::path_res;
+use clippy_utils::source::snippet_with_context;
+use clippy_utils::ty::{implements_trait, is_copy, is_type_diagnostic_item};
+use clippy_utils::usage::local_used_after_expr;
+use rustc_errors::Applicability;
+use rustc_hir::def::Res;
+use rustc_hir::{Expr, ExprKind};
+use rustc_lint::{LateContext, LateLintPass};
+use rustc_middle::ty::{self, Ty};
+use rustc_session::{declare_lint_pass, declare_tool_lint};
+use rustc_span::sym;
+
+declare_clippy_lint! {
+    /// ### What it does
+    /// Checks for `assert!(r.is_ok())` or `assert!(r.is_err())` calls.
+    ///
+    /// ### Why is this bad?
+    /// An assertion failure cannot output an useful message of the error.
+    ///
+    /// ### Example
+    /// ```rust,ignore
+    /// # let r = Ok::<_, ()>(());
+    /// assert!(r.is_ok());
+    /// # let r = Err::<_, ()>(());
+    /// assert!(r.is_err());
+    /// ```
+    #[clippy::version = "1.64.0"]
+    pub ASSERTIONS_ON_RESULT_STATES,
+    style,
+    "`assert!(r.is_ok())`/`assert!(r.is_err())` gives worse error message than directly calling `r.unwrap()`/`r.unwrap_err()`"
+}
+
+declare_lint_pass!(AssertionsOnResultStates => [ASSERTIONS_ON_RESULT_STATES]);
+
+impl<'tcx> LateLintPass<'tcx> for AssertionsOnResultStates {
+    fn check_expr(&mut self, cx: &LateContext<'tcx>, e: &'tcx Expr<'_>) {
+        if let Some(macro_call) = root_macro_call_first_node(cx, e)
+            && matches!(cx.tcx.get_diagnostic_name(macro_call.def_id), Some(sym::assert_macro))
+            && let Some((condition, panic_expn)) = find_assert_args(cx, e, macro_call.expn)
+            && matches!(panic_expn, PanicExpn::Empty)
+            && let ExprKind::MethodCall(method_segment, [recv], _) = condition.kind
+            && let result_type_with_refs = cx.typeck_results().expr_ty(recv)
+            && let result_type = result_type_with_refs.peel_refs()
+            && is_type_diagnostic_item(cx, result_type, sym::Result)
+            && let ty::Adt(_, substs) = result_type.kind()
+        {
+            if !is_copy(cx, result_type) {
+                if result_type_with_refs != result_type {
+                    return;
+                } else if let Res::Local(binding_id) = path_res(cx, recv)
+                    && local_used_after_expr(cx, binding_id, recv) {
+                    return;
+                }
+            }
+            let mut app = Applicability::MachineApplicable;
+            match method_segment.ident.as_str() {
+                "is_ok" if has_debug_impl(cx, substs.type_at(1)) => {
+                    span_lint_and_sugg(
+                        cx,
+                        ASSERTIONS_ON_RESULT_STATES,
+                        macro_call.span,
+                        "called `assert!` with `Result::is_ok`",
+                        "replace with",
+                        format!(
+                            "{}.unwrap()",
+                            snippet_with_context(cx, recv.span, condition.span.ctxt(), "..", &mut app).0
+                        ),
+                        app,
+                    );
+                }
+                "is_err" if has_debug_impl(cx, substs.type_at(0)) => {
+                    span_lint_and_sugg(
+                        cx,
+                        ASSERTIONS_ON_RESULT_STATES,
+                        macro_call.span,
+                        "called `assert!` with `Result::is_err`",
+                        "replace with",
+                        format!(
+                            "{}.unwrap_err()",
+                            snippet_with_context(cx, recv.span, condition.span.ctxt(), "..", &mut app).0
+                        ),
+                        app,
+                    );
+                }
+                _ => (),
+            };
+        }
+    }
+}
+
+/// This checks whether a given type is known to implement Debug.
+fn has_debug_impl<'tcx>(cx: &LateContext<'tcx>, ty: Ty<'tcx>) -> bool {
+    cx.tcx
+        .get_diagnostic_item(sym::Debug)
+        .map_or(false, |debug| implements_trait(cx, ty, debug, &[]))
+}

clippy_lints/src/lib.register_all.rs

@@ -6,6 +6,7 @@ store.register_group(true, "clippy::all", Some("clippy_all"), vec![
     LintId::of(almost_complete_letter_range::ALMOST_COMPLETE_LETTER_RANGE),
     LintId::of(approx_const::APPROX_CONSTANT),
     LintId::of(assertions_on_constants::ASSERTIONS_ON_CONSTANTS),
+    LintId::of(assertions_on_result_states::ASSERTIONS_ON_RESULT_STATES),
     LintId::of(async_yields_async::ASYNC_YIELDS_ASYNC),
     LintId::of(attrs::BLANKET_CLIPPY_RESTRICTION_LINTS),
     LintId::of(attrs::DEPRECATED_CFG_ATTR),

clippy_lints/src/lib.register_lints.rs

@@ -42,6 +42,7 @@ store.register_lints(&[
     asm_syntax::INLINE_ASM_X86_ATT_SYNTAX,
     asm_syntax::INLINE_ASM_X86_INTEL_SYNTAX,
     assertions_on_constants::ASSERTIONS_ON_CONSTANTS,
+    assertions_on_result_states::ASSERTIONS_ON_RESULT_STATES,
     async_yields_async::ASYNC_YIELDS_ASYNC,
     attrs::ALLOW_ATTRIBUTES_WITHOUT_REASON,
     attrs::BLANKET_CLIPPY_RESTRICTION_LINTS,

clippy_lints/src/lib.register_style.rs

@@ -4,6 +4,7 @@
 
 store.register_group(true, "clippy::style", Some("clippy_style"), vec![
     LintId::of(assertions_on_constants::ASSERTIONS_ON_CONSTANTS),
+    LintId::of(assertions_on_result_states::ASSERTIONS_ON_RESULT_STATES),
     LintId::of(blacklisted_name::BLACKLISTED_NAME),
     LintId::of(blocks_in_if_conditions::BLOCKS_IN_IF_CONDITIONS),
     LintId::of(bool_assert_comparison::BOOL_ASSERT_COMPARISON),

clippy_lints/src/lib.rs

@@ -174,6 +174,7 @@ mod as_conversions;
 mod as_underscore;
 mod asm_syntax;
 mod assertions_on_constants;
+mod assertions_on_result_states;
 mod async_yields_async;
 mod attrs;
 mod await_holding_invalid;
@@ -727,6 +728,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
     store.register_late_pass(|| Box::new(unnecessary_sort_by::UnnecessarySortBy));
     store.register_late_pass(move || Box::new(unnecessary_wraps::UnnecessaryWraps::new(avoid_breaking_exported_api)));
     store.register_late_pass(|| Box::new(assertions_on_constants::AssertionsOnConstants));
+    store.register_late_pass(|| Box::new(assertions_on_result_states::AssertionsOnResultStates));
     store.register_late_pass(|| Box::new(transmuting_null::TransmutingNull));
     store.register_late_pass(|| Box::new(path_buf_push_overwrite::PathBufPushOverwrite));
     store.register_late_pass(|| Box::new(inherent_to_string::InherentToString));

tests/ui/assertions_on_result_states.fixed

@@ -0,0 +1,69 @@
+// run-rustfix
+#![warn(clippy::assertions_on_result_states)]
+
+use std::result::Result;
+
+struct Foo;
+
+#[derive(Debug)]
+struct DebugFoo;
+
+#[derive(Copy, Clone, Debug)]
+struct CopyFoo;
+
+macro_rules! get_ok_macro {
+    () => {
+        Ok::<_, DebugFoo>(Foo)
+    };
+}
+
+fn main() {
+    // test ok
+    let r: Result<Foo, DebugFoo> = Ok(Foo);
+    debug_assert!(r.is_ok());
+    r.unwrap();
+
+    // test ok with non-debug error type
+    let r: Result<Foo, Foo> = Ok(Foo);
+    assert!(r.is_ok());
+
+    // test temporary ok
+    fn get_ok() -> Result<Foo, DebugFoo> {
+        Ok(Foo)
+    }
+    get_ok().unwrap();
+
+    // test macro ok
+    get_ok_macro!().unwrap();
+
+    // test ok that shouldn't be moved
+    let r: Result<CopyFoo, DebugFoo> = Ok(CopyFoo);
+    fn test_ref_unmoveable_ok(r: &Result<CopyFoo, DebugFoo>) {
+        assert!(r.is_ok());
+    }
+    test_ref_unmoveable_ok(&r);
+    assert!(r.is_ok());
+    r.unwrap();
+
+    // test ok that is copied
+    let r: Result<CopyFoo, CopyFoo> = Ok(CopyFoo);
+    r.unwrap();
+    r.unwrap();
+
+    // test reference to ok
+    let r: Result<CopyFoo, CopyFoo> = Ok(CopyFoo);
+    fn test_ref_copy_ok(r: &Result<CopyFoo, CopyFoo>) {
+        r.unwrap();
+    }
+    test_ref_copy_ok(&r);
+    r.unwrap();
+
+    // test err
+    let r: Result<DebugFoo, Foo> = Err(Foo);
+    debug_assert!(r.is_err());
+    r.unwrap_err();
+
+    // test err with non-debug value type
+    let r: Result<Foo, Foo> = Err(Foo);
+    assert!(r.is_err());
+}

tests/ui/assertions_on_result_states.rs

@@ -0,0 +1,69 @@
+// run-rustfix
+#![warn(clippy::assertions_on_result_states)]
+
+use std::result::Result;
+
+struct Foo;
+
+#[derive(Debug)]
+struct DebugFoo;
+
+#[derive(Copy, Clone, Debug)]
+struct CopyFoo;
+
+macro_rules! get_ok_macro {
+    () => {
+        Ok::<_, DebugFoo>(Foo)
+    };
+}
+
+fn main() {
+    // test ok
+    let r: Result<Foo, DebugFoo> = Ok(Foo);
+    debug_assert!(r.is_ok());
+    assert!(r.is_ok());
+
+    // test ok with non-debug error type
+    let r: Result<Foo, Foo> = Ok(Foo);
+    assert!(r.is_ok());
+
+    // test temporary ok
+    fn get_ok() -> Result<Foo, DebugFoo> {
+        Ok(Foo)
+    }
+    assert!(get_ok().is_ok());
+
+    // test macro ok
+    assert!(get_ok_macro!().is_ok());
+
+    // test ok that shouldn't be moved
+    let r: Result<CopyFoo, DebugFoo> = Ok(CopyFoo);
+    fn test_ref_unmoveable_ok(r: &Result<CopyFoo, DebugFoo>) {
+        assert!(r.is_ok());
+    }
+    test_ref_unmoveable_ok(&r);
+    assert!(r.is_ok());
+    r.unwrap();
+
+    // test ok that is copied
+    let r: Result<CopyFoo, CopyFoo> = Ok(CopyFoo);
+    assert!(r.is_ok());
+    r.unwrap();
+
+    // test reference to ok
+    let r: Result<CopyFoo, CopyFoo> = Ok(CopyFoo);
+    fn test_ref_copy_ok(r: &Result<CopyFoo, CopyFoo>) {
+        assert!(r.is_ok());
+    }
+    test_ref_copy_ok(&r);
+    r.unwrap();
+
+    // test err
+    let r: Result<DebugFoo, Foo> = Err(Foo);
+    debug_assert!(r.is_err());
+    assert!(r.is_err());
+
+    // test err with non-debug value type
+    let r: Result<Foo, Foo> = Err(Foo);
+    assert!(r.is_err());
+}

tests/ui/assertions_on_result_states.stderr

@@ -0,0 +1,40 @@
+error: called `assert!` with `Result::is_ok`
+  --> $DIR/assertions_on_result_states.rs:24:5
+   |
+LL |     assert!(r.is_ok());
+   |     ^^^^^^^^^^^^^^^^^^ help: replace with: `r.unwrap()`
+   |
+   = note: `-D clippy::assertions-on-result-states` implied by `-D warnings`
+
+error: called `assert!` with `Result::is_ok`
+  --> $DIR/assertions_on_result_states.rs:34:5
+   |
+LL |     assert!(get_ok().is_ok());
+   |     ^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace with: `get_ok().unwrap()`
+
+error: called `assert!` with `Result::is_ok`
+  --> $DIR/assertions_on_result_states.rs:37:5
+   |
+LL |     assert!(get_ok_macro!().is_ok());
+   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace with: `get_ok_macro!().unwrap()`
+
+error: called `assert!` with `Result::is_ok`
+  --> $DIR/assertions_on_result_states.rs:50:5
+   |
+LL |     assert!(r.is_ok());
+   |     ^^^^^^^^^^^^^^^^^^ help: replace with: `r.unwrap()`
+
+error: called `assert!` with `Result::is_ok`
+  --> $DIR/assertions_on_result_states.rs:56:9
+   |
+LL |         assert!(r.is_ok());
+   |         ^^^^^^^^^^^^^^^^^^ help: replace with: `r.unwrap()`
+
+error: called `assert!` with `Result::is_err`
+  --> $DIR/assertions_on_result_states.rs:64:5
+   |
+LL |     assert!(r.is_err());
+   |     ^^^^^^^^^^^^^^^^^^^ help: replace with: `r.unwrap_err()`
+
+error: aborting due to 6 previous errors
+