From ae82621a353096d715f2dd14680253abcb965144 Mon Sep 17 00:00:00 2001 From: Samuel Tardieu Date: Sun, 16 Feb 2025 16:05:02 +0100 Subject: [PATCH 1/2] Lint more cases in `collapsible_if` Replace the use of `Sugg::ast()` which prevented combining `if` together when they contained comments by span manipulation. A new configuration option `lint_commented_code`, which is `true` by default, opts out from this behavior. --- CHANGELOG.md | 1 + book/src/lint_configuration.md | 11 ++ clippy_config/src/conf.rs | 4 + clippy_lints/src/collapsible_if.rs | 186 +++++++++++------- clippy_lints/src/lib.rs | 2 +- tests/ui-toml/collapsible_if/clippy.toml | 1 + .../collapsible_if/collapsible_if.fixed | 18 ++ .../ui-toml/collapsible_if/collapsible_if.rs | 19 ++ .../collapsible_if/collapsible_if.stderr | 22 +++ .../toml_unknown_key/conf_unknown_key.stderr | 3 + tests/ui/collapsible_if.fixed | 116 +++++------ tests/ui/collapsible_if.rs | 69 ++++--- tests/ui/collapsible_if.stderr | 149 +++++++++++--- 13 files changed, 406 insertions(+), 195 deletions(-) create mode 100644 tests/ui-toml/collapsible_if/clippy.toml create mode 100644 tests/ui-toml/collapsible_if/collapsible_if.fixed create mode 100644 tests/ui-toml/collapsible_if/collapsible_if.rs create mode 100644 tests/ui-toml/collapsible_if/collapsible_if.stderr diff --git a/CHANGELOG.md b/CHANGELOG.md index 51441ab9fc0d..0da2079fabeb 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6361,6 +6361,7 @@ Released 2018-09-13 [`future-size-threshold`]: https://doc.rust-lang.org/clippy/lint_configuration.html#future-size-threshold [`ignore-interior-mutability`]: https://doc.rust-lang.org/clippy/lint_configuration.html#ignore-interior-mutability [`large-error-threshold`]: https://doc.rust-lang.org/clippy/lint_configuration.html#large-error-threshold +[`lint-commented-code`]: https://doc.rust-lang.org/clippy/lint_configuration.html#lint-commented-code [`lint-inconsistent-struct-field-initializers`]: https://doc.rust-lang.org/clippy/lint_configuration.html#lint-inconsistent-struct-field-initializers [`literal-representation-threshold`]: https://doc.rust-lang.org/clippy/lint_configuration.html#literal-representation-threshold [`matches-for-let-else`]: https://doc.rust-lang.org/clippy/lint_configuration.html#matches-for-let-else diff --git a/book/src/lint_configuration.md b/book/src/lint_configuration.md index 28b613ea3295..42c7135cf3cf 100644 --- a/book/src/lint_configuration.md +++ b/book/src/lint_configuration.md @@ -613,6 +613,17 @@ The maximum size of the `Err`-variant in a `Result` returned from a function * [`result_large_err`](https://rust-lang.github.io/rust-clippy/master/index.html#result_large_err) +## `lint-commented-code` +Whether collapsible `if` chains are linted if they contain comments inside the parts +that would be collapsed. + +**Default Value:** `true` + +--- +**Affected lints:** +* [`collapsible_if`](https://rust-lang.github.io/rust-clippy/master/index.html#collapsible_if) + + ## `lint-inconsistent-struct-field-initializers` Whether to suggest reordering constructor fields when initializers are present. diff --git a/clippy_config/src/conf.rs b/clippy_config/src/conf.rs index a61acbaa96bc..44bad5dbd4db 100644 --- a/clippy_config/src/conf.rs +++ b/clippy_config/src/conf.rs @@ -542,6 +542,10 @@ define_Conf! { /// The maximum size of the `Err`-variant in a `Result` returned from a function #[lints(result_large_err)] large_error_threshold: u64 = 128, + /// Whether collapsible `if` chains are linted if they contain comments inside the parts + /// that would be collapsed. + #[lints(collapsible_if)] + lint_commented_code: bool = true, /// Whether to suggest reordering constructor fields when initializers are present. /// /// Warnings produced by this configuration aren't necessarily fixed by just reordering the fields. Even if the diff --git a/clippy_lints/src/collapsible_if.rs b/clippy_lints/src/collapsible_if.rs index e73bfc6ebf7a..c9b80a64ea2c 100644 --- a/clippy_lints/src/collapsible_if.rs +++ b/clippy_lints/src/collapsible_if.rs @@ -1,10 +1,13 @@ +use clippy_config::Conf; use clippy_utils::diagnostics::{span_lint_and_sugg, span_lint_and_then}; -use clippy_utils::source::{snippet, snippet_block, snippet_block_with_applicability}; -use clippy_utils::sugg::Sugg; +use clippy_utils::source::{ + HasSession, IntoSpan as _, SpanRangeExt, snippet, snippet_block, snippet_block_with_applicability, +}; +use clippy_utils::span_contains_comment; use rustc_ast::ast; use rustc_errors::Applicability; use rustc_lint::{EarlyContext, EarlyLintPass}; -use rustc_session::declare_lint_pass; +use rustc_session::impl_lint_pass; use rustc_span::Span; declare_clippy_lint! { @@ -75,7 +78,95 @@ declare_clippy_lint! { "nested `else`-`if` expressions that can be collapsed (e.g., `else { if x { ... } }`)" } -declare_lint_pass!(CollapsibleIf => [COLLAPSIBLE_IF, COLLAPSIBLE_ELSE_IF]); +pub struct CollapsibleIf { + lint_commented_code: bool, +} + +impl CollapsibleIf { + pub fn new(conf: &'static Conf) -> Self { + Self { + lint_commented_code: conf.lint_commented_code, + } + } + + fn check_collapsible_else_if(cx: &EarlyContext<'_>, then_span: Span, else_: &ast::Expr) { + if let ast::ExprKind::Block(ref block, _) = else_.kind + && !block_starts_with_comment(cx, block) + && let Some(else_) = expr_block(block) + && else_.attrs.is_empty() + && !else_.span.from_expansion() + && let ast::ExprKind::If(..) = else_.kind + { + // Prevent "elseif" + // Check that the "else" is followed by whitespace + let up_to_else = then_span.between(block.span); + let requires_space = if let Some(c) = snippet(cx, up_to_else, "..").chars().last() { + !c.is_whitespace() + } else { + false + }; + + let mut applicability = Applicability::MachineApplicable; + span_lint_and_sugg( + cx, + COLLAPSIBLE_ELSE_IF, + block.span, + "this `else { if .. }` block can be collapsed", + "collapse nested if block", + format!( + "{}{}", + if requires_space { " " } else { "" }, + snippet_block_with_applicability(cx, else_.span, "..", Some(block.span), &mut applicability) + ), + applicability, + ); + } + } + + fn check_collapsible_if_if(&self, cx: &EarlyContext<'_>, expr: &ast::Expr, check: &ast::Expr, then: &ast::Block) { + if let Some(inner) = expr_block(then) + && inner.attrs.is_empty() + && let ast::ExprKind::If(check_inner, _, None) = &inner.kind + // Prevent triggering on `if c { if let a = b { .. } }`. + && !matches!(check_inner.kind, ast::ExprKind::Let(..)) + && let ctxt = expr.span.ctxt() + && inner.span.ctxt() == ctxt + && let contains_comment = span_contains_comment(cx.sess().source_map(), check.span.to(check_inner.span)) + && (!contains_comment || self.lint_commented_code) + { + span_lint_and_then( + cx, + COLLAPSIBLE_IF, + expr.span, + "this `if` statement can be collapsed", + |diag| { + let then_open_bracket = then.span.split_at(1).0.with_leading_whitespace(cx).into_span(); + let then_closing_bracket = { + let end = then.span.shrink_to_hi(); + end.with_lo(end.lo() - rustc_span::BytePos(1)) + .with_leading_whitespace(cx) + .into_span() + }; + let inner_if = inner.span.split_at(2).0; + let mut sugg = vec![ + // Remove the outer then block `{` + (then_open_bracket, String::new()), + // Remove the outer then block '}' + (then_closing_bracket, String::new()), + // Replace inner `if` by `&&` + (inner_if, String::from("&&")), + ]; + sugg.extend(par_around_or(check)); + sugg.extend(par_around_or(check_inner)); + + diag.multipart_suggestion("collapse nested if block", sugg, Applicability::MachineApplicable); + }, + ); + } + } +} + +impl_lint_pass!(CollapsibleIf => [COLLAPSIBLE_IF, COLLAPSIBLE_ELSE_IF]); impl EarlyLintPass for CollapsibleIf { fn check_expr(&mut self, cx: &EarlyContext<'_>, expr: &ast::Expr) { @@ -83,9 +174,10 @@ impl EarlyLintPass for CollapsibleIf { && !expr.span.from_expansion() { if let Some(else_) = else_ { - check_collapsible_maybe_if_let(cx, then.span, else_); + Self::check_collapsible_else_if(cx, then.span, else_); } else if !matches!(cond.kind, ast::ExprKind::Let(..)) { - check_collapsible_no_if_let(cx, expr, cond, then); + // Prevent triggering on `if c { if let a = b { .. } }`. + self.check_collapsible_if_if(cx, expr, cond, then); } } } @@ -99,74 +191,6 @@ fn block_starts_with_comment(cx: &EarlyContext<'_>, expr: &ast::Block) -> bool { trimmed_block_text.starts_with("//") || trimmed_block_text.starts_with("/*") } -fn check_collapsible_maybe_if_let(cx: &EarlyContext<'_>, then_span: Span, else_: &ast::Expr) { - if let ast::ExprKind::Block(ref block, _) = else_.kind - && !block_starts_with_comment(cx, block) - && let Some(else_) = expr_block(block) - && else_.attrs.is_empty() - && !else_.span.from_expansion() - && let ast::ExprKind::If(..) = else_.kind - { - // Prevent "elseif" - // Check that the "else" is followed by whitespace - let up_to_else = then_span.between(block.span); - let requires_space = if let Some(c) = snippet(cx, up_to_else, "..").chars().last() { - !c.is_whitespace() - } else { - false - }; - - let mut applicability = Applicability::MachineApplicable; - span_lint_and_sugg( - cx, - COLLAPSIBLE_ELSE_IF, - block.span, - "this `else { if .. }` block can be collapsed", - "collapse nested if block", - format!( - "{}{}", - if requires_space { " " } else { "" }, - snippet_block_with_applicability(cx, else_.span, "..", Some(block.span), &mut applicability) - ), - applicability, - ); - } -} - -fn check_collapsible_no_if_let(cx: &EarlyContext<'_>, expr: &ast::Expr, check: &ast::Expr, then: &ast::Block) { - if !block_starts_with_comment(cx, then) - && let Some(inner) = expr_block(then) - && inner.attrs.is_empty() - && let ast::ExprKind::If(ref check_inner, ref content, None) = inner.kind - // Prevent triggering on `if c { if let a = b { .. } }`. - && !matches!(check_inner.kind, ast::ExprKind::Let(..)) - && let ctxt = expr.span.ctxt() - && inner.span.ctxt() == ctxt - { - span_lint_and_then( - cx, - COLLAPSIBLE_IF, - expr.span, - "this `if` statement can be collapsed", - |diag| { - let mut app = Applicability::MachineApplicable; - let lhs = Sugg::ast(cx, check, "..", ctxt, &mut app); - let rhs = Sugg::ast(cx, check_inner, "..", ctxt, &mut app); - diag.span_suggestion( - expr.span, - "collapse nested if block", - format!( - "if {} {}", - lhs.and(&rhs), - snippet_block(cx, content.span, "..", Some(expr.span)), - ), - app, // snippet - ); - }, - ); - } -} - /// If the block contains only one expression, return it. fn expr_block(block: &ast::Block) -> Option<&ast::Expr> { if let [stmt] = &*block.stmts @@ -177,3 +201,17 @@ fn expr_block(block: &ast::Block) -> Option<&ast::Expr> { None } } + +/// If the expression is a `||`, suggest parentheses around it. +fn par_around_or(expr: &ast::Expr) -> Vec<(Span, String)> { + if let ast::ExprKind::Binary(op, _, _) = expr.kind + && op.node == ast::BinOpKind::Or + { + vec![ + (expr.span.shrink_to_lo(), String::from("(")), + (expr.span.shrink_to_hi(), String::from(")")), + ] + } else { + vec![] + } +} diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index 3fe3cd67e167..2d390be5248c 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -772,7 +772,7 @@ pub fn register_lints(store: &mut rustc_lint::LintStore, conf: &'static Conf) { store.register_late_pass(|_| Box::new(redundant_closure_call::RedundantClosureCall)); store.register_early_pass(|| Box::new(unused_unit::UnusedUnit)); store.register_late_pass(|_| Box::new(returns::Return)); - store.register_early_pass(|| Box::new(collapsible_if::CollapsibleIf)); + store.register_early_pass(move || Box::new(collapsible_if::CollapsibleIf::new(conf))); store.register_late_pass(|_| Box::new(items_after_statements::ItemsAfterStatements)); store.register_early_pass(|| Box::new(precedence::Precedence)); store.register_late_pass(|_| Box::new(needless_parens_on_range_literals::NeedlessParensOnRangeLiterals)); diff --git a/tests/ui-toml/collapsible_if/clippy.toml b/tests/ui-toml/collapsible_if/clippy.toml new file mode 100644 index 000000000000..4a9b47457f16 --- /dev/null +++ b/tests/ui-toml/collapsible_if/clippy.toml @@ -0,0 +1 @@ +lint-commented-code = false diff --git a/tests/ui-toml/collapsible_if/collapsible_if.fixed b/tests/ui-toml/collapsible_if/collapsible_if.fixed new file mode 100644 index 000000000000..7d0ae50703fb --- /dev/null +++ b/tests/ui-toml/collapsible_if/collapsible_if.fixed @@ -0,0 +1,18 @@ +#![allow(clippy::eq_op, clippy::nonminimal_bool)] + +#[rustfmt::skip] +#[warn(clippy::collapsible_if)] +fn main() { + if true + && true { + println!("No comment, linted"); + } + //~^^^^^ collapsible_if + + if true { + // Do not collapse because of this comment + if true { + println!("Hello world!"); + } + } +} diff --git a/tests/ui-toml/collapsible_if/collapsible_if.rs b/tests/ui-toml/collapsible_if/collapsible_if.rs new file mode 100644 index 000000000000..7192ed501af5 --- /dev/null +++ b/tests/ui-toml/collapsible_if/collapsible_if.rs @@ -0,0 +1,19 @@ +#![allow(clippy::eq_op, clippy::nonminimal_bool)] + +#[rustfmt::skip] +#[warn(clippy::collapsible_if)] +fn main() { + if true { + if true { + println!("No comment, linted"); + } + } + //~^^^^^ collapsible_if + + if true { + // Do not collapse because of this comment + if true { + println!("Hello world!"); + } + } +} diff --git a/tests/ui-toml/collapsible_if/collapsible_if.stderr b/tests/ui-toml/collapsible_if/collapsible_if.stderr new file mode 100644 index 000000000000..e34995458296 --- /dev/null +++ b/tests/ui-toml/collapsible_if/collapsible_if.stderr @@ -0,0 +1,22 @@ +error: this `if` statement can be collapsed + --> tests/ui-toml/collapsible_if/collapsible_if.rs:6:5 + | +LL | / if true { +LL | | if true { +LL | | println!("No comment, linted"); +LL | | } +LL | | } + | |_____^ + | + = note: `-D clippy::collapsible-if` implied by `-D warnings` + = help: to override `-D warnings` add `#[allow(clippy::collapsible_if)]` +help: collapse nested if block + | +LL ~ if true +LL ~ && true { +LL | println!("No comment, linted"); +LL ~ } + | + +error: aborting due to 1 previous error + diff --git a/tests/ui-toml/toml_unknown_key/conf_unknown_key.stderr b/tests/ui-toml/toml_unknown_key/conf_unknown_key.stderr index acfe739277cc..4ae4f450a471 100644 --- a/tests/ui-toml/toml_unknown_key/conf_unknown_key.stderr +++ b/tests/ui-toml/toml_unknown_key/conf_unknown_key.stderr @@ -49,6 +49,7 @@ error: error reading Clippy's configuration file: unknown field `foobar`, expect future-size-threshold ignore-interior-mutability large-error-threshold + lint-commented-code lint-inconsistent-struct-field-initializers literal-representation-threshold matches-for-let-else @@ -141,6 +142,7 @@ error: error reading Clippy's configuration file: unknown field `barfoo`, expect future-size-threshold ignore-interior-mutability large-error-threshold + lint-commented-code lint-inconsistent-struct-field-initializers literal-representation-threshold matches-for-let-else @@ -233,6 +235,7 @@ error: error reading Clippy's configuration file: unknown field `allow_mixed_uni future-size-threshold ignore-interior-mutability large-error-threshold + lint-commented-code lint-inconsistent-struct-field-initializers literal-representation-threshold matches-for-let-else diff --git a/tests/ui/collapsible_if.fixed b/tests/ui/collapsible_if.fixed index 6e994018aef0..4d0e6dd04688 100644 --- a/tests/ui/collapsible_if.fixed +++ b/tests/ui/collapsible_if.fixed @@ -12,34 +12,40 @@ fn main() { let x = "hello"; let y = "world"; - if x == "hello" && y == "world" { - println!("Hello world!"); - } + if x == "hello" + && y == "world" { + println!("Hello world!"); + } //~^^^^^ collapsible_if - if (x == "hello" || x == "world") && (y == "world" || y == "hello") { - println!("Hello world!"); - } + if (x == "hello" || x == "world") + && (y == "world" || y == "hello") { + println!("Hello world!"); + } //~^^^^^ collapsible_if - if x == "hello" && x == "world" && (y == "world" || y == "hello") { - println!("Hello world!"); - } + if x == "hello" && x == "world" + && (y == "world" || y == "hello") { + println!("Hello world!"); + } //~^^^^^ collapsible_if - if (x == "hello" || x == "world") && y == "world" && y == "hello" { - println!("Hello world!"); - } + if (x == "hello" || x == "world") + && y == "world" && y == "hello" { + println!("Hello world!"); + } //~^^^^^ collapsible_if - if x == "hello" && x == "world" && y == "world" && y == "hello" { - println!("Hello world!"); - } + if x == "hello" && x == "world" + && y == "world" && y == "hello" { + println!("Hello world!"); + } //~^^^^^ collapsible_if - if 42 == 1337 && 'a' != 'A' { - println!("world!") - } + if 42 == 1337 + && 'a' != 'A' { + println!("world!") + } //~^^^^^ collapsible_if // Works because any if with an else statement cannot be collapsed. @@ -71,37 +77,17 @@ fn main() { assert!(true); // assert! is just an `if` } - - // The following tests check for the fix of https://github.com/rust-lang/rust-clippy/issues/798 - if x == "hello" {// Not collapsible - if y == "world" { - println!("Hello world!"); - } - } - - if x == "hello" { // Not collapsible - if y == "world" { - println!("Hello world!"); - } - } - - if x == "hello" { - // Not collapsible - if y == "world" { + if x == "hello" + && y == "world" { // Collapsible println!("Hello world!"); } - } - - if x == "hello" && y == "world" { // Collapsible - println!("Hello world!"); - } //~^^^^^ collapsible_if if x == "hello" { print!("Hello "); } else { // Not collapsible - if y == "world" { + if let Some(42) = Some(42) { println!("world!") } } @@ -110,21 +96,8 @@ fn main() { print!("Hello "); } else { // Not collapsible - if let Some(42) = Some(42) { - println!("world!") - } - } - - if x == "hello" { - /* Not collapsible */ - if y == "world" { - println!("Hello world!"); - } - } - - if x == "hello" { /* Not collapsible */ if y == "world" { - println!("Hello world!"); + println!("world!") } } @@ -150,11 +123,13 @@ fn main() { } // Fix #5962 - if matches!(true, true) && matches!(true, true) {} + if matches!(true, true) + && matches!(true, true) {} //~^^^ collapsible_if // Issue #9375 - if matches!(true, true) && truth() && matches!(true, true) {} + if matches!(true, true) && truth() + && matches!(true, true) {} //~^^^ collapsible_if if true { @@ -163,4 +138,31 @@ fn main() { println!("Hello world!"); } } + + if x == "hello" + // Comment must be kept + && y == "world" { + println!("Hello world!"); + } + //~^^^^^^ collapsible_if + + // The following tests check for the fix of https://github.com/rust-lang/rust-clippy/issues/798 + if x == "hello" // Inner comment + && y == "world" { + println!("Hello world!"); + } + //~^^^^^ collapsible_if + + if x == "hello" + /* Inner comment */ + && y == "world" { + println!("Hello world!"); + } + //~^^^^^^ collapsible_if + + if x == "hello" /* Inner comment */ + && y == "world" { + println!("Hello world!"); + } + //~^^^^^ collapsible_if } diff --git a/tests/ui/collapsible_if.rs b/tests/ui/collapsible_if.rs index 5cf591a658c7..0fffb0f5f495 100644 --- a/tests/ui/collapsible_if.rs +++ b/tests/ui/collapsible_if.rs @@ -83,27 +83,6 @@ fn main() { assert!(true); // assert! is just an `if` } - - // The following tests check for the fix of https://github.com/rust-lang/rust-clippy/issues/798 - if x == "hello" {// Not collapsible - if y == "world" { - println!("Hello world!"); - } - } - - if x == "hello" { // Not collapsible - if y == "world" { - println!("Hello world!"); - } - } - - if x == "hello" { - // Not collapsible - if y == "world" { - println!("Hello world!"); - } - } - if x == "hello" { if y == "world" { // Collapsible println!("Hello world!"); @@ -115,7 +94,7 @@ fn main() { print!("Hello "); } else { // Not collapsible - if y == "world" { + if let Some(42) = Some(42) { println!("world!") } } @@ -124,21 +103,8 @@ fn main() { print!("Hello "); } else { // Not collapsible - if let Some(42) = Some(42) { - println!("world!") - } - } - - if x == "hello" { - /* Not collapsible */ if y == "world" { - println!("Hello world!"); - } - } - - if x == "hello" { /* Not collapsible */ - if y == "world" { - println!("Hello world!"); + println!("world!") } } @@ -181,4 +147,35 @@ fn main() { println!("Hello world!"); } } + + if x == "hello" { + // Comment must be kept + if y == "world" { + println!("Hello world!"); + } + } + //~^^^^^^ collapsible_if + + // The following tests check for the fix of https://github.com/rust-lang/rust-clippy/issues/798 + if x == "hello" { // Inner comment + if y == "world" { + println!("Hello world!"); + } + } + //~^^^^^ collapsible_if + + if x == "hello" { + /* Inner comment */ + if y == "world" { + println!("Hello world!"); + } + } + //~^^^^^^ collapsible_if + + if x == "hello" { /* Inner comment */ + if y == "world" { + println!("Hello world!"); + } + } + //~^^^^^ collapsible_if } diff --git a/tests/ui/collapsible_if.stderr b/tests/ui/collapsible_if.stderr index 3cc3fe5534f2..92e3f3171b27 100644 --- a/tests/ui/collapsible_if.stderr +++ b/tests/ui/collapsible_if.stderr @@ -12,9 +12,10 @@ LL | | } = help: to override `-D warnings` add `#[allow(clippy::collapsible_if)]` help: collapse nested if block | -LL ~ if x == "hello" && y == "world" { -LL + println!("Hello world!"); -LL + } +LL ~ if x == "hello" +LL ~ && y == "world" { +LL | println!("Hello world!"); +LL ~ } | error: this `if` statement can be collapsed @@ -29,9 +30,10 @@ LL | | } | help: collapse nested if block | -LL ~ if (x == "hello" || x == "world") && (y == "world" || y == "hello") { -LL + println!("Hello world!"); -LL + } +LL ~ if (x == "hello" || x == "world") { +LL ~ && (y == "world" || y == "hello") { +LL | println!("Hello world!"); +LL ~ } | error: this `if` statement can be collapsed @@ -46,9 +48,10 @@ LL | | } | help: collapse nested if block | -LL ~ if x == "hello" && x == "world" && (y == "world" || y == "hello") { -LL + println!("Hello world!"); -LL + } +LL ~ if x == "hello" && x == "world" +LL ~ && (y == "world" || y == "hello") { +LL | println!("Hello world!"); +LL ~ } | error: this `if` statement can be collapsed @@ -63,9 +66,10 @@ LL | | } | help: collapse nested if block | -LL ~ if (x == "hello" || x == "world") && y == "world" && y == "hello" { -LL + println!("Hello world!"); -LL + } +LL ~ if (x == "hello" || x == "world") { +LL ~ && y == "world" && y == "hello" { +LL | println!("Hello world!"); +LL ~ } | error: this `if` statement can be collapsed @@ -80,9 +84,10 @@ LL | | } | help: collapse nested if block | -LL ~ if x == "hello" && x == "world" && y == "world" && y == "hello" { -LL + println!("Hello world!"); -LL + } +LL ~ if x == "hello" && x == "world" +LL ~ && y == "world" && y == "hello" { +LL | println!("Hello world!"); +LL ~ } | error: this `if` statement can be collapsed @@ -97,13 +102,14 @@ LL | | } | help: collapse nested if block | -LL ~ if 42 == 1337 && 'a' != 'A' { -LL + println!("world!") -LL + } +LL ~ if 42 == 1337 +LL ~ && 'a' != 'A' { +LL | println!("world!") +LL ~ } | error: this `if` statement can be collapsed - --> tests/ui/collapsible_if.rs:107:5 + --> tests/ui/collapsible_if.rs:86:5 | LL | / if x == "hello" { LL | | if y == "world" { // Collapsible @@ -114,26 +120,115 @@ LL | | } | help: collapse nested if block | -LL ~ if x == "hello" && y == "world" { // Collapsible -LL + println!("Hello world!"); -LL + } +LL ~ if x == "hello" +LL ~ && y == "world" { // Collapsible +LL | println!("Hello world!"); +LL ~ } | error: this `if` statement can be collapsed - --> tests/ui/collapsible_if.rs:167:5 + --> tests/ui/collapsible_if.rs:133:5 | LL | / if matches!(true, true) { LL | | if matches!(true, true) {} LL | | } - | |_____^ help: collapse nested if block: `if matches!(true, true) && matches!(true, true) {}` + | |_____^ + | +help: collapse nested if block + | +LL ~ if matches!(true, true) +LL ~ && matches!(true, true) {} + | error: this `if` statement can be collapsed - --> tests/ui/collapsible_if.rs:173:5 + --> tests/ui/collapsible_if.rs:139:5 | LL | / if matches!(true, true) && truth() { LL | | if matches!(true, true) {} LL | | } - | |_____^ help: collapse nested if block: `if matches!(true, true) && truth() && matches!(true, true) {}` + | |_____^ + | +help: collapse nested if block + | +LL ~ if matches!(true, true) && truth() +LL ~ && matches!(true, true) {} + | + +error: this `if` statement can be collapsed + --> tests/ui/collapsible_if.rs:151:5 + | +LL | / if x == "hello" { +LL | | // Comment must be kept +LL | | if y == "world" { +LL | | println!("Hello world!"); +LL | | } +LL | | } + | |_____^ + | +help: collapse nested if block + | +LL ~ if x == "hello" +LL | // Comment must be kept +LL ~ && y == "world" { +LL | println!("Hello world!"); +LL ~ } + | + +error: this `if` statement can be collapsed + --> tests/ui/collapsible_if.rs:160:5 + | +LL | / if x == "hello" { // Inner comment +LL | | if y == "world" { +LL | | println!("Hello world!"); +LL | | } +LL | | } + | |_____^ + | +help: collapse nested if block + | +LL ~ if x == "hello" // Inner comment +LL ~ && y == "world" { +LL | println!("Hello world!"); +LL ~ } + | + +error: this `if` statement can be collapsed + --> tests/ui/collapsible_if.rs:167:5 + | +LL | / if x == "hello" { +LL | | /* Inner comment */ +LL | | if y == "world" { +LL | | println!("Hello world!"); +LL | | } +LL | | } + | |_____^ + | +help: collapse nested if block + | +LL ~ if x == "hello" +LL | /* Inner comment */ +LL ~ && y == "world" { +LL | println!("Hello world!"); +LL ~ } + | + +error: this `if` statement can be collapsed + --> tests/ui/collapsible_if.rs:175:5 + | +LL | / if x == "hello" { /* Inner comment */ +LL | | if y == "world" { +LL | | println!("Hello world!"); +LL | | } +LL | | } + | |_____^ + | +help: collapse nested if block + | +LL ~ if x == "hello" /* Inner comment */ +LL ~ && y == "world" { +LL | println!("Hello world!"); +LL ~ } + | -error: aborting due to 9 previous errors +error: aborting due to 13 previous errors From affd2a75db05b6063f2bccf2edc774f069eec2af Mon Sep 17 00:00:00 2001 From: Samuel Tardieu Date: Sun, 16 Feb 2025 16:27:09 +0100 Subject: [PATCH 2/2] Fix situations identified by `collapsible_if` new hits --- clippy_lints/src/escape.rs | 9 +++--- clippy_lints/src/item_name_repetitions.rs | 21 +++++++------ clippy_lints/src/mem_replace.rs | 8 ++--- clippy_lints/src/methods/map_clone.rs | 8 ++--- clippy_lints/src/methods/seek_from_current.rs | 8 ++--- .../src/mixed_read_write_in_expression.rs | 30 +++++++++---------- 6 files changed, 38 insertions(+), 46 deletions(-) diff --git a/clippy_lints/src/escape.rs b/clippy_lints/src/escape.rs index 8d1e893cb1af..3f73d849ea63 100644 --- a/clippy_lints/src/escape.rs +++ b/clippy_lints/src/escape.rs @@ -93,12 +93,11 @@ impl<'tcx> LateLintPass<'tcx> for BoxedLocal { // find `self` ty for this trait if relevant if let ItemKind::Trait(_, _, _, _, items) = item.kind { for trait_item in items { - if trait_item.id.owner_id.def_id == fn_def_id { + if trait_item.id.owner_id.def_id == fn_def_id // be sure we have `self` parameter in this function - if trait_item.kind == (AssocItemKind::Fn { has_self: true }) { - trait_self_ty = - Some(TraitRef::identity(cx.tcx, trait_item.id.owner_id.to_def_id()).self_ty()); - } + && trait_item.kind == (AssocItemKind::Fn { has_self: true }) + { + trait_self_ty = Some(TraitRef::identity(cx.tcx, trait_item.id.owner_id.to_def_id()).self_ty()); } } } diff --git a/clippy_lints/src/item_name_repetitions.rs b/clippy_lints/src/item_name_repetitions.rs index 6363f717a5c2..e670dbdb5a2b 100644 --- a/clippy_lints/src/item_name_repetitions.rs +++ b/clippy_lints/src/item_name_repetitions.rs @@ -282,22 +282,21 @@ fn check_struct_name_repetition(cx: &LateContext<'_>, item: &Item<'_>, fields: & "field name starts with the struct's name", ); } - if field_words.len() > item_name_words.len() { + if field_words.len() > item_name_words.len() // lint only if the end is not covered by the start - if field_words + && field_words .iter() .rev() .zip(item_name_words.iter().rev()) .all(|(a, b)| a == b) - { - span_lint_hir( - cx, - STRUCT_FIELD_NAMES, - field.hir_id, - field.span, - "field name ends with the struct's name", - ); - } + { + span_lint_hir( + cx, + STRUCT_FIELD_NAMES, + field.hir_id, + field.span, + "field name ends with the struct's name", + ); } } } diff --git a/clippy_lints/src/mem_replace.rs b/clippy_lints/src/mem_replace.rs index a0919947b3fc..d9eea8c0e24b 100644 --- a/clippy_lints/src/mem_replace.rs +++ b/clippy_lints/src/mem_replace.rs @@ -304,14 +304,12 @@ impl<'tcx> LateLintPass<'tcx> for MemReplace { && let ExprKind::Path(ref func_qpath) = func.kind && let Some(def_id) = cx.qpath_res(func_qpath, func.hir_id).opt_def_id() && cx.tcx.is_diagnostic_item(sym::mem_replace, def_id) - { // Check that second argument is `Option::None` - if !check_replace_option_with_none(cx, src, dest, expr.span) + && !check_replace_option_with_none(cx, src, dest, expr.span) && !check_replace_option_with_some(cx, src, dest, expr.span, self.msrv) && !check_replace_with_default(cx, src, dest, expr, self.msrv) - { - check_replace_with_uninit(cx, src, dest, expr.span); - } + { + check_replace_with_uninit(cx, src, dest, expr.span); } } } diff --git a/clippy_lints/src/methods/map_clone.rs b/clippy_lints/src/methods/map_clone.rs index 128b3695f48b..7b0ae60be6b9 100644 --- a/clippy_lints/src/methods/map_clone.rs +++ b/clippy_lints/src/methods/map_clone.rs @@ -114,19 +114,17 @@ fn handle_path( ) { if let Some(path_def_id) = cx.qpath_res(qpath, arg.hir_id).opt_def_id() && cx.tcx.lang_items().get(LangItem::CloneFn) == Some(path_def_id) - { // The `copied` and `cloned` methods are only available on `&T` and `&mut T` in `Option` // and `Result`. - if let ty::Adt(_, args) = cx.typeck_results().expr_ty(recv).kind() + && let ty::Adt(_, args) = cx.typeck_results().expr_ty(recv).kind() && let args = args.as_slice() && let Some(ty) = args.iter().find_map(|generic_arg| generic_arg.as_type()) && let ty::Ref(_, ty, Mutability::Not) = ty.kind() && let ty::FnDef(_, lst) = cx.typeck_results().expr_ty(arg).kind() && lst.iter().all(|l| l.as_type() == Some(*ty)) && !should_call_clone_as_function(cx, *ty) - { - lint_path(cx, e.span, recv.span, is_copy(cx, ty.peel_refs())); - } + { + lint_path(cx, e.span, recv.span, is_copy(cx, ty.peel_refs())); } } diff --git a/clippy_lints/src/methods/seek_from_current.rs b/clippy_lints/src/methods/seek_from_current.rs index d318462e5841..a82942f41936 100644 --- a/clippy_lints/src/methods/seek_from_current.rs +++ b/clippy_lints/src/methods/seek_from_current.rs @@ -38,13 +38,11 @@ fn arg_is_seek_from_current<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) && let ExprKind::Path(ref path) = f.kind && let Some(ctor_call_id) = cx.qpath_res(path, f.hir_id).opt_def_id() && is_enum_variant_ctor(cx, sym::SeekFrom, sym!(Current), ctor_call_id) - { // check if argument of `SeekFrom::Current` is `0` - if let ExprKind::Lit(lit) = arg.kind + && let ExprKind::Lit(lit) = arg.kind && let LitKind::Int(Pu128(0), LitIntType::Unsuffixed) = lit.node - { - return true; - } + { + return true; } false diff --git a/clippy_lints/src/mixed_read_write_in_expression.rs b/clippy_lints/src/mixed_read_write_in_expression.rs index a7452c8a3c84..147a151fa997 100644 --- a/clippy_lints/src/mixed_read_write_in_expression.rs +++ b/clippy_lints/src/mixed_read_write_in_expression.rs @@ -327,22 +327,22 @@ impl<'tcx> Visitor<'tcx> for ReadVisitor<'_, 'tcx> { return; } - if path_to_local_id(expr, self.var) { + if path_to_local_id(expr, self.var) // Check that this is a read, not a write. - if !is_in_assignment_position(self.cx, expr) { - span_lint_and_then( - self.cx, - MIXED_READ_WRITE_IN_EXPRESSION, - expr.span, - format!("unsequenced read of `{}`", self.cx.tcx.hir().name(self.var)), - |diag| { - diag.span_note( - self.write_expr.span, - "whether read occurs before this write depends on evaluation order", - ); - }, - ); - } + && !is_in_assignment_position(self.cx, expr) + { + span_lint_and_then( + self.cx, + MIXED_READ_WRITE_IN_EXPRESSION, + expr.span, + format!("unsequenced read of `{}`", self.cx.tcx.hir().name(self.var)), + |diag| { + diag.span_note( + self.write_expr.span, + "whether read occurs before this write depends on evaluation order", + ); + }, + ); } match expr.kind { // We're about to descend a closure. Since we don't know when (or