From 8753e568bf0d8bdc591ca56d9c3bc442efffaede Mon Sep 17 00:00:00 2001 From: Lukas Stevens Date: Tue, 16 Oct 2018 22:20:27 +0200 Subject: [PATCH 1/2] Check for comments in collapsible ifs --- clippy_lints/src/collapsible_if.rs | 9 ++++++ tests/ui/collapsible_if.rs | 45 ++++++++++++++++++++++++++++++ tests/ui/collapsible_if.stderr | 18 +++++++++++- 3 files changed, 71 insertions(+), 1 deletion(-) diff --git a/clippy_lints/src/collapsible_if.rs b/clippy_lints/src/collapsible_if.rs index 85fdca1d42131..67ef1048299fa 100644 --- a/clippy_lints/src/collapsible_if.rs +++ b/clippy_lints/src/collapsible_if.rs @@ -112,9 +112,17 @@ fn check_if(cx: &EarlyContext<'_>, expr: &ast::Expr) { } } +fn block_starts_with_comment(cx: &EarlyContext<'_>, expr: &ast::Block) -> bool { + // The zeroth character in the trimmed block text is "{", which marks the beginning of the block. + // Therefore, we check if the first string after that is a comment, i.e. starts with //. + let trimmed_block_text = snippet_block(cx, expr.span, "..").trim_left().to_owned(); + trimmed_block_text[1..trimmed_block_text.len()].trim_left().starts_with("//") +} + fn check_collapsible_maybe_if_let(cx: &EarlyContext<'_>, else_: &ast::Expr) { if_chain! { if let ast::ExprKind::Block(ref block, _) = else_.node; + if !block_starts_with_comment(cx, block); if let Some(else_) = expr_block(block); if !in_macro(else_.span); then { @@ -135,6 +143,7 @@ fn check_collapsible_maybe_if_let(cx: &EarlyContext<'_>, else_: &ast::Expr) { fn check_collapsible_no_if_let(cx: &EarlyContext<'_>, expr: &ast::Expr, check: &ast::Expr, then: &ast::Block) { if_chain! { + if !block_starts_with_comment(cx, then); if let Some(inner) = expr_block(then); if let ast::ExprKind::If(ref check_inner, ref content, None) = inner.node; then { diff --git a/tests/ui/collapsible_if.rs b/tests/ui/collapsible_if.rs index a6df9109df924..c186d9e577f33 100644 --- a/tests/ui/collapsible_if.rs +++ b/tests/ui/collapsible_if.rs @@ -151,4 +151,49 @@ fn main() { } else { assert!(true); // assert! is just an `if` } + + + // The following tests check for the fix of https://github.com/rust-lang-nursery/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!"); + } + } + + if x == "hello" { + print!("Hello "); + } else { + // Not collapsible + if y == "world" { + println!("world!") + } + } + + if x == "hello" { + print!("Hello "); + } else { + // Not collapsible + if let Some(42) = Some(42) { + println!("world!") + } + } } diff --git a/tests/ui/collapsible_if.stderr b/tests/ui/collapsible_if.stderr index 87c279cd7252b..3f06dca549522 100644 --- a/tests/ui/collapsible_if.stderr +++ b/tests/ui/collapsible_if.stderr @@ -240,5 +240,21 @@ help: try 122 | } | -error: aborting due to 13 previous errors +error: this if statement can be collapsed + --> $DIR/collapsible_if.rs:176:5 + | +176 | / if x == "hello" { +177 | | if y == "world" { // Collapsible +178 | | println!("Hello world!"); +179 | | } +180 | | } + | |_____^ +help: try + | +176 | if x == "hello" && y == "world" { // Collapsible +177 | println!("Hello world!"); +178 | } + | + +error: aborting due to 14 previous errors From 5614dcb4ead82724e5873172961a26ffbfddcd18 Mon Sep 17 00:00:00 2001 From: Lukas Stevens Date: Thu, 18 Oct 2018 18:57:16 +0200 Subject: [PATCH 2/2] Support multiline comments and hopefully fix panic --- clippy_lints/src/collapsible_if.rs | 8 ++++---- tests/ui/collapsible_if.rs | 13 +++++++++++++ 2 files changed, 17 insertions(+), 4 deletions(-) diff --git a/clippy_lints/src/collapsible_if.rs b/clippy_lints/src/collapsible_if.rs index 67ef1048299fa..a55ca04f706a7 100644 --- a/clippy_lints/src/collapsible_if.rs +++ b/clippy_lints/src/collapsible_if.rs @@ -113,10 +113,10 @@ fn check_if(cx: &EarlyContext<'_>, expr: &ast::Expr) { } fn block_starts_with_comment(cx: &EarlyContext<'_>, expr: &ast::Block) -> bool { - // The zeroth character in the trimmed block text is "{", which marks the beginning of the block. - // Therefore, we check if the first string after that is a comment, i.e. starts with //. - let trimmed_block_text = snippet_block(cx, expr.span, "..").trim_left().to_owned(); - trimmed_block_text[1..trimmed_block_text.len()].trim_left().starts_with("//") + // We trim all opening braces and whitespaces and then check if the next string is a comment. + let trimmed_block_text = + snippet_block(cx, expr.span, "..").trim_left_matches(|c: char| c.is_whitespace() || c == '{').to_owned(); + trimmed_block_text.starts_with("//") || trimmed_block_text.starts_with("/*") } fn check_collapsible_maybe_if_let(cx: &EarlyContext<'_>, else_: &ast::Expr) { diff --git a/tests/ui/collapsible_if.rs b/tests/ui/collapsible_if.rs index c186d9e577f33..1bc866010fd19 100644 --- a/tests/ui/collapsible_if.rs +++ b/tests/ui/collapsible_if.rs @@ -196,4 +196,17 @@ fn main() { println!("world!") } } + + if x == "hello" { + /* Not collapsible */ + if y == "world" { + println!("Hello world!"); + } + } + + if x == "hello" { /* Not collapsible */ + if y == "world" { + println!("Hello world!"); + } + } }