Lint more cases in collapsible_if

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

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.
 

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

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(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) {
         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 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![]
+    }
+}

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

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",
+                    );
                 }
             }
         }

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

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

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

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