Lint more cases in `collapsible_else_if`

9 files changed, 333 insertions, 45 deletions

diff --git a/book/src/lint_configuration.md b/book/src/lint_configuration.md
index 7c850b4b023..ab46aabb034 100644
--- a/book/src/lint_configuration.md
+++ b/book/src/lint_configuration.md
@@ -651,13 +651,14 @@ The maximum size of the `Err`-variant in a `Result` returned from a function
 
 
 ## `lint-commented-code`
-Whether collapsible `if` chains are linted if they contain comments inside the parts
+Whether collapsible `if` and `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)
 * [`collapsible_if`](https://rust-lang.github.io/rust-clippy/master/index.html#collapsible_if)
 
 
diff --git a/clippy_config/src/conf.rs b/clippy_config/src/conf.rs
index 87158cec42b..064825e5ab8 100644
--- a/clippy_config/src/conf.rs
+++ b/clippy_config/src/conf.rs
@@ -641,9 +641,9 @@ 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
+    /// Whether collapsible `if` and `else if` chains are linted if they contain comments inside the parts
     /// that would be collapsed.
-    #[lints(collapsible_if)]
+    #[lints(collapsible_else_if, collapsible_if)]
     lint_commented_code: bool = false,
     /// Whether to suggest reordering constructor fields when initializers are present.
     /// DEPRECATED CONFIGURATION: lint-inconsistent-struct-field-initializers
diff --git a/clippy_lints/src/collapsible_if.rs b/clippy_lints/src/collapsible_if.rs
index aef8e033207..1854d86c53b 100644
--- a/clippy_lints/src/collapsible_if.rs
+++ b/clippy_lints/src/collapsible_if.rs
@@ -1,13 +1,15 @@
 use clippy_config::Conf;
-use clippy_utils::diagnostics::{span_lint_and_sugg, span_lint_and_then};
+use clippy_utils::diagnostics::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, tokenize_with_text};
 use rustc_ast::BinOpKind;
 use rustc_errors::Applicability;
 use rustc_hir::{Block, Expr, ExprKind, Stmt, StmtKind};
+use rustc_lexer::TokenKind;
 use rustc_lint::{LateContext, LateLintPass};
 use rustc_session::impl_lint_pass;
+use rustc_span::source_map::SourceMap;
 use rustc_span::{BytePos, Span};
 
 declare_clippy_lint! {
@@ -91,37 +93,74 @@ impl CollapsibleIf {
         }
     }
 
-    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)))
+            && let ExprKind::If(else_if_cond, ..) = else_.kind
+            && !block_starts_with_significant_tokens(cx, else_block, else_, self.lint_commented_code)
         {
-            // Prevent "elseif"
-            // Check that the "else" is followed by whitespace
-            let up_to_else = then_span.between(else_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(
+            span_lint_and_then(
                 cx,
                 COLLAPSIBLE_ELSE_IF,
                 else_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(else_block.span), &mut applicability)
-                ),
-                applicability,
+                |diag| {
+                    let up_to_else = then_span.between(else_block.span);
+                    let else_before_if = else_.span.shrink_to_lo().with_hi(else_if_cond.span.lo() - BytePos(1));
+                    if self.lint_commented_code
+                        && let Some(else_keyword_span) =
+                            span_extract_keyword(cx.tcx.sess.source_map(), up_to_else, "else")
+                        && let Some(else_if_keyword_span) =
+                            span_extract_keyword(cx.tcx.sess.source_map(), else_before_if, "if")
+                    {
+                        let else_keyword_span = else_keyword_span.with_leading_whitespace(cx).into_span();
+                        let else_open_bracket = else_block.span.split_at(1).0.with_leading_whitespace(cx).into_span();
+                        let else_closing_bracket = {
+                            let end = else_block.span.shrink_to_hi();
+                            end.with_lo(end.lo() - BytePos(1))
+                                .with_leading_whitespace(cx)
+                                .into_span()
+                        };
+                        let sugg = vec![
+                            // Remove the outer else block `else`
+                            (else_keyword_span, String::new()),
+                            // Replace the inner `if` by `else if`
+                            (else_if_keyword_span, String::from("else if")),
+                            // Remove the outer else block `{`
+                            (else_open_bracket, String::new()),
+                            // Remove the outer else block '}'
+                            (else_closing_bracket, String::new()),
+                        ];
+                        diag.multipart_suggestion("collapse nested if block", sugg, Applicability::MachineApplicable);
+                        return;
+                    }
+
+                    // Prevent "elseif"
+                    // Check that the "else" is followed by whitespace
+                    let requires_space = if let Some(c) = snippet(cx, up_to_else, "..").chars().last() {
+                        !c.is_whitespace()
+                    } else {
+                        false
+                    };
+                    let mut applicability = Applicability::MachineApplicable;
+                    diag.span_suggestion(
+                        else_block.span,
+                        "collapse nested if block",
+                        format!(
+                            "{}{}",
+                            if requires_space { " " } else { "" },
+                            snippet_block_with_applicability(
+                                cx,
+                                else_.span,
+                                "..",
+                                Some(else_block.span),
+                                &mut applicability
+                            )
+                        ),
+                        applicability,
+                    );
+                },
             );
         }
     }
@@ -133,7 +172,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 +221,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 +232,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,
@@ -229,3 +272,16 @@ fn parens_around(expr: &Expr<'_>) -> Vec<(Span, String)> {
         vec![]
     }
 }
+
+fn span_extract_keyword(sm: &SourceMap, span: Span, keyword: &str) -> Option<Span> {
+    let snippet = sm.span_to_snippet(span).ok()?;
+    tokenize_with_text(&snippet)
+        .filter(|(t, s, _)| matches!(t, TokenKind::Ident if *s == keyword))
+        .map(|(_, _, inner)| {
+            span.split_at(u32::try_from(inner.start).unwrap())
+                .1
+                .split_at(u32::try_from(inner.end - inner.start).unwrap())
+                .0
+        })
+        .next()
+}
diff --git a/clippy_utils/src/lib.rs b/clippy_utils/src/lib.rs
index ad20209331b..19a2816b75a 100644
--- a/clippy_utils/src/lib.rs
+++ b/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
diff --git a/tests/ui-toml/collapsible_if/collapsible_else_if.fixed b/tests/ui-toml/collapsible_if/collapsible_else_if.fixed
new file mode 100644
index 00000000000..0dc0fc230c8
--- /dev/null
+++ b/tests/ui-toml/collapsible_if/collapsible_else_if.fixed
@@ -0,0 +1,50 @@
+#![allow(clippy::eq_op, clippy::nonminimal_bool)]
+
+#[rustfmt::skip]
+#[warn(clippy::collapsible_if)]
+fn main() {
+    let (x, y) = ("hello", "world");
+
+    if x == "hello" {
+        todo!()
+    }
+        // Comment must be kept
+        else if y == "world" {
+            println!("Hello world!");
+        }
+    //~^^^^^^ collapsible_else_if
+
+    if x == "hello" {
+        todo!()
+    } // Inner comment
+        else if y == "world" {
+            println!("Hello world!");
+        }
+    //~^^^^^ collapsible_else_if
+
+    if x == "hello" {
+        todo!()
+    }
+        /* Inner comment */
+        else if y == "world" {
+            println!("Hello world!");
+        }
+    //~^^^^^^ collapsible_else_if
+
+    if x == "hello" { 
+        todo!()
+    } /* Inner comment */
+        else if y == "world" {
+            println!("Hello world!");
+        }
+    //~^^^^^ collapsible_else_if
+
+    if x == "hello" {
+        todo!()
+    } /* This should not be removed */ /* So does this */
+        // Comment must be kept
+        else if y == "world" {
+            println!("Hello world!");
+        }
+    //~^^^^^^ collapsible_else_if
+}
diff --git a/tests/ui-toml/collapsible_if/collapsible_else_if.rs b/tests/ui-toml/collapsible_if/collapsible_else_if.rs
new file mode 100644
index 00000000000..8344c122f16
--- /dev/null
+++ b/tests/ui-toml/collapsible_if/collapsible_else_if.rs
@@ -0,0 +1,55 @@
+#![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
+
+    if x == "hello" {
+        todo!()
+    } /* This should not be removed */ else /* So does this */ {
+        // Comment must be kept
+        if y == "world" {
+            println!("Hello world!");
+        }
+    }
+    //~^^^^^^ collapsible_else_if
+}
diff --git a/tests/ui-toml/collapsible_if/collapsible_else_if.stderr b/tests/ui-toml/collapsible_if/collapsible_else_if.stderr
new file mode 100644
index 00000000000..0ffe5f0a960
--- /dev/null
+++ b/tests/ui-toml/collapsible_if/collapsible_else_if.stderr
@@ -0,0 +1,105 @@
+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 ~     }
+LL |         // Comment must be kept
+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 ~     } // Inner comment
+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 ~     }
+LL |         /* Inner comment */
+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 ~     } /* Inner comment */
+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:48:64
+   |
+LL |       } /* This should not be removed */ else /* So does this */ {
+   |  ________________________________________________________________^
+LL | |         // Comment must be kept
+LL | |         if y == "world" {
+LL | |             println!("Hello world!");
+LL | |         }
+LL | |     }
+   | |_____^
+   |
+help: collapse nested if block
+   |
+LL ~     } /* This should not be removed */ /* So does this */
+LL |         // Comment must be kept
+LL ~         else if y == "world" {
+LL |             println!("Hello world!");
+LL ~         }
+   |
+
+error: aborting due to 5 previous errors
+
diff --git a/tests/ui/collapsible_if.fixed b/tests/ui/collapsible_if.fixed
index b553182a445..77bc791ea8e 100644
--- a/tests/ui/collapsible_if.fixed
+++ b/tests/ui/collapsible_if.fixed
@@ -154,3 +154,12 @@ fn issue14722() {
         None
     };
 }
+
+fn issue14799() {
+    if true {
+        #[cfg(target_os = "freebsd")]
+        todo!();
+
+        if true {}
+    };
+}
diff --git a/tests/ui/collapsible_if.rs b/tests/ui/collapsible_if.rs
index f5998457ca6..d30df157d5e 100644
--- a/tests/ui/collapsible_if.rs
+++ b/tests/ui/collapsible_if.rs
@@ -164,3 +164,12 @@ fn issue14722() {
         None
     };
 }
+
+fn issue14799() {
+    if true {
+        #[cfg(target_os = "freebsd")]
+        todo!();
+
+        if true {}
+    };
+}