From 568f730132998e54fd9abe82ecd88f77c5803e22 Mon Sep 17 00:00:00 2001 From: Ada Alakbarova Date: Thu, 5 Jun 2025 22:24:41 +0200 Subject: [PATCH] it works --- CHANGELOG.md | 1 + clippy_lints/src/declared_lints.rs | 1 + clippy_lints/src/duplicate_match_guards.rs | 187 +++++++++++++++++++ clippy_lints/src/lib.rs | 2 + tests/ui/duplicate_match_guards.fixed | 190 +++++++++++++++++++ tests/ui/duplicate_match_guards.rs | 204 ++++++++++++++++++++ tests/ui/duplicate_match_guards.stderr | 205 +++++++++++++++++++++ 7 files changed, 790 insertions(+) create mode 100644 clippy_lints/src/duplicate_match_guards.rs create mode 100644 tests/ui/duplicate_match_guards.fixed create mode 100644 tests/ui/duplicate_match_guards.rs create mode 100644 tests/ui/duplicate_match_guards.stderr diff --git a/CHANGELOG.md b/CHANGELOG.md index b6b374e26c96..fe49fd1237ab 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6084,6 +6084,7 @@ Released 2018-09-13 [`drop_copy`]: https://rust-lang.github.io/rust-clippy/master/index.html#drop_copy [`drop_non_drop`]: https://rust-lang.github.io/rust-clippy/master/index.html#drop_non_drop [`drop_ref`]: https://rust-lang.github.io/rust-clippy/master/index.html#drop_ref +[`duplicate_match_guards`]: https://rust-lang.github.io/rust-clippy/master/index.html#duplicate_match_guards [`duplicate_mod`]: https://rust-lang.github.io/rust-clippy/master/index.html#duplicate_mod [`duplicate_underscore_argument`]: https://rust-lang.github.io/rust-clippy/master/index.html#duplicate_underscore_argument [`duplicated_attributes`]: https://rust-lang.github.io/rust-clippy/master/index.html#duplicated_attributes diff --git a/clippy_lints/src/declared_lints.rs b/clippy_lints/src/declared_lints.rs index dd5c5dcf4d1f..a54ad0e6be4d 100644 --- a/clippy_lints/src/declared_lints.rs +++ b/clippy_lints/src/declared_lints.rs @@ -132,6 +132,7 @@ pub static LINTS: &[&::declare_clippy_lint::LintInfo] = &[ crate::drop_forget_ref::DROP_NON_DROP_INFO, crate::drop_forget_ref::FORGET_NON_DROP_INFO, crate::drop_forget_ref::MEM_FORGET_INFO, + crate::duplicate_match_guards::DUPLICATE_MATCH_GUARDS_INFO, crate::duplicate_mod::DUPLICATE_MOD_INFO, crate::else_if_without_else::ELSE_IF_WITHOUT_ELSE_INFO, crate::empty_drop::EMPTY_DROP_INFO, diff --git a/clippy_lints/src/duplicate_match_guards.rs b/clippy_lints/src/duplicate_match_guards.rs new file mode 100644 index 000000000000..c0bdf38d581c --- /dev/null +++ b/clippy_lints/src/duplicate_match_guards.rs @@ -0,0 +1,187 @@ +use clippy_utils::diagnostics::{span_lint_and_sugg, span_lint_and_then}; +use clippy_utils::source::{ + HasSession, IntoSpan, SpanRangeExt, indent_of, reindent_multiline, snippet_with_applicability, trim_span, +}; +use clippy_utils::{eq_expr_value, span_contains_comment}; +use rustc_errors::Applicability; +use rustc_hir::{Arm, ExprKind}; +use rustc_lint::{LateContext, LateLintPass}; +use rustc_session::declare_lint_pass; +use rustc_span::BytePos; + +declare_clippy_lint! { + /// ### What it does + /// Checks for the same condition being checked in a match guard and in the match body + /// + /// ### Why is this bad? + /// This is usually just a typo or a copy and paste error. + /// + /// ### Example + /// ```no_run + /// # let n = 0; + /// # let a = 3; + /// # let b = 4; + /// match n { + /// 0 if a > b => { + /// if a > b { + /// return; + /// } + /// } + /// _ => {} + /// } + /// ``` + /// Use instead: + /// ```no_run + /// # let n = 0; + /// # let a = 3; + /// # let b = 4; + /// match n { + /// 0 if a > b => { + /// return; + /// } + /// _ => {} + /// } + /// ``` + #[clippy::version = "1.92.0"] + pub DUPLICATE_MATCH_GUARDS, + nursery, + "a condition in match body duplicating the match guard" +} + +declare_lint_pass!(DuplicateMatchGuards => [DUPLICATE_MATCH_GUARDS]); + +impl<'tcx> LateLintPass<'tcx> for DuplicateMatchGuards { + fn check_arm(&mut self, cx: &LateContext<'tcx>, arm: &'tcx Arm<'_>) { + let Some(guard) = arm.guard else { + return; + }; + + let (arm_body_expr, body_has_block) = if let ExprKind::Block(block, _) = arm.body.kind { + if block.stmts.is_empty() + && let Some(trailing_expr) = block.expr + { + (trailing_expr, true) + } else { + // the body contains something other than the `if` clause -- bail out + return; + } + } else { + (arm.body, false) + }; + + if let ExprKind::If(cond, then, None) = arm_body_expr.kind + && eq_expr_value(cx, guard, cond.peel_drop_temps()) + { + // Make sure that we won't swallow any comments. + // Be extra conservative and bail out on _any_ comment outside of `then`: + // + // if => { if } + // ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ ^^ + let sm = cx.sess().source_map(); + if span_contains_comment(sm, arm.span.with_hi(then.span.lo())) + || span_contains_comment(sm, arm.span.with_lo(then.span.hi())) + { + return; + } + + // The two expressions may be syntactically different, even if identical semantically + // -- the user might want to replace the condition in the guard with the one in the body. + let mut applicability = Applicability::MaybeIncorrect; + + if body_has_block { + // the common case: + // ``` + // match 0u32 { + // 0 if true => { + // if true { + // return; + // } + // } + // } + // ``` + // + // The arm body already has curlies, so we can remove the ones around `then` + + if !then + .span + .check_source_text(cx, |s| s.starts_with('{') && s.ends_with('}')) + { + // Despite being a block, `then` somehow does not start and end with curlies. + // Bail out just to be sure + return; + } + + let sugg_span = then + .span + .with_lo(then.span.lo() + BytePos(1)) + .with_hi(then.span.hi() - BytePos(1)); + + let sugg_span = trim_span(sm, sugg_span); + + let sugg = snippet_with_applicability(cx, sugg_span, "..", &mut applicability); + + // Since we remove one level of curlies, we should be able to dedent `then` left one level, so this: + // ``` + // if => { + // if { + // then + // without + // curlies + // } + // } + // ``` + // becomes this: + // ``` + // if => { + // then + // without + // curlies + // } + // ``` + let indent = indent_of(cx, sugg_span); + let sugg = reindent_multiline(&sugg, true, indent.map(|i| i.saturating_sub(4))); + + span_lint_and_sugg( + cx, + DUPLICATE_MATCH_GUARDS, + arm_body_expr.span, + "condition duplicates match guard", + "remove the condition", + sugg, + applicability, + ); + } else { + // The uncommon case (rusfmt would add the curlies here automatically) + // ``` + // match 0u32 { + // if => if { return; } + // } + // ``` + // + // the arm body doesn't have its own curlies, + // so we need to retain the ones around `then` + + let span_to_remove = arm_body_expr + .span + .with_hi(cond.span.hi()) + // NOTE: we don't remove trailing whitespace so that there's one space left between `` and `{` + .with_leading_whitespace(cx) + .into_span(); + + span_lint_and_then( + cx, + DUPLICATE_MATCH_GUARDS, + arm_body_expr.span, + "condition duplicates match guard", + |diag| { + diag.multipart_suggestion_verbose( + "remove the condition", + vec![(span_to_remove, String::new())], + applicability, + ); + }, + ); + } + } + } +} diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index e3168b8cfe02..37a8710537bd 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -119,6 +119,7 @@ mod disallowed_types; mod doc; mod double_parens; mod drop_forget_ref; +mod duplicate_match_guards; mod duplicate_mod; mod else_if_without_else; mod empty_drop; @@ -832,5 +833,6 @@ pub fn register_lint_passes(store: &mut rustc_lint::LintStore, conf: &'static Co store.register_late_pass(|_| Box::new(coerce_container_to_any::CoerceContainerToAny)); store.register_late_pass(|_| Box::new(toplevel_ref_arg::ToplevelRefArg)); store.register_late_pass(|_| Box::new(volatile_composites::VolatileComposites)); + store.register_late_pass(|_| Box::new(duplicate_match_guards::DuplicateMatchGuards)); // add lints here, do not remove this comment, it's used in `new_lint` } diff --git a/tests/ui/duplicate_match_guards.fixed b/tests/ui/duplicate_match_guards.fixed new file mode 100644 index 000000000000..29851973fa5a --- /dev/null +++ b/tests/ui/duplicate_match_guards.fixed @@ -0,0 +1,190 @@ +#![allow(clippy::needless_return, clippy::needless_else)] +#![warn(clippy::duplicate_match_guards)] + +fn main() { + let mut a = 5; + let b = 4; + let mut v: Vec = vec![]; + + // Lint + + match 0u32 { + 0 if true => { + return; + }, + //~^^^^ duplicate_match_guards + 0 if a > b => { + return; + }, + //~^^^^ duplicate_match_guards + + // Not _identical_, but the meaning is the same + 0 if a > b => { + return; + }, + //~^^^^ duplicate_match_guards + + // A bit more complicated + 0 if a > 0 && b > 0 => { + return; + }, + //~^^^^ duplicate_match_guards + + // Comments inside the inner block are preserved + // (for comments _outside_ the inner block, see below) + #[rustfmt::skip] + 0 if a > b => { + // before + return; + // after + }, + //~^^^^^^ duplicate_match_guards + #[rustfmt::skip] + 0 if a > b => { + // before, on the line of the second `if` + return; + // after + }, + //~^^^^^ duplicate_match_guards + #[rustfmt::skip] + 0 if a > b => { + /* before, + continued */ + return; + // after + }, + //~^^^^^^ duplicate_match_guards + #[rustfmt::skip] + 0 if a > b => { + // before + return; + /* after, on the line of the second `if` */ + }, + //~^^^^^ duplicate_match_guards + #[rustfmt::skip] + 0 if a > b => { + // before, on the line of the second `if` + return; + /* after, on the line of the second `if` */ + }, + //~^^^^ duplicate_match_guards + #[rustfmt::skip] + 0 if a > b => { + /* before, + continued */ + return; + /* after, on the line of the second `if` */ + }, + //~^^^^^ duplicate_match_guards + #[rustfmt::skip] + 0 if a > b => { + // before + return; + /* after, + continued */ + }, + //~^^^^^^ duplicate_match_guards + #[rustfmt::skip] + 0 if a > b => { + // before, on the line of the second `if` + return; + /* after, + continued*/ + }, + //~^^^^^ duplicate_match_guards + #[rustfmt::skip] + 0 if a > b => { + /* before, + continued */ + return; + /* after, + continued*/ + }, + //~^^^^^^ duplicate_match_guards + + // No curlies around arm body + #[rustfmt::skip] // would add the outer curlies + 0 if true => { + return; + }, + //~^^^ duplicate_match_guards + _ => unimplemented!(), + } + + // Don't lint + + match 0 { + 0 if true => { + if false { + return; + } + }, + // Has side effects + 0 if v.pop().is_some() => { + if v.pop().is_some() { + return; + } + }, + // There's _something_ in the `else` branch + 0 if a > b => { + if a > b { + a += b; + } else { + } + }, + // Body has statements before the `if` + 0 if a > b => { + a += b; + if a > b { + return; + } + }, + // Comments (putting them right next to the tested element to check for off-by-one errors): + // - before arrow + #[rustfmt::skip] + 0 if a > b/* comment */=> { + if a > b { + return; + } + }, + // - before outer opening curly + #[rustfmt::skip] + 0 if a > b =>/* comment */{ + if a > b { + return; + } + }, + // - before `if` + #[rustfmt::skip] + 0 if a > b => {/* + comment + */if a > b { + return; + } + }, + // - before condition + #[rustfmt::skip] + 0 if a > b => { + if/* + comment */a > b { + return; + } + }, + #[rustfmt::skip] + // - before inner opening curly + 0 if a > b => { + if a > b/* + comment */{ + return; + } + }, + #[rustfmt::skip] + // - before closing outer curly + 0 if a > b => { + if a > b { + return; + }/* comment + */}, + _ => unimplemented!(), + }; +} diff --git a/tests/ui/duplicate_match_guards.rs b/tests/ui/duplicate_match_guards.rs new file mode 100644 index 000000000000..5c945ac34716 --- /dev/null +++ b/tests/ui/duplicate_match_guards.rs @@ -0,0 +1,204 @@ +#![allow(clippy::needless_return, clippy::needless_else)] +#![warn(clippy::duplicate_match_guards)] + +fn main() { + let mut a = 5; + let b = 4; + let mut v: Vec = vec![]; + + // Lint + + match 0u32 { + 0 if true => { + if true { + return; + } + }, + //~^^^^ duplicate_match_guards + 0 if a > b => { + if a > b { + return; + } + }, + //~^^^^ duplicate_match_guards + + // Not _identical_, but the meaning is the same + 0 if a > b => { + if b < a { + return; + } + }, + //~^^^^ duplicate_match_guards + + // A bit more complicated + 0 if a > 0 && b > 0 => { + if a > 0 && b > 0 { + return; + } + }, + //~^^^^ duplicate_match_guards + + // Comments inside the inner block are preserved + // (for comments _outside_ the inner block, see below) + #[rustfmt::skip] + 0 if a > b => { + if a > b { + // before + return; + // after + } + }, + //~^^^^^^ duplicate_match_guards + #[rustfmt::skip] + 0 if a > b => { + if a > b {// before, on the line of the second `if` + return; + // after + } + }, + //~^^^^^ duplicate_match_guards + #[rustfmt::skip] + 0 if a > b => { + if a > b {/* before, + continued */ + return; + // after + } + }, + //~^^^^^^ duplicate_match_guards + #[rustfmt::skip] + 0 if a > b => { + if a > b { + // before + return; + /* after, on the line of the second `if` */} + }, + //~^^^^^ duplicate_match_guards + #[rustfmt::skip] + 0 if a > b => { + if a > b {// before, on the line of the second `if` + return; + /* after, on the line of the second `if` */} + }, + //~^^^^ duplicate_match_guards + #[rustfmt::skip] + 0 if a > b => { + if a > b {/* before, + continued */ + return; + /* after, on the line of the second `if` */} + }, + //~^^^^^ duplicate_match_guards + #[rustfmt::skip] + 0 if a > b => { + if a > b { + // before + return; + /* after, + continued */} + }, + //~^^^^^^ duplicate_match_guards + #[rustfmt::skip] + 0 if a > b => { + if a > b {// before, on the line of the second `if` + return; + /* after, + continued*/} + }, + //~^^^^^ duplicate_match_guards + #[rustfmt::skip] + 0 if a > b => { + if a > b {/* before, + continued */ + return; + /* after, + continued*/} + }, + //~^^^^^^ duplicate_match_guards + + // No curlies around arm body + #[rustfmt::skip] // would add the outer curlies + 0 if true => if true { + return; + }, + //~^^^ duplicate_match_guards + _ => unimplemented!(), + } + + // Don't lint + + match 0 { + 0 if true => { + if false { + return; + } + }, + // Has side effects + 0 if v.pop().is_some() => { + if v.pop().is_some() { + return; + } + }, + // There's _something_ in the `else` branch + 0 if a > b => { + if a > b { + a += b; + } else { + } + }, + // Body has statements before the `if` + 0 if a > b => { + a += b; + if a > b { + return; + } + }, + // Comments (putting them right next to the tested element to check for off-by-one errors): + // - before arrow + #[rustfmt::skip] + 0 if a > b/* comment */=> { + if a > b { + return; + } + }, + // - before outer opening curly + #[rustfmt::skip] + 0 if a > b =>/* comment */{ + if a > b { + return; + } + }, + // - before `if` + #[rustfmt::skip] + 0 if a > b => {/* + comment + */if a > b { + return; + } + }, + // - before condition + #[rustfmt::skip] + 0 if a > b => { + if/* + comment */a > b { + return; + } + }, + #[rustfmt::skip] + // - before inner opening curly + 0 if a > b => { + if a > b/* + comment */{ + return; + } + }, + #[rustfmt::skip] + // - before closing outer curly + 0 if a > b => { + if a > b { + return; + }/* comment + */}, + _ => unimplemented!(), + }; +} diff --git a/tests/ui/duplicate_match_guards.stderr b/tests/ui/duplicate_match_guards.stderr new file mode 100644 index 000000000000..36ffa1762c16 --- /dev/null +++ b/tests/ui/duplicate_match_guards.stderr @@ -0,0 +1,205 @@ +error: condition duplicates match guard + --> tests/ui/duplicate_match_guards.rs:13:13 + | +LL | / if true { +LL | | return; +LL | | } + | |_____________^ help: remove the condition: `return;` + | + = note: `-D clippy::duplicate-match-guards` implied by `-D warnings` + = help: to override `-D warnings` add `#[allow(clippy::duplicate_match_guards)]` + +error: condition duplicates match guard + --> tests/ui/duplicate_match_guards.rs:19:13 + | +LL | / if a > b { +LL | | return; +LL | | } + | |_____________^ help: remove the condition: `return;` + +error: condition duplicates match guard + --> tests/ui/duplicate_match_guards.rs:27:13 + | +LL | / if b < a { +LL | | return; +LL | | } + | |_____________^ help: remove the condition: `return;` + +error: condition duplicates match guard + --> tests/ui/duplicate_match_guards.rs:35:13 + | +LL | / if a > 0 && b > 0 { +LL | | return; +LL | | } + | |_____________^ help: remove the condition: `return;` + +error: condition duplicates match guard + --> tests/ui/duplicate_match_guards.rs:45:13 + | +LL | / if a > b { +LL | | // before +LL | | return; +LL | | // after +LL | | } + | |_____________^ + | +help: remove the condition + | +LL ~ // before +LL + return; +LL + // after + | + +error: condition duplicates match guard + --> tests/ui/duplicate_match_guards.rs:54:13 + | +LL | / if a > b {// before, on the line of the second `if` +LL | | return; +LL | | // after +LL | | } + | |_____________^ + | +help: remove the condition + | +LL ~ // before, on the line of the second `if` +LL + return; +LL + // after + | + +error: condition duplicates match guard + --> tests/ui/duplicate_match_guards.rs:62:13 + | +LL | / if a > b {/* before, +LL | | continued */ +LL | | return; +LL | | // after +LL | | } + | |_____________^ + | +help: remove the condition + | +LL ~ /* before, +LL + continued */ +LL + return; +LL + // after + | + +error: condition duplicates match guard + --> tests/ui/duplicate_match_guards.rs:71:13 + | +LL | / if a > b { +LL | | // before +LL | | return; +LL | | /* after, on the line of the second `if` */} + | |________________________________________________________^ + | +help: remove the condition + | +LL ~ // before +LL + return; +LL + /* after, on the line of the second `if` */ + | + +error: condition duplicates match guard + --> tests/ui/duplicate_match_guards.rs:79:13 + | +LL | / if a > b {// before, on the line of the second `if` +LL | | return; +LL | | /* after, on the line of the second `if` */} + | |________________________________________________________^ + | +help: remove the condition + | +LL ~ // before, on the line of the second `if` +LL + return; +LL + /* after, on the line of the second `if` */ + | + +error: condition duplicates match guard + --> tests/ui/duplicate_match_guards.rs:86:13 + | +LL | / if a > b {/* before, +LL | | continued */ +LL | | return; +LL | | /* after, on the line of the second `if` */} + | |________________________________________________________^ + | +help: remove the condition + | +LL ~ /* before, +LL + continued */ +LL + return; +LL + /* after, on the line of the second `if` */ + | + +error: condition duplicates match guard + --> tests/ui/duplicate_match_guards.rs:94:13 + | +LL | / if a > b { +LL | | // before +LL | | return; +LL | | /* after, +LL | | continued */} + | |_________________________^ + | +help: remove the condition + | +LL ~ // before +LL + return; +LL + /* after, +LL + continued */ + | + +error: condition duplicates match guard + --> tests/ui/duplicate_match_guards.rs:103:13 + | +LL | / if a > b {// before, on the line of the second `if` +LL | | return; +LL | | /* after, +LL | | continued*/} + | |________________________^ + | +help: remove the condition + | +LL ~ // before, on the line of the second `if` +LL + return; +LL + /* after, +LL + continued*/ + | + +error: condition duplicates match guard + --> tests/ui/duplicate_match_guards.rs:111:13 + | +LL | / if a > b {/* before, +LL | | continued */ +LL | | return; +LL | | /* after, +LL | | continued*/} + | |________________________^ + | +help: remove the condition + | +LL ~ /* before, +LL + continued */ +LL + return; +LL + /* after, +LL + continued*/ + | + +error: condition duplicates match guard + --> tests/ui/duplicate_match_guards.rs:121:22 + | +LL | 0 if true => if true { + | ______________________^ +LL | | return; +LL | | }, + | |_________^ + | +help: remove the condition + | +LL - 0 if true => if true { +LL + 0 if true => { + | + +error: aborting due to 14 previous errors +