diff --git a/CHANGELOG.md b/CHANGELOG.md index 2b89170073be..3f81eacf357f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2904,6 +2904,8 @@ Released 2018-09-13 [`self_assignment`]: https://rust-lang.github.io/rust-clippy/master/index.html#self_assignment [`self_named_constructors`]: https://rust-lang.github.io/rust-clippy/master/index.html#self_named_constructors [`semicolon_if_nothing_returned`]: https://rust-lang.github.io/rust-clippy/master/index.html#semicolon_if_nothing_returned +[`semicolon_inside_block`]: https://rust-lang.github.io/rust-clippy/master/index.html#semicolon_inside_block +[`semicolon_outside_block`]: https://rust-lang.github.io/rust-clippy/master/index.html#semicolon_outside_block [`serde_api_misuse`]: https://rust-lang.github.io/rust-clippy/master/index.html#serde_api_misuse [`shadow_reuse`]: https://rust-lang.github.io/rust-clippy/master/index.html#shadow_reuse [`shadow_same`]: https://rust-lang.github.io/rust-clippy/master/index.html#shadow_same diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index 18600498e1c4..c3f53db53be8 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -331,6 +331,8 @@ mod returns; mod self_assignment; mod self_named_constructors; mod semicolon_if_nothing_returned; +mod semicolon_inside_block; +mod semicolon_outside_block; mod serde_api; mod shadow; mod single_component_path_imports; @@ -903,6 +905,8 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: self_assignment::SELF_ASSIGNMENT, self_named_constructors::SELF_NAMED_CONSTRUCTORS, semicolon_if_nothing_returned::SEMICOLON_IF_NOTHING_RETURNED, + semicolon_inside_block::SEMICOLON_INSIDE_BLOCK, + semicolon_outside_block::SEMICOLON_OUTSIDE_BLOCK, serde_api::SERDE_API_MISUSE, shadow::SHADOW_REUSE, shadow::SHADOW_SAME, @@ -1133,6 +1137,8 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: LintId::of(redundant_else::REDUNDANT_ELSE), LintId::of(ref_option_ref::REF_OPTION_REF), LintId::of(semicolon_if_nothing_returned::SEMICOLON_IF_NOTHING_RETURNED), + LintId::of(semicolon_inside_block::SEMICOLON_INSIDE_BLOCK), + LintId::of(semicolon_outside_block::SEMICOLON_OUTSIDE_BLOCK), LintId::of(shadow::SHADOW_UNRELATED), LintId::of(strings::STRING_ADD_ASSIGN), LintId::of(trait_bounds::TRAIT_DUPLICATION_IN_BOUNDS), @@ -2088,6 +2094,8 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: store.register_late_pass(|| box manual_ok_or::ManualOkOr); store.register_late_pass(|| box float_equality_without_abs::FloatEqualityWithoutAbs); store.register_late_pass(|| box semicolon_if_nothing_returned::SemicolonIfNothingReturned); + store.register_late_pass(|| box semicolon_inside_block::SemicolonInsideBlock); + store.register_late_pass(|| box semicolon_outside_block::SemicolonOutsideBlock); store.register_late_pass(|| box async_yields_async::AsyncYieldsAsync); let disallowed_methods = conf.disallowed_methods.iter().cloned().collect::>(); store.register_late_pass(move || box disallowed_method::DisallowedMethod::new(&disallowed_methods)); diff --git a/clippy_lints/src/semicolon_inside_block.rs b/clippy_lints/src/semicolon_inside_block.rs new file mode 100644 index 000000000000..0fe9d022d9de --- /dev/null +++ b/clippy_lints/src/semicolon_inside_block.rs @@ -0,0 +1,75 @@ +use crate::rustc_lint::LintContext; +use clippy_utils::diagnostics::span_lint_and_sugg; +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::{Block, ExprKind}; +use rustc_lint::{LateContext, LateLintPass}; +use rustc_session::{declare_lint_pass, declare_tool_lint}; + +declare_clippy_lint! { + /// **What it does:** For () returning expressions, check that the semicolon is inside the block. + /// + /// **Why is this bad?** For consistency it's best to have the semicolon inside/outside the block. Either way is fine and this lint suggests inside the block. + /// Take a look at `semicolon_outside_block` for the other alternative. + /// + /// **Known problems:** None. + /// + /// **Example:** + /// + /// ```rust + /// unsafe { f(x) }; + /// ``` + /// Use instead: + /// ```rust + /// unsafe { f(x); } + /// ``` + pub SEMICOLON_INSIDE_BLOCK, + pedantic, + "add a semicolon inside the block" +} + +declare_lint_pass!(SemicolonInsideBlock => [SEMICOLON_INSIDE_BLOCK]); + +impl LateLintPass<'_> for SemicolonInsideBlock { + fn check_block(&mut self, cx: &LateContext<'tcx>, block: &'tcx Block<'tcx>) { + if_chain! { + if !in_macro(block.span); + if let Some(expr) = block.expr; + let t_expr = cx.typeck_results().expr_ty(expr); + if t_expr.is_unit(); + if let snippet = snippet_with_macro_callsite(cx, expr.span, "}"); + if !snippet.ends_with("};") && !snippet.ends_with('}'); + then { + // filter out the desugared `for` loop + if let ExprKind::DropTemps(..) = &expr.kind { + return; + } + + let expr_snip = snippet_with_macro_callsite(cx, expr.span, ".."); + + // check for the right suggestion and span, differs if the block spans + // multiple lines + let (suggestion, span) = if cx.sess().source_map().is_multiline(block.span) { + (format!("{};", expr_snip), expr.span.source_callsite()) + } else { + let block_with_pot_sem = cx.sess().source_map().span_extend_to_next_char(block.span, '\n', false); + let block_snip = snippet_with_macro_callsite(cx, block.span, ".."); + + (block_snip.replace(expr_snip.as_ref(), &format!("{};", expr_snip)), block_with_pot_sem) + }; + + span_lint_and_sugg( + cx, + SEMICOLON_INSIDE_BLOCK, + span, + "consider moving the `;` inside the block for consistent formatting", + "put the `;` here", + suggestion, + Applicability::MaybeIncorrect, + ); + } + } + } +} diff --git a/clippy_lints/src/semicolon_outside_block.rs b/clippy_lints/src/semicolon_outside_block.rs new file mode 100644 index 000000000000..5ec9c7fdc667 --- /dev/null +++ b/clippy_lints/src/semicolon_outside_block.rs @@ -0,0 +1,160 @@ +use crate::rustc_lint::LintContext; +use clippy_utils::diagnostics::span_lint_and_sugg; +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, Body, Expr, FnDecl, HirId, StmtKind}; +use rustc_lint::{LateContext, LateLintPass}; +use rustc_session::{declare_lint_pass, declare_tool_lint}; +use rustc_span::BytePos; +use rustc_span::Span; +use std::ops::Add; + +declare_clippy_lint! { + /// **What it does:** For () returning expressions, check that the semicolon is outside the block. + /// + /// **Why is this bad?** For consistency it's best to have the semicolon inside/outside the block. Either way is fine and this lint suggests outside the block. + /// Take a look at `semicolon_inside_block` for the other alternative. + /// + /// **Known problems:** None. + /// + /// **Example:** + /// + /// ```rust + /// unsafe { f(x); } + /// ``` + /// Use instead: + /// ```rust + /// unsafe { f(x) }; + /// ``` + pub SEMICOLON_OUTSIDE_BLOCK, + pedantic, + "add a semicolon outside the block" +} + +declare_lint_pass!(SemicolonOutsideBlock => [SEMICOLON_OUTSIDE_BLOCK]); + +impl LateLintPass<'_> for SemicolonOutsideBlock { + 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); + } + + // 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), + _ => (), + } + } + + // 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); + } + } + } +} + +/// 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); + } + + 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(); + + // 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, + ); + } + } +} + +/// Takes a span and extends it until after a semicolon in the last line of the span. +fn expand_span_to_semicolon<'tcx>(cx: &LateContext<'tcx>, expr_span: Span) -> Span { + let expr_span_with_sem = cx.sess().source_map().span_extend_to_next_char(expr_span, ';', false); + expr_span_with_sem.with_hi(expr_span_with_sem.hi().add(BytePos(1))) +} diff --git a/tests/ui/semicolon_inside_block.rs b/tests/ui/semicolon_inside_block.rs new file mode 100644 index 000000000000..0888cd625a25 --- /dev/null +++ b/tests/ui/semicolon_inside_block.rs @@ -0,0 +1,48 @@ +#![warn(clippy::semicolon_inside_block)] + +unsafe fn f(arg: u32) {} + +fn main() { + let x = 32; + + unsafe { f(x) }; +} + +fn get_unit() {} + +fn fooooo() { + unsafe { f(32) } +} + +fn moin() { + println!("Hello") +} + +fn hello() { + get_unit() +} + +fn basic101(x: i32) { + let y: i32; + y = x + 1 +} + +#[rustfmt::skip] +fn closure_error() { + let _d = || { + hello() + }; +} + +fn my_own_block() { + let x: i32; + { + let y = 42; + x = y + 1; + basic101(x) + } + assert_eq!(43, 43) +} + +#[rustfmt::skip] +fn one_line_block() { println!("Foo") } diff --git a/tests/ui/semicolon_inside_block.stderr b/tests/ui/semicolon_inside_block.stderr new file mode 100644 index 000000000000..0e900ba79b1b --- /dev/null +++ b/tests/ui/semicolon_inside_block.stderr @@ -0,0 +1,58 @@ +error: consider moving the `;` inside the block for consistent formatting + --> $DIR/semicolon_inside_block.rs:8:5 + | +LL | unsafe { f(x) }; + | ^^^^^^^^^^^^^^^^ help: put the `;` here: `unsafe { f(x); }` + | + = note: `-D clippy::semicolon-inside-block` implied by `-D warnings` + +error: consider moving the `;` inside the block for consistent formatting + --> $DIR/semicolon_inside_block.rs:14:5 + | +LL | unsafe { f(32) } + | ^^^^^^^^^^^^^^^^ help: put the `;` here: `unsafe { f(32); }` + +error: consider moving the `;` inside the block for consistent formatting + --> $DIR/semicolon_inside_block.rs:18:5 + | +LL | println!("Hello") + | ^^^^^^^^^^^^^^^^^ help: put the `;` here: `println!("Hello");` + +error: consider moving the `;` inside the block for consistent formatting + --> $DIR/semicolon_inside_block.rs:22:5 + | +LL | get_unit() + | ^^^^^^^^^^ help: put the `;` here: `get_unit();` + +error: consider moving the `;` inside the block for consistent formatting + --> $DIR/semicolon_inside_block.rs:27:5 + | +LL | y = x + 1 + | ^^^^^^^^^ help: put the `;` here: `y = x + 1;` + +error: consider moving the `;` inside the block for consistent formatting + --> $DIR/semicolon_inside_block.rs:33:9 + | +LL | hello() + | ^^^^^^^ help: put the `;` here: `hello();` + +error: consider moving the `;` inside the block for consistent formatting + --> $DIR/semicolon_inside_block.rs:44:5 + | +LL | assert_eq!(43, 43) + | ^^^^^^^^^^^^^^^^^^ help: put the `;` here: `assert_eq!(43, 43);` + +error: consider moving the `;` inside the block for consistent formatting + --> $DIR/semicolon_inside_block.rs:42:9 + | +LL | basic101(x) + | ^^^^^^^^^^^ help: put the `;` here: `basic101(x);` + +error: consider moving the `;` inside the block for consistent formatting + --> $DIR/semicolon_inside_block.rs:48:21 + | +LL | fn one_line_block() { println!("Foo") } + | ^^^^^^^^^^^^^^^^^^^ help: put the `;` here: `{ println!("Foo"); }` + +error: aborting due to 9 previous errors + diff --git a/tests/ui/semicolon_outside_block.fixed b/tests/ui/semicolon_outside_block.fixed new file mode 100644 index 000000000000..469ccfac92ab --- /dev/null +++ b/tests/ui/semicolon_outside_block.fixed @@ -0,0 +1,85 @@ +// run-rustfix +#![warn(clippy::semicolon_outside_block)] +#![allow(dead_code)] + +unsafe fn f(_arg: u32) {} + +#[rustfmt::skip] +fn main() { + let x = 32; + + unsafe { f(x) }; +} + +fn foo() { + let x = 32; + + unsafe { + f(x) + }; +} + +fn bar() { + let x = 32; + + unsafe { + let _this = 1; + let _is = 2; + let _a = 3; + let _long = 4; + let _list = 5; + let _of = 6; + let _variables = 7; + f(x) + }; +} + +fn get_unit() {} + +#[rustfmt::skip] +fn closure_error() { + let _d = || { + get_unit() + }; +} + +fn my_own_block() { + let x: i32; + { + let y = 42; + x = y + 1; + get_unit() + }; + assert_eq!(x, 43) +} + +// This is all ok + +fn just_get_unit() { + get_unit(); +} + +fn test_if() { + if 1 > 2 { + get_unit(); + } else { + println!("everything alright!"); + } +} + +fn test_for() { + for _project in &[ + "clippy_workspace_tests", + "clippy_workspace_tests/src", + "clippy_workspace_tests/subcrate", + "clippy_workspace_tests/subcrate/src", + "clippy_dev", + "clippy_lints", + "clippy_utils", + "rustc_tools_util", + ] { + get_unit(); + } + + get_unit(); +} diff --git a/tests/ui/semicolon_outside_block.rs b/tests/ui/semicolon_outside_block.rs new file mode 100644 index 000000000000..e3f4b7a40e6b --- /dev/null +++ b/tests/ui/semicolon_outside_block.rs @@ -0,0 +1,85 @@ +// run-rustfix +#![warn(clippy::semicolon_outside_block)] +#![allow(dead_code)] + +unsafe fn f(_arg: u32) {} + +#[rustfmt::skip] +fn main() { + let x = 32; + + unsafe { f(x); } +} + +fn foo() { + let x = 32; + + unsafe { + f(x); + } +} + +fn bar() { + let x = 32; + + unsafe { + let _this = 1; + let _is = 2; + let _a = 3; + let _long = 4; + let _list = 5; + let _of = 6; + let _variables = 7; + f(x); + }; +} + +fn get_unit() {} + +#[rustfmt::skip] +fn closure_error() { + let _d = || { + get_unit(); + }; +} + +fn my_own_block() { + let x: i32; + { + let y = 42; + x = y + 1; + get_unit(); + } + assert_eq!(x, 43) +} + +// This is all ok + +fn just_get_unit() { + get_unit(); +} + +fn test_if() { + if 1 > 2 { + get_unit(); + } else { + println!("everything alright!"); + } +} + +fn test_for() { + for _project in &[ + "clippy_workspace_tests", + "clippy_workspace_tests/src", + "clippy_workspace_tests/subcrate", + "clippy_workspace_tests/subcrate/src", + "clippy_dev", + "clippy_lints", + "clippy_utils", + "rustc_tools_util", + ] { + get_unit(); + } + + get_unit(); +} diff --git a/tests/ui/semicolon_outside_block.stderr b/tests/ui/semicolon_outside_block.stderr new file mode 100644 index 000000000000..ceabd0da0d2e --- /dev/null +++ b/tests/ui/semicolon_outside_block.stderr @@ -0,0 +1,82 @@ +error: consider moving the `;` outside the block for consistent formatting + --> $DIR/semicolon_outside_block.rs:11:5 + | +LL | unsafe { f(x); } + | ^^^^^^^^^^^^^^^^ help: put the `;` outside the block: `unsafe { f(x) };` + | + = note: `-D clippy::semicolon-outside-block` implied by `-D warnings` + +error: consider moving the `;` outside the block for consistent formatting + --> $DIR/semicolon_outside_block.rs:17:5 + | +LL | / unsafe { +LL | | f(x); +LL | | } + | |_____^ + | +help: put the `;` outside the block + | +LL ~ unsafe { +LL + f(x) +LL + }; + | + +error: consider moving the `;` outside the block for consistent formatting + --> $DIR/semicolon_outside_block.rs:25:5 + | +LL | / unsafe { +LL | | let _this = 1; +LL | | let _is = 2; +LL | | let _a = 3; +... | +LL | | f(x); +LL | | }; + | |______^ + | +help: put the `;` outside the block + | +LL ~ unsafe { +LL + let _this = 1; +LL + let _is = 2; +LL + let _a = 3; +LL + let _long = 4; +LL + let _list = 5; + ... + +error: consider moving the `;` outside the block for consistent formatting + --> $DIR/semicolon_outside_block.rs:41:17 + | +LL | let _d = || { + | _________________^ +LL | | get_unit(); +LL | | }; + | |______^ + | +help: put the `;` outside the block + | +LL ~ let _d = || { +LL + get_unit() +LL + }; + | + +error: consider moving the `;` outside the block for consistent formatting + --> $DIR/semicolon_outside_block.rs:48:5 + | +LL | / { +LL | | let y = 42; +LL | | x = y + 1; +LL | | get_unit(); +LL | | } + | |_____^ + | +help: put the `;` outside the block + | +LL ~ { +LL + let y = 42; +LL + x = y + 1; +LL + get_unit() +LL + }; + | + +error: aborting due to 5 previous errors +