X-Git-Url: https://git.lizzy.rs/?a=blobdiff_plain;f=clippy_lints%2Fsrc%2Fcollapsible_if.rs;h=0fd0abdc39d2326d5087aa875580cf21f4e134c6;hb=e5a5b0a0774625eebbe7b29c67b49dc6431544d1;hp=55cc94f399f52bf5029d57175741138861d639dd;hpb=8e0de3d12046dd349e21e0cb09fd06b07d3230d9;p=rust.git diff --git a/clippy_lints/src/collapsible_if.rs b/clippy_lints/src/collapsible_if.rs index 55cc94f399f..0fd0abdc39d 100644 --- a/clippy_lints/src/collapsible_if.rs +++ b/clippy_lints/src/collapsible_if.rs @@ -12,132 +12,145 @@ //! //! This lint is **warn** by default -use rustc::lint::{EarlyContext, EarlyLintPass, LintArray, LintPass}; -use rustc::{declare_tool_lint, lint_array}; use if_chain::if_chain; +use rustc::declare_lint_pass; +use rustc::lint::{EarlyContext, EarlyLintPass, LintArray, LintPass}; +use rustc_session::declare_tool_lint; use syntax::ast; -use crate::utils::{in_macro, snippet_block, span_lint_and_sugg, span_lint_and_then}; use crate::utils::sugg::Sugg; +use crate::utils::{snippet_block, snippet_block_with_applicability, span_lint_and_sugg, span_lint_and_then}; +use rustc_errors::Applicability; -/// **What it does:** Checks for nested `if` statements which can be collapsed -/// by `&&`-combining their conditions and for `else { if ... }` expressions -/// that -/// can be collapsed to `else if ...`. -/// -/// **Why is this bad?** Each `if`-statement adds one level of nesting, which -/// makes code look more complex than it really is. -/// -/// **Known problems:** None. -/// -/// **Example:** -/// ```rust,ignore -/// if x { -/// if y { -/// … -/// } -/// } -/// -/// // or -/// -/// if x { -/// … -/// } else { -/// if y { -/// … -/// } -/// } -/// ``` -/// -/// Should be written: -/// -/// ```rust.ignore -/// if x && y { -/// … -/// } -/// -/// // or -/// -/// if x { -/// … -/// } else if y { -/// … -/// } -/// ``` declare_clippy_lint! { + /// **What it does:** Checks for nested `if` statements which can be collapsed + /// by `&&`-combining their conditions and for `else { if ... }` expressions + /// that + /// can be collapsed to `else if ...`. + /// + /// **Why is this bad?** Each `if`-statement adds one level of nesting, which + /// makes code look more complex than it really is. + /// + /// **Known problems:** None. + /// + /// **Example:** + /// ```rust,ignore + /// if x { + /// if y { + /// … + /// } + /// } + /// + /// // or + /// + /// if x { + /// … + /// } else { + /// if y { + /// … + /// } + /// } + /// ``` + /// + /// Should be written: + /// + /// ```rust.ignore + /// if x && y { + /// … + /// } + /// + /// // or + /// + /// if x { + /// … + /// } else if y { + /// … + /// } + /// ``` pub COLLAPSIBLE_IF, style, - "`if`s that can be collapsed (e.g. `if x { if y { ... } }` and `else { if x { ... } }`)" + "`if`s that can be collapsed (e.g., `if x { if y { ... } }` and `else { if x { ... } }`)" } -#[derive(Copy, Clone)] -pub struct CollapsibleIf; - -impl LintPass for CollapsibleIf { - fn get_lints(&self) -> LintArray { - lint_array!(COLLAPSIBLE_IF) - } -} +declare_lint_pass!(CollapsibleIf => [COLLAPSIBLE_IF]); impl EarlyLintPass for CollapsibleIf { fn check_expr(&mut self, cx: &EarlyContext<'_>, expr: &ast::Expr) { - if !in_macro(expr.span) { + if !expr.span.from_expansion() { check_if(cx, expr) } } } fn check_if(cx: &EarlyContext<'_>, expr: &ast::Expr) { - match expr.node { - ast::ExprKind::If(ref check, ref then, ref else_) => if let Some(ref else_) = *else_ { + if let ast::ExprKind::If(check, then, else_) = &expr.kind { + if let Some(else_) = else_ { check_collapsible_maybe_if_let(cx, else_); + } else if let ast::ExprKind::Let(..) = check.kind { + // Prevent triggering on `if let a = b { if c { .. } }`. } else { check_collapsible_no_if_let(cx, expr, check, then); - }, - ast::ExprKind::IfLet(_, _, _, Some(ref else_)) => { - check_collapsible_maybe_if_let(cx, else_); - }, - _ => (), + } } } +fn block_starts_with_comment(cx: &EarlyContext<'_>, expr: &ast::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, expr.span, "..") + .trim_start_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) { if_chain! { - if let ast::ExprKind::Block(ref block, _) = else_.node; + if let ast::ExprKind::Block(ref block, _) = else_.kind; + if !block_starts_with_comment(cx, block); if let Some(else_) = expr_block(block); - if !in_macro(else_.span); + if !else_.span.from_expansion(); + if let ast::ExprKind::If(..) = else_.kind; then { - match else_.node { - ast::ExprKind::If(..) | ast::ExprKind::IfLet(..) => { - span_lint_and_sugg(cx, - COLLAPSIBLE_IF, - block.span, - "this `else { if .. }` block can be collapsed", - "try", - snippet_block(cx, else_.span, "..").into_owned()); - } - _ => (), - } + let mut applicability = Applicability::MachineApplicable; + span_lint_and_sugg( + cx, + COLLAPSIBLE_IF, + block.span, + "this `else { if .. }` block can be collapsed", + "try", + snippet_block_with_applicability(cx, else_.span, "..", &mut applicability).into_owned(), + applicability, + ); } } } 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; + if let ast::ExprKind::If(ref check_inner, ref content, None) = inner.kind; then { + if let ast::ExprKind::Let(..) = check_inner.kind { + // Prevent triggering on `if c { if let a = b { .. } }`. + return; + } + if expr.span.ctxt() != inner.span.ctxt() { return; } span_lint_and_then(cx, COLLAPSIBLE_IF, expr.span, "this if statement can be collapsed", |db| { let lhs = Sugg::ast(cx, check, ".."); let rhs = Sugg::ast(cx, check_inner, ".."); - db.span_suggestion(expr.span, - "try", - format!("if {} {}", - lhs.and(&rhs), - snippet_block(cx, content.span, ".."))); + db.span_suggestion( + expr.span, + "try", + format!( + "if {} {}", + lhs.and(&rhs), + snippet_block(cx, content.span, ".."), + ), + Applicability::MachineApplicable, // snippet + ); }); } } @@ -148,7 +161,7 @@ fn expr_block(block: &ast::Block) -> Option<&ast::Expr> { let mut it = block.stmts.iter(); if let (Some(stmt), None) = (it.next(), it.next()) { - match stmt.node { + match stmt.kind { ast::StmtKind::Expr(ref expr) | ast::StmtKind::Semi(ref expr) => Some(expr), _ => None, }