From 93b5c893e6394104b5ae80cf42d7220ad3d18779 Mon Sep 17 00:00:00 2001 From: Lukas Wirth Date: Thu, 10 Nov 2022 13:19:15 +0100 Subject: [PATCH 1/8] Add semicolon-outside/inside-block lints --- CHANGELOG.md | 2 + clippy_lints/src/declared_lints.rs | 2 + clippy_lints/src/lib.rs | 2 + clippy_lints/src/semicolon_block.rs | 131 ++++++++++++++++++++++++ tests/ui/semicolon_inside_block.fixed | 83 +++++++++++++++ tests/ui/semicolon_inside_block.rs | 83 +++++++++++++++ tests/ui/semicolon_inside_block.stderr | 39 +++++++ tests/ui/semicolon_outside_block.fixed | 83 +++++++++++++++ tests/ui/semicolon_outside_block.rs | 83 +++++++++++++++ tests/ui/semicolon_outside_block.stderr | 39 +++++++ 10 files changed, 547 insertions(+) create mode 100644 clippy_lints/src/semicolon_block.rs create mode 100644 tests/ui/semicolon_inside_block.fixed create mode 100644 tests/ui/semicolon_inside_block.rs create mode 100644 tests/ui/semicolon_inside_block.stderr create mode 100644 tests/ui/semicolon_outside_block.fixed create mode 100644 tests/ui/semicolon_outside_block.rs create mode 100644 tests/ui/semicolon_outside_block.stderr diff --git a/CHANGELOG.md b/CHANGELOG.md index 5b6b12c623af..dac4a54b11fd 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4353,6 +4353,8 @@ Released 2018-09-13 [`self_named_constructors`]: https://rust-lang.github.io/rust-clippy/master/index.html#self_named_constructors [`self_named_module_files`]: https://rust-lang.github.io/rust-clippy/master/index.html#self_named_module_files [`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 [`separated_literal_suffix`]: https://rust-lang.github.io/rust-clippy/master/index.html#separated_literal_suffix [`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 diff --git a/clippy_lints/src/declared_lints.rs b/clippy_lints/src/declared_lints.rs index eb3210946f11..76b4a2ccf5e1 100644 --- a/clippy_lints/src/declared_lints.rs +++ b/clippy_lints/src/declared_lints.rs @@ -525,6 +525,8 @@ pub(crate) static LINTS: &[&crate::LintInfo] = &[ crate::returns::NEEDLESS_RETURN_INFO, crate::same_name_method::SAME_NAME_METHOD_INFO, crate::self_named_constructors::SELF_NAMED_CONSTRUCTORS_INFO, + crate::semicolon_block::SEMICOLON_INSIDE_BLOCK_INFO, + crate::semicolon_block::SEMICOLON_OUTSIDE_BLOCK_INFO, crate::semicolon_if_nothing_returned::SEMICOLON_IF_NOTHING_RETURNED_INFO, crate::serde_api::SERDE_API_MISUSE_INFO, crate::shadow::SHADOW_REUSE_INFO, diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index 17dbd983b6c0..cbca317a1463 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -257,6 +257,7 @@ mod return_self_not_must_use; mod returns; mod same_name_method; mod self_named_constructors; +mod semicolon_block; mod semicolon_if_nothing_returned; mod serde_api; mod shadow; @@ -884,6 +885,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: store.register_late_pass(|_| Box::new(from_raw_with_void_ptr::FromRawWithVoidPtr)); store.register_late_pass(|_| Box::new(suspicious_xor_used_as_pow::ConfusingXorAndPow)); store.register_late_pass(move |_| Box::new(manual_is_ascii_check::ManualIsAsciiCheck::new(msrv()))); + store.register_late_pass(|_| Box::new(semicolon_block::SemicolonBlock)); // add lints here, do not remove this comment, it's used in `new_lint` } diff --git a/clippy_lints/src/semicolon_block.rs b/clippy_lints/src/semicolon_block.rs new file mode 100644 index 000000000000..911d38f65d6c --- /dev/null +++ b/clippy_lints/src/semicolon_block.rs @@ -0,0 +1,131 @@ +use clippy_utils::diagnostics::span_lint_and_sugg; +use clippy_utils::source::snippet_with_macro_callsite; +use clippy_utils::{get_parent_expr_for_hir, get_parent_node}; +use rustc_errors::Applicability; +use rustc_hir::{Block, Expr, ExprKind, Node, Stmt, StmtKind}; +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. + /// + /// ### Example + /// + /// ```rust + /// # fn f(_: u32) {} + /// # let x = 0; + /// unsafe { f(x) }; + /// ``` + /// Use instead: + /// ```rust + /// # fn f(_: u32) {} + /// # let x = 0; + /// unsafe { f(x); } + /// ``` + #[clippy::version = "1.66.0"] + pub SEMICOLON_INSIDE_BLOCK, + restriction, + "add a semicolon inside the block" +} +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. + /// + /// ### Example + /// + /// ```rust + /// # fn f(_: u32) {} + /// # let x = 0; + /// unsafe { f(x); } + /// ``` + /// Use instead: + /// ```rust + /// # fn f(_: u32) {} + /// # let x = 0; + /// unsafe { f(x) }; + /// ``` + #[clippy::version = "1.66.0"] + pub SEMICOLON_OUTSIDE_BLOCK, + restriction, + "add a semicolon outside the block" +} +declare_lint_pass!(SemicolonBlock => [SEMICOLON_INSIDE_BLOCK, SEMICOLON_OUTSIDE_BLOCK]); + +impl LateLintPass<'_> for SemicolonBlock { + fn check_block(&mut self, cx: &LateContext<'_>, block: &Block<'_>) { + semicolon_inside_block(cx, block); + semicolon_outside_block(cx, block); + } +} + +fn semicolon_inside_block(cx: &LateContext<'_>, block: &Block<'_>) { + if !block.span.from_expansion() + && let Some(tail) = block.expr + && let Some(block_expr @ Expr { kind: ExprKind::Block(_, _), ..}) = get_parent_expr_for_hir(cx, block.hir_id) + && let Some(Node::Stmt(Stmt { kind: StmtKind::Semi(_), span, .. })) = get_parent_node(cx.tcx, block_expr.hir_id) + { + let expr_snip = snippet_with_macro_callsite(cx, tail.span, ".."); + + let mut suggestion: String = snippet_with_macro_callsite(cx, block.span, "..").to_string(); + + if let Some((expr_offset, _)) = suggestion.rmatch_indices(&*expr_snip).next() { + suggestion.insert(expr_offset + expr_snip.len(), ';'); + } else { + return; + } + + span_lint_and_sugg( + cx, + SEMICOLON_INSIDE_BLOCK, + *span, + "consider moving the `;` inside the block for consistent formatting", + "put the `;` here", + suggestion, + Applicability::MaybeIncorrect, + ); + } +} + +fn semicolon_outside_block(cx: &LateContext<'_>, block: &Block<'_>) { + if !block.span.from_expansion() + && block.expr.is_none() + && let [.., Stmt { kind: StmtKind::Semi(expr), .. }] = block.stmts + && let Some(block_expr @ Expr { kind: ExprKind::Block(_, _), ..}) = get_parent_expr_for_hir(cx,block.hir_id) + && let Some(Node::Stmt(Stmt { kind: StmtKind::Expr(_), .. })) = get_parent_node(cx.tcx, block_expr.hir_id) { + let expr_snip = snippet_with_macro_callsite(cx, expr.span, ".."); + + let mut suggestion: String = snippet_with_macro_callsite(cx, block.span, "..").to_string(); + + if let Some((expr_offset, _)) = suggestion.rmatch_indices(&*expr_snip).next() + && let Some(semi_offset) = suggestion[expr_offset + expr_snip.len()..].find(';') { + suggestion.remove(expr_offset + expr_snip.len() + semi_offset); + } else { + return; + } + + suggestion.push(';'); + + span_lint_and_sugg( + cx, + SEMICOLON_OUTSIDE_BLOCK, + block.span, + "consider moving the `;` outside the block for consistent formatting", + "put the `;` outside the block", + suggestion, + Applicability::MaybeIncorrect, + ); + } +} diff --git a/tests/ui/semicolon_inside_block.fixed b/tests/ui/semicolon_inside_block.fixed new file mode 100644 index 000000000000..4cd112dd5e12 --- /dev/null +++ b/tests/ui/semicolon_inside_block.fixed @@ -0,0 +1,83 @@ +// run-rustfix +#![allow( + unused, + clippy::unused_unit, + clippy::unnecessary_operation, + clippy::no_effect, + clippy::single_element_loop +)] +#![warn(clippy::semicolon_inside_block)] + +macro_rules! m { + (()) => { + () + }; + (0) => {{ + 0 + };}; + (1) => {{ + 1; + }}; + (2) => {{ + 2; + }}; +} + +fn unit_fn_block() { + () +} + +#[rustfmt::skip] +fn main() { + { unit_fn_block() } + unsafe { unit_fn_block() } + + { + unit_fn_block() + } + + { unit_fn_block(); } + unsafe { unit_fn_block(); } + + { unit_fn_block(); } + unsafe { unit_fn_block(); } + + { unit_fn_block(); }; + unsafe { unit_fn_block(); }; + + { + unit_fn_block(); + unit_fn_block(); + } + { + unit_fn_block(); + unit_fn_block(); + } + { + unit_fn_block(); + unit_fn_block(); + }; + + { m!(()); } + { m!(()); } + { m!(()); }; + m!(0); + m!(1); + m!(2); + + for _ in [()] { + unit_fn_block(); + } + for _ in [()] { + unit_fn_block() + } + + let _d = || { + unit_fn_block(); + }; + let _d = || { + unit_fn_block() + }; + + unit_fn_block() +} diff --git a/tests/ui/semicolon_inside_block.rs b/tests/ui/semicolon_inside_block.rs new file mode 100644 index 000000000000..7512125c051d --- /dev/null +++ b/tests/ui/semicolon_inside_block.rs @@ -0,0 +1,83 @@ +// run-rustfix +#![allow( + unused, + clippy::unused_unit, + clippy::unnecessary_operation, + clippy::no_effect, + clippy::single_element_loop +)] +#![warn(clippy::semicolon_inside_block)] + +macro_rules! m { + (()) => { + () + }; + (0) => {{ + 0 + };}; + (1) => {{ + 1; + }}; + (2) => {{ + 2; + }}; +} + +fn unit_fn_block() { + () +} + +#[rustfmt::skip] +fn main() { + { unit_fn_block() } + unsafe { unit_fn_block() } + + { + unit_fn_block() + } + + { unit_fn_block() }; + unsafe { unit_fn_block() }; + + { unit_fn_block(); } + unsafe { unit_fn_block(); } + + { unit_fn_block(); }; + unsafe { unit_fn_block(); }; + + { + unit_fn_block(); + unit_fn_block() + }; + { + unit_fn_block(); + unit_fn_block(); + } + { + unit_fn_block(); + unit_fn_block(); + }; + + { m!(()) }; + { m!(()); } + { m!(()); }; + m!(0); + m!(1); + m!(2); + + for _ in [()] { + unit_fn_block(); + } + for _ in [()] { + unit_fn_block() + } + + let _d = || { + unit_fn_block(); + }; + let _d = || { + unit_fn_block() + }; + + unit_fn_block() +} diff --git a/tests/ui/semicolon_inside_block.stderr b/tests/ui/semicolon_inside_block.stderr new file mode 100644 index 000000000000..febe74b49bda --- /dev/null +++ b/tests/ui/semicolon_inside_block.stderr @@ -0,0 +1,39 @@ +error: consider moving the `;` inside the block for consistent formatting + --> $DIR/semicolon_inside_block.rs:39:5 + | +LL | { unit_fn_block() }; + | ^^^^^^^^^^^^^^^^^^^^ help: put the `;` here: `{ unit_fn_block(); }` + | + = 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:40:5 + | +LL | unsafe { unit_fn_block() }; + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: put the `;` here: `unsafe { unit_fn_block(); }` + +error: consider moving the `;` inside the block for consistent formatting + --> $DIR/semicolon_inside_block.rs:48:5 + | +LL | / { +LL | | unit_fn_block(); +LL | | unit_fn_block() +LL | | }; + | |______^ + | +help: put the `;` here + | +LL ~ { +LL + unit_fn_block(); +LL + unit_fn_block(); +LL + } + | + +error: consider moving the `;` inside the block for consistent formatting + --> $DIR/semicolon_inside_block.rs:61:5 + | +LL | { m!(()) }; + | ^^^^^^^^^^^ help: put the `;` here: `{ m!(()); }` + +error: aborting due to 4 previous errors + diff --git a/tests/ui/semicolon_outside_block.fixed b/tests/ui/semicolon_outside_block.fixed new file mode 100644 index 000000000000..5bc18faaad8e --- /dev/null +++ b/tests/ui/semicolon_outside_block.fixed @@ -0,0 +1,83 @@ +// run-rustfix +#![allow( + unused, + clippy::unused_unit, + clippy::unnecessary_operation, + clippy::no_effect, + clippy::single_element_loop +)] +#![warn(clippy::semicolon_outside_block)] + +macro_rules! m { + (()) => { + () + }; + (0) => {{ + 0 + };}; + (1) => {{ + 1; + }}; + (2) => {{ + 2; + }}; +} + +fn unit_fn_block() { + () +} + +#[rustfmt::skip] +fn main() { + { unit_fn_block() } + unsafe { unit_fn_block() } + + { + unit_fn_block() + } + + { unit_fn_block() }; + unsafe { unit_fn_block() }; + + { unit_fn_block() }; + unsafe { unit_fn_block() }; + + { unit_fn_block(); }; + unsafe { unit_fn_block(); }; + + { + unit_fn_block(); + unit_fn_block() + }; + { + unit_fn_block(); + unit_fn_block() + }; + { + unit_fn_block(); + unit_fn_block(); + }; + + { m!(()) }; + { m!(()) }; + { m!(()); }; + m!(0); + m!(1); + m!(2); + + for _ in [()] { + unit_fn_block(); + } + for _ in [()] { + unit_fn_block() + } + + let _d = || { + unit_fn_block(); + }; + let _d = || { + unit_fn_block() + }; + + unit_fn_block() +} diff --git a/tests/ui/semicolon_outside_block.rs b/tests/ui/semicolon_outside_block.rs new file mode 100644 index 000000000000..0a4293238763 --- /dev/null +++ b/tests/ui/semicolon_outside_block.rs @@ -0,0 +1,83 @@ +// run-rustfix +#![allow( + unused, + clippy::unused_unit, + clippy::unnecessary_operation, + clippy::no_effect, + clippy::single_element_loop +)] +#![warn(clippy::semicolon_outside_block)] + +macro_rules! m { + (()) => { + () + }; + (0) => {{ + 0 + };}; + (1) => {{ + 1; + }}; + (2) => {{ + 2; + }}; +} + +fn unit_fn_block() { + () +} + +#[rustfmt::skip] +fn main() { + { unit_fn_block() } + unsafe { unit_fn_block() } + + { + unit_fn_block() + } + + { unit_fn_block() }; + unsafe { unit_fn_block() }; + + { unit_fn_block(); } + unsafe { unit_fn_block(); } + + { unit_fn_block(); }; + unsafe { unit_fn_block(); }; + + { + unit_fn_block(); + unit_fn_block() + }; + { + unit_fn_block(); + unit_fn_block(); + } + { + unit_fn_block(); + unit_fn_block(); + }; + + { m!(()) }; + { m!(()); } + { m!(()); }; + m!(0); + m!(1); + m!(2); + + for _ in [()] { + unit_fn_block(); + } + for _ in [()] { + unit_fn_block() + } + + let _d = || { + unit_fn_block(); + }; + let _d = || { + unit_fn_block() + }; + + unit_fn_block() +} diff --git a/tests/ui/semicolon_outside_block.stderr b/tests/ui/semicolon_outside_block.stderr new file mode 100644 index 000000000000..fddf51208130 --- /dev/null +++ b/tests/ui/semicolon_outside_block.stderr @@ -0,0 +1,39 @@ +error: consider moving the `;` outside the block for consistent formatting + --> $DIR/semicolon_outside_block.rs:42:5 + | +LL | { unit_fn_block(); } + | ^^^^^^^^^^^^^^^^^^^^ help: put the `;` outside the block: `{ unit_fn_block() };` + | + = 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:43:5 + | +LL | unsafe { unit_fn_block(); } + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: put the `;` outside the block: `unsafe { unit_fn_block() };` + +error: consider moving the `;` outside the block for consistent formatting + --> $DIR/semicolon_outside_block.rs:52:5 + | +LL | / { +LL | | unit_fn_block(); +LL | | unit_fn_block(); +LL | | } + | |_____^ + | +help: put the `;` outside the block + | +LL ~ { +LL + unit_fn_block(); +LL + unit_fn_block() +LL + }; + | + +error: consider moving the `;` outside the block for consistent formatting + --> $DIR/semicolon_outside_block.rs:62:5 + | +LL | { m!(()); } + | ^^^^^^^^^^^ help: put the `;` outside the block: `{ m!(()) };` + +error: aborting due to 4 previous errors + From ba951e3ca72283d39757bd39d20bfd75ce901147 Mon Sep 17 00:00:00 2001 From: Lukas Wirth Date: Tue, 15 Nov 2022 18:29:43 +0100 Subject: [PATCH 2/8] Fix formatting of let chains --- clippy_lints/src/semicolon_block.rs | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-) diff --git a/clippy_lints/src/semicolon_block.rs b/clippy_lints/src/semicolon_block.rs index 911d38f65d6c..a41f89a6aa33 100644 --- a/clippy_lints/src/semicolon_block.rs +++ b/clippy_lints/src/semicolon_block.rs @@ -73,9 +73,9 @@ impl LateLintPass<'_> for SemicolonBlock { fn semicolon_inside_block(cx: &LateContext<'_>, block: &Block<'_>) { if !block.span.from_expansion() - && let Some(tail) = block.expr - && let Some(block_expr @ Expr { kind: ExprKind::Block(_, _), ..}) = get_parent_expr_for_hir(cx, block.hir_id) - && let Some(Node::Stmt(Stmt { kind: StmtKind::Semi(_), span, .. })) = get_parent_node(cx.tcx, block_expr.hir_id) + && let Some(tail) = block.expr + && let Some(block_expr @ Expr { kind: ExprKind::Block(_, _), ..}) = get_parent_expr_for_hir(cx, block.hir_id) + && let Some(Node::Stmt(Stmt { kind: StmtKind::Semi(_), span, .. })) = get_parent_node(cx.tcx, block_expr.hir_id) { let expr_snip = snippet_with_macro_callsite(cx, tail.span, ".."); @@ -101,16 +101,18 @@ fn semicolon_inside_block(cx: &LateContext<'_>, block: &Block<'_>) { fn semicolon_outside_block(cx: &LateContext<'_>, block: &Block<'_>) { if !block.span.from_expansion() - && block.expr.is_none() - && let [.., Stmt { kind: StmtKind::Semi(expr), .. }] = block.stmts - && let Some(block_expr @ Expr { kind: ExprKind::Block(_, _), ..}) = get_parent_expr_for_hir(cx,block.hir_id) - && let Some(Node::Stmt(Stmt { kind: StmtKind::Expr(_), .. })) = get_parent_node(cx.tcx, block_expr.hir_id) { + && block.expr.is_none() + && let [.., Stmt { kind: StmtKind::Semi(expr), .. }] = block.stmts + && let Some(block_expr @ Expr { kind: ExprKind::Block(_, _), ..}) = get_parent_expr_for_hir(cx,block.hir_id) + && let Some(Node::Stmt(Stmt { kind: StmtKind::Expr(_), .. })) = get_parent_node(cx.tcx, block_expr.hir_id) + { let expr_snip = snippet_with_macro_callsite(cx, expr.span, ".."); let mut suggestion: String = snippet_with_macro_callsite(cx, block.span, "..").to_string(); if let Some((expr_offset, _)) = suggestion.rmatch_indices(&*expr_snip).next() - && let Some(semi_offset) = suggestion[expr_offset + expr_snip.len()..].find(';') { + && let Some(semi_offset) = suggestion[expr_offset + expr_snip.len()..].find(';') + { suggestion.remove(expr_offset + expr_snip.len() + semi_offset); } else { return; From 23744cd4ba57d2fc5c57459002282fe2d896193b Mon Sep 17 00:00:00 2001 From: Lukas Wirth Date: Tue, 15 Nov 2022 19:22:00 +0100 Subject: [PATCH 3/8] Use multi-span suggestions --- clippy_lints/src/semicolon_block.rs | 127 ++++++++++++------------ tests/ui/semicolon_inside_block.fixed | 4 +- tests/ui/semicolon_inside_block.stderr | 32 ++++-- tests/ui/semicolon_outside_block.fixed | 14 ++- tests/ui/semicolon_outside_block.stderr | 31 ++++-- 5 files changed, 124 insertions(+), 84 deletions(-) diff --git a/clippy_lints/src/semicolon_block.rs b/clippy_lints/src/semicolon_block.rs index a41f89a6aa33..34bf97448273 100644 --- a/clippy_lints/src/semicolon_block.rs +++ b/clippy_lints/src/semicolon_block.rs @@ -1,10 +1,9 @@ -use clippy_utils::diagnostics::span_lint_and_sugg; -use clippy_utils::source::snippet_with_macro_callsite; -use clippy_utils::{get_parent_expr_for_hir, get_parent_node}; +use clippy_utils::diagnostics::{multispan_sugg_with_applicability, span_lint_and_then}; use rustc_errors::Applicability; -use rustc_hir::{Block, Expr, ExprKind, Node, Stmt, StmtKind}; +use rustc_hir::{Block, Expr, ExprKind, Stmt, StmtKind}; use rustc_lint::{LateContext, LateLintPass}; use rustc_session::{declare_lint_pass, declare_tool_lint}; +use rustc_span::Span; declare_clippy_lint! { /// ### What it does @@ -65,69 +64,73 @@ declare_clippy_lint! { declare_lint_pass!(SemicolonBlock => [SEMICOLON_INSIDE_BLOCK, SEMICOLON_OUTSIDE_BLOCK]); impl LateLintPass<'_> for SemicolonBlock { - fn check_block(&mut self, cx: &LateContext<'_>, block: &Block<'_>) { - semicolon_inside_block(cx, block); - semicolon_outside_block(cx, block); - } -} - -fn semicolon_inside_block(cx: &LateContext<'_>, block: &Block<'_>) { - if !block.span.from_expansion() - && let Some(tail) = block.expr - && let Some(block_expr @ Expr { kind: ExprKind::Block(_, _), ..}) = get_parent_expr_for_hir(cx, block.hir_id) - && let Some(Node::Stmt(Stmt { kind: StmtKind::Semi(_), span, .. })) = get_parent_node(cx.tcx, block_expr.hir_id) - { - let expr_snip = snippet_with_macro_callsite(cx, tail.span, ".."); - - let mut suggestion: String = snippet_with_macro_callsite(cx, block.span, "..").to_string(); - - if let Some((expr_offset, _)) = suggestion.rmatch_indices(&*expr_snip).next() { - suggestion.insert(expr_offset + expr_snip.len(), ';'); - } else { - return; + fn check_stmt(&mut self, cx: &LateContext<'_>, stmt: &Stmt<'_>) { + match stmt.kind { + StmtKind::Expr(Expr { + kind: + ExprKind::Block( + block @ Block { + expr: None, + stmts: + &[ + .., + Stmt { + kind: StmtKind::Semi(expr), + span, + .. + }, + ], + .. + }, + _, + ), + .. + }) if !block.span.from_expansion() => semicolon_outside_block(cx, block, expr, span), + StmtKind::Semi(Expr { + kind: ExprKind::Block(block @ Block { expr: Some(tail), .. }, _), + .. + }) if !block.span.from_expansion() => semicolon_inside_block(cx, block, tail, stmt.span), + _ => (), } - - span_lint_and_sugg( - cx, - SEMICOLON_INSIDE_BLOCK, - *span, - "consider moving the `;` inside the block for consistent formatting", - "put the `;` here", - suggestion, - Applicability::MaybeIncorrect, - ); } } -fn semicolon_outside_block(cx: &LateContext<'_>, block: &Block<'_>) { - if !block.span.from_expansion() - && block.expr.is_none() - && let [.., Stmt { kind: StmtKind::Semi(expr), .. }] = block.stmts - && let Some(block_expr @ Expr { kind: ExprKind::Block(_, _), ..}) = get_parent_expr_for_hir(cx,block.hir_id) - && let Some(Node::Stmt(Stmt { kind: StmtKind::Expr(_), .. })) = get_parent_node(cx.tcx, block_expr.hir_id) - { - let expr_snip = snippet_with_macro_callsite(cx, expr.span, ".."); - - let mut suggestion: String = snippet_with_macro_callsite(cx, block.span, "..").to_string(); +fn semicolon_inside_block(cx: &LateContext<'_>, block: &Block<'_>, tail: &Expr<'_>, semi_span: Span) { + let insert_span = tail.span.with_lo(tail.span.hi()); + let remove_span = semi_span.with_lo(block.span.hi()); - if let Some((expr_offset, _)) = suggestion.rmatch_indices(&*expr_snip).next() - && let Some(semi_offset) = suggestion[expr_offset + expr_snip.len()..].find(';') - { - suggestion.remove(expr_offset + expr_snip.len() + semi_offset); - } else { - return; - } + span_lint_and_then( + cx, + SEMICOLON_INSIDE_BLOCK, + semi_span, + "consider moving the `;` inside the block for consistent formatting", + |diag| { + multispan_sugg_with_applicability( + diag, + "put the `;` here", + Applicability::MachineApplicable, + [(remove_span, String::new()), (insert_span, ";".to_owned())], + ); + }, + ); +} - suggestion.push(';'); +fn semicolon_outside_block(cx: &LateContext<'_>, block: &Block<'_>, tail_stmt_expr: &Expr<'_>, semi_span: Span) { + let insert_span = block.span.with_lo(block.span.hi()); + let remove_span = semi_span.with_lo(tail_stmt_expr.span.source_callsite().hi()); - span_lint_and_sugg( - cx, - SEMICOLON_OUTSIDE_BLOCK, - block.span, - "consider moving the `;` outside the block for consistent formatting", - "put the `;` outside the block", - suggestion, - Applicability::MaybeIncorrect, - ); - } + span_lint_and_then( + cx, + SEMICOLON_OUTSIDE_BLOCK, + block.span, + "consider moving the `;` outside the block for consistent formatting", + |diag| { + multispan_sugg_with_applicability( + diag, + "put the `;` here", + Applicability::MachineApplicable, + [(remove_span, String::new()), (insert_span, ";".to_owned())], + ); + }, + ); } diff --git a/tests/ui/semicolon_inside_block.fixed b/tests/ui/semicolon_inside_block.fixed index 4cd112dd5e12..d2cd8b218829 100644 --- a/tests/ui/semicolon_inside_block.fixed +++ b/tests/ui/semicolon_inside_block.fixed @@ -10,7 +10,7 @@ macro_rules! m { (()) => { - () + (); }; (0) => {{ 0 @@ -58,7 +58,7 @@ fn main() { unit_fn_block(); }; - { m!(()); } + { m!(()) } { m!(()); } { m!(()); }; m!(0); diff --git a/tests/ui/semicolon_inside_block.stderr b/tests/ui/semicolon_inside_block.stderr index febe74b49bda..99f3b65be0f8 100644 --- a/tests/ui/semicolon_inside_block.stderr +++ b/tests/ui/semicolon_inside_block.stderr @@ -2,15 +2,26 @@ error: consider moving the `;` inside the block for consistent formatting --> $DIR/semicolon_inside_block.rs:39:5 | LL | { unit_fn_block() }; - | ^^^^^^^^^^^^^^^^^^^^ help: put the `;` here: `{ unit_fn_block(); }` + | ^^^^^^^^^^^^^^^^^^^^ | = note: `-D clippy::semicolon-inside-block` implied by `-D warnings` +help: put the `;` here + | +LL - { unit_fn_block() }; +LL + { unit_fn_block(); } + | error: consider moving the `;` inside the block for consistent formatting --> $DIR/semicolon_inside_block.rs:40:5 | LL | unsafe { unit_fn_block() }; - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: put the `;` here: `unsafe { unit_fn_block(); }` + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | +help: put the `;` here + | +LL - unsafe { unit_fn_block() }; +LL + unsafe { unit_fn_block(); } + | error: consider moving the `;` inside the block for consistent formatting --> $DIR/semicolon_inside_block.rs:48:5 @@ -23,17 +34,24 @@ LL | | }; | help: put the `;` here | -LL ~ { -LL + unit_fn_block(); -LL + unit_fn_block(); -LL + } +LL ~ unit_fn_block(); +LL ~ } | error: consider moving the `;` inside the block for consistent formatting --> $DIR/semicolon_inside_block.rs:61:5 | LL | { m!(()) }; - | ^^^^^^^^^^^ help: put the `;` here: `{ m!(()); }` + | ^^^^^^^^^^^ + | +help: put the `;` here + | +LL ~ (); +LL | }; + ... +LL | +LL ~ { m!(()) } + | error: aborting due to 4 previous errors diff --git a/tests/ui/semicolon_outside_block.fixed b/tests/ui/semicolon_outside_block.fixed index 5bc18faaad8e..35eacfe6d3f1 100644 --- a/tests/ui/semicolon_outside_block.fixed +++ b/tests/ui/semicolon_outside_block.fixed @@ -21,6 +21,9 @@ macro_rules! m { (2) => {{ 2; }}; + (stmt) => { + stmt; + }; } fn unit_fn_block() { @@ -39,8 +42,8 @@ fn main() { { unit_fn_block() }; unsafe { unit_fn_block() }; - { unit_fn_block() }; - unsafe { unit_fn_block() }; + { unit_fn_block(); } + unsafe { unit_fn_block(); } { unit_fn_block(); }; unsafe { unit_fn_block(); }; @@ -51,19 +54,20 @@ fn main() { }; { unit_fn_block(); - unit_fn_block() - }; + unit_fn_block(); + } { unit_fn_block(); unit_fn_block(); }; { m!(()) }; - { m!(()) }; + { m!(()); } { m!(()); }; m!(0); m!(1); m!(2); + { m!(stmt) }; for _ in [()] { unit_fn_block(); diff --git a/tests/ui/semicolon_outside_block.stderr b/tests/ui/semicolon_outside_block.stderr index fddf51208130..c2151f7af637 100644 --- a/tests/ui/semicolon_outside_block.stderr +++ b/tests/ui/semicolon_outside_block.stderr @@ -2,15 +2,26 @@ error: consider moving the `;` outside the block for consistent formatting --> $DIR/semicolon_outside_block.rs:42:5 | LL | { unit_fn_block(); } - | ^^^^^^^^^^^^^^^^^^^^ help: put the `;` outside the block: `{ unit_fn_block() };` + | ^^^^^^^^^^^^^^^^^^^^ | = note: `-D clippy::semicolon-outside-block` implied by `-D warnings` +help: put the `;` here + | +LL - { unit_fn_block(); } +LL + { unit_fn_block() }; + | error: consider moving the `;` outside the block for consistent formatting --> $DIR/semicolon_outside_block.rs:43:5 | LL | unsafe { unit_fn_block(); } - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: put the `;` outside the block: `unsafe { unit_fn_block() };` + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | +help: put the `;` here + | +LL - unsafe { unit_fn_block(); } +LL + unsafe { unit_fn_block() }; + | error: consider moving the `;` outside the block for consistent formatting --> $DIR/semicolon_outside_block.rs:52:5 @@ -21,19 +32,23 @@ LL | | unit_fn_block(); LL | | } | |_____^ | -help: put the `;` outside the block +help: put the `;` here | -LL ~ { -LL + unit_fn_block(); -LL + unit_fn_block() -LL + }; +LL ~ unit_fn_block() +LL ~ }; | error: consider moving the `;` outside the block for consistent formatting --> $DIR/semicolon_outside_block.rs:62:5 | LL | { m!(()); } - | ^^^^^^^^^^^ help: put the `;` outside the block: `{ m!(()) };` + | ^^^^^^^^^^^ + | +help: put the `;` here + | +LL - () +LL + (); }; + | error: aborting due to 4 previous errors From dd1163f4611d8597d03d2efd569d63a1d82bfd15 Mon Sep 17 00:00:00 2001 From: Lukas Wirth Date: Wed, 16 Nov 2022 14:30:53 +0100 Subject: [PATCH 4/8] Fix macro statement handling --- clippy_lints/src/semicolon_block.rs | 7 +++++-- tests/ui/semicolon_inside_block.fixed | 4 ++-- tests/ui/semicolon_inside_block.stderr | 7 ++----- tests/ui/semicolon_outside_block.fixed | 14 +++++--------- tests/ui/semicolon_outside_block.stderr | 4 ++-- 5 files changed, 16 insertions(+), 20 deletions(-) diff --git a/clippy_lints/src/semicolon_block.rs b/clippy_lints/src/semicolon_block.rs index 34bf97448273..57f835d45b12 100644 --- a/clippy_lints/src/semicolon_block.rs +++ b/clippy_lints/src/semicolon_block.rs @@ -1,7 +1,7 @@ use clippy_utils::diagnostics::{multispan_sugg_with_applicability, span_lint_and_then}; use rustc_errors::Applicability; use rustc_hir::{Block, Expr, ExprKind, Stmt, StmtKind}; -use rustc_lint::{LateContext, LateLintPass}; +use rustc_lint::{LateContext, LateLintPass, LintContext}; use rustc_session::{declare_lint_pass, declare_tool_lint}; use rustc_span::Span; @@ -96,7 +96,8 @@ impl LateLintPass<'_> for SemicolonBlock { } fn semicolon_inside_block(cx: &LateContext<'_>, block: &Block<'_>, tail: &Expr<'_>, semi_span: Span) { - let insert_span = tail.span.with_lo(tail.span.hi()); + let tail_span_end = tail.span.source_callsite().hi(); + let insert_span = Span::with_root_ctxt(tail_span_end, tail_span_end); let remove_span = semi_span.with_lo(block.span.hi()); span_lint_and_then( @@ -117,6 +118,8 @@ fn semicolon_inside_block(cx: &LateContext<'_>, block: &Block<'_>, tail: &Expr<' fn semicolon_outside_block(cx: &LateContext<'_>, block: &Block<'_>, tail_stmt_expr: &Expr<'_>, semi_span: Span) { let insert_span = block.span.with_lo(block.span.hi()); + // account for macro calls + let semi_span = cx.sess().source_map().stmt_span(semi_span, block.span); let remove_span = semi_span.with_lo(tail_stmt_expr.span.source_callsite().hi()); span_lint_and_then( diff --git a/tests/ui/semicolon_inside_block.fixed b/tests/ui/semicolon_inside_block.fixed index d2cd8b218829..4cd112dd5e12 100644 --- a/tests/ui/semicolon_inside_block.fixed +++ b/tests/ui/semicolon_inside_block.fixed @@ -10,7 +10,7 @@ macro_rules! m { (()) => { - (); + () }; (0) => {{ 0 @@ -58,7 +58,7 @@ fn main() { unit_fn_block(); }; - { m!(()) } + { m!(()); } { m!(()); } { m!(()); }; m!(0); diff --git a/tests/ui/semicolon_inside_block.stderr b/tests/ui/semicolon_inside_block.stderr index 99f3b65be0f8..48d3690e2bde 100644 --- a/tests/ui/semicolon_inside_block.stderr +++ b/tests/ui/semicolon_inside_block.stderr @@ -46,11 +46,8 @@ LL | { m!(()) }; | help: put the `;` here | -LL ~ (); -LL | }; - ... -LL | -LL ~ { m!(()) } +LL - { m!(()) }; +LL + { m!(()); } | error: aborting due to 4 previous errors diff --git a/tests/ui/semicolon_outside_block.fixed b/tests/ui/semicolon_outside_block.fixed index 35eacfe6d3f1..5bc18faaad8e 100644 --- a/tests/ui/semicolon_outside_block.fixed +++ b/tests/ui/semicolon_outside_block.fixed @@ -21,9 +21,6 @@ macro_rules! m { (2) => {{ 2; }}; - (stmt) => { - stmt; - }; } fn unit_fn_block() { @@ -42,8 +39,8 @@ fn main() { { unit_fn_block() }; unsafe { unit_fn_block() }; - { unit_fn_block(); } - unsafe { unit_fn_block(); } + { unit_fn_block() }; + unsafe { unit_fn_block() }; { unit_fn_block(); }; unsafe { unit_fn_block(); }; @@ -54,20 +51,19 @@ fn main() { }; { unit_fn_block(); - unit_fn_block(); - } + unit_fn_block() + }; { unit_fn_block(); unit_fn_block(); }; { m!(()) }; - { m!(()); } + { m!(()) }; { m!(()); }; m!(0); m!(1); m!(2); - { m!(stmt) }; for _ in [()] { unit_fn_block(); diff --git a/tests/ui/semicolon_outside_block.stderr b/tests/ui/semicolon_outside_block.stderr index c2151f7af637..dcc102e60994 100644 --- a/tests/ui/semicolon_outside_block.stderr +++ b/tests/ui/semicolon_outside_block.stderr @@ -46,8 +46,8 @@ LL | { m!(()); } | help: put the `;` here | -LL - () -LL + (); }; +LL - { m!(()); } +LL + { m!(()) }; | error: aborting due to 4 previous errors From ab4154697512ec076c4c5550d218d46638353eb9 Mon Sep 17 00:00:00 2001 From: Lukas Wirth Date: Thu, 24 Nov 2022 09:54:09 +0100 Subject: [PATCH 5/8] Address reviews --- clippy_lints/src/semicolon_block.rs | 35 +++++++++++++---------------- 1 file changed, 15 insertions(+), 20 deletions(-) diff --git a/clippy_lints/src/semicolon_block.rs b/clippy_lints/src/semicolon_block.rs index 57f835d45b12..d06f21c89c4a 100644 --- a/clippy_lints/src/semicolon_block.rs +++ b/clippy_lints/src/semicolon_block.rs @@ -67,25 +67,21 @@ impl LateLintPass<'_> for SemicolonBlock { fn check_stmt(&mut self, cx: &LateContext<'_>, stmt: &Stmt<'_>) { match stmt.kind { StmtKind::Expr(Expr { - kind: - ExprKind::Block( - block @ Block { - expr: None, - stmts: - &[ - .., - Stmt { - kind: StmtKind::Semi(expr), - span, - .. - }, - ], - .. - }, - _, - ), + kind: ExprKind::Block(block, _), .. - }) if !block.span.from_expansion() => semicolon_outside_block(cx, block, expr, span), + }) if !block.span.from_expansion() => { + let Block { + expr: None, + stmts: [.., stmt], + .. + } = block else { return }; + let &Stmt { + kind: StmtKind::Semi(expr), + span, + .. + } = stmt else { return }; + semicolon_outside_block(cx, block, expr, span) + }, StmtKind::Semi(Expr { kind: ExprKind::Block(block @ Block { expr: Some(tail), .. }, _), .. @@ -96,8 +92,7 @@ impl LateLintPass<'_> for SemicolonBlock { } fn semicolon_inside_block(cx: &LateContext<'_>, block: &Block<'_>, tail: &Expr<'_>, semi_span: Span) { - let tail_span_end = tail.span.source_callsite().hi(); - let insert_span = Span::with_root_ctxt(tail_span_end, tail_span_end); + let insert_span = tail.span.source_callsite().shrink_to_hi(); let remove_span = semi_span.with_lo(block.span.hi()); span_lint_and_then( From f585414cd2fc635fecfe0437f9f71b08aba51d16 Mon Sep 17 00:00:00 2001 From: Lukas Wirth Date: Thu, 24 Nov 2022 20:37:07 +0100 Subject: [PATCH 6/8] Adjust semicolon block lint descriptions --- clippy_lints/src/semicolon_block.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/clippy_lints/src/semicolon_block.rs b/clippy_lints/src/semicolon_block.rs index d06f21c89c4a..6861b3709394 100644 --- a/clippy_lints/src/semicolon_block.rs +++ b/clippy_lints/src/semicolon_block.rs @@ -8,7 +8,7 @@ use rustc_span::Span; declare_clippy_lint! { /// ### What it does /// - /// For () returning expressions, check that the semicolon is inside the block. + /// Checks for semicolon terminated blocks containing only a single expression. /// /// ### Why is this bad? /// @@ -36,7 +36,7 @@ declare_clippy_lint! { declare_clippy_lint! { /// ### What it does /// - /// For () returning expressions, check that the semicolon is outside the block. + /// Checks for blocks containing only a single semicolon terminated statement. /// /// ### Why is this bad? /// From f62eab43123ac463d7ff95590c285cb30e86b1f7 Mon Sep 17 00:00:00 2001 From: Lukas Wirth Date: Fri, 25 Nov 2022 17:10:05 +0100 Subject: [PATCH 7/8] Adjust description once more --- clippy_lints/src/semicolon_block.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/clippy_lints/src/semicolon_block.rs b/clippy_lints/src/semicolon_block.rs index 6861b3709394..d3cab68137c4 100644 --- a/clippy_lints/src/semicolon_block.rs +++ b/clippy_lints/src/semicolon_block.rs @@ -8,7 +8,7 @@ use rustc_span::Span; declare_clippy_lint! { /// ### What it does /// - /// Checks for semicolon terminated blocks containing only a single expression. + /// Suggests moving the semicolon from a block inside of the block to its kast expression. /// /// ### Why is this bad? /// @@ -36,7 +36,7 @@ declare_clippy_lint! { declare_clippy_lint! { /// ### What it does /// - /// Checks for blocks containing only a single semicolon terminated statement. + /// Suggests moving the semicolon from a block's final expression outside of the block. /// /// ### Why is this bad? /// @@ -80,7 +80,7 @@ impl LateLintPass<'_> for SemicolonBlock { span, .. } = stmt else { return }; - semicolon_outside_block(cx, block, expr, span) + semicolon_outside_block(cx, block, expr, span); }, StmtKind::Semi(Expr { kind: ExprKind::Block(block @ Block { expr: Some(tail), .. }, _), From 20ec2ceab8450e1a0611fbc54a17a03ffc0bd39b Mon Sep 17 00:00:00 2001 From: Lukas Wirth Date: Mon, 5 Dec 2022 11:02:10 +0100 Subject: [PATCH 8/8] Add test case for blocks with semicolon inside and outside a block --- clippy_lints/src/semicolon_block.rs | 9 ++++++--- tests/ui/semicolon_inside_block.fixed | 2 ++ tests/ui/semicolon_inside_block.rs | 2 ++ tests/ui/semicolon_outside_block.fixed | 2 ++ tests/ui/semicolon_outside_block.rs | 2 ++ 5 files changed, 14 insertions(+), 3 deletions(-) diff --git a/clippy_lints/src/semicolon_block.rs b/clippy_lints/src/semicolon_block.rs index d3cab68137c4..8f1d1490e1f0 100644 --- a/clippy_lints/src/semicolon_block.rs +++ b/clippy_lints/src/semicolon_block.rs @@ -8,11 +8,13 @@ use rustc_span::Span; declare_clippy_lint! { /// ### What it does /// - /// Suggests moving the semicolon from a block inside of the block to its kast expression. + /// Suggests moving the semicolon after a block to the inside of the block, after its last + /// expression. /// /// ### 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. + /// 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. /// /// ### Example @@ -40,7 +42,8 @@ declare_clippy_lint! { /// /// ### 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. + /// 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. /// /// ### Example diff --git a/tests/ui/semicolon_inside_block.fixed b/tests/ui/semicolon_inside_block.fixed index 4cd112dd5e12..42e97e1ca358 100644 --- a/tests/ui/semicolon_inside_block.fixed +++ b/tests/ui/semicolon_inside_block.fixed @@ -79,5 +79,7 @@ fn main() { unit_fn_block() }; + { unit_fn_block(); }; + unit_fn_block() } diff --git a/tests/ui/semicolon_inside_block.rs b/tests/ui/semicolon_inside_block.rs index 7512125c051d..f40848f702e1 100644 --- a/tests/ui/semicolon_inside_block.rs +++ b/tests/ui/semicolon_inside_block.rs @@ -79,5 +79,7 @@ fn main() { unit_fn_block() }; + { unit_fn_block(); }; + unit_fn_block() } diff --git a/tests/ui/semicolon_outside_block.fixed b/tests/ui/semicolon_outside_block.fixed index 5bc18faaad8e..091eaa7518e9 100644 --- a/tests/ui/semicolon_outside_block.fixed +++ b/tests/ui/semicolon_outside_block.fixed @@ -79,5 +79,7 @@ fn main() { unit_fn_block() }; + { unit_fn_block(); }; + unit_fn_block() } diff --git a/tests/ui/semicolon_outside_block.rs b/tests/ui/semicolon_outside_block.rs index 0a4293238763..7ce46431fac9 100644 --- a/tests/ui/semicolon_outside_block.rs +++ b/tests/ui/semicolon_outside_block.rs @@ -79,5 +79,7 @@ fn main() { unit_fn_block() }; + { unit_fn_block(); }; + unit_fn_block() }