From 7d94db5de14b3e3b333e87bf83e8ed88b5f9f9d6 Mon Sep 17 00:00:00 2001 From: saberoueslati Date: Wed, 5 Aug 2026 15:46:01 +0100 Subject: [PATCH 1/3] fix(items_after_statements): handle cfg_select! arms --- clippy_lints/src/items_after_statements.rs | 74 +++++++++++++++++++++- tests/ui/items_after_statement.rs | 32 ++++++++++ tests/ui/items_after_statement.stderr | 14 +++- 3 files changed, 116 insertions(+), 4 deletions(-) diff --git a/clippy_lints/src/items_after_statements.rs b/clippy_lints/src/items_after_statements.rs index afb8863f1b94..d4968cfa43d2 100644 --- a/clippy_lints/src/items_after_statements.rs +++ b/clippy_lints/src/items_after_statements.rs @@ -1,7 +1,10 @@ use clippy_utils::diagnostics::span_lint_hir; +use clippy_utils::source::SpanExt as _; use rustc_hir::{Block, ItemKind, StmtKind}; +use rustc_lexer::{FrontmatterAllowed, TokenKind, tokenize}; use rustc_lint::{LateContext, LateLintPass, LintContext as _}; use rustc_session::declare_lint_pass; +use rustc_span::{Pos as _, Span}; declare_clippy_lint! { /// ### What it does @@ -49,12 +52,71 @@ declare_clippy_lint! { declare_lint_pass!(ItemsAfterStatements => [ITEMS_AFTER_STATEMENTS]); +/// Checks that each item is a direct child of `block`, rather than being nested inside a token tree +/// such as a `cfg_select!` arm. +/// +/// `cfg_select!` splices the tokens of the selected arm into the enclosing block without applying +/// any expansion marker, so both the span and the syntax context of the resulting items are +/// indistinguishable from items written directly in the block. +/// +/// This deliberately gives up on a few true positives, e.g. an item after a statement within the +/// same `cfg_select!` arm, or `some_macro! { fn f() {} }` in statement position, as a false +/// negative is preferable to a false positive here. +fn are_direct_children_of_block(cx: &LateContext<'_>, block: &Block<'_>, item_spans: &[Span]) -> Vec { + let mut direct_children = vec![false; item_spans.len()]; + if item_spans.is_empty() { + return direct_children; + } + + // Only the block's own opening delimiter may be left open at an item's position. Comments and + // string literals are single tokens, so delimiters inside them can't skew the count. + block.span.with_source_text(cx, |src| { + let mut item_offsets = item_spans + .iter() + .enumerate() + .filter_map(|(index, item_span)| { + (item_span.lo() >= block.span.lo() && item_span.lo() <= block.span.hi()) + .then(|| (index, (item_span.lo() - block.span.lo()).to_usize())) + }) + .filter(|&(_, offset)| offset <= src.len()) + .collect::>(); + item_offsets.sort_unstable_by_key(|&(_, offset)| offset); + + let mut depth = 0i32; + let mut offset = 0; + let mut next_item = 0; + for token in tokenize(src, FrontmatterAllowed::No) { + while let Some(&(index, item_offset)) = item_offsets.get(next_item) + && item_offset <= offset + { + direct_children[index] = depth == 1; + next_item += 1; + } + + match token.kind { + TokenKind::OpenParen | TokenKind::OpenBrace | TokenKind::OpenBracket => depth += 1, + TokenKind::CloseParen | TokenKind::CloseBrace | TokenKind::CloseBracket => depth -= 1, + _ => {}, + } + offset += token.len as usize; + } + + while let Some(&(index, _)) = item_offsets.get(next_item) { + direct_children[index] = depth == 1; + next_item += 1; + } + }); + + // The source isn't available, or the range spans multiple files (e.g. `include!`). + direct_children +} + impl LateLintPass<'_> for ItemsAfterStatements { fn check_block(&mut self, cx: &LateContext<'_>, block: &Block<'_>) { if block.stmts.len() > 1 { let ctxt = block.span.ctxt(); let mut in_external = None; - block + let items = block .stmts .iter() .skip_while(|stmt| matches!(stmt.kind, StmtKind::Item(..))) @@ -66,8 +128,14 @@ impl LateLintPass<'_> for ItemsAfterStatements { .filter(|item| !matches!(item.kind, ItemKind::Macro(..))) // Stop linting if macros define items. .take_while(|item| item.span.ctxt() == ctxt) - // Don't use `next` due to the complex filter chain. - .for_each(|item| { + .collect::>(); + let item_spans = items.iter().map(|item| item.span).collect::>(); + let direct_children = are_direct_children_of_block(cx, block, &item_spans); + items + .into_iter() + .zip(direct_children) + .filter(|(_, direct)| *direct) + .for_each(|(item, _)| { // Only do the macro check once, but delay it until it's needed. if !*in_external.get_or_insert_with(|| block.span.in_external_macro(cx.sess().source_map())) { span_lint_hir( diff --git a/tests/ui/items_after_statement.rs b/tests/ui/items_after_statement.rs index a308d8d78934..c8d4084d8a91 100644 --- a/tests/ui/items_after_statement.rs +++ b/tests/ui/items_after_statement.rs @@ -68,6 +68,38 @@ fn item_from_macro() { static_assert_size!(u32, 4); } +fn cfg_select_arm() { + let x = 1; + // `cfg_select!` splices the tokens of the arm into this block, without any expansion marker + std::cfg_select! { + true => { + use std::mem; + let _ = (x, mem::size_of::()); + }, + } +} + +fn item_after_cfg_select() { + let x = 1; + std::cfg_select! { + true => { + use std::mem; + let _ = (x, mem::size_of::()); + }, + } + fn foo() {} + //~^ items_after_statements +} + +fn nested_block() { + let _ = 1; + { + let _ = 2; + fn foo() {} + //~^ items_after_statements + } +} + fn allow_attribute() { let _ = 1; #[allow(clippy::items_after_statements)] diff --git a/tests/ui/items_after_statement.stderr b/tests/ui/items_after_statement.stderr index 7a5ae2c8d47d..60f4090e0dd5 100644 --- a/tests/ui/items_after_statement.stderr +++ b/tests/ui/items_after_statement.stderr @@ -35,5 +35,17 @@ LL | b!(); | = note: this error originates in the macro `b` (in Nightly builds, run with -Z macro-backtrace for more info) -error: aborting due to 3 previous errors +error: adding items after statements is confusing, since items exist from the start of the scope + --> tests/ui/items_after_statement.rs:90:5 + | +LL | fn foo() {} + | ^^^^^^^^^^^ + +error: adding items after statements is confusing, since items exist from the start of the scope + --> tests/ui/items_after_statement.rs:98:9 + | +LL | fn foo() {} + | ^^^^^^^^^^^ + +error: aborting due to 5 previous errors From c2858a038c208a19da47fc634669339cdd8a99c5 Mon Sep 17 00:00:00 2001 From: saberoueslati Date: Wed, 5 Aug 2026 16:37:19 +0100 Subject: [PATCH 2/3] fixed CI/CD issue --- clippy_lints/src/items_after_statements.rs | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/clippy_lints/src/items_after_statements.rs b/clippy_lints/src/items_after_statements.rs index d4968cfa43d2..4ebdc8a07b3d 100644 --- a/clippy_lints/src/items_after_statements.rs +++ b/clippy_lints/src/items_after_statements.rs @@ -74,10 +74,8 @@ fn are_direct_children_of_block(cx: &LateContext<'_>, block: &Block<'_>, item_sp let mut item_offsets = item_spans .iter() .enumerate() - .filter_map(|(index, item_span)| { - (item_span.lo() >= block.span.lo() && item_span.lo() <= block.span.hi()) - .then(|| (index, (item_span.lo() - block.span.lo()).to_usize())) - }) + .filter(|(_, item_span)| item_span.lo() >= block.span.lo() && item_span.lo() <= block.span.hi()) + .map(|(index, item_span)| (index, (item_span.lo() - block.span.lo()).to_usize())) .filter(|&(_, offset)| offset <= src.len()) .collect::>(); item_offsets.sort_unstable_by_key(|&(_, offset)| offset); From c4442e5909b2c2741ef23aedc5df6c9b1092236a Mon Sep 17 00:00:00 2001 From: saberoueslati Date: Tue, 11 Aug 2026 17:53:13 +0100 Subject: [PATCH 3/3] adressed review comments --- clippy_lints/src/items_after_statements.rs | 61 +++++++++++----------- tests/ui/items_after_statement.rs | 17 ++++-- tests/ui/items_after_statement.stderr | 22 +++++++- 3 files changed, 63 insertions(+), 37 deletions(-) diff --git a/clippy_lints/src/items_after_statements.rs b/clippy_lints/src/items_after_statements.rs index 4ebdc8a07b3d..0e5f9f12432f 100644 --- a/clippy_lints/src/items_after_statements.rs +++ b/clippy_lints/src/items_after_statements.rs @@ -52,42 +52,41 @@ declare_clippy_lint! { declare_lint_pass!(ItemsAfterStatements => [ITEMS_AFTER_STATEMENTS]); -/// Checks that each item is a direct child of `block`, rather than being nested inside a token tree -/// such as a `cfg_select!` arm. -/// /// `cfg_select!` splices the tokens of the selected arm into the enclosing block without applying /// any expansion marker, so both the span and the syntax context of the resulting items are /// indistinguishable from items written directly in the block. -/// -/// This deliberately gives up on a few true positives, e.g. an item after a statement within the -/// same `cfg_select!` arm, or `some_macro! { fn f() {} }` in statement position, as a false -/// negative is preferable to a false positive here. -fn are_direct_children_of_block(cx: &LateContext<'_>, block: &Block<'_>, item_spans: &[Span]) -> Vec { - let mut direct_children = vec![false; item_spans.len()]; - if item_spans.is_empty() { - return direct_children; +fn are_direct_children_of_block(cx: &LateContext<'_>, block: &Block<'_>, stmt_spans: &[Span]) -> Vec { + if stmt_spans.is_empty() { + return vec![]; } + let mut direct_children = vec![true; stmt_spans.len()]; - // Only the block's own opening delimiter may be left open at an item's position. Comments and + // Only the block's own opening delimiter may be left open at a statement's position. Comments and // string literals are single tokens, so delimiters inside them can't skew the count. block.span.with_source_text(cx, |src| { - let mut item_offsets = item_spans + let Some(stmt_offsets) = stmt_spans .iter() - .enumerate() - .filter(|(_, item_span)| item_span.lo() >= block.span.lo() && item_span.lo() <= block.span.hi()) - .map(|(index, item_span)| (index, (item_span.lo() - block.span.lo()).to_usize())) - .filter(|&(_, offset)| offset <= src.len()) - .collect::>(); - item_offsets.sort_unstable_by_key(|&(_, offset)| offset); + .map(|stmt_span| { + (stmt_span.lo() >= block.span.lo() && stmt_span.lo() <= block.span.hi()) + .then(|| (stmt_span.lo() - block.span.lo()).to_usize()) + .filter(|&offset| offset <= src.len()) + }) + .collect::>>() + else { + return; + }; + if !stmt_offsets.is_sorted() { + return; + } let mut depth = 0i32; let mut offset = 0; let mut next_item = 0; for token in tokenize(src, FrontmatterAllowed::No) { - while let Some(&(index, item_offset)) = item_offsets.get(next_item) - && item_offset <= offset + while let Some(&stmt_offset) = stmt_offsets.get(next_item) + && stmt_offset <= offset { - direct_children[index] = depth == 1; + direct_children[next_item] = depth == 1; next_item += 1; } @@ -99,13 +98,13 @@ fn are_direct_children_of_block(cx: &LateContext<'_>, block: &Block<'_>, item_sp offset += token.len as usize; } - while let Some(&(index, _)) = item_offsets.get(next_item) { - direct_children[index] = depth == 1; + while stmt_offsets.get(next_item).is_some() { + direct_children[next_item] = depth == 1; next_item += 1; } }); - // The source isn't available, or the range spans multiple files (e.g. `include!`). + // If source text isn't available or the spans can't be mapped, preserve the lint. direct_children } @@ -119,21 +118,21 @@ impl LateLintPass<'_> for ItemsAfterStatements { .iter() .skip_while(|stmt| matches!(stmt.kind, StmtKind::Item(..))) .filter_map(|stmt| match stmt.kind { - StmtKind::Item(id) => Some(cx.tcx.hir_item(id)), + StmtKind::Item(id) => Some((stmt.span, cx.tcx.hir_item(id))), _ => None, }) // Ignore macros since they can only see previously defined locals. - .filter(|item| !matches!(item.kind, ItemKind::Macro(..))) + .filter(|(_, item)| !matches!(item.kind, ItemKind::Macro(..))) // Stop linting if macros define items. - .take_while(|item| item.span.ctxt() == ctxt) + .take_while(|(_, item)| item.span.ctxt() == ctxt) .collect::>(); - let item_spans = items.iter().map(|item| item.span).collect::>(); - let direct_children = are_direct_children_of_block(cx, block, &item_spans); + let stmt_spans = items.iter().map(|(stmt_span, _)| *stmt_span).collect::>(); + let direct_children = are_direct_children_of_block(cx, block, &stmt_spans); items .into_iter() .zip(direct_children) .filter(|(_, direct)| *direct) - .for_each(|(item, _)| { + .for_each(|((_, item), _)| { // Only do the macro check once, but delay it until it's needed. if !*in_external.get_or_insert_with(|| block.span.in_external_macro(cx.sess().source_map())) { span_lint_hir( diff --git a/tests/ui/items_after_statement.rs b/tests/ui/items_after_statement.rs index c8d4084d8a91..60d50ec4de58 100644 --- a/tests/ui/items_after_statement.rs +++ b/tests/ui/items_after_statement.rs @@ -73,8 +73,8 @@ fn cfg_select_arm() { // `cfg_select!` splices the tokens of the arm into this block, without any expansion marker std::cfg_select! { true => { - use std::mem; - let _ = (x, mem::size_of::()); + use std::{cmp, mem}; + let _ = (x, cmp::max(0, 1), mem::size_of::()); }, } } @@ -83,14 +83,23 @@ fn item_after_cfg_select() { let x = 1; std::cfg_select! { true => { - use std::mem; - let _ = (x, mem::size_of::()); + use std::{cmp, mem}; + let _ = (x, cmp::max(0, 1), mem::size_of::()); }, } fn foo() {} //~^ items_after_statements } +fn grouped_use_after_statement() { + let x = 1; + use std::cmp::{max, min}; + //~^ items_after_statements + //~| items_after_statements + //~| items_after_statements + let _ = (x, max(0, 1), min(0, 1)); +} + fn nested_block() { let _ = 1; { diff --git a/tests/ui/items_after_statement.stderr b/tests/ui/items_after_statement.stderr index 60f4090e0dd5..105ea1e5bb3b 100644 --- a/tests/ui/items_after_statement.stderr +++ b/tests/ui/items_after_statement.stderr @@ -42,10 +42,28 @@ LL | fn foo() {} | ^^^^^^^^^^^ error: adding items after statements is confusing, since items exist from the start of the scope - --> tests/ui/items_after_statement.rs:98:9 + --> tests/ui/items_after_statement.rs:96:5 + | +LL | use std::cmp::{max, min}; + | ^^^^^^^^^^^^^^^^^^^^^^^^^ + +error: adding items after statements is confusing, since items exist from the start of the scope + --> tests/ui/items_after_statement.rs:96:20 + | +LL | use std::cmp::{max, min}; + | ^^^ + +error: adding items after statements is confusing, since items exist from the start of the scope + --> tests/ui/items_after_statement.rs:96:25 + | +LL | use std::cmp::{max, min}; + | ^^^ + +error: adding items after statements is confusing, since items exist from the start of the scope + --> tests/ui/items_after_statement.rs:107:9 | LL | fn foo() {} | ^^^^^^^^^^^ -error: aborting due to 5 previous errors +error: aborting due to 8 previous errors