Auto merge of #9273 - tabokie:assert_ok_fp, r=flip1995

Move [`assertions_on_result_states`] to restriction

Close #9263

This lint causes regression on readability of code and log output. And printing runtime values is not particularly useful for majority of tests which should be reproducible.

changelog: Move [`assertions_on_result_states`] to restriction and don't lint it for unit type

Signed-off-by: tabokie <[email protected]>

Author: bors

clippy_lints/src/assertions_on_result_states.rs

@@ -19,6 +19,9 @@ declare_clippy_lint! {
     /// ### Why is this bad?
     /// An assertion failure cannot output an useful message of the error.
     ///
+    /// ### Known problems
+    /// The suggested replacement decreases the readability of code and log output.
+    ///
     /// ### Example
     /// ```rust,ignore
     /// # let r = Ok::<_, ()>(());
@@ -28,7 +31,7 @@ declare_clippy_lint! {
     /// ```
     #[clippy::version = "1.64.0"]
     pub ASSERTIONS_ON_RESULT_STATES,
-    style,
+    restriction,
     "`assert!(r.is_ok())`/`assert!(r.is_err())` gives worse error message than directly calling `r.unwrap()`/`r.unwrap_err()`"
 }
 
@@ -50,13 +53,14 @@ impl<'tcx> LateLintPass<'tcx> for AssertionsOnResultStates {
                 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) {
+                    && 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)) => {
+                "is_ok" if type_suitable_to_unwrap(cx, substs.type_at(1)) => {
                     span_lint_and_sugg(
                         cx,
                         ASSERTIONS_ON_RESULT_STATES,
@@ -70,7 +74,7 @@ impl<'tcx> LateLintPass<'tcx> for AssertionsOnResultStates {
                         app,
                     );
                 }
-                "is_err" if has_debug_impl(cx, substs.type_at(0)) => {
+                "is_err" if type_suitable_to_unwrap(cx, substs.type_at(0)) => {
                     span_lint_and_sugg(
                         cx,
                         ASSERTIONS_ON_RESULT_STATES,
@@ -96,3 +100,7 @@ fn has_debug_impl<'tcx>(cx: &LateContext<'tcx>, ty: Ty<'tcx>) -> bool {
         .get_diagnostic_item(sym::Debug)
         .map_or(false, |debug| implements_trait(cx, ty, debug, &[]))
 }
+
+fn type_suitable_to_unwrap<'tcx>(cx: &LateContext<'tcx>, ty: Ty<'tcx>) -> bool {
+    has_debug_impl(cx, ty) && !ty.is_unit() && !ty.is_never()
+}

clippy_lints/src/lib.register_all.rs

@@ -6,7 +6,6 @@ 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_restriction.rs

@@ -7,6 +7,7 @@ store.register_group(true, "clippy::restriction", Some("clippy_restriction"), ve
     LintId::of(as_underscore::AS_UNDERSCORE),
     LintId::of(asm_syntax::INLINE_ASM_X86_ATT_SYNTAX),
     LintId::of(asm_syntax::INLINE_ASM_X86_INTEL_SYNTAX),
+    LintId::of(assertions_on_result_states::ASSERTIONS_ON_RESULT_STATES),
     LintId::of(attrs::ALLOW_ATTRIBUTES_WITHOUT_REASON),
     LintId::of(casts::FN_TO_NUMERIC_CAST_ANY),
     LintId::of(create_dir::CREATE_DIR),

clippy_lints/src/lib.register_style.rs

@@ -4,7 +4,6 @@
 
 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(blocks_in_if_conditions::BLOCKS_IN_IF_CONDITIONS),
     LintId::of(bool_assert_comparison::BOOL_ASSERT_COMPARISON),
     LintId::of(casts::FN_TO_NUMERIC_CAST),

tests/ui/assertions_on_result_states.fixed

@@ -27,6 +27,14 @@ fn main() {
     let r: Result<Foo, Foo> = Ok(Foo);
     assert!(r.is_ok());
 
+    // test ok with some messages
+    let r: Result<Foo, DebugFoo> = Ok(Foo);
+    assert!(r.is_ok(), "oops");
+
+    // test ok with unit error
+    let r: Result<Foo, ()> = Ok(Foo);
+    assert!(r.is_ok());
+
     // test temporary ok
     fn get_ok() -> Result<Foo, DebugFoo> {
         Ok(Foo)

tests/ui/assertions_on_result_states.rs

@@ -27,6 +27,14 @@ fn main() {
     let r: Result<Foo, Foo> = Ok(Foo);
     assert!(r.is_ok());
 
+    // test ok with some messages
+    let r: Result<Foo, DebugFoo> = Ok(Foo);
+    assert!(r.is_ok(), "oops");
+
+    // test ok with unit error
+    let r: Result<Foo, ()> = Ok(Foo);
+    assert!(r.is_ok());
+
     // test temporary ok
     fn get_ok() -> Result<Foo, DebugFoo> {
         Ok(Foo)

tests/ui/assertions_on_result_states.stderr

@@ -7,31 +7,31 @@ LL |     assert!(r.is_ok());
    = 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
+  --> $DIR/assertions_on_result_states.rs:42: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
+  --> $DIR/assertions_on_result_states.rs:45: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
+  --> $DIR/assertions_on_result_states.rs:58: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
+  --> $DIR/assertions_on_result_states.rs:64: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
+  --> $DIR/assertions_on_result_states.rs:72:5
    |
 LL |     assert!(r.is_err());
    |     ^^^^^^^^^^^^^^^^^^^ help: replace with: `r.unwrap_err()`