Lint more cases in collapsible_else_if

Author: profetia

CHANGELOG.md

@@ -6529,6 +6529,7 @@ Released 2018-09-13
 [`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-commented-else-if`]: https://doc.rust-lang.org/clippy/lint_configuration.html#lint-commented-else-if
 [`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
 [`max-fn-params-bools`]: https://doc.rust-lang.org/clippy/lint_configuration.html#max-fn-params-bools

book/src/lint_configuration.md

@@ -661,6 +661,17 @@ that would be collapsed.
 * [`collapsible_if`](https://rust-lang.github.io/rust-clippy/master/index.html#collapsible_if)
 
 
+## `lint-commented-else-if`
+Whether collapsible `else if` chains are linted if they contain comments inside the parts
+that would be collapsed.
+
+**Default Value:** `false`
+
+---
+**Affected lints:**
+* [`collapsible_else_if`](https://rust-lang.github.io/rust-clippy/master/index.html#collapsible_else_if)
+
+
 ## `literal-representation-threshold`
 The lower bound for linting decimal literals
 

clippy_config/src/conf.rs

@@ -645,6 +645,10 @@ define_Conf! {
     /// that would be collapsed.
     #[lints(collapsible_if)]
     lint_commented_code: bool = false,
+    /// Whether collapsible `else if` chains are linted if they contain comments inside the parts
+    /// that would be collapsed.
+    #[lints(collapsible_else_if)]
+    lint_commented_else_if: bool = false,
     /// Whether to suggest reordering constructor fields when initializers are present.
     /// DEPRECATED CONFIGURATION: lint-inconsistent-struct-field-initializers
     ///

clippy_lints/src/collapsible_if.rs

@@ -1,8 +1,8 @@
 use clippy_config::Conf;
 use clippy_utils::diagnostics::{span_lint_and_sugg, span_lint_and_then};
 use clippy_utils::msrvs::{self, Msrv};
-use clippy_utils::source::{IntoSpan as _, SpanRangeExt, snippet, snippet_block, snippet_block_with_applicability};
-use clippy_utils::span_contains_non_comment;
+use clippy_utils::source::{IntoSpan as _, SpanRangeExt, snippet, snippet_block_with_applicability};
+use clippy_utils::span_contains_non_whitespace;
 use rustc_ast::BinOpKind;
 use rustc_errors::Applicability;
 use rustc_hir::{Block, Expr, ExprKind, Stmt, StmtKind};
@@ -81,24 +81,24 @@ declare_clippy_lint! {
 pub struct CollapsibleIf {
     msrv: Msrv,
     lint_commented_code: bool,
+    lint_commented_else_if: bool,
 }
 
 impl CollapsibleIf {
     pub fn new(conf: &'static Conf) -> Self {
         Self {
             msrv: conf.msrv,
             lint_commented_code: conf.lint_commented_code,
+            lint_commented_else_if: conf.lint_commented_else_if,
         }
     }
 
-    fn check_collapsible_else_if(cx: &LateContext<'_>, then_span: Span, else_block: &Block<'_>) {
-        if !block_starts_with_comment(cx, else_block)
-            && let Some(else_) = expr_block(else_block)
+    fn check_collapsible_else_if(&self, cx: &LateContext<'_>, then_span: Span, else_block: &Block<'_>) {
+        if let Some(else_) = expr_block(else_block)
             && cx.tcx.hir_attrs(else_.hir_id).is_empty()
             && !else_.span.from_expansion()
             && let ExprKind::If(..) = else_.kind
-            && let up_to_if = else_block.span.until(else_.span)
-            && !span_contains_non_comment(cx, up_to_if.with_lo(BytePos(up_to_if.lo().0 + 1)))
+            && !block_starts_with_significant_tokens(cx, else_block, else_, self.lint_commented_else_if)
         {
             // Prevent "elseif"
             // Check that the "else" is followed by whitespace
@@ -133,7 +133,7 @@ impl CollapsibleIf {
             && self.eligible_condition(cx, check_inner)
             && let ctxt = expr.span.ctxt()
             && inner.span.ctxt() == ctxt
-            && (self.lint_commented_code || !block_starts_with_comment(cx, then))
+            && !block_starts_with_significant_tokens(cx, then, inner, self.lint_commented_code)
         {
             span_lint_and_then(
                 cx,
@@ -182,7 +182,7 @@ impl LateLintPass<'_> for CollapsibleIf {
             if let Some(else_) = else_
                 && let ExprKind::Block(else_, None) = else_.kind
             {
-                Self::check_collapsible_else_if(cx, then.span, else_);
+                self.check_collapsible_else_if(cx, then.span, else_);
             } else if else_.is_none()
                 && self.eligible_condition(cx, cond)
                 && let ExprKind::Block(then, None) = then.kind
@@ -193,12 +193,16 @@ impl LateLintPass<'_> for CollapsibleIf {
     }
 }
 
-fn block_starts_with_comment(cx: &LateContext<'_>, block: &Block<'_>) -> bool {
-    // We trim all opening braces and whitespaces and then check if the next string is a comment.
-    let trimmed_block_text = snippet_block(cx, block.span, "..", None)
-        .trim_start_matches(|c: char| c.is_whitespace() || c == '{')
-        .to_owned();
-    trimmed_block_text.starts_with("//") || trimmed_block_text.starts_with("/*")
+// Check that nothing significant can be found but whitespaces between the initial `{` of `block`
+// and the beginning of `stop_at`.
+fn block_starts_with_significant_tokens(
+    cx: &LateContext<'_>,
+    block: &Block<'_>,
+    stop_at: &Expr<'_>,
+    lint_commented_code: bool,
+) -> bool {
+    let span = block.span.split_at(1).1.until(stop_at.span);
+    span_contains_non_whitespace(cx, span, lint_commented_code)
 }
 
 /// If `block` is a block with either one expression or a statement containing an expression,

clippy_utils/src/lib.rs

@@ -2788,16 +2788,19 @@ pub fn span_contains_comment(sm: &SourceMap, span: Span) -> bool {
     });
 }
 
-/// Checks whether a given span has any non-comment token. This checks for all types of token other
-/// than line comment "//", block comment "/**", doc "///" "//!" and whitespace
+/// Checks whether a given span has any significant token. A significant token is a non-whitespace
+/// token, including comments unless `skip_comments` is set.
 /// This is useful to determine if there are any actual code tokens in the span that are omitted in
 /// the late pass, such as platform-specific code.
-pub fn span_contains_non_comment(cx: &impl source::HasSession, span: Span) -> bool {
-    matches!(span.get_source_text(cx), Some(snippet) if tokenize_with_text(&snippet).any(|(token, _, _)| {
-        !matches!(token, TokenKind::LineComment { .. } | TokenKind::BlockComment { .. } | TokenKind::Whitespace)
-    }))
+pub fn span_contains_non_whitespace(cx: &impl source::HasSession, span: Span, skip_comments: bool) -> bool {
+    matches!(span.get_source_text(cx), Some(snippet) if tokenize_with_text(&snippet).any(|(token, _, _)|
+        match token {
+            TokenKind::Whitespace => false,
+            TokenKind::BlockComment { .. } | TokenKind::LineComment { .. } => !skip_comments,
+            _ => true,
+        }
+    ))
 }
-
 /// Returns all the comments a given span contains
 ///
 /// Comments are returned wrapped with their relevant delimiters

tests/ui-toml/collapsible_if/clippy.toml

@@ -1 +1,2 @@
 lint-commented-code = true
+lint-commented-else-if = true

tests/ui-toml/collapsible_if/collapsible_else_if.fixed

@@ -0,0 +1,35 @@
+#![allow(clippy::eq_op, clippy::nonminimal_bool)]
+
+#[rustfmt::skip]
+#[warn(clippy::collapsible_if)]
+fn main() {
+    let (x, y) = ("hello", "world");
+
+    if x == "hello" {
+        todo!()
+    } else if y == "world" {
+        println!("Hello world!");
+    }
+    //~^^^^^^ collapsible_else_if
+
+    if x == "hello" {
+        todo!()
+    } else if y == "world" {
+        println!("Hello world!");
+    }
+    //~^^^^^ collapsible_else_if
+
+    if x == "hello" {
+        todo!()
+    } else if y == "world" {
+        println!("Hello world!");
+    }
+    //~^^^^^^ collapsible_else_if
+
+    if x == "hello" { 
+        todo!()
+    } else if y == "world" {
+        println!("Hello world!");
+    }
+    //~^^^^^ collapsible_else_if
+}

tests/ui-toml/collapsible_if/collapsible_else_if.rs

@@ -0,0 +1,45 @@
+#![allow(clippy::eq_op, clippy::nonminimal_bool)]
+
+#[rustfmt::skip]
+#[warn(clippy::collapsible_if)]
+fn main() {
+    let (x, y) = ("hello", "world");
+
+    if x == "hello" {
+        todo!()
+    } else {
+        // Comment must be kept
+        if y == "world" {
+            println!("Hello world!");
+        }
+    }
+    //~^^^^^^ collapsible_else_if
+
+    if x == "hello" {
+        todo!()
+    } else { // Inner comment
+        if y == "world" {
+            println!("Hello world!");
+        }
+    }
+    //~^^^^^ collapsible_else_if
+
+    if x == "hello" {
+        todo!()
+    } else {
+        /* Inner comment */
+        if y == "world" {
+            println!("Hello world!");
+        }
+    }
+    //~^^^^^^ collapsible_else_if
+
+    if x == "hello" { 
+        todo!()
+    } else { /* Inner comment */
+        if y == "world" {
+            println!("Hello world!");
+        }
+    }
+    //~^^^^^ collapsible_else_if
+}

tests/ui-toml/collapsible_if/collapsible_else_if.stderr

@@ -0,0 +1,78 @@
+error: this `else { if .. }` block can be collapsed
+  --> tests/ui-toml/collapsible_if/collapsible_else_if.rs:10:12
+   |
+LL |       } else {
+   |  ____________^
+LL | |         // Comment must be kept
+LL | |         if y == "world" {
+LL | |             println!("Hello world!");
+LL | |         }
+LL | |     }
+   | |_____^
+   |
+   = note: `-D clippy::collapsible-else-if` implied by `-D warnings`
+   = help: to override `-D warnings` add `#[allow(clippy::collapsible_else_if)]`
+help: collapse nested if block
+   |
+LL ~     } else if y == "world" {
+LL +         println!("Hello world!");
+LL +     }
+   |
+
+error: this `else { if .. }` block can be collapsed
+  --> tests/ui-toml/collapsible_if/collapsible_else_if.rs:20:12
+   |
+LL |       } else { // Inner comment
+   |  ____________^
+LL | |         if y == "world" {
+LL | |             println!("Hello world!");
+LL | |         }
+LL | |     }
+   | |_____^
+   |
+help: collapse nested if block
+   |
+LL ~     } else if y == "world" {
+LL +         println!("Hello world!");
+LL +     }
+   |
+
+error: this `else { if .. }` block can be collapsed
+  --> tests/ui-toml/collapsible_if/collapsible_else_if.rs:29:12
+   |
+LL |       } else {
+   |  ____________^
+LL | |         /* Inner comment */
+LL | |         if y == "world" {
+LL | |             println!("Hello world!");
+LL | |         }
+LL | |     }
+   | |_____^
+   |
+help: collapse nested if block
+   |
+LL ~     } else if y == "world" {
+LL +         println!("Hello world!");
+LL +     }
+   |
+
+error: this `else { if .. }` block can be collapsed
+  --> tests/ui-toml/collapsible_if/collapsible_else_if.rs:39:12
+   |
+LL |       } else { /* Inner comment */
+   |  ____________^
+LL | |         if y == "world" {
+LL | |             println!("Hello world!");
+LL | |         }
+LL | |     }
+   | |_____^
+   |
+help: collapse nested if block
+   |
+LL ~     } else if y == "world" {
+LL +         println!("Hello world!");
+LL +     }
+   |
+
+error: aborting due to 4 previous errors
+

tests/ui-toml/toml_unknown_key/conf_unknown_key.stderr

@@ -50,6 +50,7 @@ error: error reading Clippy's configuration file: unknown field `foobar`, expect
            ignore-interior-mutability
            large-error-threshold
            lint-commented-code
+           lint-commented-else-if
            literal-representation-threshold
            matches-for-let-else
            max-fn-params-bools
@@ -144,6 +145,7 @@ error: error reading Clippy's configuration file: unknown field `barfoo`, expect
            ignore-interior-mutability
            large-error-threshold
            lint-commented-code
+           lint-commented-else-if
            literal-representation-threshold
            matches-for-let-else
            max-fn-params-bools
@@ -238,6 +240,7 @@ error: error reading Clippy's configuration file: unknown field `allow_mixed_uni
            ignore-interior-mutability
            large-error-threshold
            lint-commented-code
+           lint-commented-else-if
            literal-representation-threshold
            matches-for-let-else
            max-fn-params-bools

tests/ui/collapsible_if.fixed

@@ -154,3 +154,12 @@ fn issue14722() {
         None
     };
 }
+
+fn issue14799() {
+    if true {
+        #[cfg(target_os = "freebsd")]
+        todo!();
+
+        if true {}
+    };
+}

tests/ui/collapsible_if.rs

@@ -164,3 +164,12 @@ fn issue14722() {
         None
     };
 }
+
+fn issue14799() {
+    if true {
+        #[cfg(target_os = "freebsd")]
+        todo!();
+
+        if true {}
+    };
+}