From efc87cd8e2d1892aff254427d503181de7a1a762 Mon Sep 17 00:00:00 2001 From: Daine Mamacos Date: Thu, 25 Jun 2026 14:35:55 +0200 Subject: [PATCH] Add `ignored_result_err` lint to detect discarded Result error variants Add a new restriction lint that warns when the Err variant of a Result is implicitly discarded. The lint detects three patterns: - `if let Ok(x) = expr` (with or without else) - `while let Ok(x) = expr` - `let Ok(x) = expr else { ... }` In all these cases, the error value is lost, preventing detailed logging and making error recovery impossible. The lint suggests using `match` with an explicit `Err(e)` binding instead. This is an opt-in restriction lint, enabled with: #![warn(clippy::ignored_result_err)] A configuration option `allow-ignored-result-err-in-tests = true` can be set in clippy.toml to suppress the lint in `#[test]` functions and `#[cfg(test)]` modules. Tested via the compile-test UI test harness with cases covering all three patterns, a negative case for `match` with bound Err, and a ui-toml test verifying the allow-in-tests configuration works correctly. --- CHANGELOG.md | 2 + book/src/lint_configuration.md | 10 ++ clippy_config/src/conf.rs | 3 + clippy_lints/src/declared_lints.rs | 1 + clippy_lints/src/ignored_result_err.rs | 141 ++++++++++++++++++ clippy_lints/src/lib.rs | 2 + tests/ui-toml/ignored_result_err/clippy.toml | 1 + .../ignored_result_err/ignored_result_err.rs | 33 ++++ .../ignored_result_err.stderr | 15 ++ .../toml_unknown_key/conf_unknown_key.stderr | 3 + tests/ui/ignored_result_err.rs | 40 +++++ tests/ui/ignored_result_err.stderr | 50 +++++++ 12 files changed, 301 insertions(+) create mode 100644 clippy_lints/src/ignored_result_err.rs create mode 100644 tests/ui-toml/ignored_result_err/clippy.toml create mode 100644 tests/ui-toml/ignored_result_err/ignored_result_err.rs create mode 100644 tests/ui-toml/ignored_result_err/ignored_result_err.stderr create mode 100644 tests/ui/ignored_result_err.rs create mode 100644 tests/ui/ignored_result_err.stderr diff --git a/CHANGELOG.md b/CHANGELOG.md index 856c229f4679..2b0df3d514b3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6978,6 +6978,7 @@ Released 2018-09-13 [`if_then_some_else_none`]: https://rust-lang.github.io/rust-clippy/master/index.html#if_then_some_else_none [`ifs_same_cond`]: https://rust-lang.github.io/rust-clippy/master/index.html#ifs_same_cond [`ignore_without_reason`]: https://rust-lang.github.io/rust-clippy/master/index.html#ignore_without_reason +[`ignored_result_err`]: https://rust-lang.github.io/rust-clippy/master/index.html#ignored_result_err [`ignored_unit_patterns`]: https://rust-lang.github.io/rust-clippy/master/index.html#ignored_unit_patterns [`impl_hash_borrow_with_str_and_bytes`]: https://rust-lang.github.io/rust-clippy/master/index.html#impl_hash_borrow_with_str_and_bytes [`impl_trait_in_params`]: https://rust-lang.github.io/rust-clippy/master/index.html#impl_trait_in_params @@ -7662,6 +7663,7 @@ Released 2018-09-13 [`allow-exact-repetitions`]: https://doc.rust-lang.org/clippy/lint_configuration.html#allow-exact-repetitions [`allow-expect-in-consts`]: https://doc.rust-lang.org/clippy/lint_configuration.html#allow-expect-in-consts [`allow-expect-in-tests`]: https://doc.rust-lang.org/clippy/lint_configuration.html#allow-expect-in-tests +[`allow-ignored-result-err-in-tests`]: https://doc.rust-lang.org/clippy/lint_configuration.html#allow-ignored-result-err-in-tests [`allow-indexing-slicing-in-tests`]: https://doc.rust-lang.org/clippy/lint_configuration.html#allow-indexing-slicing-in-tests [`allow-large-stack-frames-in-tests`]: https://doc.rust-lang.org/clippy/lint_configuration.html#allow-large-stack-frames-in-tests [`allow-mixed-uninlined-format-args`]: https://doc.rust-lang.org/clippy/lint_configuration.html#allow-mixed-uninlined-format-args diff --git a/book/src/lint_configuration.md b/book/src/lint_configuration.md index fb4df6234dac..030437f9c0a5 100644 --- a/book/src/lint_configuration.md +++ b/book/src/lint_configuration.md @@ -101,6 +101,16 @@ Whether `expect` should be allowed in test functions or `#[cfg(test)]` * [`expect_used`](https://rust-lang.github.io/rust-clippy/master/index.html#expect_used) +## `allow-ignored-result-err-in-tests` +Whether `ignored_result_err` should be allowed in test functions or `#[cfg(test)]` + +**Default Value:** `false` + +--- +**Affected lints:** +* [`ignored_result_err`](https://rust-lang.github.io/rust-clippy/master/index.html#ignored_result_err) + + ## `allow-indexing-slicing-in-tests` Whether `indexing_slicing` should be allowed in test functions or `#[cfg(test)]` diff --git a/clippy_config/src/conf.rs b/clippy_config/src/conf.rs index 8a2795defc08..29725ba89a30 100644 --- a/clippy_config/src/conf.rs +++ b/clippy_config/src/conf.rs @@ -371,6 +371,9 @@ define_Conf! { /// Whether `expect` should be allowed in test functions or `#[cfg(test)]` #[lints(expect_used)] allow_expect_in_tests: bool = false, + /// Whether `ignored_result_err` should be allowed in test functions or `#[cfg(test)]` + #[lints(ignored_result_err)] + allow_ignored_result_err_in_tests: bool = false, /// Whether `indexing_slicing` should be allowed in test functions or `#[cfg(test)]` #[lints(indexing_slicing)] allow_indexing_slicing_in_tests: bool = false, diff --git a/clippy_lints/src/declared_lints.rs b/clippy_lints/src/declared_lints.rs index e77a0ff4a8ab..ef589139dbc7 100644 --- a/clippy_lints/src/declared_lints.rs +++ b/clippy_lints/src/declared_lints.rs @@ -213,6 +213,7 @@ pub static LINTS: &[&::declare_clippy_lint::LintInfo] = &[ crate::ifs::IF_SAME_THEN_ELSE_INFO, crate::ifs::IFS_SAME_COND_INFO, crate::ifs::SAME_FUNCTIONS_IN_IF_CONDITION_INFO, + crate::ignored_result_err::IGNORED_RESULT_ERR_INFO, crate::ignored_unit_patterns::IGNORED_UNIT_PATTERNS_INFO, crate::impl_hash_with_borrow_str_and_bytes::IMPL_HASH_BORROW_WITH_STR_AND_BYTES_INFO, crate::implicit_hasher::IMPLICIT_HASHER_INFO, diff --git a/clippy_lints/src/ignored_result_err.rs b/clippy_lints/src/ignored_result_err.rs new file mode 100644 index 000000000000..5e6327c944b2 --- /dev/null +++ b/clippy_lints/src/ignored_result_err.rs @@ -0,0 +1,141 @@ +use clippy_config::Conf; +use clippy_utils::diagnostics::span_lint_and_help; +use clippy_utils::res::MaybeDef; +use clippy_utils::{higher, is_in_test}; +use rustc_hir::{Expr, LetStmt, Pat, PatKind, QPath}; +use rustc_lint::{LateContext, LateLintPass}; +use rustc_session::impl_lint_pass; +use rustc_span::sym; + +declare_clippy_lint! { + /// ### What it does + /// Checks for `if let Ok(x) = expr`, `while let Ok(x) = expr`, and + /// `let Ok(x) = expr else { ... }` where the `Err` variant is discarded + /// without binding. + /// + /// ### Why is this bad? + /// The error value contains context about what went wrong. Discarding it + /// prevents detailed logging and makes error recovery impossible. + /// + /// ### Example + /// ```rust,ignore + /// if let Ok(res) = some_call() { + /// use_res(res); + /// } else { + /// error!("Something went wrong"); + /// } + /// + /// while let Ok(line) = reader.read_line() { + /// process(line); + /// } + /// + /// let Ok(val) = some_call() else { return; }; + /// ``` + /// Use instead: + /// ```rust,ignore + /// match some_call() { + /// Ok(res) => use_res(res), + /// Err(e) => error!("Something went wrong: {}", e), + /// } + /// + /// loop { + /// match reader.read_line() { + /// Ok(line) => process(line), + /// Err(e) => { + /// error!("Read failed: {}", e); + /// break; + /// } + /// } + /// } + /// + /// match some_call() { + /// Ok(val) => { /* use val */ }, + /// Err(e) => { + /// error!("Failed: {}", e); + /// return; + /// } + /// } + /// ``` + /// + /// ### Configuration + /// - `allow-ignored-result-err-in-tests`: set to `true` to disable this lint in test code + #[clippy::version = "1.98.0"] + pub IGNORED_RESULT_ERR, + restriction, + "`if let Ok(x) = ...`, `while let Ok(x) = ...`, or `let Ok(x) = ... else` discards the error variant without binding it" +} + +impl_lint_pass!(IgnoredResultErr => [IGNORED_RESULT_ERR]); + +pub struct IgnoredResultErr { + allow_in_tests: bool, +} + +impl IgnoredResultErr { + pub fn new(conf: &'static Conf) -> Self { + Self { + allow_in_tests: conf.allow_ignored_result_err_in_tests, + } + } +} + +fn is_ok_pattern_on_result(cx: &LateContext<'_>, pat: &Pat<'_>, scrutinee: &Expr<'_>) -> bool { + cx.typeck_results().expr_ty(scrutinee).is_diag_item(cx, sym::Result) + && matches!( + pat.kind, + PatKind::TupleStruct(QPath::Resolved(_, path), _, _) + if path.segments.last().is_some_and(|s| s.ident.name == sym::Ok) + ) +} + +impl<'tcx> LateLintPass<'tcx> for IgnoredResultErr { + fn check_local(&mut self, cx: &LateContext<'tcx>, local: &'tcx LetStmt<'_>) { + if self.allow_in_tests && is_in_test(cx.tcx, local.hir_id) { + return; + } + + if local.els.is_some() + && let Some(init) = local.init + && is_ok_pattern_on_result(cx, local.pat, init) + { + span_lint_and_help( + cx, + IGNORED_RESULT_ERR, + local.span, + "this `let Ok(...) = ... else` discards the `Err` variant", + None, + "consider using `match` and binding the `Err` value for logging or recovery", + ); + } + } + + fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) { + if self.allow_in_tests && is_in_test(cx.tcx, expr.hir_id) { + return; + } + + if let Some(if_let) = higher::IfLet::hir(cx, expr) + && is_ok_pattern_on_result(cx, if_let.let_pat, if_let.let_expr) + { + span_lint_and_help( + cx, + IGNORED_RESULT_ERR, + expr.span, + "this `if let Ok(...)` discards the `Err` variant", + None, + "consider using `match` and binding the `Err` value for logging or recovery", + ); + } else if let Some(while_let) = higher::WhileLet::hir(expr) + && is_ok_pattern_on_result(cx, while_let.let_pat, while_let.let_expr) + { + span_lint_and_help( + cx, + IGNORED_RESULT_ERR, + expr.span, + "this `while let Ok(...)` discards the `Err` variant", + None, + "consider using `loop` + `match` and binding the `Err` value for logging or recovery", + ); + } + } +} diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index 74e03c9b3859..d9cd0283e102 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -155,6 +155,7 @@ mod if_let_mutex; mod if_not_else; mod if_then_some_else_none; mod ifs; +mod ignored_result_err; mod ignored_unit_patterns; mod impl_hash_with_borrow_str_and_bytes; mod implicit_hasher; @@ -866,6 +867,7 @@ rustc_lint::late_lint_methods!( RefPatterns: ref_patterns::RefPatterns = ref_patterns::RefPatterns, RedundantElse: redundant_else::RedundantElse = redundant_else::RedundantElse, RestWhenDestructuringStruct: rest_when_destructuring_struct::RestWhenDestructuringStruct = rest_when_destructuring_struct::RestWhenDestructuringStruct, + IgnoredResultErr: ignored_result_err::IgnoredResultErr = ignored_result_err::IgnoredResultErr::new(conf), // add late passes here, used by `cargo dev new_lint` ]] ); diff --git a/tests/ui-toml/ignored_result_err/clippy.toml b/tests/ui-toml/ignored_result_err/clippy.toml new file mode 100644 index 000000000000..2e4483aebb9f --- /dev/null +++ b/tests/ui-toml/ignored_result_err/clippy.toml @@ -0,0 +1 @@ +allow-ignored-result-err-in-tests = true diff --git a/tests/ui-toml/ignored_result_err/ignored_result_err.rs b/tests/ui-toml/ignored_result_err/ignored_result_err.rs new file mode 100644 index 000000000000..567a3ed8578b --- /dev/null +++ b/tests/ui-toml/ignored_result_err/ignored_result_err.rs @@ -0,0 +1,33 @@ +#![warn(clippy::ignored_result_err)] + +fn some_call() -> Result { + Ok(42) +} + +// Should lint — not in test context +fn main() { + if let Ok(res) = some_call() { + //~^ ignored_result_err + println!("{res}"); + } +} + +// Should NOT lint — in a #[test] function (allow-ignored-result-err-in-tests = true) +#[test] +fn test_something() { + if let Ok(res) = some_call() { + println!("{res}"); + } +} + +// Should NOT lint — in a #[cfg(test)] module +#[cfg(test)] +mod tests { + use super::some_call; + + fn helper() { + if let Ok(res) = some_call() { + println!("{res}"); + } + } +} diff --git a/tests/ui-toml/ignored_result_err/ignored_result_err.stderr b/tests/ui-toml/ignored_result_err/ignored_result_err.stderr new file mode 100644 index 000000000000..79344563e4db --- /dev/null +++ b/tests/ui-toml/ignored_result_err/ignored_result_err.stderr @@ -0,0 +1,15 @@ +error: this `if let Ok(...)` discards the `Err` variant + --> tests/ui-toml/ignored_result_err/ignored_result_err.rs:9:5 + | +LL | / if let Ok(res) = some_call() { +LL | | +LL | | println!("{res}"); +LL | | } + | |_____^ + | + = help: consider using `match` and binding the `Err` value for logging or recovery + = note: `-D clippy::ignored-result-err` implied by `-D warnings` + = help: to override `-D warnings` add `#[allow(clippy::ignored_result_err)]` + +error: aborting due to 1 previous error + diff --git a/tests/ui-toml/toml_unknown_key/conf_unknown_key.stderr b/tests/ui-toml/toml_unknown_key/conf_unknown_key.stderr index 537b80dbace2..a5229e7862bb 100644 --- a/tests/ui-toml/toml_unknown_key/conf_unknown_key.stderr +++ b/tests/ui-toml/toml_unknown_key/conf_unknown_key.stderr @@ -8,6 +8,7 @@ error: error reading Clippy's configuration file: unknown field `foobar`, expect allow-exact-repetitions allow-expect-in-consts allow-expect-in-tests + allow-ignored-result-err-in-tests allow-indexing-slicing-in-tests allow-large-stack-frames-in-tests allow-mixed-uninlined-format-args @@ -111,6 +112,7 @@ error: error reading Clippy's configuration file: unknown field `barfoo`, expect allow-exact-repetitions allow-expect-in-consts allow-expect-in-tests + allow-ignored-result-err-in-tests allow-indexing-slicing-in-tests allow-large-stack-frames-in-tests allow-mixed-uninlined-format-args @@ -214,6 +216,7 @@ error: error reading Clippy's configuration file: unknown field `allow_mixed_uni allow-exact-repetitions allow-expect-in-consts allow-expect-in-tests + allow-ignored-result-err-in-tests allow-indexing-slicing-in-tests allow-large-stack-frames-in-tests allow-mixed-uninlined-format-args diff --git a/tests/ui/ignored_result_err.rs b/tests/ui/ignored_result_err.rs new file mode 100644 index 000000000000..ac4042114886 --- /dev/null +++ b/tests/ui/ignored_result_err.rs @@ -0,0 +1,40 @@ +#![warn(clippy::ignored_result_err)] + +fn some_call() -> Result { + Ok(42) +} + +fn main() { + // Should lint — if let with else + if let Ok(res) = some_call() { + //~^ ignored_result_err + println!("{res}"); + } else { + println!("something went wrong"); + } + + // Should lint — if let without else + if let Ok(res) = some_call() { + //~^ ignored_result_err + println!("{res}"); + } + + // Should lint — while let + while let Ok(res) = some_call() { + //~^ ignored_result_err + println!("{res}"); + } + + // Should lint — let else + let Ok(res) = some_call() else { + //~^ ignored_result_err + return; + }; + println!("{res}"); + + // Should NOT lint — match with Err bound + match some_call() { + Ok(res) => println!("{res}"), + Err(e) => println!("failed: {e}"), + } +} diff --git a/tests/ui/ignored_result_err.stderr b/tests/ui/ignored_result_err.stderr new file mode 100644 index 000000000000..2c7b96e66685 --- /dev/null +++ b/tests/ui/ignored_result_err.stderr @@ -0,0 +1,50 @@ +error: this `if let Ok(...)` discards the `Err` variant + --> tests/ui/ignored_result_err.rs:9:5 + | +LL | / if let Ok(res) = some_call() { +LL | | +LL | | println!("{res}"); +LL | | } else { +LL | | println!("something went wrong"); +LL | | } + | |_____^ + | + = help: consider using `match` and binding the `Err` value for logging or recovery + = note: `-D clippy::ignored-result-err` implied by `-D warnings` + = help: to override `-D warnings` add `#[allow(clippy::ignored_result_err)]` + +error: this `if let Ok(...)` discards the `Err` variant + --> tests/ui/ignored_result_err.rs:17:5 + | +LL | / if let Ok(res) = some_call() { +LL | | +LL | | println!("{res}"); +LL | | } + | |_____^ + | + = help: consider using `match` and binding the `Err` value for logging or recovery + +error: this `while let Ok(...)` discards the `Err` variant + --> tests/ui/ignored_result_err.rs:23:5 + | +LL | / while let Ok(res) = some_call() { +LL | | +LL | | println!("{res}"); +LL | | } + | |_____^ + | + = help: consider using `loop` + `match` and binding the `Err` value for logging or recovery + +error: this `let Ok(...) = ... else` discards the `Err` variant + --> tests/ui/ignored_result_err.rs:29:5 + | +LL | / let Ok(res) = some_call() else { +LL | | +LL | | return; +LL | | }; + | |______^ + | + = help: consider using `match` and binding the `Err` value for logging or recovery + +error: aborting due to 4 previous errors +