From 8454602cef2d4ce1271425c0924303955ee47e82 Mon Sep 17 00:00:00 2001 From: tabokie Date: Fri, 22 Jul 2022 13:17:10 +0800 Subject: [PATCH] Add `[assertions_on_result_states]` lint Signed-off-by: tabokie --- CHANGELOG.md | 1 + .../src/assertions_on_result_states.rs | 98 +++++++++++++++++++ clippy_lints/src/lib.register_all.rs | 1 + clippy_lints/src/lib.register_lints.rs | 1 + clippy_lints/src/lib.register_style.rs | 1 + clippy_lints/src/lib.rs | 2 + tests/ui/assertions_on_result_states.fixed | 69 +++++++++++++ tests/ui/assertions_on_result_states.rs | 69 +++++++++++++ tests/ui/assertions_on_result_states.stderr | 40 ++++++++ 9 files changed, 282 insertions(+) create mode 100644 clippy_lints/src/assertions_on_result_states.rs create mode 100644 tests/ui/assertions_on_result_states.fixed create mode 100644 tests/ui/assertions_on_result_states.rs create mode 100644 tests/ui/assertions_on_result_states.stderr diff --git a/CHANGELOG.md b/CHANGELOG.md index 1d5c216c154a6..e64e5d94bc95d 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/clippy_lints/src/assertions_on_result_states.rs b/clippy_lints/src/assertions_on_result_states.rs new file mode 100644 index 0000000000000..b6affdee52364 --- /dev/null +++ b/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, &[])) +} diff --git a/clippy_lints/src/lib.register_all.rs b/clippy_lints/src/lib.register_all.rs index 0ba9b7ae7e581..5be1c417bf8f6 100644 --- a/clippy_lints/src/lib.register_all.rs +++ b/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), diff --git a/clippy_lints/src/lib.register_lints.rs b/clippy_lints/src/lib.register_lints.rs index d40c6aec04ae3..65e01de2b1601 100644 --- a/clippy_lints/src/lib.register_lints.rs +++ b/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, diff --git a/clippy_lints/src/lib.register_style.rs b/clippy_lints/src/lib.register_style.rs index e95bab1d0454d..e029a5235e720 100644 --- a/clippy_lints/src/lib.register_style.rs +++ b/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), diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index 8444532f6bbf9..6cd1928e9c1c6 100644 --- a/clippy_lints/src/lib.rs +++ b/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)); diff --git a/tests/ui/assertions_on_result_states.fixed b/tests/ui/assertions_on_result_states.fixed new file mode 100644 index 0000000000000..7bde72e4b6b57 --- /dev/null +++ b/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 = Ok(Foo); + debug_assert!(r.is_ok()); + r.unwrap(); + + // test ok with non-debug error type + let r: Result = Ok(Foo); + assert!(r.is_ok()); + + // test temporary ok + fn get_ok() -> Result { + Ok(Foo) + } + get_ok().unwrap(); + + // test macro ok + get_ok_macro!().unwrap(); + + // test ok that shouldn't be moved + let r: Result = Ok(CopyFoo); + fn test_ref_unmoveable_ok(r: &Result) { + assert!(r.is_ok()); + } + test_ref_unmoveable_ok(&r); + assert!(r.is_ok()); + r.unwrap(); + + // test ok that is copied + let r: Result = Ok(CopyFoo); + r.unwrap(); + r.unwrap(); + + // test reference to ok + let r: Result = Ok(CopyFoo); + fn test_ref_copy_ok(r: &Result) { + r.unwrap(); + } + test_ref_copy_ok(&r); + r.unwrap(); + + // test err + let r: Result = Err(Foo); + debug_assert!(r.is_err()); + r.unwrap_err(); + + // test err with non-debug value type + let r: Result = Err(Foo); + assert!(r.is_err()); +} diff --git a/tests/ui/assertions_on_result_states.rs b/tests/ui/assertions_on_result_states.rs new file mode 100644 index 0000000000000..4c5af81efc23f --- /dev/null +++ b/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 = Ok(Foo); + debug_assert!(r.is_ok()); + assert!(r.is_ok()); + + // test ok with non-debug error type + let r: Result = Ok(Foo); + assert!(r.is_ok()); + + // test temporary ok + fn get_ok() -> Result { + 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 = Ok(CopyFoo); + fn test_ref_unmoveable_ok(r: &Result) { + assert!(r.is_ok()); + } + test_ref_unmoveable_ok(&r); + assert!(r.is_ok()); + r.unwrap(); + + // test ok that is copied + let r: Result = Ok(CopyFoo); + assert!(r.is_ok()); + r.unwrap(); + + // test reference to ok + let r: Result = Ok(CopyFoo); + fn test_ref_copy_ok(r: &Result) { + assert!(r.is_ok()); + } + test_ref_copy_ok(&r); + r.unwrap(); + + // test err + let r: Result = Err(Foo); + debug_assert!(r.is_err()); + assert!(r.is_err()); + + // test err with non-debug value type + let r: Result = Err(Foo); + assert!(r.is_err()); +} diff --git a/tests/ui/assertions_on_result_states.stderr b/tests/ui/assertions_on_result_states.stderr new file mode 100644 index 0000000000000..13c2dd877a976 --- /dev/null +++ b/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 +