Refactored lint to use check_fn

Author: 1c3t3a

clippy_lints/src/semicolon_outside_block.rs

@@ -4,8 +4,9 @@ use clippy_utils::in_macro;
 use clippy_utils::source::snippet_with_macro_callsite;
 use if_chain::if_chain;
 use rustc_errors::Applicability;
+use rustc_hir::intravisit::FnKind;
 use rustc_hir::ExprKind;
-use rustc_hir::{Block, BodyOwnerKind, StmtKind};
+use rustc_hir::{Block, Body, Expr, FnDecl, HirId, StmtKind};
 use rustc_lint::{LateContext, LateLintPass};
 use rustc_session::{declare_lint_pass, declare_tool_lint};
 use rustc_span::BytePos;
@@ -36,94 +37,118 @@ declare_clippy_lint! {
 
 declare_lint_pass!(SemicolonOutsideBlock => [SEMICOLON_OUTSIDE_BLOCK]);
 
-/// Checks if an ExprKind is of an illegal variant (aka blocks that we don't want)
-/// to lint on as it's illegal or unnecessary to put a semicolon afterwards.
-/// This macro then inserts a `return` statement to return out of the check_block
-/// method.
-macro_rules! check_expr_return {
-    ($l:expr) => {
-        if let ExprKind::If(..) | ExprKind::Loop(..) | ExprKind::DropTemps(..) | ExprKind::Match(..) = $l {
-            return;
-        }
-    };
-}
-
 impl LateLintPass<'_> for SemicolonOutsideBlock {
-    fn check_block(&mut self, cx: &LateContext<'tcx>, block: &'tcx Block<'tcx>) {
-        if_chain! {
-            if !in_macro(block.span);
-            if block.expr.is_none();
-            if let Some(last) = block.stmts.last();
-            if let StmtKind::Semi(expr) = last.kind;
-            let t_expr = cx.typeck_results().expr_ty(expr);
-            if t_expr.is_unit();
-            then {
-                // make sure that the block does not belong to a function by iterating over the parents
-                for (hir_id, _) in cx.tcx.hir().parent_iter(block.hir_id) {
-                    if let Some(body_id) = cx.tcx.hir().maybe_body_owned_by(hir_id) {
-                        // if we're in a body, check if it's an fn or a closure
-                        if cx.tcx.hir().body_owner_kind(hir_id).is_fn_or_closure() {
-                            let item_body = cx.tcx.hir().body(body_id);
-                            if let ExprKind::Block(fn_block, _) = item_body.value.kind {
-                                // check for an illegal statement in the list of statements...
-                                for stmt in fn_block.stmts {
-                                    if let StmtKind::Expr(pot_ille_expr) = stmt.kind {
-                                        check_expr_return!(pot_ille_expr.kind);
-                                    }
-                                }
-
-                                //.. the potential last statement ..
-                                if let Some(last_expr) = fn_block.expr {
-                                    check_expr_return!(last_expr.kind);
-                                }
+    fn check_fn(
+        &mut self,
+        cx: &LateContext<'tcx>,
+        fn_kind: FnKind<'tcx>,
+        _: &'tcx FnDecl<'_>,
+        body: &'tcx Body<'_>,
+        _: Span,
+        _: HirId,
+    ) {
+        if let ExprKind::Block(block) = body.value.kind {
+            // also check this block if we're inside a closure
+            if matches!(fn_kind, FnKind::Closure) {
+                check_block(cx, block);
+            }
 
-                                // .. or if this is the body of an fn
-                                if fn_block.hir_id == block.hir_id &&
-                                    !matches!(cx.tcx.hir().body_owner_kind(hir_id), BodyOwnerKind::Closure) {
-                                    return
-                                }
-                            }
-                        }
-                    }
+            // iterate over the statements and check if we find a potential
+            // block to check
+            for stmt in block.stmts {
+                match stmt.kind {
+                    StmtKind::Expr(Expr {
+                        kind: ExprKind::Block(bl, ..),
+                        ..
+                    })
+                    | StmtKind::Semi(Expr {
+                        kind: ExprKind::Block(bl, ..),
+                        ..
+                    }) => check_block(cx, bl),
+                    _ => (),
                 }
+            }
 
-                // filter out other blocks and the desugared for loop
-                check_expr_return!(expr.kind);
-
-                // make sure we're also having the semicolon at the end of the expression...
-                let expr_w_sem = expand_span_to_semicolon(cx, expr.span);
-                let expr_snip = snippet_with_macro_callsite(cx, expr_w_sem, "..");
-                let mut expr_sugg = expr_snip.to_string();
-                expr_sugg.pop();
+            // check if the potential trailing expr is a block and check it if necessary
+            if let Some(Expr {
+                kind: ExprKind::Block(bl, ..),
+                ..
+            }) = block.expr
+            {
+                check_block(cx, bl);
+            }
+        }
+    }
+}
 
-                // and the block
-                let block_w_sem = expand_span_to_semicolon(cx, block.span);
-                let mut block_snip: String = snippet_with_macro_callsite(cx, block_w_sem, "..").to_string();
-                if block_snip.ends_with('\n') {
-                    block_snip.pop();
-                }
+/// Checks for a block if it's a target of this lint and spans a suggestion
+/// if applicable. This method also recurses through all other statements that
+/// are blocks.
+fn check_block(cx: &LateContext<'_>, block: &Block<'_>) {
+    // check all subblocks
+    for stmt in block.stmts {
+        match stmt.kind {
+            StmtKind::Expr(Expr {
+                kind: ExprKind::Block(bl, ..),
+                ..
+            })
+            | StmtKind::Semi(Expr {
+                kind: ExprKind::Block(bl, ..),
+                ..
+            }) => check_block(cx, bl),
+            _ => (),
+        }
+    }
+    // check the potential trailing expression
+    if let Some(Expr {
+        kind: ExprKind::Block(bl, ..),
+        ..
+    }) = block.expr
+    {
+        check_block(cx, bl);
+    }
 
-                // retrieve the suggestion
-                let suggestion = if block_snip.ends_with(';') {
-                    block_snip.replace(expr_snip.as_ref(), &format!("{}", expr_sugg.as_str()))
-                } else {
-                    format!("{};", block_snip.replace(expr_snip.as_ref(), &format!("{}", expr_sugg.as_str())))
-                };
+    if_chain! {
+        if !in_macro(block.span);
+        if block.expr.is_none();
+        if let Some(last) = block.stmts.last();
+        if let StmtKind::Semi(expr) = last.kind;
+        let t_expr = cx.typeck_results().expr_ty(expr);
+        if t_expr.is_unit();
+        then {
+            // make sure we're also having the semicolon at the end of the expression...
+            let expr_w_sem = expand_span_to_semicolon(cx, expr.span);
+            let expr_snip = snippet_with_macro_callsite(cx, expr_w_sem, "..");
+            let mut expr_sugg = expr_snip.to_string();
+            expr_sugg.pop();
 
-                span_lint_and_sugg(
-                    cx,
-                    SEMICOLON_OUTSIDE_BLOCK,
-                    if block_snip.ends_with(';') {
-                        block_w_sem
-                    } else {
-                        block.span
-                    },
-                    "consider moving the `;` outside the block for consistent formatting",
-                    "put the `;` outside the block",
-                    suggestion,
-                    Applicability::MaybeIncorrect,
-                );
+            // and the block
+            let block_w_sem = expand_span_to_semicolon(cx, block.span);
+            let mut block_snip: String = snippet_with_macro_callsite(cx, block_w_sem, "..").to_string();
+            if block_snip.ends_with('\n') {
+                block_snip.pop();
             }
+
+            // retrieve the suggestion
+            let suggestion = if block_snip.ends_with(';') {
+                block_snip.replace(expr_snip.as_ref(), &format!("{}", expr_sugg.as_str()))
+            } else {
+                format!("{};", block_snip.replace(expr_snip.as_ref(), &format!("{}", expr_sugg.as_str())))
+            };
+
+            span_lint_and_sugg(
+                cx,
+                SEMICOLON_OUTSIDE_BLOCK,
+                if block_snip.ends_with(';') {
+                    block_w_sem
+                } else {
+                    block.span
+                },
+                "consider moving the `;` outside the block for consistent formatting",
+                "put the `;` outside the block",
+                suggestion,
+                Applicability::MaybeIncorrect,
+            );
         }
     }
 }