From 4637f05adb4b0118c6e4db2947e7d2eac22d217f Mon Sep 17 00:00:00 2001 From: xFrednet Date: Wed, 16 Dec 2020 12:17:07 +0000 Subject: [PATCH] A new lint for shared code in if blocks * WIP working front and and detection * A working lint * Fixed the last test * Cleaning up some code * Fixed another test * Ret renamed the lint * Formatting --- CHANGELOG.md | 1 + clippy_lints/src/copies.rs | 160 +++++++++++++++--- clippy_lints/src/lib.rs | 3 + clippy_lints/src/utils/hir_utils.rs | 9 + clippy_lints/src/utils/mod.rs | 2 +- .../ui/checked_unwrap/complex_conditionals.rs | 2 +- tests/ui/if_same_then_else.rs | 3 +- tests/ui/if_same_then_else.stderr | 88 +++++----- tests/ui/if_same_then_else2.rs | 3 +- tests/ui/if_same_then_else2.stderr | 104 ++++++------ tests/ui/let_if_seq.rs | 3 +- tests/ui/let_if_seq.stderr | 8 +- tests/ui/shared_code_in_if_blocks.rs | 114 ++++++------- tests/ui/shared_code_in_if_blocks.stderr | 126 ++++++++++++++ 14 files changed, 436 insertions(+), 190 deletions(-) create mode 100644 tests/ui/shared_code_in_if_blocks.stderr diff --git a/CHANGELOG.md b/CHANGELOG.md index af3b1c1db2ae..ae4f38d56e42 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2049,6 +2049,7 @@ Released 2018-09-13 [`shadow_reuse`]: https://rust-lang.github.io/rust-clippy/master/index.html#shadow_reuse [`shadow_same`]: https://rust-lang.github.io/rust-clippy/master/index.html#shadow_same [`shadow_unrelated`]: https://rust-lang.github.io/rust-clippy/master/index.html#shadow_unrelated +[`shared_code_in_if_blocks`]: https://rust-lang.github.io/rust-clippy/master/index.html#shared_code_in_if_blocks [`short_circuit_statement`]: https://rust-lang.github.io/rust-clippy/master/index.html#short_circuit_statement [`should_assert_eq`]: https://rust-lang.github.io/rust-clippy/master/index.html#should_assert_eq [`should_implement_trait`]: https://rust-lang.github.io/rust-clippy/master/index.html#should_implement_trait diff --git a/clippy_lints/src/copies.rs b/clippy_lints/src/copies.rs index 46ce92ea6d78..0dbb7f5925bc 100644 --- a/clippy_lints/src/copies.rs +++ b/clippy_lints/src/copies.rs @@ -1,8 +1,9 @@ -use crate::utils::{eq_expr_value, in_macro, search_same, SpanlessEq, SpanlessHash}; -use crate::utils::{get_parent_expr, higher, if_sequence, span_lint_and_note}; +use crate::utils::{both, count_eq, eq_expr_value, in_macro, search_same, SpanlessEq, SpanlessHash}; +use crate::utils::{get_parent_expr, higher, if_sequence, span_lint_and_help, span_lint_and_note}; use rustc_hir::{Block, Expr}; use rustc_lint::{LateContext, LateLintPass}; use rustc_session::{declare_lint_pass, declare_tool_lint}; +use rustc_span::source_map::Span; declare_clippy_lint! { /// **What it does:** Checks for consecutive `if`s with the same condition. @@ -103,7 +104,40 @@ declare_clippy_lint! { "`if` with the same `then` and `else` blocks" } -declare_lint_pass!(CopyAndPaste => [IFS_SAME_COND, SAME_FUNCTIONS_IN_IF_CONDITION, IF_SAME_THEN_ELSE]); +declare_clippy_lint! { + /// **What it does:** Checks if the `if` and `else` block contain shared that can be + /// moved out of the blocks. + /// + /// **Why is this bad?** Duplicate code is less maintainable. + /// + /// **Known problems:** Hopefully none. + /// + /// **Example:** + /// ```ignore + /// let foo = if … { + /// println!("Hello World"); + /// 13 + /// } else { + /// println!("Hello World"); + /// 42 + /// }; + /// ``` + /// + /// Could be written as: + /// ```ignore + /// println!("Hello World"); + /// let foo = if … { + /// 13 + /// } else { + /// 42 + /// }; + /// ``` + pub SHARED_CODE_IN_IF_BLOCKS, + complexity, + "`if` statement with shared code in all blocks" +} + +declare_lint_pass!(CopyAndPaste => [IFS_SAME_COND, SAME_FUNCTIONS_IN_IF_CONDITION, IF_SAME_THEN_ELSE, SHARED_CODE_IN_IF_BLOCKS]); impl<'tcx> LateLintPass<'tcx> for CopyAndPaste { fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) { @@ -118,28 +152,112 @@ impl<'tcx> LateLintPass<'tcx> for CopyAndPaste { } let (conds, blocks) = if_sequence(expr); - lint_same_then_else(cx, &blocks); + // Conditions lint_same_cond(cx, &conds); lint_same_fns_in_if_cond(cx, &conds); + // Block duplication + lint_same_then_else(cx, &blocks, conds.len() != blocks.len()); } } } -/// Implementation of `IF_SAME_THEN_ELSE`. -fn lint_same_then_else(cx: &LateContext<'_>, blocks: &[&Block<'_>]) { - let eq: &dyn Fn(&&Block<'_>, &&Block<'_>) -> bool = - &|&lhs, &rhs| -> bool { SpanlessEq::new(cx).eq_block(lhs, rhs) }; +/// Implementation of `SHARED_CODE_IN_IF_BLOCKS` and `IF_SAME_THEN_ELSE` if the blocks are equal. +fn lint_same_then_else(cx: &LateContext<'_>, blocks: &[&Block<'_>], has_unconditional_else: bool) { + /// This retrieves the span of the actual call site. + fn get_source_span(span: Span) -> Span { + if span.from_expansion() { + span.source_callee().unwrap().call_site + } else { + span + } + } - if let Some((i, j)) = search_same_sequenced(blocks, eq) { - span_lint_and_note( + fn min(a: usize, b: usize) -> usize { + if a < b { + a + } else { + b + } + } + + fn lint_duplicate_code(cx: &LateContext<'_>, position: &str, lint_span: Span) { + span_lint_and_help( cx, - IF_SAME_THEN_ELSE, - j.span, - "this `if` has identical blocks", - Some(i.span), - "same as this", + SHARED_CODE_IN_IF_BLOCKS, + lint_span, + format!("All if blocks contain the same code at the {}", position).as_str(), + None, + "Consider moving the code out of the if statement to prevent code duplication", ); } + + // We only lint ifs with multiple blocks + if blocks.len() < 2 { + return; + } + + // Check if each block has shared code + let mut start_eq = usize::MAX; + let mut end_eq = usize::MAX; + for (index, win) in blocks.windows(2).enumerate() { + let l_stmts = win[0].stmts; + let r_stmts = win[1].stmts; + + let mut evaluator = SpanlessEq::new(cx); + let current_start_eq = count_eq(&mut l_stmts.iter(), &mut r_stmts.iter(), |l, r| evaluator.eq_stmt(l, r)); + let current_end_eq = count_eq(&mut l_stmts.iter().rev(), &mut r_stmts.iter().rev(), |l, r| { + evaluator.eq_stmt(l, r) + }); + let current_block_expr_eq = both(&win[0].expr, &win[1].expr, |l, r| evaluator.eq_expr(l, r)); + + // IF_SAME_THEN_ELSE + // We only lint the first two blocks (index == 0). Further blocks will be linted when that if + // statement is checked + if index == 0 && current_block_expr_eq && l_stmts.len() == r_stmts.len() && l_stmts.len() == current_start_eq { + span_lint_and_note( + cx, + IF_SAME_THEN_ELSE, + win[0].span, + "this `if` has identical blocks", + Some(win[1].span), + "same as this", + ); + + return; + } + + start_eq = min(start_eq, current_start_eq); + end_eq = min(end_eq, current_end_eq); + + // We can return if the eq count is 0 from both sides or if it has no unconditional else case + if !has_unconditional_else || start_eq == 0 && end_eq == 0 { + return; + }; + } + + let first_block = blocks[0]; + + // prevent double lint if the `start_eq` and `end_eq` cover the entire block + if start_eq == first_block.stmts.len() { + end_eq = 0; + } + + if start_eq != 0 { + let start = first_block.span.shrink_to_lo(); + let end = get_source_span(first_block.stmts[start_eq - 1].span); + let lint_span = start.to(end); + + lint_duplicate_code(cx, "start", lint_span); + } + + if end_eq != 0 { + let index = first_block.stmts.len() - end_eq; + let start = get_source_span(first_block.stmts[index].span); + let end = first_block.span.shrink_to_hi(); + let lint_span = start.to(end); + + lint_duplicate_code(cx, "end", lint_span); + } } /// Implementation of `IFS_SAME_COND`. @@ -195,15 +313,3 @@ fn lint_same_fns_in_if_cond(cx: &LateContext<'_>, conds: &[&Expr<'_>]) { ); } } - -fn search_same_sequenced(exprs: &[T], eq: Eq) -> Option<(&T, &T)> -where - Eq: Fn(&T, &T) -> bool, -{ - for win in exprs.windows(2) { - if eq(&win[0], &win[1]) { - return Some((&win[0], &win[1])); - } - } - None -} diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index f06926fa91d3..b3c9a00cd952 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -558,6 +558,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: &copies::IFS_SAME_COND, &copies::IF_SAME_THEN_ELSE, &copies::SAME_FUNCTIONS_IN_IF_CONDITION, + &copies::SHARED_CODE_IN_IF_BLOCKS, ©_iterator::COPY_ITERATOR, &create_dir::CREATE_DIR, &dbg_macro::DBG_MACRO, @@ -1381,6 +1382,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: LintId::of(&comparison_chain::COMPARISON_CHAIN), LintId::of(&copies::IFS_SAME_COND), LintId::of(&copies::IF_SAME_THEN_ELSE), + LintId::of(&copies::SHARED_CODE_IN_IF_BLOCKS), LintId::of(&default::FIELD_REASSIGN_WITH_DEFAULT), LintId::of(&derive::DERIVE_HASH_XOR_EQ), LintId::of(&derive::DERIVE_ORD_XOR_PARTIAL_ORD), @@ -1751,6 +1753,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: LintId::of(&assign_ops::MISREFACTORED_ASSIGN_OP), LintId::of(&attrs::DEPRECATED_CFG_ATTR), LintId::of(&booleans::NONMINIMAL_BOOL), + LintId::of(&copies::SHARED_CODE_IN_IF_BLOCKS), LintId::of(&double_comparison::DOUBLE_COMPARISONS), LintId::of(&double_parens::DOUBLE_PARENS), LintId::of(&duration_subsec::DURATION_SUBSEC), diff --git a/clippy_lints/src/utils/hir_utils.rs b/clippy_lints/src/utils/hir_utils.rs index d942d4e12b10..142f59973fdc 100644 --- a/clippy_lints/src/utils/hir_utils.rs +++ b/clippy_lints/src/utils/hir_utils.rs @@ -368,6 +368,15 @@ pub fn over(left: &[X], right: &[X], mut eq_fn: impl FnMut(&X, &X) -> bool) - left.len() == right.len() && left.iter().zip(right).all(|(x, y)| eq_fn(x, y)) } +/// Counts how many elements of the slices are equal as per `eq_fn`. +pub fn count_eq( + left: &mut dyn Iterator, + right: &mut dyn Iterator, + mut eq_fn: impl FnMut(&X, &X) -> bool, +) -> usize { + left.zip(right).take_while(|(l, r)| eq_fn(l, r)).count() +} + /// Checks if two expressions evaluate to the same value, and don't contain any side effects. pub fn eq_expr_value(cx: &LateContext<'_>, left: &Expr<'_>, right: &Expr<'_>) -> bool { SpanlessEq::new(cx).deny_side_effects().eq_expr(left, right) diff --git a/clippy_lints/src/utils/mod.rs b/clippy_lints/src/utils/mod.rs index e83371f8b99a..e963be520dd6 100644 --- a/clippy_lints/src/utils/mod.rs +++ b/clippy_lints/src/utils/mod.rs @@ -26,7 +26,7 @@ pub mod visitors; pub use self::attrs::*; pub use self::diagnostics::*; -pub use self::hir_utils::{both, eq_expr_value, over, SpanlessEq, SpanlessHash}; +pub use self::hir_utils::{both, count_eq, eq_expr_value, over, SpanlessEq, SpanlessHash}; use std::borrow::Cow; use std::collections::hash_map::Entry; diff --git a/tests/ui/checked_unwrap/complex_conditionals.rs b/tests/ui/checked_unwrap/complex_conditionals.rs index c986c992a07a..bb5b6f5ec043 100644 --- a/tests/ui/checked_unwrap/complex_conditionals.rs +++ b/tests/ui/checked_unwrap/complex_conditionals.rs @@ -1,5 +1,5 @@ #![deny(clippy::panicking_unwrap, clippy::unnecessary_unwrap)] -#![allow(clippy::if_same_then_else)] +#![allow(clippy::if_same_then_else, clippy::shared_code_in_if_blocks)] fn test_complex_conditions() { let x: Result<(), ()> = Ok(()); diff --git a/tests/ui/if_same_then_else.rs b/tests/ui/if_same_then_else.rs index 9c5fe02f7519..35a2e139c11d 100644 --- a/tests/ui/if_same_then_else.rs +++ b/tests/ui/if_same_then_else.rs @@ -5,7 +5,8 @@ clippy::never_loop, clippy::no_effect, clippy::unused_unit, - clippy::zero_divided_by_zero + clippy::zero_divided_by_zero, + clippy::shared_code_in_if_blocks )] struct Foo { diff --git a/tests/ui/if_same_then_else.stderr b/tests/ui/if_same_then_else.stderr index d9fdf06fa8b7..2f38052fc209 100644 --- a/tests/ui/if_same_then_else.stderr +++ b/tests/ui/if_same_then_else.stderr @@ -1,111 +1,111 @@ error: this `if` has identical blocks - --> $DIR/if_same_then_else.rs:28:12 + --> $DIR/if_same_then_else.rs:21:13 | -LL | } else { - | ____________^ -LL | | //~ ERROR same body as `if` block +LL | if true { + | _____________^ LL | | Foo { bar: 42 }; LL | | 0..10; +LL | | ..; ... | LL | | foo(); -LL | | } +LL | | } else { | |_____^ | = note: `-D clippy::if-same-then-else` implied by `-D warnings` note: same as this - --> $DIR/if_same_then_else.rs:20:13 + --> $DIR/if_same_then_else.rs:29:12 | -LL | if true { - | _____________^ +LL | } else { + | ____________^ +LL | | //~ ERROR same body as `if` block LL | | Foo { bar: 42 }; LL | | 0..10; -LL | | ..; ... | LL | | foo(); -LL | | } else { +LL | | } | |_____^ error: this `if` has identical blocks - --> $DIR/if_same_then_else.rs:66:12 - | -LL | } else { - | ____________^ -LL | | //~ ERROR same body as `if` block -LL | | 0.0 -LL | | }; - | |_____^ - | -note: same as this - --> $DIR/if_same_then_else.rs:64:21 + --> $DIR/if_same_then_else.rs:65:21 | LL | let _ = if true { | _____________________^ LL | | 0.0 LL | | } else { | |_____^ - -error: this `if` has identical blocks - --> $DIR/if_same_then_else.rs:73:12 + | +note: same as this + --> $DIR/if_same_then_else.rs:67:12 | LL | } else { | ____________^ LL | | //~ ERROR same body as `if` block -LL | | -0.0 +LL | | 0.0 LL | | }; | |_____^ - | -note: same as this - --> $DIR/if_same_then_else.rs:71:21 + +error: this `if` has identical blocks + --> $DIR/if_same_then_else.rs:72:21 | LL | let _ = if true { | _____________________^ LL | | -0.0 LL | | } else { | |_____^ - -error: this `if` has identical blocks - --> $DIR/if_same_then_else.rs:89:12 + | +note: same as this + --> $DIR/if_same_then_else.rs:74:12 | LL | } else { | ____________^ LL | | //~ ERROR same body as `if` block -LL | | 42 +LL | | -0.0 LL | | }; | |_____^ - | -note: same as this - --> $DIR/if_same_then_else.rs:87:21 + +error: this `if` has identical blocks + --> $DIR/if_same_then_else.rs:88:21 | LL | let _ = if true { | _____________________^ LL | | 42 LL | | } else { | |_____^ - -error: this `if` has identical blocks - --> $DIR/if_same_then_else.rs:101:12 + | +note: same as this + --> $DIR/if_same_then_else.rs:90:12 | LL | } else { | ____________^ LL | | //~ ERROR same body as `if` block +LL | | 42 +LL | | }; + | |_____^ + +error: this `if` has identical blocks + --> $DIR/if_same_then_else.rs:95:13 + | +LL | if true { + | _____________^ LL | | let bar = if true { 42 } else { 43 }; LL | | +LL | | while foo() { ... | LL | | bar + 1; -LL | | } +LL | | } else { | |_____^ | note: same as this - --> $DIR/if_same_then_else.rs:94:13 + --> $DIR/if_same_then_else.rs:102:12 | -LL | if true { - | _____________^ +LL | } else { + | ____________^ +LL | | //~ ERROR same body as `if` block LL | | let bar = if true { 42 } else { 43 }; LL | | -LL | | while foo() { ... | LL | | bar + 1; -LL | | } else { +LL | | } | |_____^ error: aborting due to 5 previous errors diff --git a/tests/ui/if_same_then_else2.rs b/tests/ui/if_same_then_else2.rs index 8d54f75b5d19..1a933878c74a 100644 --- a/tests/ui/if_same_then_else2.rs +++ b/tests/ui/if_same_then_else2.rs @@ -4,7 +4,8 @@ clippy::collapsible_if, clippy::ifs_same_cond, clippy::needless_return, - clippy::single_element_loop + clippy::single_element_loop, + clippy::shared_code_in_if_blocks )] fn if_same_then_else2() -> Result<&'static str, ()> { diff --git a/tests/ui/if_same_then_else2.stderr b/tests/ui/if_same_then_else2.stderr index da2be6c8aa5a..3eadaf62d139 100644 --- a/tests/ui/if_same_then_else2.stderr +++ b/tests/ui/if_same_then_else2.stderr @@ -1,118 +1,108 @@ error: this `if` has identical blocks - --> $DIR/if_same_then_else2.rs:20:12 + --> $DIR/if_same_then_else2.rs:12:13 | -LL | } else { - | ____________^ -LL | | //~ ERROR same body as `if` block +LL | if true { + | _____________^ LL | | for _ in &[42] { LL | | let foo: &Option<_> = &Some::(42); +LL | | if true { ... | LL | | } -LL | | } +LL | | } else { | |_____^ | = note: `-D clippy::if-same-then-else` implied by `-D warnings` note: same as this - --> $DIR/if_same_then_else2.rs:11:13 + --> $DIR/if_same_then_else2.rs:21:12 | -LL | if true { - | _____________^ +LL | } else { + | ____________^ +LL | | //~ ERROR same body as `if` block LL | | for _ in &[42] { LL | | let foo: &Option<_> = &Some::(42); -LL | | if true { ... | LL | | } -LL | | } else { +LL | | } | |_____^ error: this `if` has identical blocks - --> $DIR/if_same_then_else2.rs:34:12 - | -LL | } else { - | ____________^ -LL | | //~ ERROR same body as `if` block -LL | | if let Some(a) = Some(42) {} -LL | | } - | |_____^ - | -note: same as this - --> $DIR/if_same_then_else2.rs:32:13 + --> $DIR/if_same_then_else2.rs:33:13 | LL | if true { | _____________^ LL | | if let Some(a) = Some(42) {} LL | | } else { | |_____^ - -error: this `if` has identical blocks - --> $DIR/if_same_then_else2.rs:41:12 + | +note: same as this + --> $DIR/if_same_then_else2.rs:35:12 | LL | } else { | ____________^ LL | | //~ ERROR same body as `if` block -LL | | if let (1, .., 3) = (1, 2, 3) {} +LL | | if let Some(a) = Some(42) {} LL | | } | |_____^ - | -note: same as this - --> $DIR/if_same_then_else2.rs:39:13 + +error: this `if` has identical blocks + --> $DIR/if_same_then_else2.rs:40:13 | LL | if true { | _____________^ LL | | if let (1, .., 3) = (1, 2, 3) {} LL | | } else { | |_____^ - -error: this `if` has identical blocks - --> $DIR/if_same_then_else2.rs:91:12 + | +note: same as this + --> $DIR/if_same_then_else2.rs:42:12 | LL | } else { | ____________^ LL | | //~ ERROR same body as `if` block -LL | | f32::NAN -LL | | }; +LL | | if let (1, .., 3) = (1, 2, 3) {} +LL | | } | |_____^ - | -note: same as this - --> $DIR/if_same_then_else2.rs:89:21 + +error: this `if` has identical blocks + --> $DIR/if_same_then_else2.rs:90:21 | LL | let _ = if true { | _____________________^ LL | | f32::NAN LL | | } else { | |_____^ - -error: this `if` has identical blocks - --> $DIR/if_same_then_else2.rs:98:12 + | +note: same as this + --> $DIR/if_same_then_else2.rs:92:12 | LL | } else { | ____________^ LL | | //~ ERROR same body as `if` block -LL | | Ok("foo")?; -LL | | } +LL | | f32::NAN +LL | | }; | |_____^ - | -note: same as this - --> $DIR/if_same_then_else2.rs:96:13 + +error: this `if` has identical blocks + --> $DIR/if_same_then_else2.rs:97:13 | LL | if true { | _____________^ LL | | Ok("foo")?; LL | | } else { | |_____^ - -error: this `if` has identical blocks - --> $DIR/if_same_then_else2.rs:123:12 + | +note: same as this + --> $DIR/if_same_then_else2.rs:99:12 | LL | } else { | ____________^ -LL | | let foo = ""; -LL | | return Ok(&foo[0..]); +LL | | //~ ERROR same body as `if` block +LL | | Ok("foo")?; LL | | } | |_____^ - | -note: same as this - --> $DIR/if_same_then_else2.rs:120:20 + +error: this `if` has identical blocks + --> $DIR/if_same_then_else2.rs:121:20 | LL | } else if true { | ____________________^ @@ -120,6 +110,16 @@ LL | | let foo = ""; LL | | return Ok(&foo[0..]); LL | | } else { | |_____^ + | +note: same as this + --> $DIR/if_same_then_else2.rs:124:12 + | +LL | } else { + | ____________^ +LL | | let foo = ""; +LL | | return Ok(&foo[0..]); +LL | | } + | |_____^ error: aborting due to 6 previous errors diff --git a/tests/ui/let_if_seq.rs b/tests/ui/let_if_seq.rs index 32a67f181df4..9fd3f875a5f1 100644 --- a/tests/ui/let_if_seq.rs +++ b/tests/ui/let_if_seq.rs @@ -2,7 +2,8 @@ unused_variables, unused_assignments, clippy::similar_names, - clippy::blacklisted_name + clippy::blacklisted_name, + clippy::shared_code_in_if_blocks )] #![warn(clippy::useless_let_if_seq)] diff --git a/tests/ui/let_if_seq.stderr b/tests/ui/let_if_seq.stderr index 7de560c73486..9cf2e10a5ee5 100644 --- a/tests/ui/let_if_seq.stderr +++ b/tests/ui/let_if_seq.stderr @@ -1,5 +1,5 @@ error: `if _ { .. } else { .. }` is an expression - --> $DIR/let_if_seq.rs:64:5 + --> $DIR/let_if_seq.rs:65:5 | LL | / let mut foo = 0; LL | | if f() { @@ -11,7 +11,7 @@ LL | | } = note: you might not need `mut` at all error: `if _ { .. } else { .. }` is an expression - --> $DIR/let_if_seq.rs:69:5 + --> $DIR/let_if_seq.rs:70:5 | LL | / let mut bar = 0; LL | | if f() { @@ -25,7 +25,7 @@ LL | | } = note: you might not need `mut` at all error: `if _ { .. } else { .. }` is an expression - --> $DIR/let_if_seq.rs:77:5 + --> $DIR/let_if_seq.rs:78:5 | LL | / let quz; LL | | if f() { @@ -36,7 +36,7 @@ LL | | } | |_____^ help: it is more idiomatic to write: `let quz = if f() { 42 } else { 0 };` error: `if _ { .. } else { .. }` is an expression - --> $DIR/let_if_seq.rs:106:5 + --> $DIR/let_if_seq.rs:107:5 | LL | / let mut baz = 0; LL | | if f() { diff --git a/tests/ui/shared_code_in_if_blocks.rs b/tests/ui/shared_code_in_if_blocks.rs index 41525f16e2da..0019294b5f61 100644 --- a/tests/ui/shared_code_in_if_blocks.rs +++ b/tests/ui/shared_code_in_if_blocks.rs @@ -1,5 +1,5 @@ #![allow(dead_code)] -#![warn(clippy::if_same_then_else)] +#![deny(clippy::if_same_then_else, clippy::shared_code_in_if_blocks)] macro_rules! foo { () => { @@ -7,39 +7,10 @@ macro_rules! foo { }; } - -macro_rules! bar { - () => { - println!("I'm a macro, yay"); - }; -} - -/// This should be caught by the `if_same_then_else` lint and be ignored by the +/// This should be caught by the `if_same_then_else` lint and be ignored by the /// `shared_code_in_if_blocks` lint fn other_lint() { let x = 10; - if x == 0 { - // I'm the real start of arrays - println!("Hello World"); - } else { - // I'm an awesome number - // Hello @reviewer - println!("Hello World"); - } - - let _me = if x == 9 { - 42 - } else { - 42 - }; - - let _duck = if x == 8 { - 7 - } else if x == 5 { - 7 - } else { - 7 - }; // Only two blocks are the same let _ = if x == 9 { @@ -53,19 +24,13 @@ fn other_lint() { fn everything_is_okay() { let x = 10; - // This is currently a false positive - if x == 0 { - foo!(); - } else { - bar!(); - } // Single branch if x == 1 { println!("I'm an awesome clippy test") } - // Same start in two blocks + // Same start in only two blocks if x == 10 { println!("x is a value"); println!("x is 10"); @@ -77,12 +42,14 @@ fn everything_is_okay() { // Shared code in the middle let _ = if x == 7 { - let z = 3; + let mut z = 3; println!("x is a value"); + z *= 2; z } else if x == 100 { - let y = 1; + let mut y = 1; println!("x is a value"); + y += 9; y } else { let x = 2; @@ -90,24 +57,34 @@ fn everything_is_okay() { x }; - // Different macros and just a return should be simple - let _z = if x == 8 { - println!("Branch 1"); - let z = 10; - foo!(); - z + // Returns value with the same name + let _ = if x == 9 { + let x = 9; + println!("--"); + x } else { - println!("Branch 2"); - let z = 10; - bar!(); - z + let x = 18; + println!("^^"); + x }; -} + for index in 0..2 { + // Okay because we don't have an unconditional else case + if index == 6 { + println!("Six"); + continue; + } else if index == 9 { + println!("Nine"); + continue; + } + + println!("---"); + } +} fn shared_code_at_start() { let x = 17; - + // First statement is the same let _ = if x == 1 { // I'm a comment that shouldn't matter for the lint @@ -118,7 +95,7 @@ fn shared_code_at_start() { // I'm another comment, // Nice to see you here in the tests println!("Hello World"); - + 16 }; @@ -131,15 +108,24 @@ fn shared_code_at_start() { } else { println!("Hello World"); }; - + if x == 8 { println!("Hello World"); + println!("How are you today?"); + println!("From: Test"); + let _x = 10; let _ = true; } else if x != 2 { println!("Hello World"); + println!("How are you today?"); + println!("From: Test"); + let _x = 10; let _ = false; } else { println!("Hello World"); + println!("How are you today?"); + println!("From: Test"); + let _x = 10; } } @@ -160,15 +146,27 @@ fn shared_code_at_end() { let _z = if x == 8 { println!("Branch 1"); let z = 10; - bar!(); + foo!(); z } else { println!("Branch 2"); let z = 10; - bar!(); + foo!(); z }; -} -fn main () {} + // Lint at start and end + let _ = if x == 1 { + println!("I'm the same as my brother branch"); + let _a = 99; + println!("End of block"); + false + } else { + println!("I'm the same as my brother branch"); + let _b = 17; + println!("End of block"); + false + }; +} +fn main() {} diff --git a/tests/ui/shared_code_in_if_blocks.stderr b/tests/ui/shared_code_in_if_blocks.stderr new file mode 100644 index 000000000000..b7e13a5d6066 --- /dev/null +++ b/tests/ui/shared_code_in_if_blocks.stderr @@ -0,0 +1,126 @@ +error: this `if` has identical blocks + --> $DIR/shared_code_in_if_blocks.rs:18:22 + | +LL | } else if x == 7 { + | ______________________^ +LL | | 7 +LL | | } else { + | |_____^ + | +note: the lint level is defined here + --> $DIR/shared_code_in_if_blocks.rs:2:9 + | +LL | #![deny(clippy::if_same_then_else, clippy::shared_code_in_if_blocks)] + | ^^^^^^^^^^^^^^^^^^^^^^^^^ +note: same as this + --> $DIR/shared_code_in_if_blocks.rs:20:12 + | +LL | } else { + | ____________^ +LL | | 7 +LL | | }; + | |_____^ + +error: All if blocks contain the same code at the start + --> $DIR/shared_code_in_if_blocks.rs:89:23 + | +LL | let _ = if x == 1 { + | _______________________^ +LL | | // I'm a comment that shouldn't matter for the lint +LL | | println!("Hello World"); + | |________________________________^ + | +note: the lint level is defined here + --> $DIR/shared_code_in_if_blocks.rs:2:36 + | +LL | #![deny(clippy::if_same_then_else, clippy::shared_code_in_if_blocks)] + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + = help: Consider moving the code out of the if statement to prevent code duplication + +error: All if blocks contain the same code at the start + --> $DIR/shared_code_in_if_blocks.rs:103:16 + | +LL | if x == 19 { + | ________________^ +LL | | println!("Hello World"); + | |________________________________^ + | + = help: Consider moving the code out of the if statement to prevent code duplication + +error: All if blocks contain the same code at the start + --> $DIR/shared_code_in_if_blocks.rs:112:15 + | +LL | if x == 8 { + | _______________^ +LL | | println!("Hello World"); +LL | | println!("How are you today?"); +LL | | println!("From: Test"); +LL | | let _x = 10; + | |____________________^ + | + = help: Consider moving the code out of the if statement to prevent code duplication + +error: All if blocks contain the same code at the start + --> $DIR/shared_code_in_if_blocks.rs:118:22 + | +LL | } else if x != 2 { + | ______________________^ +LL | | println!("Hello World"); +LL | | println!("How are you today?"); +LL | | println!("From: Test"); +LL | | let _x = 10; + | |____________________^ + | + = help: Consider moving the code out of the if statement to prevent code duplication + +error: All if blocks contain the same code at the end + --> $DIR/shared_code_in_if_blocks.rs:138:9 + | +LL | / println!("Hello World"); +LL | | } else if x != 2 { + | |_____^ + | + = help: Consider moving the code out of the if statement to prevent code duplication + +error: All if blocks contain the same code at the end + --> $DIR/shared_code_in_if_blocks.rs:141:9 + | +LL | / println!("Hello World"); +LL | | } else { + | |_____^ + | + = help: Consider moving the code out of the if statement to prevent code duplication + +error: All if blocks contain the same code at the end + --> $DIR/shared_code_in_if_blocks.rs:148:9 + | +LL | / let z = 10; +LL | | foo!(); +LL | | z +LL | | } else { + | |_____^ + | + = help: Consider moving the code out of the if statement to prevent code duplication + +error: All if blocks contain the same code at the start + --> $DIR/shared_code_in_if_blocks.rs:159:23 + | +LL | let _ = if x == 1 { + | _______________________^ +LL | | println!("I'm the same as my brother branch"); + | |______________________________________________________^ + | + = help: Consider moving the code out of the if statement to prevent code duplication + +error: All if blocks contain the same code at the end + --> $DIR/shared_code_in_if_blocks.rs:162:9 + | +LL | / println!("End of block"); +LL | | false +LL | | } else { + | |_____^ + | + = help: Consider moving the code out of the if statement to prevent code duplication + +error: aborting due to 10 previous errors +