-
Notifications
You must be signed in to change notification settings - Fork 1.9k
unusable_matches_binding lint implementation
#11661
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -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,39 @@ 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, | ||||||
| /// which was introduced by the `matches!` macro expansion, also, it could introduce | ||||||
| /// behavior that was not intended 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, | ||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
Lint names should be in plural, so that it makes sense when read together with the |
||||||
| correctness, | ||||||
| "checks for unused bindings in `matches!` macro" | ||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
I assume this was meant? More consistent with the lint name |
||||||
| } | ||||||
|
|
||||||
| pub struct Matches { | ||||||
| msrv: Msrv, | ||||||
| infallible_destructuring_match_linted: bool, | ||||||
|
|
@@ -1009,6 +1043,7 @@ impl_lint_pass!(Matches => [ | |||||
| MANUAL_MAP, | ||||||
| MANUAL_FILTER, | ||||||
| REDUNDANT_GUARDS, | ||||||
| UNUSABLE_MATCHES_BINDING, | ||||||
| ]); | ||||||
|
|
||||||
| impl<'tcx> LateLintPass<'tcx> for Matches { | ||||||
|
|
@@ -1021,6 +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, arms); | ||||||
| } | ||||||
|
|
||||||
| if source == MatchSource::Normal && !is_span_match(cx, expr.span) { | ||||||
|
|
||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,36 @@ | ||
| use clippy_utils::diagnostics::{span_lint_and_help, span_lint_and_then}; | ||
| use clippy_utils::visitors::is_local_used; | ||
| use rustc_hir::{Arm, PatKind}; | ||
| use rustc_lint::LateContext; | ||
|
|
||
| use super::UNUSABLE_MATCHES_BINDING; | ||
|
|
||
| pub(crate) fn check_matches<'tcx>(cx: &LateContext<'tcx>, arms: &'tcx [Arm<'tcx>]) { | ||
| for arm in arms { | ||
| if let PatKind::Binding(_, id, _, None) = arm.pat.kind { | ||
| if !is_local_used(cx, arm.body, id) { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think this check here does anything. The body of the arm should always be the literal expression |
||
| if let Some(guard) = arm.guard { | ||
| 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_then( | ||
| cx, | ||
| UNUSABLE_MATCHES_BINDING, | ||
| arm.pat.span, | ||
| "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)`"); | ||
| }, | ||
| ); | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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); | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,29 @@ | ||
| 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: 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)]` | ||
|
|
||
| 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()); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think correctness would be a good category for Something like
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think it's just as bad and should not be used so I agree with the current implementation. |
||
| | ^^^^^^^^^^^^^^^^^^^^^^^ | ||
| | | ||
| = help: if you meant to check predicate, then try changing `matches!` macro into predicate the guard's checking | ||
|
|
||
| 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 | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I find this section a little bit ambiguous/unclear.
I think it would help to explain more clearly where the variable is actually coming from. How about something like this:
Contrary to the
==operator, which compares two values, thematches!macro compares a value to a pattern.An invocation such as
matches!(5, x)expands to the following code:The identifier pattern
xmatches any value and creates a binding that stores its value.This makes identifier patterns in
matches!without any use in guards useless: the macro always evaluates totrueas no equality checking actually takes place and the binding that is created is unusable outside of the macro.