Lint more cases in collapsible_if (rust-lang#14231)

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, allows opting out from this behavior.

If reviewed on GitHub, the second commit of this PR is best looked at
side by side, with whitespace differences turned off.

changelog: [`collapsible_if`]: lint more cases

Author: flip1995

CHANGELOG.md

@@ -6363,6 +6363,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

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:** `false`
+
+---
+**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.
 

clippy_config/src/conf.rs

@@ -549,6 +549,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 = false,
     /// 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

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,17 +78,106 @@ 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(parens_around(check));
+                    sugg.extend(parens_around(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) {
         if let ast::ExprKind::If(cond, then, else_) = &expr.kind
             && !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 parens_around(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![]
+    }
+}

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));

tests/ui-toml/collapsible_if/clippy.toml

@@ -0,0 +1 @@
+lint-commented-code = true

tests/ui-toml/collapsible_if/collapsible_if.fixed

@@ -0,0 +1,34 @@
+#![allow(clippy::eq_op, clippy::nonminimal_bool)]
+
+#[rustfmt::skip]
+#[warn(clippy::collapsible_if)]
+fn main() {
+    let (x, y) = ("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
+}

tests/ui-toml/collapsible_if/collapsible_if.rs

@@ -0,0 +1,38 @@
+#![allow(clippy::eq_op, clippy::nonminimal_bool)]
+
+#[rustfmt::skip]
+#[warn(clippy::collapsible_if)]
+fn main() {
+    let (x, y) = ("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
+}