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

Move [`assertions_on_result_states`] to restriction Close rust-lang#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 <xy.tao@outlook.com>

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()`