From 05b419c1f85cd45a00906c71ec93daada6478691 Mon Sep 17 00:00:00 2001 From: Guillaume Gomez Date: Tue, 18 Mar 2025 23:09:15 +0100 Subject: [PATCH 1/2] Add new `eq_should_be_match` lint --- CHANGELOG.md | 1 + clippy_lints/src/declared_lints.rs | 1 + clippy_lints/src/eq_should_be_match.rs | 242 +++++++++++++++++++++++++ clippy_lints/src/lib.rs | 2 + 4 files changed, 246 insertions(+) create mode 100644 clippy_lints/src/eq_should_be_match.rs diff --git a/CHANGELOG.md b/CHANGELOG.md index 1bf4b51ff0fe..d0be85c88f69 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5607,6 +5607,7 @@ Released 2018-09-13 [`enum_glob_use`]: https://rust-lang.github.io/rust-clippy/master/index.html#enum_glob_use [`enum_variant_names`]: https://rust-lang.github.io/rust-clippy/master/index.html#enum_variant_names [`eq_op`]: https://rust-lang.github.io/rust-clippy/master/index.html#eq_op +[`eq_should_be_match`]: https://rust-lang.github.io/rust-clippy/master/index.html#eq_should_be_match [`equatable_if_let`]: https://rust-lang.github.io/rust-clippy/master/index.html#equatable_if_let [`erasing_op`]: https://rust-lang.github.io/rust-clippy/master/index.html#erasing_op [`err_expect`]: https://rust-lang.github.io/rust-clippy/master/index.html#err_expect diff --git a/clippy_lints/src/declared_lints.rs b/clippy_lints/src/declared_lints.rs index 7fa23dad6981..723c5c1ca8a4 100644 --- a/clippy_lints/src/declared_lints.rs +++ b/clippy_lints/src/declared_lints.rs @@ -171,6 +171,7 @@ pub static LINTS: &[&crate::LintInfo] = &[ crate::endian_bytes::LITTLE_ENDIAN_BYTES_INFO, crate::entry::MAP_ENTRY_INFO, crate::enum_clike::ENUM_CLIKE_UNPORTABLE_VARIANT_INFO, + crate::eq_should_be_match::EQ_SHOULD_BE_MATCH_INFO, crate::equatable_if_let::EQUATABLE_IF_LET_INFO, crate::error_impl_error::ERROR_IMPL_ERROR_INFO, crate::escape::BOXED_LOCAL_INFO, diff --git a/clippy_lints/src/eq_should_be_match.rs b/clippy_lints/src/eq_should_be_match.rs new file mode 100644 index 000000000000..fb80affae66f --- /dev/null +++ b/clippy_lints/src/eq_should_be_match.rs @@ -0,0 +1,242 @@ +use rustc_data_structures::fx::FxHashSet; +use rustc_errors::Applicability; +use rustc_hir::{BinOpKind, Expr, ExprKind, Path, QPath}; +use rustc_lint::{LateContext, LateLintPass}; +use rustc_middle::ty::{self, Ty}; +use rustc_session::impl_lint_pass; +use rustc_span::Span; + +use clippy_utils::diagnostics::span_lint_and_then; +use clippy_utils::get_parent_expr; +use clippy_utils::source::snippet_opt; + +declare_clippy_lint! { + /// ### What it does + /// Suggests to replace `==` and `!=` comparisons with `matches!` macro. + /// + /// ### Why is this bad? + /// It generates smaller assembly. + /// + /// ### Example + /// ```no_run + /// let d1 = [0, 1]; + /// let d2 = Foo { first: 0, second: 4 }; + /// let d3 = (0, 1); + /// + /// d1 == [1, 2]; + /// d2 == Foo { first: 1, second: 2 }; + /// d3 == (1, 2) || d3 == (2, 3) || d3 == (1, 4); + /// ``` + /// Use instead: + /// ```no_run + /// let d1 = [0, 1]; + /// let d2 = Foo { first: 0, second: 4 }; + /// let d3 = (0, 1); + /// + /// matches!(d1, [1, 2]); + /// matches!(d2, Foo { first: 1, second: 2 }); + /// matches!(d3, (1, 2) | (2, 3) | (1, 4)); + /// ``` + #[clippy::version = "1.87.0"] + pub EQ_SHOULD_BE_MATCH, + perf, + "eq comparison should be a pattern matching" +} + +impl_lint_pass!(EqShouldBeMatch => [EQ_SHOULD_BE_MATCH]); + +#[derive(Default)] +pub(crate) struct EqShouldBeMatch { + handled_spans: FxHashSet, +} + +fn is_primitive(typ: Ty<'_>) -> bool { + matches!( + typ.kind(), + ty::Bool | ty::Char | ty::Int(_) | ty::Uint(_) | ty::Float(_) | ty::Str + ) +} + +fn get_if_literal<'a>( + cx: &LateContext<'_>, + left: &'a Expr<'a>, + right: &'a Expr<'a>, +) -> Option<(&'a Expr<'a>, &'a Expr<'a>)> { + match (left.kind, right.kind) { + (_, ExprKind::Tup(_) | ExprKind::Struct(..) | ExprKind::Array(_)) => { + // All good! + let typeck = cx.typeck_results(); + if let Some(left_ty) = typeck.expr_ty_opt(left).map(|ty| ty.peel_refs()) + && !is_primitive(left_ty) + { + Some((left, right)) + } else { + None + } + }, + (ExprKind::Tup(_) | ExprKind::Struct(..) | ExprKind::Array(_), _) => { + // We switch args. + get_if_literal(cx, right, left) + }, + (_, _) => None, + } +} + +fn emit_lint(cx: &LateContext<'_>, expr_span: Span, left: &Expr<'_>, right: String, op: BinOpKind) { + if let Some(left_expr) = snippet_opt(cx, left.span) { + span_lint_and_then( + cx, + EQ_SHOULD_BE_MATCH, + expr_span, + "this comparison would be faster with pattern matching", + |diag| { + diag.span_suggestion( + expr_span, + "try", + format!( + "{}matches!({left_expr}, {right})", + if op == BinOpKind::Ne { "!" } else { "" }, + ), + Applicability::MachineApplicable, + ); + }, + ); + } +} + +// Extend the span if the `expr_span` starts sooner or ends later than `span`. +fn extend_span(span: &mut Span, expr_span: Span) { + if span.lo() > expr_span.lo() { + *span = span.with_lo(expr_span.lo()) + } + if span.hi() < expr_span.hi() { + *span = span.with_hi(expr_span.hi()) + } +} + +impl EqShouldBeMatch { + // This method generates the suggestion to group items if there is more than one, + // we simply suggest the basic `matches!` replacement. + fn handle_current( + &mut self, + cx: &LateContext<'_>, + current: &mut Vec<(&Expr<'_>, &Expr<'_>, BinOpKind)>, + current_info: &mut Option<(&Path<'_>, BinOpKind)>, + ) { + if let Some((left, _, op)) = current.first() { + let mut span = left.span; + let mut out = String::new(); + let mut should_emit_lint = true; + + for (left, right, _) in current.iter() { + extend_span(&mut span, left.span); + extend_span(&mut span, right.span); + + if let Some(snippet) = snippet_opt(cx, right.span) { + if !out.is_empty() { + out.push_str(" | "); + } + out.push_str(&snippet); + } else { + should_emit_lint = false; + break; + } + } + if should_emit_lint { + for (left, _, _) in current.iter() { + if let Some(parent_expr) = get_parent_expr(cx, left) { + self.handled_spans.insert(parent_expr.span); + } + } + emit_lint(cx, span, left, out, *op); + } + } + current.clear(); + *current_info = None; + } + + fn handle_or_expr<'a>( + &self, + cx: &LateContext<'_>, + expr: &'a Expr<'a>, + comparisons: &mut Vec<(&'a Expr<'a>, &'a Expr<'a>, BinOpKind)>, + ) -> bool { + if self.handled_spans.contains(&expr.span) { + return true; + } + match expr.kind { + ExprKind::Binary(bin_op, sub_left, sub_right) => match bin_op.node { + BinOpKind::Eq | BinOpKind::Ne => { + if let Some((sub_left, sub_right)) = get_if_literal(cx, sub_left, sub_right) { + comparisons.push((sub_left, sub_right, bin_op.node)); + true + } else { + false + } + }, + BinOpKind::Or => { + self.handle_or_expr(cx, sub_left, comparisons) && self.handle_or_expr(cx, sub_right, comparisons) + }, + _ => false, + }, + _ => false, + } + } +} + +impl<'tcx> LateLintPass<'tcx> for EqShouldBeMatch { + fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'tcx>) { + if let ExprKind::Binary(op, left, right) = expr.kind { + match op.node { + BinOpKind::Eq | BinOpKind::Ne if !self.handled_spans.contains(&expr.span) => { + if let Some((left, right)) = get_if_literal(cx, left, right) + && let Some(right) = snippet_opt(cx, right.span) + { + emit_lint(cx, expr.span, left, right, op.node) + } + }, + // In case we have `x == (1, 2) || x == (3, 4)`, we go through it and check + // if it can be simplified. For that, we need to check that `x` is actually the + // same one. + // + // If there is any comparison in between that cannot be grouped with the others, + // then we render for the group we already have and then we resume. + BinOpKind::Or if !self.handled_spans.contains(&expr.span) => { + let mut comparisons = Vec::new(); + + self.handle_or_expr(cx, left, &mut comparisons); + self.handle_or_expr(cx, right, &mut comparisons); + if comparisons.len() > 1 { + // There might be multiple parts that can be grouped, so let's group + // them if possible. + let mut current_info = None; + let mut current = Vec::new(); + + for (left, right, op) in comparisons.iter() { + // If we encounter an item which is not a variable, we render the rest + // and skip this one. + let ExprKind::Path(QPath::Resolved(_, path)) = left.kind else { + self.handle_current(cx, &mut current, &mut current_info); + continue; + }; + match current_info { + Some((current_path, current_op)) => { + // If the current comparison cannot be grouped with the previous + // one, we render the previous one then resume. + if current_path.res != path.res || current_op != *op { + self.handle_current(cx, &mut current, &mut current_info); + } + }, + None => {}, + } + current.push((left, right, *op)); + current_info = Some((path, *op)); + } + self.handle_current(cx, &mut current, &mut current_info); + } + }, + _ => {}, + } + } + } +} diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index 3fe3cd67e167..eaee8553a2b6 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -131,6 +131,7 @@ mod empty_with_brackets; mod endian_bytes; mod entry; mod enum_clike; +mod eq_should_be_match; mod equatable_if_let; mod error_impl_error; mod escape; @@ -984,5 +985,6 @@ pub fn register_lints(store: &mut rustc_lint::LintStore, conf: &'static Conf) { store.register_late_pass(move |_| Box::new(non_std_lazy_statics::NonStdLazyStatic::new(conf))); store.register_late_pass(|_| Box::new(manual_option_as_slice::ManualOptionAsSlice::new(conf))); store.register_late_pass(|_| Box::new(single_option_map::SingleOptionMap)); + store.register_late_pass(|_| Box::new(eq_should_be_match::EqShouldBeMatch::default())); // add lints here, do not remove this comment, it's used in `new_lint` } From 4e5b4bfc882af773d3fb73450d33048a2a6bc5d9 Mon Sep 17 00:00:00 2001 From: Guillaume Gomez Date: Tue, 18 Mar 2025 23:09:26 +0100 Subject: [PATCH 2/2] Add ui regression test for `eq_should_be_match` lint --- tests/ui/eq_should_be_match.fixed | 45 ++++++++++++ tests/ui/eq_should_be_match.rs | 45 ++++++++++++ tests/ui/eq_should_be_match.stderr | 107 +++++++++++++++++++++++++++++ 3 files changed, 197 insertions(+) create mode 100644 tests/ui/eq_should_be_match.fixed create mode 100644 tests/ui/eq_should_be_match.rs create mode 100644 tests/ui/eq_should_be_match.stderr diff --git a/tests/ui/eq_should_be_match.fixed b/tests/ui/eq_should_be_match.fixed new file mode 100644 index 000000000000..e8c453fbd359 --- /dev/null +++ b/tests/ui/eq_should_be_match.fixed @@ -0,0 +1,45 @@ +#![warn(clippy::eq_should_be_match)] + +#[derive(PartialEq)] +struct Foo { + first: u8, + second: u8, +} + +fn main() { + let d1 = [0, 1]; + let d2 = Foo { first: 0, second: 4 }; + let d3 = (0, 1); + + let _ = matches!(d1, [1, 2]); + //~^ eq_should_be_match + let _ = matches!(d2, Foo { first: 1, second: 2 }); + //~^ eq_should_be_match + let _ = !matches!(d3, (1, 2)); + //~^ eq_should_be_match + let _ = matches!(d3, (1, 2) | (2, 3) | (1, 4)); + //~^ eq_should_be_match + // It should only suggest to group the last two items. + let _ = matches!(d2, Foo { first: 1, second: 2 }) || matches!(d3, (2, 3) | (1, 4)); + //~^ eq_should_be_match + //~| eq_should_be_match + + let _ = matches!(d3, (2, 3)) || matches!(d2, Foo { first: 1, second: 2 }) || matches!(d3, (1, 4)); + //~^ eq_should_be_match + //~| eq_should_be_match + //~| eq_should_be_match + + let _ = !matches!(d1, [1, 2]); + //~^ eq_should_be_match + let _ = !matches!(d2, Foo { first: 1, second: 2 }); + //~^ eq_should_be_match + let _ = !matches!(d3, (1, 2)); + //~^ eq_should_be_match + let _ = matches!(d3, (1, 2)) || !matches!(d3, (2, 3)) || matches!(d3, (1, 4)); + //~^ eq_should_be_match + //~| eq_should_be_match + //~| eq_should_be_match + let _ = !matches!(d3, (1, 2)) || matches!(d3, (2, 3) | (1, 4)); + //~^ eq_should_be_match + //~| eq_should_be_match +} diff --git a/tests/ui/eq_should_be_match.rs b/tests/ui/eq_should_be_match.rs new file mode 100644 index 000000000000..89a6948de89d --- /dev/null +++ b/tests/ui/eq_should_be_match.rs @@ -0,0 +1,45 @@ +#![warn(clippy::eq_should_be_match)] + +#[derive(PartialEq)] +struct Foo { + first: u8, + second: u8, +} + +fn main() { + let d1 = [0, 1]; + let d2 = Foo { first: 0, second: 4 }; + let d3 = (0, 1); + + let _ = d1 == [1, 2]; + //~^ eq_should_be_match + let _ = d2 == Foo { first: 1, second: 2 }; + //~^ eq_should_be_match + let _ = d3 != (1, 2); + //~^ eq_should_be_match + let _ = d3 == (1, 2) || d3 == (2, 3) || d3 == (1, 4); + //~^ eq_should_be_match + // It should only suggest to group the last two items. + let _ = d2 == Foo { first: 1, second: 2 } || d3 == (2, 3) || d3 == (1, 4); + //~^ eq_should_be_match + //~| eq_should_be_match + + let _ = d3 == (2, 3) || d2 == Foo { first: 1, second: 2 } || d3 == (1, 4); + //~^ eq_should_be_match + //~| eq_should_be_match + //~| eq_should_be_match + + let _ = d1 != [1, 2]; + //~^ eq_should_be_match + let _ = d2 != Foo { first: 1, second: 2 }; + //~^ eq_should_be_match + let _ = d3 != (1, 2); + //~^ eq_should_be_match + let _ = d3 == (1, 2) || d3 != (2, 3) || d3 == (1, 4); + //~^ eq_should_be_match + //~| eq_should_be_match + //~| eq_should_be_match + let _ = d3 != (1, 2) || d3 == (2, 3) || d3 == (1, 4); + //~^ eq_should_be_match + //~| eq_should_be_match +} diff --git a/tests/ui/eq_should_be_match.stderr b/tests/ui/eq_should_be_match.stderr new file mode 100644 index 000000000000..a7de97d1d2d6 --- /dev/null +++ b/tests/ui/eq_should_be_match.stderr @@ -0,0 +1,107 @@ +error: this comparison would be faster with pattern matching + --> tests/ui/eq_should_be_match.rs:14:13 + | +LL | let _ = d1 == [1, 2]; + | ^^^^^^^^^^^^ help: try: `matches!(d1, [1, 2])` + | + = note: `-D clippy::eq-should-be-match` implied by `-D warnings` + = help: to override `-D warnings` add `#[allow(clippy::eq_should_be_match)]` + +error: this comparison would be faster with pattern matching + --> tests/ui/eq_should_be_match.rs:16:13 + | +LL | let _ = d2 == Foo { first: 1, second: 2 }; + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `matches!(d2, Foo { first: 1, second: 2 })` + +error: this comparison would be faster with pattern matching + --> tests/ui/eq_should_be_match.rs:18:13 + | +LL | let _ = d3 != (1, 2); + | ^^^^^^^^^^^^ help: try: `!matches!(d3, (1, 2))` + +error: this comparison would be faster with pattern matching + --> tests/ui/eq_should_be_match.rs:20:13 + | +LL | let _ = d3 == (1, 2) || d3 == (2, 3) || d3 == (1, 4); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `matches!(d3, (1, 2) | (2, 3) | (1, 4))` + +error: this comparison would be faster with pattern matching + --> tests/ui/eq_should_be_match.rs:23:13 + | +LL | let _ = d2 == Foo { first: 1, second: 2 } || d3 == (2, 3) || d3 == (1, 4); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `matches!(d2, Foo { first: 1, second: 2 })` + +error: this comparison would be faster with pattern matching + --> tests/ui/eq_should_be_match.rs:23:50 + | +LL | let _ = d2 == Foo { first: 1, second: 2 } || d3 == (2, 3) || d3 == (1, 4); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `matches!(d3, (2, 3) | (1, 4))` + +error: this comparison would be faster with pattern matching + --> tests/ui/eq_should_be_match.rs:27:13 + | +LL | let _ = d3 == (2, 3) || d2 == Foo { first: 1, second: 2 } || d3 == (1, 4); + | ^^^^^^^^^^^^ help: try: `matches!(d3, (2, 3))` + +error: this comparison would be faster with pattern matching + --> tests/ui/eq_should_be_match.rs:27:29 + | +LL | let _ = d3 == (2, 3) || d2 == Foo { first: 1, second: 2 } || d3 == (1, 4); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `matches!(d2, Foo { first: 1, second: 2 })` + +error: this comparison would be faster with pattern matching + --> tests/ui/eq_should_be_match.rs:27:66 + | +LL | let _ = d3 == (2, 3) || d2 == Foo { first: 1, second: 2 } || d3 == (1, 4); + | ^^^^^^^^^^^^ help: try: `matches!(d3, (1, 4))` + +error: this comparison would be faster with pattern matching + --> tests/ui/eq_should_be_match.rs:32:13 + | +LL | let _ = d1 != [1, 2]; + | ^^^^^^^^^^^^ help: try: `!matches!(d1, [1, 2])` + +error: this comparison would be faster with pattern matching + --> tests/ui/eq_should_be_match.rs:34:13 + | +LL | let _ = d2 != Foo { first: 1, second: 2 }; + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `!matches!(d2, Foo { first: 1, second: 2 })` + +error: this comparison would be faster with pattern matching + --> tests/ui/eq_should_be_match.rs:36:13 + | +LL | let _ = d3 != (1, 2); + | ^^^^^^^^^^^^ help: try: `!matches!(d3, (1, 2))` + +error: this comparison would be faster with pattern matching + --> tests/ui/eq_should_be_match.rs:38:13 + | +LL | let _ = d3 == (1, 2) || d3 != (2, 3) || d3 == (1, 4); + | ^^^^^^^^^^^^ help: try: `matches!(d3, (1, 2))` + +error: this comparison would be faster with pattern matching + --> tests/ui/eq_should_be_match.rs:38:29 + | +LL | let _ = d3 == (1, 2) || d3 != (2, 3) || d3 == (1, 4); + | ^^^^^^^^^^^^ help: try: `!matches!(d3, (2, 3))` + +error: this comparison would be faster with pattern matching + --> tests/ui/eq_should_be_match.rs:38:45 + | +LL | let _ = d3 == (1, 2) || d3 != (2, 3) || d3 == (1, 4); + | ^^^^^^^^^^^^ help: try: `matches!(d3, (1, 4))` + +error: this comparison would be faster with pattern matching + --> tests/ui/eq_should_be_match.rs:42:13 + | +LL | let _ = d3 != (1, 2) || d3 == (2, 3) || d3 == (1, 4); + | ^^^^^^^^^^^^ help: try: `!matches!(d3, (1, 2))` + +error: this comparison would be faster with pattern matching + --> tests/ui/eq_should_be_match.rs:42:29 + | +LL | let _ = d3 != (1, 2) || d3 == (2, 3) || d3 == (1, 4); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `matches!(d3, (2, 3) | (1, 4))` + +error: aborting due to 17 previous errors +