From 1358569c531e13e1641fa7ddde51fe4abfd19bfb Mon Sep 17 00:00:00 2001 From: Guillaume Gomez Date: Thu, 6 Feb 2025 21:15:51 +0100 Subject: [PATCH 1/5] Convert `EMPTY_LINE_AFTER_OUTER_ATTR` and `EMPTY_LINE_AFTER_OUTER_ATTR` lint into early lints --- .../clippy/clippy_lints/src/declared_lints.rs | 4 +- .../clippy_lints/src/doc/empty_line_after.rs | 345 ------------- src/tools/clippy/clippy_lints/src/doc/mod.rs | 81 +--- .../clippy_lints/src/empty_line_after.rs | 453 ++++++++++++++++++ src/tools/clippy/clippy_lints/src/lib.rs | 2 + 5 files changed, 458 insertions(+), 427 deletions(-) delete mode 100644 src/tools/clippy/clippy_lints/src/doc/empty_line_after.rs create mode 100644 src/tools/clippy/clippy_lints/src/empty_line_after.rs diff --git a/src/tools/clippy/clippy_lints/src/declared_lints.rs b/src/tools/clippy/clippy_lints/src/declared_lints.rs index 9fbeab5bf2e1f..5c5978b555985 100644 --- a/src/tools/clippy/clippy_lints/src/declared_lints.rs +++ b/src/tools/clippy/clippy_lints/src/declared_lints.rs @@ -144,8 +144,6 @@ pub static LINTS: &[&crate::LintInfo] = &[ crate::doc::DOC_NESTED_REFDEFS_INFO, crate::doc::DOC_OVERINDENTED_LIST_ITEMS_INFO, crate::doc::EMPTY_DOCS_INFO, - crate::doc::EMPTY_LINE_AFTER_DOC_COMMENTS_INFO, - crate::doc::EMPTY_LINE_AFTER_OUTER_ATTR_INFO, crate::doc::MISSING_ERRORS_DOC_INFO, crate::doc::MISSING_PANICS_DOC_INFO, crate::doc::MISSING_SAFETY_DOC_INFO, @@ -162,6 +160,8 @@ pub static LINTS: &[&crate::LintInfo] = &[ crate::else_if_without_else::ELSE_IF_WITHOUT_ELSE_INFO, crate::empty_drop::EMPTY_DROP_INFO, crate::empty_enum::EMPTY_ENUM_INFO, + crate::empty_line_after::EMPTY_LINE_AFTER_DOC_COMMENTS_INFO, + crate::empty_line_after::EMPTY_LINE_AFTER_OUTER_ATTR_INFO, crate::empty_with_brackets::EMPTY_ENUM_VARIANTS_WITH_BRACKETS_INFO, crate::empty_with_brackets::EMPTY_STRUCTS_WITH_BRACKETS_INFO, crate::endian_bytes::BIG_ENDIAN_BYTES_INFO, diff --git a/src/tools/clippy/clippy_lints/src/doc/empty_line_after.rs b/src/tools/clippy/clippy_lints/src/doc/empty_line_after.rs deleted file mode 100644 index 6e85c6af642b5..0000000000000 --- a/src/tools/clippy/clippy_lints/src/doc/empty_line_after.rs +++ /dev/null @@ -1,345 +0,0 @@ -use clippy_utils::diagnostics::span_lint_and_then; -use clippy_utils::source::{SpanRangeExt, snippet_indent}; -use clippy_utils::tokenize_with_text; -use itertools::Itertools; -use rustc_ast::AttrStyle; -use rustc_ast::token::CommentKind; -use rustc_errors::{Applicability, Diag, SuggestionStyle}; -use rustc_hir::{AttrKind, Attribute, ItemKind, Node}; -use rustc_lexer::TokenKind; -use rustc_lint::LateContext; -use rustc_span::{BytePos, ExpnKind, InnerSpan, Span, SpanData}; - -use super::{EMPTY_LINE_AFTER_DOC_COMMENTS, EMPTY_LINE_AFTER_OUTER_ATTR}; - -#[derive(Debug, PartialEq, Clone, Copy)] -enum StopKind { - Attr, - Doc(CommentKind), -} - -impl StopKind { - fn is_doc(self) -> bool { - matches!(self, StopKind::Doc(_)) - } -} - -#[derive(Debug)] -struct Stop { - span: Span, - kind: StopKind, - first: usize, - last: usize, -} - -impl Stop { - fn convert_to_inner(&self) -> (Span, String) { - let inner = match self.kind { - // #|[...] - StopKind::Attr => InnerSpan::new(1, 1), - // /// or /** - // ^ ^ - StopKind::Doc(_) => InnerSpan::new(2, 3), - }; - (self.span.from_inner(inner), "!".into()) - } - - fn comment_out(&self, cx: &LateContext<'_>, suggestions: &mut Vec<(Span, String)>) { - match self.kind { - StopKind::Attr => { - if cx.tcx.sess.source_map().is_multiline(self.span) { - suggestions.extend([ - (self.span.shrink_to_lo(), "/* ".into()), - (self.span.shrink_to_hi(), " */".into()), - ]); - } else { - suggestions.push((self.span.shrink_to_lo(), "// ".into())); - } - }, - StopKind::Doc(CommentKind::Line) => suggestions.push((self.span.shrink_to_lo(), "// ".into())), - StopKind::Doc(CommentKind::Block) => { - // /** outer */ /*! inner */ - // ^ ^ - let asterisk = self.span.from_inner(InnerSpan::new(1, 2)); - suggestions.push((asterisk, String::new())); - }, - } - } - - fn from_attr(cx: &LateContext<'_>, attr: &Attribute) -> Option { - let SpanData { lo, hi, .. } = attr.span.data(); - let file = cx.tcx.sess.source_map().lookup_source_file(lo); - - Some(Self { - span: attr.span, - kind: match attr.kind { - AttrKind::Normal(_) => StopKind::Attr, - AttrKind::DocComment(comment_kind, _) => StopKind::Doc(comment_kind), - }, - first: file.lookup_line(file.relative_position(lo))?, - last: file.lookup_line(file.relative_position(hi))?, - }) - } -} - -/// Represents a set of attrs/doc comments separated by 1 or more empty lines -/// -/// ```ignore -/// /// chunk 1 docs -/// // not an empty line so also part of chunk 1 -/// #[chunk_1_attrs] // <-- prev_stop -/// -/// /* gap */ -/// -/// /// chunk 2 docs // <-- next_stop -/// #[chunk_2_attrs] -/// ``` -struct Gap<'a> { - /// The span of individual empty lines including the newline at the end of the line - empty_lines: Vec, - has_comment: bool, - next_stop: &'a Stop, - prev_stop: &'a Stop, - /// The chunk that includes [`prev_stop`](Self::prev_stop) - prev_chunk: &'a [Stop], -} - -impl<'a> Gap<'a> { - fn new(cx: &LateContext<'_>, prev_chunk: &'a [Stop], next_chunk: &'a [Stop]) -> Option { - let prev_stop = prev_chunk.last()?; - let next_stop = next_chunk.first()?; - let gap_span = prev_stop.span.between(next_stop.span); - let gap_snippet = gap_span.get_source_text(cx)?; - - let mut has_comment = false; - let mut empty_lines = Vec::new(); - - for (token, source, inner_span) in tokenize_with_text(&gap_snippet) { - match token { - TokenKind::BlockComment { - doc_style: None, - terminated: true, - } - | TokenKind::LineComment { doc_style: None } => has_comment = true, - TokenKind::Whitespace => { - let newlines = source.bytes().positions(|b| b == b'\n'); - empty_lines.extend( - newlines - .tuple_windows() - .map(|(a, b)| InnerSpan::new(inner_span.start + a + 1, inner_span.start + b)) - .map(|inner_span| gap_span.from_inner(inner_span)), - ); - }, - // Ignore cfg_attr'd out attributes as they may contain empty lines, could also be from macro - // shenanigans - _ => return None, - } - } - - (!empty_lines.is_empty()).then_some(Self { - empty_lines, - has_comment, - next_stop, - prev_stop, - prev_chunk, - }) - } - - fn contiguous_empty_lines(&self) -> impl Iterator + '_ { - self.empty_lines - // The `+ BytePos(1)` means "next line", because each empty line span is "N:1-N:1". - .chunk_by(|a, b| a.hi() + BytePos(1) == b.lo()) - .map(|chunk| { - let first = chunk.first().expect("at least one empty line"); - let last = chunk.last().expect("at least one empty line"); - // The BytePos subtraction here is safe, as before an empty line, there must be at least one - // attribute/comment. The span needs to start at the end of the previous line. - first.with_lo(first.lo() - BytePos(1)).with_hi(last.hi()) - }) - } -} - -/// If the node the attributes/docs apply to is the first in the module/crate suggest converting -/// them to inner attributes/docs -fn suggest_inner(cx: &LateContext<'_>, diag: &mut Diag<'_, ()>, kind: StopKind, gaps: &[Gap<'_>]) { - let Some(owner) = cx.last_node_with_lint_attrs.as_owner() else { - return; - }; - let parent_desc = match cx.tcx.parent_hir_node(owner.into()) { - Node::Item(item) - if let ItemKind::Mod(parent_mod) = item.kind - && let [first, ..] = parent_mod.item_ids - && first.owner_id == owner => - { - "parent module" - }, - Node::Crate(crate_mod) - if let Some(first) = crate_mod - .item_ids - .iter() - .map(|&id| cx.tcx.hir().item(id)) - // skip prelude imports - .find(|item| !matches!(item.span.ctxt().outer_expn_data().kind, ExpnKind::AstPass(_))) - && first.owner_id == owner => - { - "crate" - }, - _ => return, - }; - - diag.multipart_suggestion_verbose( - match kind { - StopKind::Attr => format!("if the attribute should apply to the {parent_desc} use an inner attribute"), - StopKind::Doc(_) => format!("if the comment should document the {parent_desc} use an inner doc comment"), - }, - gaps.iter() - .flat_map(|gap| gap.prev_chunk) - .map(Stop::convert_to_inner) - .collect(), - Applicability::MaybeIncorrect, - ); -} - -fn check_gaps(cx: &LateContext<'_>, gaps: &[Gap<'_>]) -> bool { - let Some(first_gap) = gaps.first() else { - return false; - }; - let empty_lines = || gaps.iter().flat_map(|gap| gap.empty_lines.iter().copied()); - let contiguous_empty_lines = || gaps.iter().flat_map(Gap::contiguous_empty_lines); - let mut has_comment = false; - let mut has_attr = false; - for gap in gaps { - has_comment |= gap.has_comment; - if !has_attr { - has_attr = gap.prev_chunk.iter().any(|stop| stop.kind == StopKind::Attr); - } - } - let kind = first_gap.prev_stop.kind; - let (lint, kind_desc) = match kind { - StopKind::Attr => (EMPTY_LINE_AFTER_OUTER_ATTR, "outer attribute"), - StopKind::Doc(_) => (EMPTY_LINE_AFTER_DOC_COMMENTS, "doc comment"), - }; - let (lines, are, them) = if empty_lines().nth(1).is_some() { - ("lines", "are", "them") - } else { - ("line", "is", "it") - }; - span_lint_and_then( - cx, - lint, - first_gap.prev_stop.span.to(empty_lines().last().unwrap()), - format!("empty {lines} after {kind_desc}"), - |diag| { - if let Some(owner) = cx.last_node_with_lint_attrs.as_owner() { - let def_id = owner.to_def_id(); - let def_descr = cx.tcx.def_descr(def_id); - diag.span_label( - cx.tcx.def_span(def_id), - match kind { - StopKind::Attr => format!("the attribute applies to this {def_descr}"), - StopKind::Doc(_) => format!("the comment documents this {def_descr}"), - }, - ); - } - - diag.multipart_suggestion_with_style( - format!("if the empty {lines} {are} unintentional remove {them}"), - contiguous_empty_lines() - .map(|empty_lines| (empty_lines, String::new())) - .collect(), - Applicability::MaybeIncorrect, - SuggestionStyle::HideCodeAlways, - ); - - if has_comment && kind.is_doc() { - // Likely doc comments that applied to some now commented out code - // - // /// Old docs for Foo - // // struct Foo; - - let mut suggestions = Vec::new(); - for stop in gaps.iter().flat_map(|gap| gap.prev_chunk) { - stop.comment_out(cx, &mut suggestions); - } - let name = match cx.tcx.hir().opt_name(cx.last_node_with_lint_attrs) { - Some(name) => format!("`{name}`"), - None => "this".into(), - }; - diag.multipart_suggestion_verbose( - format!("if the doc comment should not document {name} comment it out"), - suggestions, - Applicability::MaybeIncorrect, - ); - } else { - suggest_inner(cx, diag, kind, gaps); - } - - if kind == StopKind::Doc(CommentKind::Line) - && gaps - .iter() - .all(|gap| !gap.has_comment && gap.next_stop.kind == StopKind::Doc(CommentKind::Line)) - { - // Commentless empty gaps between line doc comments, possibly intended to be part of the markdown - - let indent = snippet_indent(cx, first_gap.prev_stop.span).unwrap_or_default(); - diag.multipart_suggestion_verbose( - format!("if the documentation should include the empty {lines} include {them} in the comment"), - empty_lines() - .map(|empty_line| (empty_line, format!("{indent}///"))) - .collect(), - Applicability::MaybeIncorrect, - ); - } - }, - ); - kind.is_doc() -} - -/// Returns `true` if [`EMPTY_LINE_AFTER_DOC_COMMENTS`] triggered, used to skip other doc comment -/// lints where they would be confusing -/// -/// [`EMPTY_LINE_AFTER_OUTER_ATTR`] is also here to share an implementation but does not return -/// `true` if it triggers -pub(super) fn check(cx: &LateContext<'_>, attrs: &[Attribute]) -> bool { - let mut outer = attrs - .iter() - .filter(|attr| attr.style == AttrStyle::Outer && !attr.span.from_expansion()) - .map(|attr| Stop::from_attr(cx, attr)) - .collect::>>() - .unwrap_or_default(); - - if outer.is_empty() { - return false; - } - - // Push a fake attribute Stop for the item itself so we check for gaps between the last outer - // attr/doc comment and the item they apply to - let span = cx.tcx.hir().span(cx.last_node_with_lint_attrs); - if !span.from_expansion() - && let Ok(line) = cx.tcx.sess.source_map().lookup_line(span.lo()) - { - outer.push(Stop { - span, - kind: StopKind::Attr, - first: line.line, - // last doesn't need to be accurate here, we don't compare it with anything - last: line.line, - }); - } - - let mut gaps = Vec::new(); - let mut last = 0; - for pos in outer - .array_windows() - .positions(|[a, b]| b.first.saturating_sub(a.last) > 1) - { - // we want to be after the first stop in the window - let pos = pos + 1; - if let Some(gap) = Gap::new(cx, &outer[last..pos], &outer[pos..]) { - last = pos; - gaps.push(gap); - } - } - - check_gaps(cx, &gaps) -} diff --git a/src/tools/clippy/clippy_lints/src/doc/mod.rs b/src/tools/clippy/clippy_lints/src/doc/mod.rs index 3d8ce7becdbda..42e1f7fd950d4 100644 --- a/src/tools/clippy/clippy_lints/src/doc/mod.rs +++ b/src/tools/clippy/clippy_lints/src/doc/mod.rs @@ -33,7 +33,6 @@ use rustc_span::{Span, sym}; use std::ops::Range; use url::Url; -mod empty_line_after; mod include_in_doc_without_cfg; mod link_with_quotes; mod markdown; @@ -491,82 +490,6 @@ declare_clippy_lint! { "ensure the first documentation paragraph is short" } -declare_clippy_lint! { - /// ### What it does - /// Checks for empty lines after outer attributes - /// - /// ### Why is this bad? - /// The attribute may have meant to be an inner attribute (`#![attr]`). If - /// it was meant to be an outer attribute (`#[attr]`) then the empty line - /// should be removed - /// - /// ### Example - /// ```no_run - /// #[allow(dead_code)] - /// - /// fn not_quite_good_code() {} - /// ``` - /// - /// Use instead: - /// ```no_run - /// // Good (as inner attribute) - /// #![allow(dead_code)] - /// - /// fn this_is_fine() {} - /// - /// // or - /// - /// // Good (as outer attribute) - /// #[allow(dead_code)] - /// fn this_is_fine_too() {} - /// ``` - #[clippy::version = "pre 1.29.0"] - pub EMPTY_LINE_AFTER_OUTER_ATTR, - suspicious, - "empty line after outer attribute" -} - -declare_clippy_lint! { - /// ### What it does - /// Checks for empty lines after doc comments. - /// - /// ### Why is this bad? - /// The doc comment may have meant to be an inner doc comment, regular - /// comment or applied to some old code that is now commented out. If it was - /// intended to be a doc comment, then the empty line should be removed. - /// - /// ### Example - /// ```no_run - /// /// Some doc comment with a blank line after it. - /// - /// fn f() {} - /// - /// /// Docs for `old_code` - /// // fn old_code() {} - /// - /// fn new_code() {} - /// ``` - /// - /// Use instead: - /// ```no_run - /// //! Convert it to an inner doc comment - /// - /// // Or a regular comment - /// - /// /// Or remove the empty line - /// fn f() {} - /// - /// // /// Docs for `old_code` - /// // fn old_code() {} - /// - /// fn new_code() {} - /// ``` - #[clippy::version = "1.70.0"] - pub EMPTY_LINE_AFTER_DOC_COMMENTS, - suspicious, - "empty line after doc comments" -} - declare_clippy_lint! { /// ### What it does /// Checks if included files in doc comments are included only for `cfg(doc)`. @@ -650,8 +573,6 @@ impl_lint_pass!(Documentation => [ EMPTY_DOCS, DOC_LAZY_CONTINUATION, DOC_OVERINDENTED_LIST_ITEMS, - EMPTY_LINE_AFTER_OUTER_ATTR, - EMPTY_LINE_AFTER_DOC_COMMENTS, TOO_LONG_FIRST_DOC_PARAGRAPH, DOC_INCLUDE_WITHOUT_CFG, ]); @@ -784,7 +705,7 @@ fn check_attrs(cx: &LateContext<'_>, valid_idents: &FxHashSet, attrs: &[ } include_in_doc_without_cfg::check(cx, attrs); - if suspicious_doc_comments::check(cx, attrs) || empty_line_after::check(cx, attrs) || is_doc_hidden(attrs) { + if suspicious_doc_comments::check(cx, attrs) || is_doc_hidden(attrs) { return None; } diff --git a/src/tools/clippy/clippy_lints/src/empty_line_after.rs b/src/tools/clippy/clippy_lints/src/empty_line_after.rs new file mode 100644 index 0000000000000..7e0a2690d54e5 --- /dev/null +++ b/src/tools/clippy/clippy_lints/src/empty_line_after.rs @@ -0,0 +1,453 @@ +use clippy_utils::diagnostics::span_lint_and_then; +use clippy_utils::source::{SpanRangeExt, snippet_indent}; +use clippy_utils::tokenize_with_text; +use itertools::Itertools; +use rustc_ast::token::CommentKind; +use rustc_ast::{AttrKind, AttrStyle, Attribute, Crate, Item, ItemKind, ModKind, NodeId}; +use rustc_errors::{Applicability, Diag, SuggestionStyle}; +use rustc_lexer::TokenKind; +use rustc_lint::{EarlyContext, EarlyLintPass, LintContext}; +use rustc_session::impl_lint_pass; +use rustc_span::symbol::kw; +use rustc_span::{BytePos, ExpnKind, InnerSpan, Span, SpanData, Symbol}; + +declare_clippy_lint! { + /// ### What it does + /// Checks for empty lines after outer attributes + /// + /// ### Why is this bad? + /// The attribute may have meant to be an inner attribute (`#![attr]`). If + /// it was meant to be an outer attribute (`#[attr]`) then the empty line + /// should be removed + /// + /// ### Example + /// ```no_run + /// #[allow(dead_code)] + /// + /// fn not_quite_good_code() {} + /// ``` + /// + /// Use instead: + /// ```no_run + /// // Good (as inner attribute) + /// #![allow(dead_code)] + /// + /// fn this_is_fine() {} + /// + /// // or + /// + /// // Good (as outer attribute) + /// #[allow(dead_code)] + /// fn this_is_fine_too() {} + /// ``` + #[clippy::version = "pre 1.29.0"] + pub EMPTY_LINE_AFTER_OUTER_ATTR, + suspicious, + "empty line after outer attribute" +} + +declare_clippy_lint! { + /// ### What it does + /// Checks for empty lines after doc comments. + /// + /// ### Why is this bad? + /// The doc comment may have meant to be an inner doc comment, regular + /// comment or applied to some old code that is now commented out. If it was + /// intended to be a doc comment, then the empty line should be removed. + /// + /// ### Example + /// ```no_run + /// /// Some doc comment with a blank line after it. + /// + /// fn f() {} + /// + /// /// Docs for `old_code` + /// // fn old_code() {} + /// + /// fn new_code() {} + /// ``` + /// + /// Use instead: + /// ```no_run + /// //! Convert it to an inner doc comment + /// + /// // Or a regular comment + /// + /// /// Or remove the empty line + /// fn f() {} + /// + /// // /// Docs for `old_code` + /// // fn old_code() {} + /// + /// fn new_code() {} + /// ``` + #[clippy::version = "1.70.0"] + pub EMPTY_LINE_AFTER_DOC_COMMENTS, + suspicious, + "empty line after doc comments" +} + +#[derive(Debug)] +struct ItemInfo { + kind: &'static str, + name: Symbol, + span: Span, + mod_items: Vec, +} + +pub struct EmptyLineAfter { + items: Vec, +} + +impl_lint_pass!(EmptyLineAfter => [ + EMPTY_LINE_AFTER_OUTER_ATTR, + EMPTY_LINE_AFTER_DOC_COMMENTS, +]); + +impl EmptyLineAfter { + pub fn new() -> Self { + Self { items: Vec::new() } + } +} + +#[derive(Debug, PartialEq, Clone, Copy)] +enum StopKind { + Attr, + Doc(CommentKind), +} + +impl StopKind { + fn is_doc(self) -> bool { + matches!(self, StopKind::Doc(_)) + } +} + +#[derive(Debug)] +struct Stop { + span: Span, + kind: StopKind, + first: usize, + last: usize, +} + +impl Stop { + fn convert_to_inner(&self) -> (Span, String) { + let inner = match self.kind { + // #|[...] + StopKind::Attr => InnerSpan::new(1, 1), + // /// or /** + // ^ ^ + StopKind::Doc(_) => InnerSpan::new(2, 3), + }; + (self.span.from_inner(inner), "!".into()) + } + + fn comment_out(&self, cx: &EarlyContext<'_>, suggestions: &mut Vec<(Span, String)>) { + match self.kind { + StopKind::Attr => { + if cx.sess().source_map().is_multiline(self.span) { + suggestions.extend([ + (self.span.shrink_to_lo(), "/* ".into()), + (self.span.shrink_to_hi(), " */".into()), + ]); + } else { + suggestions.push((self.span.shrink_to_lo(), "// ".into())); + } + }, + StopKind::Doc(CommentKind::Line) => suggestions.push((self.span.shrink_to_lo(), "// ".into())), + StopKind::Doc(CommentKind::Block) => { + // /** outer */ /*! inner */ + // ^ ^ + let asterisk = self.span.from_inner(InnerSpan::new(1, 2)); + suggestions.push((asterisk, String::new())); + }, + } + } + + fn from_attr(cx: &EarlyContext<'_>, attr: &Attribute) -> Option { + let SpanData { lo, hi, .. } = attr.span.data(); + let file = cx.sess().source_map().lookup_source_file(lo); + + Some(Self { + span: attr.span, + kind: match attr.kind { + AttrKind::Normal(_) => StopKind::Attr, + AttrKind::DocComment(comment_kind, _) => StopKind::Doc(comment_kind), + }, + first: file.lookup_line(file.relative_position(lo))?, + last: file.lookup_line(file.relative_position(hi))?, + }) + } +} + +/// Represents a set of attrs/doc comments separated by 1 or more empty lines +/// +/// ```ignore +/// /// chunk 1 docs +/// // not an empty line so also part of chunk 1 +/// #[chunk_1_attrs] // <-- prev_stop +/// +/// /* gap */ +/// +/// /// chunk 2 docs // <-- next_stop +/// #[chunk_2_attrs] +/// ``` +struct Gap<'a> { + /// The span of individual empty lines including the newline at the end of the line + empty_lines: Vec, + has_comment: bool, + next_stop: &'a Stop, + prev_stop: &'a Stop, + /// The chunk that includes [`prev_stop`](Self::prev_stop) + prev_chunk: &'a [Stop], +} + +impl<'a> Gap<'a> { + fn new(cx: &EarlyContext<'_>, prev_chunk: &'a [Stop], next_chunk: &'a [Stop]) -> Option { + let prev_stop = prev_chunk.last()?; + let next_stop = next_chunk.first()?; + let gap_span = prev_stop.span.between(next_stop.span); + let gap_snippet = gap_span.get_source_text(cx)?; + + let mut has_comment = false; + let mut empty_lines = Vec::new(); + + for (token, source, inner_span) in tokenize_with_text(&gap_snippet) { + match token { + TokenKind::BlockComment { + doc_style: None, + terminated: true, + } + | TokenKind::LineComment { doc_style: None } => has_comment = true, + TokenKind::Whitespace => { + let newlines = source.bytes().positions(|b| b == b'\n'); + empty_lines.extend( + newlines + .tuple_windows() + .map(|(a, b)| InnerSpan::new(inner_span.start + a + 1, inner_span.start + b)) + .map(|inner_span| gap_span.from_inner(inner_span)), + ); + }, + // Ignore cfg_attr'd out attributes as they may contain empty lines, could also be from macro + // shenanigans + _ => return None, + } + } + + (!empty_lines.is_empty()).then_some(Self { + empty_lines, + has_comment, + next_stop, + prev_stop, + prev_chunk, + }) + } + + fn contiguous_empty_lines(&self) -> impl Iterator + '_ { + self.empty_lines + // The `+ BytePos(1)` means "next line", because each empty line span is "N:1-N:1". + .chunk_by(|a, b| a.hi() + BytePos(1) == b.lo()) + .map(|chunk| { + let first = chunk.first().expect("at least one empty line"); + let last = chunk.last().expect("at least one empty line"); + // The BytePos subtraction here is safe, as before an empty line, there must be at least one + // attribute/comment. The span needs to start at the end of the previous line. + first.with_lo(first.lo() - BytePos(1)).with_hi(last.hi()) + }) + } +} + +impl EmptyLineAfter { + fn check_gaps(&self, cx: &EarlyContext<'_>, gaps: &[Gap<'_>], id: NodeId) { + let Some(first_gap) = gaps.first() else { + return; + }; + let empty_lines = || gaps.iter().flat_map(|gap| gap.empty_lines.iter().copied()); + let contiguous_empty_lines = || gaps.iter().flat_map(Gap::contiguous_empty_lines); + let mut has_comment = false; + let mut has_attr = false; + for gap in gaps { + has_comment |= gap.has_comment; + if !has_attr { + has_attr = gap.prev_chunk.iter().any(|stop| stop.kind == StopKind::Attr); + } + } + let kind = first_gap.prev_stop.kind; + let (lint, kind_desc) = match kind { + StopKind::Attr => (EMPTY_LINE_AFTER_OUTER_ATTR, "outer attribute"), + StopKind::Doc(_) => (EMPTY_LINE_AFTER_DOC_COMMENTS, "doc comment"), + }; + let (lines, are, them) = if empty_lines().nth(1).is_some() { + ("lines", "are", "them") + } else { + ("line", "is", "it") + }; + span_lint_and_then( + cx, + lint, + first_gap.prev_stop.span.to(empty_lines().last().unwrap()), + format!("empty {lines} after {kind_desc}"), + |diag| { + let info = self.items.last().unwrap(); + diag.span_label(info.span, match kind { + StopKind::Attr => format!("the attribute applies to this {}", info.kind), + StopKind::Doc(_) => format!("the comment documents this {}", info.kind), + }); + + diag.multipart_suggestion_with_style( + format!("if the empty {lines} {are} unintentional remove {them}"), + contiguous_empty_lines() + .map(|empty_lines| (empty_lines, String::new())) + .collect(), + Applicability::MaybeIncorrect, + SuggestionStyle::HideCodeAlways, + ); + + if has_comment && kind.is_doc() { + // Likely doc comments that applied to some now commented out code + // + // /// Old docs for Foo + // // struct Foo; + + let mut suggestions = Vec::new(); + for stop in gaps.iter().flat_map(|gap| gap.prev_chunk) { + stop.comment_out(cx, &mut suggestions); + } + diag.multipart_suggestion_verbose( + format!("if the doc comment should not document `{}` comment it out", info.name), + suggestions, + Applicability::MaybeIncorrect, + ); + } else { + self.suggest_inner(diag, kind, gaps, id); + } + + if kind == StopKind::Doc(CommentKind::Line) + && gaps + .iter() + .all(|gap| !gap.has_comment && gap.next_stop.kind == StopKind::Doc(CommentKind::Line)) + { + // Commentless empty gaps between line doc comments, possibly intended to be part of the markdown + + let indent = snippet_indent(cx, first_gap.prev_stop.span).unwrap_or_default(); + diag.multipart_suggestion_verbose( + format!("if the documentation should include the empty {lines} include {them} in the comment"), + empty_lines() + .map(|empty_line| (empty_line, format!("{indent}///"))) + .collect(), + Applicability::MaybeIncorrect, + ); + } + }, + ); + } + + /// If the node the attributes/docs apply to is the first in the module/crate suggest converting + /// them to inner attributes/docs + fn suggest_inner(&self, diag: &mut Diag<'_, ()>, kind: StopKind, gaps: &[Gap<'_>], id: NodeId) { + if let Some(parent) = self.items.iter().rev().nth(1) + && (parent.kind == "module" || parent.kind == "crate") + && parent.mod_items.first() == Some(&id) + { + let desc = if parent.kind == "module" { + "parent module" + } else { + parent.kind + }; + diag.multipart_suggestion_verbose( + match kind { + StopKind::Attr => format!("if the attribute should apply to the {desc} use an inner attribute"), + StopKind::Doc(_) => format!("if the comment should document the {desc} use an inner doc comment"), + }, + gaps.iter() + .flat_map(|gap| gap.prev_chunk) + .map(Stop::convert_to_inner) + .collect(), + Applicability::MaybeIncorrect, + ); + } + } +} + +impl EarlyLintPass for EmptyLineAfter { + fn check_crate(&mut self, _: &EarlyContext<'_>, krate: &Crate) { + self.items.push(ItemInfo { + kind: "crate", + name: kw::Crate, + span: krate.spans.inner_span.with_hi(krate.spans.inner_span.lo()), + mod_items: krate + .items + .iter() + .filter(|i| !matches!(i.span.ctxt().outer_expn_data().kind, ExpnKind::AstPass(_))) + .map(|i| i.id) + .collect::>(), + }); + } + + fn check_item_post(&mut self, _: &EarlyContext<'_>, _: &Item) { + self.items.pop(); + } + + fn check_item(&mut self, cx: &EarlyContext<'_>, item: &Item) { + self.items.push(ItemInfo { + kind: item.kind.descr(), + name: item.ident.name, + span: if item.span.contains(item.ident.span) { + item.span.with_hi(item.ident.span.hi()) + } else { + item.span.with_hi(item.span.lo()) + }, + mod_items: match item.kind { + ItemKind::Mod(_, ModKind::Loaded(ref items, _, _, _)) => items + .iter() + .filter(|i| !matches!(i.span.ctxt().outer_expn_data().kind, ExpnKind::AstPass(_))) + .map(|i| i.id) + .collect::>(), + _ => Vec::new(), + }, + }); + + let mut outer = item + .attrs + .iter() + .filter(|attr| attr.style == AttrStyle::Outer && !attr.span.from_expansion()) + .map(|attr| Stop::from_attr(cx, attr)) + .collect::>>() + .unwrap_or_default(); + + if outer.is_empty() { + return; + } + + // Push a fake attribute Stop for the item itself so we check for gaps between the last outer + // attr/doc comment and the item they apply to + let span = self.items.last().unwrap().span; + if !span.from_expansion() + && let Ok(line) = cx.sess().source_map().lookup_line(span.lo()) + { + outer.push(Stop { + span, + kind: StopKind::Attr, + first: line.line, + // last doesn't need to be accurate here, we don't compare it with anything + last: line.line, + }); + } + + let mut gaps = Vec::new(); + let mut last = 0; + for pos in outer + .array_windows() + .positions(|[a, b]| b.first.saturating_sub(a.last) > 1) + { + // we want to be after the first stop in the window + let pos = pos + 1; + if let Some(gap) = Gap::new(cx, &outer[last..pos], &outer[pos..]) { + last = pos; + gaps.push(gap); + } + } + + self.check_gaps(cx, &gaps, item.id); + } +} diff --git a/src/tools/clippy/clippy_lints/src/lib.rs b/src/tools/clippy/clippy_lints/src/lib.rs index 8887ab7ec0d7b..13218331a67b2 100644 --- a/src/tools/clippy/clippy_lints/src/lib.rs +++ b/src/tools/clippy/clippy_lints/src/lib.rs @@ -126,6 +126,7 @@ mod duplicate_mod; mod else_if_without_else; mod empty_drop; mod empty_enum; +mod empty_line_after; mod empty_with_brackets; mod endian_bytes; mod entry; @@ -973,6 +974,7 @@ pub fn register_lints(store: &mut rustc_lint::LintStore, conf: &'static Conf) { store.register_late_pass(move |_| Box::new(unused_trait_names::UnusedTraitNames::new(conf))); store.register_late_pass(|_| Box::new(manual_ignore_case_cmp::ManualIgnoreCaseCmp)); store.register_late_pass(|_| Box::new(unnecessary_literal_bound::UnnecessaryLiteralBound)); + store.register_early_pass(|| Box::new(empty_line_after::EmptyLineAfter::new())); store.register_late_pass(move |_| Box::new(arbitrary_source_item_ordering::ArbitrarySourceItemOrdering::new(conf))); store.register_late_pass(|_| Box::new(unneeded_struct_pattern::UnneededStructPattern)); store.register_late_pass(|_| Box::::default()); From cca89952a29beaf9909e5ab7aaa9ec8edde0c104 Mon Sep 17 00:00:00 2001 From: Guillaume Gomez Date: Thu, 6 Feb 2025 21:28:44 +0100 Subject: [PATCH 2/5] Update UI tests --- .../clippy_lints/src/empty_line_after.rs | 2 +- .../ui/empty_line_after/doc_comments.stderr | 18 +++++++++--------- .../ui/empty_line_after/outer_attribute.stderr | 12 ++++++------ .../tests/ui/suspicious_doc_comments.fixed | 1 + .../clippy/tests/ui/suspicious_doc_comments.rs | 1 + .../tests/ui/suspicious_doc_comments.stderr | 18 +++++++++--------- 6 files changed, 27 insertions(+), 25 deletions(-) diff --git a/src/tools/clippy/clippy_lints/src/empty_line_after.rs b/src/tools/clippy/clippy_lints/src/empty_line_after.rs index 7e0a2690d54e5..4f1c6186cfe87 100644 --- a/src/tools/clippy/clippy_lints/src/empty_line_after.rs +++ b/src/tools/clippy/clippy_lints/src/empty_line_after.rs @@ -133,7 +133,7 @@ struct Stop { impl Stop { fn convert_to_inner(&self) -> (Span, String) { let inner = match self.kind { - // #|[...] + // #![...] StopKind::Attr => InnerSpan::new(1, 1), // /// or /** // ^ ^ diff --git a/src/tools/clippy/tests/ui/empty_line_after/doc_comments.stderr b/src/tools/clippy/tests/ui/empty_line_after/doc_comments.stderr index c5d5f3d375947..d71c888e19662 100644 --- a/src/tools/clippy/tests/ui/empty_line_after/doc_comments.stderr +++ b/src/tools/clippy/tests/ui/empty_line_after/doc_comments.stderr @@ -5,7 +5,7 @@ LL | / /// for the crate LL | | | |_^ LL | fn first_in_crate() {} - | ------------------- the comment documents this function + | ----------------- the comment documents this function | = note: `-D clippy::empty-line-after-doc-comments` implied by `-D warnings` = help: to override `-D warnings` add `#[allow(clippy::empty_line_after_doc_comments)]` @@ -24,7 +24,7 @@ LL | / /// for the module LL | | | |_^ LL | fn first_in_module() {} - | -------------------- the comment documents this function + | ------------------ the comment documents this function | = help: if the empty line is unintentional remove it help: if the comment should document the parent module use an inner doc comment @@ -42,7 +42,7 @@ LL | | | |_^ LL | /// Blank line LL | fn indented() {} - | ------------- the comment documents this function + | ----------- the comment documents this function | = help: if the empty line is unintentional remove it help: if the documentation should include the empty line include it in the comment @@ -57,7 +57,7 @@ LL | / /// This should produce a warning LL | | | |_^ LL | fn with_doc_and_newline() {} - | ------------------------- the comment documents this function + | ----------------------- the comment documents this function | = help: if the empty line is unintentional remove it @@ -72,7 +72,7 @@ LL | | | |_^ ... LL | fn three_attributes() {} - | --------------------- the comment documents this function + | ------------------- the comment documents this function | = help: if the empty lines are unintentional remove them @@ -84,7 +84,7 @@ LL | | // fn old_code() {} LL | | | |_^ LL | fn new_code() {} - | ------------- the comment documents this function + | ----------- the comment documents this function | = help: if the empty line is unintentional remove it help: if the doc comment should not document `new_code` comment it out @@ -126,7 +126,7 @@ LL | | */ LL | | | |_^ LL | fn first_in_module() {} - | -------------------- the comment documents this function + | ------------------ the comment documents this function | = help: if the empty line is unintentional remove it help: if the comment should document the parent module use an inner doc comment @@ -145,7 +145,7 @@ LL | | | |_^ ... LL | fn new_code() {} - | ------------- the comment documents this function + | ----------- the comment documents this function | = help: if the empty line is unintentional remove it help: if the doc comment should not document `new_code` comment it out @@ -163,7 +163,7 @@ LL | | | |_^ LL | /// Docs for `new_code2` LL | fn new_code2() {} - | -------------- the comment documents this function + | ------------ the comment documents this function | = help: if the empty line is unintentional remove it help: if the doc comment should not document `new_code2` comment it out diff --git a/src/tools/clippy/tests/ui/empty_line_after/outer_attribute.stderr b/src/tools/clippy/tests/ui/empty_line_after/outer_attribute.stderr index a95306e2fa335..b4c49d0b31661 100644 --- a/src/tools/clippy/tests/ui/empty_line_after/outer_attribute.stderr +++ b/src/tools/clippy/tests/ui/empty_line_after/outer_attribute.stderr @@ -5,7 +5,7 @@ LL | / #[crate_type = "lib"] LL | | | |_^ LL | fn first_in_crate() {} - | ------------------- the attribute applies to this function + | ----------------- the attribute applies to this function | = note: `-D clippy::empty-line-after-outer-attr` implied by `-D warnings` = help: to override `-D warnings` add `#[allow(clippy::empty_line_after_outer_attr)]` @@ -23,7 +23,7 @@ LL | | | |_^ LL | /// some comment LL | fn with_one_newline_and_comment() {} - | --------------------------------- the attribute applies to this function + | ------------------------------- the attribute applies to this function | = help: if the empty line is unintentional remove it @@ -34,7 +34,7 @@ LL | / #[inline] LL | | | |_^ LL | fn with_one_newline() {} - | --------------------- the attribute applies to this function + | ------------------- the attribute applies to this function | = help: if the empty line is unintentional remove it @@ -46,7 +46,7 @@ LL | | LL | | | |_^ LL | fn with_two_newlines() {} - | ---------------------- the attribute applies to this function + | -------------------- the attribute applies to this function | = help: if the empty lines are unintentional remove them help: if the attribute should apply to the parent module use an inner attribute @@ -95,7 +95,7 @@ LL | | // Still lint cases where the empty line does not immediately follow the LL | | | |_^ LL | fn comment_before_empty_line() {} - | ------------------------------ the attribute applies to this function + | ---------------------------- the attribute applies to this function | = help: if the empty line is unintentional remove it @@ -107,7 +107,7 @@ LL | / #[allow(unused)] LL | | | |_^ LL | pub fn isolated_comment() {} - | ------------------------- the attribute applies to this function + | ----------------------- the attribute applies to this function | = help: if the empty lines are unintentional remove them diff --git a/src/tools/clippy/tests/ui/suspicious_doc_comments.fixed b/src/tools/clippy/tests/ui/suspicious_doc_comments.fixed index 614fc03571e53..d3df6a41cb12a 100644 --- a/src/tools/clippy/tests/ui/suspicious_doc_comments.fixed +++ b/src/tools/clippy/tests/ui/suspicious_doc_comments.fixed @@ -1,5 +1,6 @@ #![allow(unused)] #![warn(clippy::suspicious_doc_comments)] +#![allow(clippy::empty_line_after_doc_comments)] //! Real module documentation. //! Fake module documentation. diff --git a/src/tools/clippy/tests/ui/suspicious_doc_comments.rs b/src/tools/clippy/tests/ui/suspicious_doc_comments.rs index 7dcba0fefc981..04db2b199c097 100644 --- a/src/tools/clippy/tests/ui/suspicious_doc_comments.rs +++ b/src/tools/clippy/tests/ui/suspicious_doc_comments.rs @@ -1,5 +1,6 @@ #![allow(unused)] #![warn(clippy::suspicious_doc_comments)] +#![allow(clippy::empty_line_after_doc_comments)] //! Real module documentation. ///! Fake module documentation. diff --git a/src/tools/clippy/tests/ui/suspicious_doc_comments.stderr b/src/tools/clippy/tests/ui/suspicious_doc_comments.stderr index f12053b1595a1..c34e39cd0fcb5 100644 --- a/src/tools/clippy/tests/ui/suspicious_doc_comments.stderr +++ b/src/tools/clippy/tests/ui/suspicious_doc_comments.stderr @@ -1,5 +1,5 @@ error: this is an outer doc comment and does not apply to the parent module or crate - --> tests/ui/suspicious_doc_comments.rs:5:1 + --> tests/ui/suspicious_doc_comments.rs:6:1 | LL | ///! Fake module documentation. | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ @@ -12,7 +12,7 @@ LL | //! Fake module documentation. | error: this is an outer doc comment and does not apply to the parent module or crate - --> tests/ui/suspicious_doc_comments.rs:9:5 + --> tests/ui/suspicious_doc_comments.rs:10:5 | LL | ///! This module contains useful functions. | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ @@ -23,7 +23,7 @@ LL | //! This module contains useful functions. | error: this is an outer doc comment and does not apply to the parent module or crate - --> tests/ui/suspicious_doc_comments.rs:21:5 + --> tests/ui/suspicious_doc_comments.rs:22:5 | LL | / /**! This module contains useful functions. LL | | */ @@ -36,7 +36,7 @@ LL + */ | error: this is an outer doc comment and does not apply to the parent module or crate - --> tests/ui/suspicious_doc_comments.rs:35:5 + --> tests/ui/suspicious_doc_comments.rs:36:5 | LL | / ///! This module LL | | ///! contains @@ -51,7 +51,7 @@ LL ~ //! useful functions. | error: this is an outer doc comment and does not apply to the parent module or crate - --> tests/ui/suspicious_doc_comments.rs:43:5 + --> tests/ui/suspicious_doc_comments.rs:44:5 | LL | / ///! a LL | | ///! b @@ -64,7 +64,7 @@ LL ~ //! b | error: this is an outer doc comment and does not apply to the parent module or crate - --> tests/ui/suspicious_doc_comments.rs:51:5 + --> tests/ui/suspicious_doc_comments.rs:52:5 | LL | ///! a | ^^^^^^ @@ -75,7 +75,7 @@ LL | //! a | error: this is an outer doc comment and does not apply to the parent module or crate - --> tests/ui/suspicious_doc_comments.rs:57:5 + --> tests/ui/suspicious_doc_comments.rs:58:5 | LL | / ///! a LL | | @@ -90,7 +90,7 @@ LL ~ //! b | error: this is an outer doc comment and does not apply to the parent module or crate - --> tests/ui/suspicious_doc_comments.rs:69:5 + --> tests/ui/suspicious_doc_comments.rs:70:5 | LL | ///! Very cool macro | ^^^^^^^^^^^^^^^^^^^^ @@ -101,7 +101,7 @@ LL | //! Very cool macro | error: this is an outer doc comment and does not apply to the parent module or crate - --> tests/ui/suspicious_doc_comments.rs:76:5 + --> tests/ui/suspicious_doc_comments.rs:77:5 | LL | ///! Huh. | ^^^^^^^^^ From 9221e33766172b229559a2e3f44e9164963d8519 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jonathan=20D=C3=B6nszelmann?= Date: Thu, 6 Feb 2025 21:44:00 +0100 Subject: [PATCH 3/5] fix empty after lint on impl/trait items Co-authored-by: Guillaume Gomez --- compiler/rustc_lint/src/early.rs | 8 ++ compiler/rustc_lint/src/passes.rs | 2 + .../clippy_lints/src/empty_line_after.rs | 113 ++++++++++++------ .../ui/empty_line_after/doc_comments.1.fixed | 9 ++ .../ui/empty_line_after/doc_comments.2.fixed | 9 ++ .../tests/ui/empty_line_after/doc_comments.rs | 10 ++ .../ui/empty_line_after/doc_comments.stderr | 13 +- 7 files changed, 126 insertions(+), 38 deletions(-) diff --git a/compiler/rustc_lint/src/early.rs b/compiler/rustc_lint/src/early.rs index bc7cd3d118c5a..723b894c43bc4 100644 --- a/compiler/rustc_lint/src/early.rs +++ b/compiler/rustc_lint/src/early.rs @@ -246,6 +246,14 @@ impl<'ast, 'ecx, 'tcx, T: EarlyLintPass> ast_visit::Visitor<'ast> } } ast_visit::walk_assoc_item(cx, item, ctxt); + match ctxt { + ast_visit::AssocCtxt::Trait => { + lint_callback!(cx, check_trait_item_post, item); + } + ast_visit::AssocCtxt::Impl => { + lint_callback!(cx, check_impl_item_post, item); + } + } }); } diff --git a/compiler/rustc_lint/src/passes.rs b/compiler/rustc_lint/src/passes.rs index 77bd13aacf737..409a23d1da039 100644 --- a/compiler/rustc_lint/src/passes.rs +++ b/compiler/rustc_lint/src/passes.rs @@ -162,7 +162,9 @@ macro_rules! early_lint_methods { c: rustc_span::Span, d_: rustc_ast::NodeId); fn check_trait_item(a: &rustc_ast::AssocItem); + fn check_trait_item_post(a: &rustc_ast::AssocItem); fn check_impl_item(a: &rustc_ast::AssocItem); + fn check_impl_item_post(a: &rustc_ast::AssocItem); fn check_variant(a: &rustc_ast::Variant); fn check_attribute(a: &rustc_ast::Attribute); fn check_attributes(a: &[rustc_ast::Attribute]); diff --git a/src/tools/clippy/clippy_lints/src/empty_line_after.rs b/src/tools/clippy/clippy_lints/src/empty_line_after.rs index 4f1c6186cfe87..89ddd885b6e45 100644 --- a/src/tools/clippy/clippy_lints/src/empty_line_after.rs +++ b/src/tools/clippy/clippy_lints/src/empty_line_after.rs @@ -3,13 +3,13 @@ use clippy_utils::source::{SpanRangeExt, snippet_indent}; use clippy_utils::tokenize_with_text; use itertools::Itertools; use rustc_ast::token::CommentKind; -use rustc_ast::{AttrKind, AttrStyle, Attribute, Crate, Item, ItemKind, ModKind, NodeId}; +use rustc_ast::{AssocItemKind, AttrKind, AttrStyle, Attribute, Crate, Item, ItemKind, ModKind, NodeId}; use rustc_errors::{Applicability, Diag, SuggestionStyle}; use rustc_lexer::TokenKind; use rustc_lint::{EarlyContext, EarlyLintPass, LintContext}; use rustc_session::impl_lint_pass; use rustc_span::symbol::kw; -use rustc_span::{BytePos, ExpnKind, InnerSpan, Span, SpanData, Symbol}; +use rustc_span::{BytePos, ExpnKind, Ident, InnerSpan, Span, SpanData, Symbol}; declare_clippy_lint! { /// ### What it does @@ -92,7 +92,7 @@ struct ItemInfo { kind: &'static str, name: Symbol, span: Span, - mod_items: Vec, + mod_items: Option, } pub struct EmptyLineAfter { @@ -347,7 +347,7 @@ impl EmptyLineAfter { fn suggest_inner(&self, diag: &mut Diag<'_, ()>, kind: StopKind, gaps: &[Gap<'_>], id: NodeId) { if let Some(parent) = self.items.iter().rev().nth(1) && (parent.kind == "module" || parent.kind == "crate") - && parent.mod_items.first() == Some(&id) + && parent.mod_items == Some(id) { let desc = if parent.kind == "module" { "parent module" @@ -367,48 +367,35 @@ impl EmptyLineAfter { ); } } -} -impl EarlyLintPass for EmptyLineAfter { - fn check_crate(&mut self, _: &EarlyContext<'_>, krate: &Crate) { + fn check_item_kind( + &mut self, + cx: &EarlyContext<'_>, + kind: &ItemKind, + ident: &Ident, + span: Span, + attrs: &[Attribute], + id: NodeId, + ) { self.items.push(ItemInfo { - kind: "crate", - name: kw::Crate, - span: krate.spans.inner_span.with_hi(krate.spans.inner_span.lo()), - mod_items: krate - .items - .iter() - .filter(|i| !matches!(i.span.ctxt().outer_expn_data().kind, ExpnKind::AstPass(_))) - .map(|i| i.id) - .collect::>(), - }); - } - - fn check_item_post(&mut self, _: &EarlyContext<'_>, _: &Item) { - self.items.pop(); - } - - fn check_item(&mut self, cx: &EarlyContext<'_>, item: &Item) { - self.items.push(ItemInfo { - kind: item.kind.descr(), - name: item.ident.name, - span: if item.span.contains(item.ident.span) { - item.span.with_hi(item.ident.span.hi()) + kind: kind.descr(), + name: ident.name, + span: if span.contains(ident.span) { + span.with_hi(ident.span.hi()) } else { - item.span.with_hi(item.span.lo()) + span.with_hi(span.lo()) }, - mod_items: match item.kind { - ItemKind::Mod(_, ModKind::Loaded(ref items, _, _, _)) => items + mod_items: match kind { + ItemKind::Mod(_, ModKind::Loaded(items, _, _, _)) => items .iter() .filter(|i| !matches!(i.span.ctxt().outer_expn_data().kind, ExpnKind::AstPass(_))) .map(|i| i.id) - .collect::>(), - _ => Vec::new(), + .next(), + _ => None, }, }); - let mut outer = item - .attrs + let mut outer = attrs .iter() .filter(|attr| attr.style == AttrStyle::Outer && !attr.span.from_expansion()) .map(|attr| Stop::from_attr(cx, attr)) @@ -448,6 +435,58 @@ impl EarlyLintPass for EmptyLineAfter { } } - self.check_gaps(cx, &gaps, item.id); + self.check_gaps(cx, &gaps, id); + } +} + +impl EarlyLintPass for EmptyLineAfter { + fn check_crate(&mut self, _: &EarlyContext<'_>, krate: &Crate) { + self.items.push(ItemInfo { + kind: "crate", + name: kw::Crate, + span: krate.spans.inner_span.with_hi(krate.spans.inner_span.lo()), + mod_items: krate + .items + .iter() + .filter(|i| !matches!(i.span.ctxt().outer_expn_data().kind, ExpnKind::AstPass(_))) + .map(|i| i.id) + .next(), + }); + } + + fn check_item_post(&mut self, _: &EarlyContext<'_>, _: &Item) { + self.items.pop(); + } + fn check_impl_item_post(&mut self, _: &EarlyContext<'_>, _: &Item) { + self.items.pop(); + } + fn check_trait_item_post(&mut self, _: &EarlyContext<'_>, _: &Item) { + self.items.pop(); + } + + fn check_impl_item(&mut self, cx: &EarlyContext<'_>, item: &Item) { + self.check_item_kind( + cx, + &item.kind.clone().into(), + &item.ident, + item.span, + &item.attrs, + item.id, + ); + } + + fn check_trait_item(&mut self, cx: &EarlyContext<'_>, item: &Item) { + self.check_item_kind( + cx, + &item.kind.clone().into(), + &item.ident, + item.span, + &item.attrs, + item.id, + ); + } + + fn check_item(&mut self, cx: &EarlyContext<'_>, item: &Item) { + self.check_item_kind(cx, &item.kind, &item.ident, item.span, &item.attrs, item.id); } } diff --git a/src/tools/clippy/tests/ui/empty_line_after/doc_comments.1.fixed b/src/tools/clippy/tests/ui/empty_line_after/doc_comments.1.fixed index fd6a94b6a80c8..3772b465fdb2a 100644 --- a/src/tools/clippy/tests/ui/empty_line_after/doc_comments.1.fixed +++ b/src/tools/clippy/tests/ui/empty_line_after/doc_comments.1.fixed @@ -132,4 +132,13 @@ pub struct BlockComment; ))] fn empty_line_in_cfg_attr() {} +trait Foo { + fn bar(); +} + +impl Foo for LineComment { + /// comment on assoc item + fn bar() {} +} + fn main() {} diff --git a/src/tools/clippy/tests/ui/empty_line_after/doc_comments.2.fixed b/src/tools/clippy/tests/ui/empty_line_after/doc_comments.2.fixed index 7a57dcd92332b..3028d03b66994 100644 --- a/src/tools/clippy/tests/ui/empty_line_after/doc_comments.2.fixed +++ b/src/tools/clippy/tests/ui/empty_line_after/doc_comments.2.fixed @@ -141,4 +141,13 @@ pub struct BlockComment; ))] fn empty_line_in_cfg_attr() {} +trait Foo { + fn bar(); +} + +impl Foo for LineComment { + /// comment on assoc item + fn bar() {} +} + fn main() {} diff --git a/src/tools/clippy/tests/ui/empty_line_after/doc_comments.rs b/src/tools/clippy/tests/ui/empty_line_after/doc_comments.rs index 1da761a5c3d52..ae4ebc271fa1c 100644 --- a/src/tools/clippy/tests/ui/empty_line_after/doc_comments.rs +++ b/src/tools/clippy/tests/ui/empty_line_after/doc_comments.rs @@ -144,4 +144,14 @@ pub struct BlockComment; ))] fn empty_line_in_cfg_attr() {} +trait Foo { + fn bar(); +} + +impl Foo for LineComment { + /// comment on assoc item + + fn bar() {} +} + fn main() {} diff --git a/src/tools/clippy/tests/ui/empty_line_after/doc_comments.stderr b/src/tools/clippy/tests/ui/empty_line_after/doc_comments.stderr index d71c888e19662..10189665f014a 100644 --- a/src/tools/clippy/tests/ui/empty_line_after/doc_comments.stderr +++ b/src/tools/clippy/tests/ui/empty_line_after/doc_comments.stderr @@ -171,5 +171,16 @@ help: if the doc comment should not document `new_code2` comment it out LL | // /// Docs for `old_code2` | ++ -error: aborting due to 10 previous errors +error: empty line after doc comment + --> tests/ui/empty_line_after/doc_comments.rs:152:5 + | +LL | / /// comment on assoc item +LL | | + | |_^ +LL | fn bar() {} + | ------ the comment documents this function + | + = help: if the empty line is unintentional remove it + +error: aborting due to 11 previous errors From bed71f93358daecc12130e2ee2ba8a5ebd7b3f6e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jana=20D=C3=B6nszelmann?= Date: Fri, 7 Feb 2025 16:00:27 +0100 Subject: [PATCH 4/5] fix typo --- src/tools/clippy/clippy_lints/src/empty_line_after.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/tools/clippy/clippy_lints/src/empty_line_after.rs b/src/tools/clippy/clippy_lints/src/empty_line_after.rs index 89ddd885b6e45..578ff6e38a6a6 100644 --- a/src/tools/clippy/clippy_lints/src/empty_line_after.rs +++ b/src/tools/clippy/clippy_lints/src/empty_line_after.rs @@ -295,7 +295,7 @@ impl EmptyLineAfter { }); diag.multipart_suggestion_with_style( - format!("if the empty {lines} {are} unintentional remove {them}"), + format!("if the empty {lines} {are} unintentional, remove {them}"), contiguous_empty_lines() .map(|empty_lines| (empty_lines, String::new())) .collect(), From cd52a95b052560e63e08e456f4429faf498c30eb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jana=20D=C3=B6nszelmann?= Date: Fri, 7 Feb 2025 16:42:20 +0100 Subject: [PATCH 5/5] add tests for spurious failure and fix typo --- .../clippy/tests/ui/doc/unbalanced_ticks.rs | 16 ++++++++++++++ .../tests/ui/doc/unbalanced_ticks.stderr | 14 +++++++++++- .../ui/empty_line_after/doc_comments.stderr | 22 +++++++++---------- .../empty_line_after/outer_attribute.stderr | 18 +++++++-------- 4 files changed, 49 insertions(+), 21 deletions(-) diff --git a/src/tools/clippy/tests/ui/doc/unbalanced_ticks.rs b/src/tools/clippy/tests/ui/doc/unbalanced_ticks.rs index 04446787b6c29..a065654e319d0 100644 --- a/src/tools/clippy/tests/ui/doc/unbalanced_ticks.rs +++ b/src/tools/clippy/tests/ui/doc/unbalanced_ticks.rs @@ -66,3 +66,19 @@ fn escape_3() {} /// Backslashes ` \` within code blocks don't count. fn escape_4() {} + +trait Foo { + fn bar(); +} + +struct Bar; +impl Foo for Bar { + // NOTE: false positive + /// Returns an `Option` from a i64, assuming a 1-index, January = 1. + /// + /// `Month::from_i64(n: i64)`: | `1` | `2` | ... | `12` + /// ---------------------------| -------------------- | --------------------- | ... | ----- + /// ``: | Some(Month::January) | Some(Month::February) | ... | + /// Some(Month::December) + fn bar() {} +} diff --git a/src/tools/clippy/tests/ui/doc/unbalanced_ticks.stderr b/src/tools/clippy/tests/ui/doc/unbalanced_ticks.stderr index 50324010e97f7..c9fd25eb1a1c8 100644 --- a/src/tools/clippy/tests/ui/doc/unbalanced_ticks.stderr +++ b/src/tools/clippy/tests/ui/doc/unbalanced_ticks.stderr @@ -94,5 +94,17 @@ LL | /// Escaped \` ` backticks don't count, but unescaped backticks do. | = help: a backtick may be missing a pair -error: aborting due to 10 previous errors +error: backticks are unbalanced + --> tests/ui/doc/unbalanced_ticks.rs:79:9 + | +LL | /// `Month::from_i64(n: i64)`: | `1` | `2` | ... | `12` + | _________^ +LL | | /// ---------------------------| -------------------- | --------------------- | ... | ----- +LL | | /// ``: | Some(Month::January) | Some(Month::February) | ... | +LL | | /// Some(Month::December) + | |_____________________________^ + | + = help: a backtick may be missing a pair + +error: aborting due to 11 previous errors diff --git a/src/tools/clippy/tests/ui/empty_line_after/doc_comments.stderr b/src/tools/clippy/tests/ui/empty_line_after/doc_comments.stderr index 10189665f014a..7b197ae67e00a 100644 --- a/src/tools/clippy/tests/ui/empty_line_after/doc_comments.stderr +++ b/src/tools/clippy/tests/ui/empty_line_after/doc_comments.stderr @@ -9,7 +9,7 @@ LL | fn first_in_crate() {} | = note: `-D clippy::empty-line-after-doc-comments` implied by `-D warnings` = help: to override `-D warnings` add `#[allow(clippy::empty_line_after_doc_comments)]` - = help: if the empty line is unintentional remove it + = help: if the empty line is unintentional, remove it help: if the comment should document the crate use an inner doc comment | LL ~ //! Meant to be an @@ -26,7 +26,7 @@ LL | | LL | fn first_in_module() {} | ------------------ the comment documents this function | - = help: if the empty line is unintentional remove it + = help: if the empty line is unintentional, remove it help: if the comment should document the parent module use an inner doc comment | LL ~ //! Meant to be an @@ -44,7 +44,7 @@ LL | /// Blank line LL | fn indented() {} | ----------- the comment documents this function | - = help: if the empty line is unintentional remove it + = help: if the empty line is unintentional, remove it help: if the documentation should include the empty line include it in the comment | LL | /// @@ -59,7 +59,7 @@ LL | | LL | fn with_doc_and_newline() {} | ----------------------- the comment documents this function | - = help: if the empty line is unintentional remove it + = help: if the empty line is unintentional, remove it error: empty lines after doc comment --> tests/ui/empty_line_after/doc_comments.rs:44:1 @@ -74,7 +74,7 @@ LL | | LL | fn three_attributes() {} | ------------------- the comment documents this function | - = help: if the empty lines are unintentional remove them + = help: if the empty lines are unintentional, remove them error: empty line after doc comment --> tests/ui/empty_line_after/doc_comments.rs:56:5 @@ -86,7 +86,7 @@ LL | | LL | fn new_code() {} | ----------- the comment documents this function | - = help: if the empty line is unintentional remove it + = help: if the empty line is unintentional, remove it help: if the doc comment should not document `new_code` comment it out | LL | // /// docs for `old_code` @@ -106,7 +106,7 @@ LL | | LL | struct Multiple; | --------------- the comment documents this struct | - = help: if the empty lines are unintentional remove them + = help: if the empty lines are unintentional, remove them help: if the doc comment should not document `Multiple` comment it out | LL ~ // /// Docs @@ -128,7 +128,7 @@ LL | | LL | fn first_in_module() {} | ------------------ the comment documents this function | - = help: if the empty line is unintentional remove it + = help: if the empty line is unintentional, remove it help: if the comment should document the parent module use an inner doc comment | LL | /*! @@ -147,7 +147,7 @@ LL | | LL | fn new_code() {} | ----------- the comment documents this function | - = help: if the empty line is unintentional remove it + = help: if the empty line is unintentional, remove it help: if the doc comment should not document `new_code` comment it out | LL - /** @@ -165,7 +165,7 @@ LL | /// Docs for `new_code2` LL | fn new_code2() {} | ------------ the comment documents this function | - = help: if the empty line is unintentional remove it + = help: if the empty line is unintentional, remove it help: if the doc comment should not document `new_code2` comment it out | LL | // /// Docs for `old_code2` @@ -180,7 +180,7 @@ LL | | LL | fn bar() {} | ------ the comment documents this function | - = help: if the empty line is unintentional remove it + = help: if the empty line is unintentional, remove it error: aborting due to 11 previous errors diff --git a/src/tools/clippy/tests/ui/empty_line_after/outer_attribute.stderr b/src/tools/clippy/tests/ui/empty_line_after/outer_attribute.stderr index b4c49d0b31661..519ba6e67615c 100644 --- a/src/tools/clippy/tests/ui/empty_line_after/outer_attribute.stderr +++ b/src/tools/clippy/tests/ui/empty_line_after/outer_attribute.stderr @@ -9,7 +9,7 @@ LL | fn first_in_crate() {} | = note: `-D clippy::empty-line-after-outer-attr` implied by `-D warnings` = help: to override `-D warnings` add `#[allow(clippy::empty_line_after_outer_attr)]` - = help: if the empty line is unintentional remove it + = help: if the empty line is unintentional, remove it help: if the attribute should apply to the crate use an inner attribute | LL | #![crate_type = "lib"] @@ -25,7 +25,7 @@ LL | /// some comment LL | fn with_one_newline_and_comment() {} | ------------------------------- the attribute applies to this function | - = help: if the empty line is unintentional remove it + = help: if the empty line is unintentional, remove it error: empty line after outer attribute --> tests/ui/empty_line_after/outer_attribute.rs:23:1 @@ -36,7 +36,7 @@ LL | | LL | fn with_one_newline() {} | ------------------- the attribute applies to this function | - = help: if the empty line is unintentional remove it + = help: if the empty line is unintentional, remove it error: empty lines after outer attribute --> tests/ui/empty_line_after/outer_attribute.rs:30:5 @@ -48,7 +48,7 @@ LL | | LL | fn with_two_newlines() {} | -------------------- the attribute applies to this function | - = help: if the empty lines are unintentional remove them + = help: if the empty lines are unintentional, remove them help: if the attribute should apply to the parent module use an inner attribute | LL | #![crate_type = "lib"] @@ -63,7 +63,7 @@ LL | | LL | enum Baz { | -------- the attribute applies to this enum | - = help: if the empty line is unintentional remove it + = help: if the empty line is unintentional, remove it error: empty line after outer attribute --> tests/ui/empty_line_after/outer_attribute.rs:45:1 @@ -74,7 +74,7 @@ LL | | LL | struct Foo { | ---------- the attribute applies to this struct | - = help: if the empty line is unintentional remove it + = help: if the empty line is unintentional, remove it error: empty line after outer attribute --> tests/ui/empty_line_after/outer_attribute.rs:53:1 @@ -85,7 +85,7 @@ LL | | LL | mod foo {} | ------- the attribute applies to this module | - = help: if the empty line is unintentional remove it + = help: if the empty line is unintentional, remove it error: empty line after outer attribute --> tests/ui/empty_line_after/outer_attribute.rs:58:1 @@ -97,7 +97,7 @@ LL | | LL | fn comment_before_empty_line() {} | ---------------------------- the attribute applies to this function | - = help: if the empty line is unintentional remove it + = help: if the empty line is unintentional, remove it error: empty lines after outer attribute --> tests/ui/empty_line_after/outer_attribute.rs:64:1 @@ -109,7 +109,7 @@ LL | | LL | pub fn isolated_comment() {} | ----------------------- the attribute applies to this function | - = help: if the empty lines are unintentional remove them + = help: if the empty lines are unintentional, remove them error: aborting due to 9 previous errors