From 7cda04aec4f445fd9a1e07696c057218dfacf671 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Przemys=C5=82aw=20Fuchs?= Date: Thu, 19 Oct 2023 15:46:31 +0200 Subject: [PATCH 1/3] `unusable_matches_binding` lint implementation Closes #7351 --- CHANGELOG.md | 1 + clippy_lints/src/declared_lints.rs | 1 + clippy_lints/src/matches/mod.rs | 35 +++++++++++ .../src/matches/unusable_matches_binding.rs | 61 +++++++++++++++++++ tests/ui/unusable_matches_binding.rs | 22 +++++++ tests/ui/unusable_matches_binding.stderr | 42 +++++++++++++ 6 files changed, 162 insertions(+) create mode 100644 clippy_lints/src/matches/unusable_matches_binding.rs create mode 100644 tests/ui/unusable_matches_binding.rs create mode 100644 tests/ui/unusable_matches_binding.stderr diff --git a/CHANGELOG.md b/CHANGELOG.md index 26c9877dbffb..261093ead9fa 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5558,6 +5558,7 @@ Released 2018-09-13 [`unsound_collection_transmute`]: https://rust-lang.github.io/rust-clippy/master/index.html#unsound_collection_transmute [`unstable_as_mut_slice`]: https://rust-lang.github.io/rust-clippy/master/index.html#unstable_as_mut_slice [`unstable_as_slice`]: https://rust-lang.github.io/rust-clippy/master/index.html#unstable_as_slice +[`unusable_matches_binding`]: https://rust-lang.github.io/rust-clippy/master/index.html#unusable_matches_binding [`unused_async`]: https://rust-lang.github.io/rust-clippy/master/index.html#unused_async [`unused_collect`]: https://rust-lang.github.io/rust-clippy/master/index.html#unused_collect [`unused_format_specs`]: https://rust-lang.github.io/rust-clippy/master/index.html#unused_format_specs diff --git a/clippy_lints/src/declared_lints.rs b/clippy_lints/src/declared_lints.rs index 77438b27f900..0cb66d3aff0f 100644 --- a/clippy_lints/src/declared_lints.rs +++ b/clippy_lints/src/declared_lints.rs @@ -321,6 +321,7 @@ pub(crate) static LINTS: &[&crate::LintInfo] = &[ crate::matches::SINGLE_MATCH_INFO, crate::matches::SINGLE_MATCH_ELSE_INFO, crate::matches::TRY_ERR_INFO, + crate::matches::UNUSABLE_MATCHES_BINDING_INFO, crate::matches::WILDCARD_ENUM_MATCH_ARM_INFO, crate::matches::WILDCARD_IN_OR_PATTERNS_INFO, crate::mem_replace::MEM_REPLACE_OPTION_WITH_NONE_INFO, diff --git a/clippy_lints/src/matches/mod.rs b/clippy_lints/src/matches/mod.rs index b5ab94b3a2f8..a45903642f83 100644 --- a/clippy_lints/src/matches/mod.rs +++ b/clippy_lints/src/matches/mod.rs @@ -22,6 +22,7 @@ mod rest_pat_in_fully_bound_struct; mod significant_drop_in_scrutinee; mod single_match; mod try_err; +mod unusable_matches_binding; mod wild_in_or_pats; use clippy_utils::msrvs::{self, Msrv}; @@ -967,6 +968,38 @@ declare_clippy_lint! { "checks for unnecessary guards in match expressions" } +declare_clippy_lint! { + /// ### What it does + /// Checks for redundant and error prone bindings inside `matches!` macro. + /// + /// ### Why is this bad? + /// It could be hard to determine why the compiler complains about an unused variable, + /// also it could introduce behavior that was not intened by the programmer. + /// + /// ### Example + /// ```rust + /// let data_source = Some(5); + /// let unrelated_data_source = 5; + /// + /// let x = matches!(data_source, unused_binding); + /// let y = matches!(data_source, used_binding if used_binding.is_some()); + /// let z = matches!(data_source, unused_binding if unrelated_data_source > 6); + /// ``` + /// Use instead: + /// ```rust + /// let data_source = Some(5); + /// let unrelated_data_source = 5; + /// + /// let x = true; + /// let y = data_source.is_some(); + /// let z = unrelated_data_source > 6; + /// ``` + #[clippy::version = "1.75.0"] + pub UNUSABLE_MATCHES_BINDING, + correctness, + "default lint description" +} + pub struct Matches { msrv: Msrv, infallible_destructuring_match_linted: bool, @@ -1009,6 +1042,7 @@ impl_lint_pass!(Matches => [ MANUAL_MAP, MANUAL_FILTER, REDUNDANT_GUARDS, + UNUSABLE_MATCHES_BINDING, ]); impl<'tcx> LateLintPass<'tcx> for Matches { @@ -1021,6 +1055,7 @@ impl<'tcx> LateLintPass<'tcx> for Matches { if let ExprKind::Match(ex, arms, source) = expr.kind { if is_direct_expn_of(expr.span, "matches").is_some() { redundant_pattern_match::check_match(cx, expr, ex, arms); + unusable_matches_binding::check_matches(cx, ex, arms, expr); } if source == MatchSource::Normal && !is_span_match(cx, expr.span) { diff --git a/clippy_lints/src/matches/unusable_matches_binding.rs b/clippy_lints/src/matches/unusable_matches_binding.rs new file mode 100644 index 000000000000..c9c6e98fc9c4 --- /dev/null +++ b/clippy_lints/src/matches/unusable_matches_binding.rs @@ -0,0 +1,61 @@ +use clippy_utils::diagnostics::span_lint_and_note; +use clippy_utils::source::snippet_block; +use clippy_utils::visitors::is_local_used; +use rustc_hir::{Arm, Expr, PatKind}; +use rustc_lint::LateContext; + +use super::UNUSABLE_MATCHES_BINDING; + +pub(crate) fn check_matches<'tcx>(cx: &LateContext<'tcx>, ex: &Expr<'tcx>, arms: &'tcx [Arm<'tcx>], expr: &Expr<'tcx>) { + for arm in arms { + if let PatKind::Binding(_, id, ident, None) = arm.pat.kind { + let is_used_in_guard = arm.guard.is_some_and(|guard| is_local_used(cx, guard.body(), id)); + + if !is_local_used(cx, arm.body, id) { + if let Some(guard) = arm.guard { + if is_used_in_guard { + let first_matches_argument = snippet_block(cx, ex.span, "..", None); + span_lint_and_note( + cx, + UNUSABLE_MATCHES_BINDING, + arm.pat.span, + &format!( + "using identificator (`{}`) as matches! pattern argument matches all cases", + ident.name + ), + Some(expr.span), + &format!( + "matches! invocation always evaluates to guard where `{}` was replaced by `{}`", + ident.name, first_matches_argument + ), + ); + } else { + span_lint_and_note( + cx, + UNUSABLE_MATCHES_BINDING, + arm.pat.span, + &format!( + "using identificator (`{}`) as matches! pattern argument without usage in guard has the same effect as evaluating guard", + ident.name + ), + Some(guard.body().span), + "try replacing matches! expression with the content of the guard's body", + ); + } + } else { + span_lint_and_note( + cx, + UNUSABLE_MATCHES_BINDING, + arm.pat.span, + &format!( + "using identificator (`{}`) as matches! pattern argument without guard matches all cases", + ident.name + ), + Some(expr.span), + "matches! invocation always evaluates to `true`", + ); + } + } + } + } +} diff --git a/tests/ui/unusable_matches_binding.rs b/tests/ui/unusable_matches_binding.rs new file mode 100644 index 000000000000..dbd24cbb29db --- /dev/null +++ b/tests/ui/unusable_matches_binding.rs @@ -0,0 +1,22 @@ +#![warn(clippy::unusable_matches_binding)] + +#[derive(Clone, Copy)] +struct TestingStructure(i32); + +impl TestingStructure { + fn is_valid(&self) -> bool { + self.0 > 5 + } +} + +fn main() { + let matching_data_source = TestingStructure(5); + let unrelated_data = 5; + + let _ = matches!(matching_data_source, TestingStructure(4)); + + let _ = matches!(matching_data_source, unusable_binding); + let _ = matches!(matching_data_source, used_binding if used_binding.is_valid()); + + let _ = matches!(matching_data_source, unusable_binding if unrelated_data < 4); +} diff --git a/tests/ui/unusable_matches_binding.stderr b/tests/ui/unusable_matches_binding.stderr new file mode 100644 index 000000000000..fa6c6efaf19b --- /dev/null +++ b/tests/ui/unusable_matches_binding.stderr @@ -0,0 +1,42 @@ +error: using identificator (`unusable_binding`) as matches! pattern argument without guard matches all cases + --> $DIR/unusable_matches_binding.rs:18:44 + | +LL | let _ = matches!(matching_data_source, unusable_binding); + | ^^^^^^^^^^^^^^^^ + | +note: matches! invocation always evaluates to `true` + --> $DIR/unusable_matches_binding.rs:18:13 + | +LL | let _ = matches!(matching_data_source, unusable_binding); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + = note: `-D clippy::unusable-matches-binding` implied by `-D warnings` + = help: to override `-D warnings` add `#[allow(clippy::unusable_matches_binding)]` + = note: this error originates in the macro `matches` (in Nightly builds, run with -Z macro-backtrace for more info) + +error: using identificator (`used_binding`) as matches! pattern argument matches all cases + --> $DIR/unusable_matches_binding.rs:19:44 + | +LL | let _ = matches!(matching_data_source, used_binding if used_binding.is_valid()); + | ^^^^^^^^^^^^ + | +note: matches! invocation always evaluates to guard where `used_binding` was replaced by `matching_data_source` + --> $DIR/unusable_matches_binding.rs:19:13 + | +LL | let _ = matches!(matching_data_source, used_binding if used_binding.is_valid()); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + = note: this error originates in the macro `matches` (in Nightly builds, run with -Z macro-backtrace for more info) + +error: using identificator (`unusable_binding`) as matches! pattern argument without usage in guard has the same effect as evaluating guard + --> $DIR/unusable_matches_binding.rs:21:44 + | +LL | let _ = matches!(matching_data_source, unusable_binding if unrelated_data < 4); + | ^^^^^^^^^^^^^^^^ + | +note: try replacing matches! expression with the content of the guard's body + --> $DIR/unusable_matches_binding.rs:21:64 + | +LL | let _ = matches!(matching_data_source, unusable_binding if unrelated_data < 4); + | ^^^^^^^^^^^^^^^^^^ + +error: aborting due to 3 previous errors + From 40a05e0b1bff51ecf992f9fc5a9c1f37d783e7f2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Przemys=C5=82aw=20Fuchs?= Date: Thu, 12 Oct 2023 20:47:25 +0200 Subject: [PATCH 2/3] Improved documentation of `unusable_matches_binding` lint --- clippy_lints/src/matches/mod.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/clippy_lints/src/matches/mod.rs b/clippy_lints/src/matches/mod.rs index a45903642f83..dfa389a3dc80 100644 --- a/clippy_lints/src/matches/mod.rs +++ b/clippy_lints/src/matches/mod.rs @@ -997,7 +997,7 @@ declare_clippy_lint! { #[clippy::version = "1.75.0"] pub UNUSABLE_MATCHES_BINDING, correctness, - "default lint description" + "checks for unused bindings in `matches!` macro" } pub struct Matches { From 7223b92a5830b2ca62d8f8da4ecc5d01a78cf383 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Przemys=C5=82aw=20Fuchs?= Date: Thu, 19 Oct 2023 16:36:47 +0200 Subject: [PATCH 3/3] Lint (and generated messages) simplified --- clippy_lints/src/matches/mod.rs | 5 +- .../src/matches/unusable_matches_binding.rs | 61 ++++++------------- tests/ui/unusable_matches_binding.stderr | 33 +++------- 3 files changed, 31 insertions(+), 68 deletions(-) diff --git a/clippy_lints/src/matches/mod.rs b/clippy_lints/src/matches/mod.rs index dfa389a3dc80..363ac7b64cf0 100644 --- a/clippy_lints/src/matches/mod.rs +++ b/clippy_lints/src/matches/mod.rs @@ -974,7 +974,8 @@ declare_clippy_lint! { /// /// ### Why is this bad? /// It could be hard to determine why the compiler complains about an unused variable, - /// also it could introduce behavior that was not intened by the programmer. + /// which was introduced by the `matches!` macro expansion, also, it could introduce + /// behavior that was not intended by the programmer. /// /// ### Example /// ```rust @@ -1055,7 +1056,7 @@ impl<'tcx> LateLintPass<'tcx> for Matches { if let ExprKind::Match(ex, arms, source) = expr.kind { if is_direct_expn_of(expr.span, "matches").is_some() { redundant_pattern_match::check_match(cx, expr, ex, arms); - unusable_matches_binding::check_matches(cx, ex, arms, expr); + unusable_matches_binding::check_matches(cx, arms); } if source == MatchSource::Normal && !is_span_match(cx, expr.span) { diff --git a/clippy_lints/src/matches/unusable_matches_binding.rs b/clippy_lints/src/matches/unusable_matches_binding.rs index c9c6e98fc9c4..35751be046eb 100644 --- a/clippy_lints/src/matches/unusable_matches_binding.rs +++ b/clippy_lints/src/matches/unusable_matches_binding.rs @@ -1,58 +1,33 @@ -use clippy_utils::diagnostics::span_lint_and_note; -use clippy_utils::source::snippet_block; +use clippy_utils::diagnostics::{span_lint_and_help, span_lint_and_then}; use clippy_utils::visitors::is_local_used; -use rustc_hir::{Arm, Expr, PatKind}; +use rustc_hir::{Arm, PatKind}; use rustc_lint::LateContext; use super::UNUSABLE_MATCHES_BINDING; -pub(crate) fn check_matches<'tcx>(cx: &LateContext<'tcx>, ex: &Expr<'tcx>, arms: &'tcx [Arm<'tcx>], expr: &Expr<'tcx>) { +pub(crate) fn check_matches<'tcx>(cx: &LateContext<'tcx>, arms: &'tcx [Arm<'tcx>]) { for arm in arms { - if let PatKind::Binding(_, id, ident, None) = arm.pat.kind { - let is_used_in_guard = arm.guard.is_some_and(|guard| is_local_used(cx, guard.body(), id)); - + if let PatKind::Binding(_, id, _, None) = arm.pat.kind { if !is_local_used(cx, arm.body, id) { if let Some(guard) = arm.guard { - if is_used_in_guard { - let first_matches_argument = snippet_block(cx, ex.span, "..", None); - span_lint_and_note( - cx, - UNUSABLE_MATCHES_BINDING, - arm.pat.span, - &format!( - "using identificator (`{}`) as matches! pattern argument matches all cases", - ident.name - ), - Some(expr.span), - &format!( - "matches! invocation always evaluates to guard where `{}` was replaced by `{}`", - ident.name, first_matches_argument - ), - ); - } else { - span_lint_and_note( - cx, - UNUSABLE_MATCHES_BINDING, - arm.pat.span, - &format!( - "using identificator (`{}`) as matches! pattern argument without usage in guard has the same effect as evaluating guard", - ident.name - ), - Some(guard.body().span), - "try replacing matches! expression with the content of the guard's body", - ); - } + span_lint_and_help( + cx, + UNUSABLE_MATCHES_BINDING, + guard.body().span, + "identifier pattern in `matches!` macro always evaluates to the value of the guard", + None, + "if you meant to check predicate, then try changing `matches!` macro into predicate the guard's checking", + ); } else { - span_lint_and_note( + span_lint_and_then( cx, UNUSABLE_MATCHES_BINDING, arm.pat.span, - &format!( - "using identificator (`{}`) as matches! pattern argument without guard matches all cases", - ident.name - ), - Some(expr.span), - "matches! invocation always evaluates to `true`", + "identifier pattern in `matches!` macro always evaluates to true", + |diag| { + diag.note("the identifier pattern matches any value and creates an unusable binding in the process") + .help("if you meant to compare two values, use `x == y` or `discriminant(x) == discriminant(y)`"); + }, ); } } diff --git a/tests/ui/unusable_matches_binding.stderr b/tests/ui/unusable_matches_binding.stderr index fa6c6efaf19b..6da27321aec5 100644 --- a/tests/ui/unusable_matches_binding.stderr +++ b/tests/ui/unusable_matches_binding.stderr @@ -1,42 +1,29 @@ -error: using identificator (`unusable_binding`) as matches! pattern argument without guard matches all cases +error: identifier pattern in `matches!` macro always evaluates to true --> $DIR/unusable_matches_binding.rs:18:44 | LL | let _ = matches!(matching_data_source, unusable_binding); | ^^^^^^^^^^^^^^^^ | -note: matches! invocation always evaluates to `true` - --> $DIR/unusable_matches_binding.rs:18:13 - | -LL | let _ = matches!(matching_data_source, unusable_binding); - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + = note: the identifier pattern matches any value and creates an unusable binding in the process + = help: if you meant to compare two values, use `x == y` or `discriminant(x) == discriminant(y)` = note: `-D clippy::unusable-matches-binding` implied by `-D warnings` = help: to override `-D warnings` add `#[allow(clippy::unusable_matches_binding)]` - = note: this error originates in the macro `matches` (in Nightly builds, run with -Z macro-backtrace for more info) -error: using identificator (`used_binding`) as matches! pattern argument matches all cases - --> $DIR/unusable_matches_binding.rs:19:44 +error: identifier pattern in `matches!` macro always evaluates to the value of the guard + --> $DIR/unusable_matches_binding.rs:19:60 | LL | let _ = matches!(matching_data_source, used_binding if used_binding.is_valid()); - | ^^^^^^^^^^^^ - | -note: matches! invocation always evaluates to guard where `used_binding` was replaced by `matching_data_source` - --> $DIR/unusable_matches_binding.rs:19:13 + | ^^^^^^^^^^^^^^^^^^^^^^^ | -LL | let _ = matches!(matching_data_source, used_binding if used_binding.is_valid()); - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ - = note: this error originates in the macro `matches` (in Nightly builds, run with -Z macro-backtrace for more info) + = help: if you meant to check predicate, then try changing `matches!` macro into predicate the guard's checking -error: using identificator (`unusable_binding`) as matches! pattern argument without usage in guard has the same effect as evaluating guard - --> $DIR/unusable_matches_binding.rs:21:44 - | -LL | let _ = matches!(matching_data_source, unusable_binding if unrelated_data < 4); - | ^^^^^^^^^^^^^^^^ - | -note: try replacing matches! expression with the content of the guard's body +error: identifier pattern in `matches!` macro always evaluates to the value of the guard --> $DIR/unusable_matches_binding.rs:21:64 | LL | let _ = matches!(matching_data_source, unusable_binding if unrelated_data < 4); | ^^^^^^^^^^^^^^^^^^ + | + = help: if you meant to check predicate, then try changing `matches!` macro into predicate the guard's checking error: aborting due to 3 previous errors