From d3bc248e6d20462e539b9d7a4f84d6255838f735 Mon Sep 17 00:00:00 2001 From: Jane Lewis Date: Mon, 5 Feb 2024 10:42:35 -0500 Subject: [PATCH 01/13] Move `SuppressionKind` and `CommentLinePosition` to `ruff_python_trivia` --- .../src/comments/debug.rs | 4 +- .../src/comments/format.rs | 4 +- .../ruff_python_formatter/src/comments/mod.rs | 109 +----------------- .../src/comments/visitor.rs | 4 +- crates/ruff_python_formatter/src/lib.rs | 12 +- .../src/other/decorator.rs | 6 +- .../src/other/parameters.rs | 4 +- .../src/statement/clause.rs | 8 +- .../src/statement/stmt_ann_assign.rs | 6 +- .../src/statement/stmt_assert.rs | 6 +- .../src/statement/stmt_assign.rs | 6 +- .../src/statement/stmt_aug_assign.rs | 6 +- .../src/statement/stmt_break.rs | 6 +- .../src/statement/stmt_continue.rs | 6 +- .../src/statement/stmt_delete.rs | 6 +- .../src/statement/stmt_expr.rs | 6 +- .../src/statement/stmt_global.rs | 5 +- .../src/statement/stmt_import.rs | 6 +- .../src/statement/stmt_import_from.rs | 5 +- .../src/statement/stmt_ipy_escape_command.rs | 6 +- .../src/statement/stmt_nonlocal.rs | 5 +- .../src/statement/stmt_pass.rs | 6 +- .../src/statement/stmt_raise.rs | 6 +- .../src/statement/stmt_return.rs | 6 +- .../src/statement/stmt_type_alias.rs | 6 +- crates/ruff_python_trivia/src/lib.rs | 2 + crates/ruff_python_trivia/src/suppression.rs | 98 ++++++++++++++++ 27 files changed, 181 insertions(+), 169 deletions(-) create mode 100644 crates/ruff_python_trivia/src/suppression.rs diff --git a/crates/ruff_python_formatter/src/comments/debug.rs b/crates/ruff_python_formatter/src/comments/debug.rs index b28d992058464..a91b61bc2e6f9 100644 --- a/crates/ruff_python_formatter/src/comments/debug.rs +++ b/crates/ruff_python_formatter/src/comments/debug.rs @@ -182,11 +182,11 @@ mod tests { use ruff_formatter::SourceCode; use ruff_python_ast::AnyNode; use ruff_python_ast::{StmtBreak, StmtContinue}; - use ruff_python_trivia::CommentRanges; + use ruff_python_trivia::{CommentLinePosition, CommentRanges}; use ruff_text_size::{TextRange, TextSize}; use crate::comments::map::MultiMap; - use crate::comments::{CommentLinePosition, Comments, CommentsMap, SourceComment}; + use crate::comments::{Comments, CommentsMap, SourceComment}; #[test] fn debug() { diff --git a/crates/ruff_python_formatter/src/comments/format.rs b/crates/ruff_python_formatter/src/comments/format.rs index 13f1cdf2880f1..b6cc694def354 100644 --- a/crates/ruff_python_formatter/src/comments/format.rs +++ b/crates/ruff_python_formatter/src/comments/format.rs @@ -3,11 +3,11 @@ use std::borrow::Cow; use ruff_formatter::{format_args, write, FormatError, FormatOptions, SourceCode}; use ruff_python_ast::{AnyNodeRef, AstNode, NodeKind, PySourceType}; use ruff_python_trivia::{ - is_pragma_comment, lines_after, lines_after_ignoring_trivia, lines_before, + is_pragma_comment, lines_after, lines_after_ignoring_trivia, lines_before, CommentLinePosition, }; use ruff_text_size::{Ranged, TextLen, TextRange}; -use crate::comments::{CommentLinePosition, SourceComment}; +use crate::comments::SourceComment; use crate::context::NodeLevel; use crate::prelude::*; use crate::preview::is_blank_line_after_nested_stub_class_enabled; diff --git a/crates/ruff_python_formatter/src/comments/mod.rs b/crates/ruff_python_formatter/src/comments/mod.rs index 92c52ff09ce83..97939fb9d7706 100644 --- a/crates/ruff_python_formatter/src/comments/mod.rs +++ b/crates/ruff_python_formatter/src/comments/mod.rs @@ -98,7 +98,7 @@ pub(crate) use format::{ use ruff_formatter::{SourceCode, SourceCodeSlice}; use ruff_python_ast::visitor::preorder::{PreorderVisitor, TraversalSignal}; use ruff_python_ast::AnyNodeRef; -use ruff_python_trivia::{CommentRanges, PythonWhitespace}; +use ruff_python_trivia::{CommentLinePosition, CommentRanges, SuppressionKind}; use ruff_source_file::Locator; use ruff_text_size::{Ranged, TextRange}; pub(crate) use visitor::collect_comments; @@ -168,73 +168,16 @@ impl SourceComment { DebugComment::new(self, source_code) } - pub(crate) fn suppression_kind(&self, source: &str) -> Option { - let text = self.slice.text(SourceCode::new(source)); - - // Match against `# fmt: on`, `# fmt: off`, `# yapf: disable`, and `# yapf: enable`, which - // must be on their own lines. - let trimmed = text.strip_prefix('#').unwrap_or(text).trim_whitespace(); - if let Some(command) = trimmed.strip_prefix("fmt:") { - match command.trim_whitespace_start() { - "off" => return Some(SuppressionKind::Off), - "on" => return Some(SuppressionKind::On), - "skip" => return Some(SuppressionKind::Skip), - _ => {} - } - } else if let Some(command) = trimmed.strip_prefix("yapf:") { - match command.trim_whitespace_start() { - "disable" => return Some(SuppressionKind::Off), - "enable" => return Some(SuppressionKind::On), - _ => {} - } - } - - // Search for `# fmt: skip` comments, which can be interspersed with other comments (e.g., - // `# fmt: skip # noqa: E501`). - for segment in text.split('#') { - let trimmed = segment.trim_whitespace(); - if let Some(command) = trimmed.strip_prefix("fmt:") { - if command.trim_whitespace_start() == "skip" { - return Some(SuppressionKind::Skip); - } - } - } - - None - } - - /// Returns true if this comment is a `fmt: off` or `yapf: disable` own line suppression comment. pub(crate) fn is_suppression_off_comment(&self, source: &str) -> bool { - self.line_position.is_own_line() - && matches!(self.suppression_kind(source), Some(SuppressionKind::Off)) + SuppressionKind::is_suppression_off(self.slice_source(source), self.line_position) } - /// Returns true if this comment is a `fmt: on` or `yapf: enable` own line suppression comment. pub(crate) fn is_suppression_on_comment(&self, source: &str) -> bool { - self.line_position.is_own_line() - && matches!(self.suppression_kind(source), Some(SuppressionKind::On)) + SuppressionKind::is_suppression_on(self.slice_source(source), self.line_position) } -} - -#[derive(Copy, Clone, Eq, PartialEq, Debug)] -pub(crate) enum SuppressionKind { - /// A `fmt: off` or `yapf: disable` comment - Off, - /// A `fmt: on` or `yapf: enable` comment - On, - /// A `fmt: skip` comment - Skip, -} -impl SuppressionKind { - pub(crate) fn has_skip_comment(trailing_comments: &[SourceComment], source: &str) -> bool { - trailing_comments.iter().any(|comment| { - comment.line_position().is_end_of_line() - && matches!( - comment.suppression_kind(source), - Some(SuppressionKind::Skip | SuppressionKind::Off) - ) - }) + fn slice_source<'a>(&self, source: &'a str) -> &'a str { + self.slice.text(SourceCode::new(source)) } } @@ -245,48 +188,6 @@ impl Ranged for SourceComment { } } -/// The position of a comment in the source text. -#[derive(Debug, Copy, Clone, Eq, PartialEq)] -pub(crate) enum CommentLinePosition { - /// A comment that is on the same line as the preceding token and is separated by at least one line break from the following token. - /// - /// # Examples - /// - /// ## End of line - /// - /// ```python - /// a; # comment - /// b; - /// ``` - /// - /// `# comment` is an end of line comments because it is separated by at least one line break from the following token `b`. - /// Comments that not only end, but also start on a new line are [`OwnLine`](CommentLinePosition::OwnLine) comments. - EndOfLine, - - /// A Comment that is separated by at least one line break from the preceding token. - /// - /// # Examples - /// - /// ```python - /// a; - /// # comment - /// b; - /// ``` - /// - /// `# comment` line comments because they are separated by one line break from the preceding token `a`. - OwnLine, -} - -impl CommentLinePosition { - pub(crate) const fn is_own_line(self) -> bool { - matches!(self, CommentLinePosition::OwnLine) - } - - pub(crate) const fn is_end_of_line(self) -> bool { - matches!(self, CommentLinePosition::EndOfLine) - } -} - type CommentsMap<'a> = MultiMap, SourceComment>; /// The comments of a syntax tree stored by node. diff --git a/crates/ruff_python_formatter/src/comments/visitor.rs b/crates/ruff_python_formatter/src/comments/visitor.rs index f98aaabb169ef..b98fb7180fd96 100644 --- a/crates/ruff_python_formatter/src/comments/visitor.rs +++ b/crates/ruff_python_formatter/src/comments/visitor.rs @@ -8,13 +8,13 @@ use ruff_python_ast::{Mod, Stmt}; // pre-order. #[allow(clippy::wildcard_imports)] use ruff_python_ast::visitor::preorder::*; -use ruff_python_trivia::{is_python_whitespace, CommentRanges}; +use ruff_python_trivia::{is_python_whitespace, CommentLinePosition, CommentRanges}; use ruff_source_file::Locator; use ruff_text_size::{Ranged, TextRange, TextSize}; use crate::comments::node_key::NodeRefEqualityKey; use crate::comments::placement::place_comment; -use crate::comments::{CommentLinePosition, CommentsMap, SourceComment}; +use crate::comments::{CommentsMap, SourceComment}; /// Collect the preceding, following and enclosing node for each comment without applying /// [`place_comment`] for debugging. diff --git a/crates/ruff_python_formatter/src/lib.rs b/crates/ruff_python_formatter/src/lib.rs index 011ba245a91eb..40a03b63ede0c 100644 --- a/crates/ruff_python_formatter/src/lib.rs +++ b/crates/ruff_python_formatter/src/lib.rs @@ -8,7 +8,7 @@ use ruff_python_ast::AstNode; use ruff_python_ast::Mod; use ruff_python_index::tokens_and_ranges; use ruff_python_parser::{parse_tokens, AsMode, ParseError, ParseErrorType}; -use ruff_python_trivia::CommentRanges; +use ruff_python_trivia::{CommentRanges, SuppressionKind}; use ruff_source_file::Locator; use crate::comments::{ @@ -116,6 +116,16 @@ where } } +fn has_skip_comment(trailing_comments: &[SourceComment], source: &str) -> bool { + trailing_comments.iter().any(|comment| { + comment.line_position().is_end_of_line() + && matches!( + SuppressionKind::from_slice(comment.slice().text(SourceCode::new(source))), + Some(SuppressionKind::Skip | SuppressionKind::Off) + ) + }) +} + #[derive(Error, Debug)] pub enum FormatModuleError { #[error(transparent)] diff --git a/crates/ruff_python_formatter/src/other/decorator.rs b/crates/ruff_python_formatter/src/other/decorator.rs index 9754ac4b4aef1..38e534d9048bb 100644 --- a/crates/ruff_python_formatter/src/other/decorator.rs +++ b/crates/ruff_python_formatter/src/other/decorator.rs @@ -1,10 +1,10 @@ use ruff_formatter::write; use ruff_python_ast::Decorator; -use crate::comments::{SourceComment, SuppressionKind}; +use crate::comments::SourceComment; use crate::expression::maybe_parenthesize_expression; use crate::expression::parentheses::Parenthesize; -use crate::prelude::*; +use crate::{has_skip_comment, prelude::*}; #[derive(Default)] pub struct FormatDecorator; @@ -30,6 +30,6 @@ impl FormatNodeRule for FormatDecorator { trailing_comments: &[SourceComment], context: &PyFormatContext, ) -> bool { - SuppressionKind::has_skip_comment(trailing_comments, context.source()) + has_skip_comment(trailing_comments, context.source()) } } diff --git a/crates/ruff_python_formatter/src/other/parameters.rs b/crates/ruff_python_formatter/src/other/parameters.rs index c3e251ebe97e5..ef5cb96bc0977 100644 --- a/crates/ruff_python_formatter/src/other/parameters.rs +++ b/crates/ruff_python_formatter/src/other/parameters.rs @@ -3,12 +3,12 @@ use std::usize; use ruff_formatter::{format_args, write, FormatRuleWithOptions}; use ruff_python_ast::Parameters; use ruff_python_ast::{AnyNodeRef, AstNode}; -use ruff_python_trivia::{SimpleToken, SimpleTokenKind, SimpleTokenizer}; +use ruff_python_trivia::{CommentLinePosition, SimpleToken, SimpleTokenKind, SimpleTokenizer}; use ruff_text_size::{Ranged, TextRange, TextSize}; use crate::comments::{ dangling_comments, dangling_open_parenthesis_comments, leading_comments, leading_node_comments, - trailing_comments, CommentLinePosition, SourceComment, + trailing_comments, SourceComment, }; use crate::context::{NodeLevel, WithNodeLevel}; use crate::expression::parentheses::empty_parenthesized; diff --git a/crates/ruff_python_formatter/src/statement/clause.rs b/crates/ruff_python_formatter/src/statement/clause.rs index 7592c94f1c0e2..34cb2dfc5527e 100644 --- a/crates/ruff_python_formatter/src/statement/clause.rs +++ b/crates/ruff_python_formatter/src/statement/clause.rs @@ -7,13 +7,11 @@ use ruff_python_ast::{ use ruff_python_trivia::{SimpleToken, SimpleTokenKind, SimpleTokenizer}; use ruff_text_size::{Ranged, TextRange, TextSize}; -use crate::comments::{ - leading_alternate_branch_comments, trailing_comments, SourceComment, SuppressionKind, -}; -use crate::prelude::*; +use crate::comments::{leading_alternate_branch_comments, trailing_comments, SourceComment}; use crate::preview::is_dummy_implementations_enabled; use crate::statement::suite::{contains_only_an_ellipsis, SuiteKind}; use crate::verbatim::write_suppressed_clause_header; +use crate::{has_skip_comment, prelude::*}; /// The header of a compound statement clause. /// @@ -354,7 +352,7 @@ impl<'ast> Format> for FormatClauseHeader<'_, 'ast> { leading_alternate_branch_comments(leading_comments, last_node).fmt(f)?; } - if SuppressionKind::has_skip_comment(self.trailing_colon_comment, f.context().source()) { + if has_skip_comment(self.trailing_colon_comment, f.context().source()) { write_suppressed_clause_header(self.header, f)?; } else { // Write a source map entry for the colon for range formatting to support formatting the clause header without diff --git a/crates/ruff_python_formatter/src/statement/stmt_ann_assign.rs b/crates/ruff_python_formatter/src/statement/stmt_ann_assign.rs index 5d0a2d2f31eed..aae1ff2473ae2 100644 --- a/crates/ruff_python_formatter/src/statement/stmt_ann_assign.rs +++ b/crates/ruff_python_formatter/src/statement/stmt_ann_assign.rs @@ -1,10 +1,9 @@ use ruff_formatter::write; use ruff_python_ast::StmtAnnAssign; -use crate::comments::{SourceComment, SuppressionKind}; +use crate::comments::SourceComment; use crate::expression::parentheses::Parentheses; use crate::expression::{has_parentheses, is_splittable_expression}; -use crate::prelude::*; use crate::preview::{ is_parenthesize_long_type_hints_enabled, is_prefer_splitting_right_hand_side_of_assignments_enabled, @@ -13,6 +12,7 @@ use crate::statement::stmt_assign::{ AnyAssignmentOperator, AnyBeforeOperator, FormatStatementsLastExpression, }; use crate::statement::trailing_semicolon; +use crate::{has_skip_comment, prelude::*}; #[derive(Default)] pub struct FormatStmtAnnAssign; @@ -106,6 +106,6 @@ impl FormatNodeRule for FormatStmtAnnAssign { trailing_comments: &[SourceComment], context: &PyFormatContext, ) -> bool { - SuppressionKind::has_skip_comment(trailing_comments, context.source()) + has_skip_comment(trailing_comments, context.source()) } } diff --git a/crates/ruff_python_formatter/src/statement/stmt_assert.rs b/crates/ruff_python_formatter/src/statement/stmt_assert.rs index 7b3930d252d74..91a85fe04925b 100644 --- a/crates/ruff_python_formatter/src/statement/stmt_assert.rs +++ b/crates/ruff_python_formatter/src/statement/stmt_assert.rs @@ -2,11 +2,11 @@ use ruff_formatter::prelude::{space, token}; use ruff_formatter::write; use ruff_python_ast::StmtAssert; -use crate::comments::{SourceComment, SuppressionKind}; +use crate::comments::SourceComment; use crate::expression::maybe_parenthesize_expression; use crate::expression::parentheses::Parenthesize; -use crate::prelude::*; +use crate::{has_skip_comment, prelude::*}; #[derive(Default)] pub struct FormatStmtAssert; @@ -47,6 +47,6 @@ impl FormatNodeRule for FormatStmtAssert { trailing_comments: &[SourceComment], context: &PyFormatContext, ) -> bool { - SuppressionKind::has_skip_comment(trailing_comments, context.source()) + has_skip_comment(trailing_comments, context.source()) } } diff --git a/crates/ruff_python_formatter/src/statement/stmt_assign.rs b/crates/ruff_python_formatter/src/statement/stmt_assign.rs index b709c1a08340f..a3e74aaa9b8eb 100644 --- a/crates/ruff_python_formatter/src/statement/stmt_assign.rs +++ b/crates/ruff_python_formatter/src/statement/stmt_assign.rs @@ -5,7 +5,7 @@ use ruff_python_ast::{ use crate::builders::parenthesize_if_expands; use crate::comments::{ - trailing_comments, Comments, LeadingDanglingTrailingComments, SourceComment, SuppressionKind, + trailing_comments, Comments, LeadingDanglingTrailingComments, SourceComment, }; use crate::context::{NodeLevel, WithNodeLevel}; use crate::expression::parentheses::{ @@ -16,12 +16,12 @@ use crate::expression::{ can_omit_optional_parentheses, has_own_parentheses, has_parentheses, maybe_parenthesize_expression, }; -use crate::prelude::*; use crate::preview::{ is_parenthesize_long_type_hints_enabled, is_prefer_splitting_right_hand_side_of_assignments_enabled, }; use crate::statement::trailing_semicolon; +use crate::{has_skip_comment, prelude::*}; #[derive(Default)] pub struct FormatStmtAssign; @@ -110,7 +110,7 @@ impl FormatNodeRule for FormatStmtAssign { trailing_comments: &[SourceComment], context: &PyFormatContext, ) -> bool { - SuppressionKind::has_skip_comment(trailing_comments, context.source()) + has_skip_comment(trailing_comments, context.source()) } } diff --git a/crates/ruff_python_formatter/src/statement/stmt_aug_assign.rs b/crates/ruff_python_formatter/src/statement/stmt_aug_assign.rs index 003c10a32a8e1..705b0989140a8 100644 --- a/crates/ruff_python_formatter/src/statement/stmt_aug_assign.rs +++ b/crates/ruff_python_formatter/src/statement/stmt_aug_assign.rs @@ -1,15 +1,15 @@ use ruff_formatter::write; use ruff_python_ast::StmtAugAssign; -use crate::comments::{SourceComment, SuppressionKind}; +use crate::comments::SourceComment; use crate::expression::parentheses::is_expression_parenthesized; -use crate::prelude::*; use crate::preview::is_prefer_splitting_right_hand_side_of_assignments_enabled; use crate::statement::stmt_assign::{ has_target_own_parentheses, AnyAssignmentOperator, AnyBeforeOperator, FormatStatementsLastExpression, }; use crate::statement::trailing_semicolon; +use crate::{has_skip_comment, prelude::*}; use crate::{AsFormat, FormatNodeRule}; #[derive(Default)] @@ -69,6 +69,6 @@ impl FormatNodeRule for FormatStmtAugAssign { trailing_comments: &[SourceComment], context: &PyFormatContext, ) -> bool { - SuppressionKind::has_skip_comment(trailing_comments, context.source()) + has_skip_comment(trailing_comments, context.source()) } } diff --git a/crates/ruff_python_formatter/src/statement/stmt_break.rs b/crates/ruff_python_formatter/src/statement/stmt_break.rs index 67926e0a0e9ed..1e94c01f8b16b 100644 --- a/crates/ruff_python_formatter/src/statement/stmt_break.rs +++ b/crates/ruff_python_formatter/src/statement/stmt_break.rs @@ -1,7 +1,7 @@ use ruff_python_ast::StmtBreak; -use crate::comments::{SourceComment, SuppressionKind}; -use crate::prelude::*; +use crate::comments::SourceComment; +use crate::{has_skip_comment, prelude::*}; #[derive(Default)] pub struct FormatStmtBreak; @@ -16,6 +16,6 @@ impl FormatNodeRule for FormatStmtBreak { trailing_comments: &[SourceComment], context: &PyFormatContext, ) -> bool { - SuppressionKind::has_skip_comment(trailing_comments, context.source()) + has_skip_comment(trailing_comments, context.source()) } } diff --git a/crates/ruff_python_formatter/src/statement/stmt_continue.rs b/crates/ruff_python_formatter/src/statement/stmt_continue.rs index af045d241814b..39e14b511608b 100644 --- a/crates/ruff_python_formatter/src/statement/stmt_continue.rs +++ b/crates/ruff_python_formatter/src/statement/stmt_continue.rs @@ -1,7 +1,7 @@ use ruff_python_ast::StmtContinue; -use crate::comments::{SourceComment, SuppressionKind}; -use crate::prelude::*; +use crate::comments::SourceComment; +use crate::{has_skip_comment, prelude::*}; #[derive(Default)] pub struct FormatStmtContinue; @@ -16,6 +16,6 @@ impl FormatNodeRule for FormatStmtContinue { trailing_comments: &[SourceComment], context: &PyFormatContext, ) -> bool { - SuppressionKind::has_skip_comment(trailing_comments, context.source()) + has_skip_comment(trailing_comments, context.source()) } } diff --git a/crates/ruff_python_formatter/src/statement/stmt_delete.rs b/crates/ruff_python_formatter/src/statement/stmt_delete.rs index 4af373c801258..52158347c0fa0 100644 --- a/crates/ruff_python_formatter/src/statement/stmt_delete.rs +++ b/crates/ruff_python_formatter/src/statement/stmt_delete.rs @@ -3,10 +3,10 @@ use ruff_python_ast::StmtDelete; use ruff_text_size::Ranged; use crate::builders::{parenthesize_if_expands, PyFormatterExtensions}; -use crate::comments::{dangling_node_comments, SourceComment, SuppressionKind}; +use crate::comments::{dangling_node_comments, SourceComment}; use crate::expression::maybe_parenthesize_expression; use crate::expression::parentheses::Parenthesize; -use crate::prelude::*; +use crate::{has_skip_comment, prelude::*}; #[derive(Default)] pub struct FormatStmtDelete; @@ -68,6 +68,6 @@ impl FormatNodeRule for FormatStmtDelete { trailing_comments: &[SourceComment], context: &PyFormatContext, ) -> bool { - SuppressionKind::has_skip_comment(trailing_comments, context.source()) + has_skip_comment(trailing_comments, context.source()) } } diff --git a/crates/ruff_python_formatter/src/statement/stmt_expr.rs b/crates/ruff_python_formatter/src/statement/stmt_expr.rs index c0a6361b71c81..79f9c48abb1c8 100644 --- a/crates/ruff_python_formatter/src/statement/stmt_expr.rs +++ b/crates/ruff_python_formatter/src/statement/stmt_expr.rs @@ -1,12 +1,12 @@ use ruff_python_ast as ast; use ruff_python_ast::{Expr, Operator, StmtExpr}; -use crate::comments::{SourceComment, SuppressionKind}; +use crate::comments::SourceComment; use crate::expression::maybe_parenthesize_expression; use crate::expression::parentheses::Parenthesize; -use crate::prelude::*; use crate::statement::trailing_semicolon; +use crate::{has_skip_comment, prelude::*}; #[derive(Default)] pub struct FormatStmtExpr; @@ -36,7 +36,7 @@ impl FormatNodeRule for FormatStmtExpr { trailing_comments: &[SourceComment], context: &PyFormatContext, ) -> bool { - SuppressionKind::has_skip_comment(trailing_comments, context.source()) + has_skip_comment(trailing_comments, context.source()) } } diff --git a/crates/ruff_python_formatter/src/statement/stmt_global.rs b/crates/ruff_python_formatter/src/statement/stmt_global.rs index dac5c0b520c0d..ea70ee34a4230 100644 --- a/crates/ruff_python_formatter/src/statement/stmt_global.rs +++ b/crates/ruff_python_formatter/src/statement/stmt_global.rs @@ -2,7 +2,8 @@ use ruff_formatter::{format_args, write}; use ruff_python_ast::AstNode; use ruff_python_ast::StmtGlobal; -use crate::comments::{SourceComment, SuppressionKind}; +use crate::comments::SourceComment; +use crate::has_skip_comment; use crate::prelude::*; #[derive(Default)] @@ -53,6 +54,6 @@ impl FormatNodeRule for FormatStmtGlobal { trailing_comments: &[SourceComment], context: &PyFormatContext, ) -> bool { - SuppressionKind::has_skip_comment(trailing_comments, context.source()) + has_skip_comment(trailing_comments, context.source()) } } diff --git a/crates/ruff_python_formatter/src/statement/stmt_import.rs b/crates/ruff_python_formatter/src/statement/stmt_import.rs index 97dc4fe830f08..8045ae7864db5 100644 --- a/crates/ruff_python_formatter/src/statement/stmt_import.rs +++ b/crates/ruff_python_formatter/src/statement/stmt_import.rs @@ -1,8 +1,8 @@ use ruff_formatter::{format_args, write}; use ruff_python_ast::StmtImport; -use crate::comments::{SourceComment, SuppressionKind}; -use crate::prelude::*; +use crate::comments::SourceComment; +use crate::{has_skip_comment, prelude::*}; #[derive(Default)] pub struct FormatStmtImport; @@ -23,6 +23,6 @@ impl FormatNodeRule for FormatStmtImport { trailing_comments: &[SourceComment], context: &PyFormatContext, ) -> bool { - SuppressionKind::has_skip_comment(trailing_comments, context.source()) + has_skip_comment(trailing_comments, context.source()) } } diff --git a/crates/ruff_python_formatter/src/statement/stmt_import_from.rs b/crates/ruff_python_formatter/src/statement/stmt_import_from.rs index e0535268a002e..7f8d1ce4e3fdd 100644 --- a/crates/ruff_python_formatter/src/statement/stmt_import_from.rs +++ b/crates/ruff_python_formatter/src/statement/stmt_import_from.rs @@ -4,8 +4,9 @@ use ruff_python_ast::StmtImportFrom; use ruff_text_size::Ranged; use crate::builders::{parenthesize_if_expands, PyFormatterExtensions, TrailingComma}; -use crate::comments::{SourceComment, SuppressionKind}; +use crate::comments::SourceComment; use crate::expression::parentheses::parenthesized; +use crate::has_skip_comment; use crate::other::identifier::DotDelimitedIdentifier; use crate::prelude::*; @@ -86,6 +87,6 @@ impl FormatNodeRule for FormatStmtImportFrom { trailing_comments: &[SourceComment], context: &PyFormatContext, ) -> bool { - SuppressionKind::has_skip_comment(trailing_comments, context.source()) + has_skip_comment(trailing_comments, context.source()) } } diff --git a/crates/ruff_python_formatter/src/statement/stmt_ipy_escape_command.rs b/crates/ruff_python_formatter/src/statement/stmt_ipy_escape_command.rs index b41305f42abe8..66d52739db4bc 100644 --- a/crates/ruff_python_formatter/src/statement/stmt_ipy_escape_command.rs +++ b/crates/ruff_python_formatter/src/statement/stmt_ipy_escape_command.rs @@ -1,8 +1,8 @@ use ruff_python_ast::StmtIpyEscapeCommand; use ruff_text_size::Ranged; -use crate::comments::{SourceComment, SuppressionKind}; -use crate::prelude::*; +use crate::comments::SourceComment; +use crate::{has_skip_comment, prelude::*}; #[derive(Default)] pub struct FormatStmtIpyEscapeCommand; @@ -17,6 +17,6 @@ impl FormatNodeRule for FormatStmtIpyEscapeCommand { trailing_comments: &[SourceComment], context: &PyFormatContext, ) -> bool { - SuppressionKind::has_skip_comment(trailing_comments, context.source()) + has_skip_comment(trailing_comments, context.source()) } } diff --git a/crates/ruff_python_formatter/src/statement/stmt_nonlocal.rs b/crates/ruff_python_formatter/src/statement/stmt_nonlocal.rs index e39965e5b1c12..3280e0d93bbda 100644 --- a/crates/ruff_python_formatter/src/statement/stmt_nonlocal.rs +++ b/crates/ruff_python_formatter/src/statement/stmt_nonlocal.rs @@ -2,7 +2,8 @@ use ruff_formatter::{format_args, write}; use ruff_python_ast::AstNode; use ruff_python_ast::StmtNonlocal; -use crate::comments::{SourceComment, SuppressionKind}; +use crate::comments::SourceComment; +use crate::has_skip_comment; use crate::prelude::*; #[derive(Default)] @@ -53,6 +54,6 @@ impl FormatNodeRule for FormatStmtNonlocal { trailing_comments: &[SourceComment], context: &PyFormatContext, ) -> bool { - SuppressionKind::has_skip_comment(trailing_comments, context.source()) + has_skip_comment(trailing_comments, context.source()) } } diff --git a/crates/ruff_python_formatter/src/statement/stmt_pass.rs b/crates/ruff_python_formatter/src/statement/stmt_pass.rs index d6a1e74564c63..bd542ec74bb00 100644 --- a/crates/ruff_python_formatter/src/statement/stmt_pass.rs +++ b/crates/ruff_python_formatter/src/statement/stmt_pass.rs @@ -1,7 +1,7 @@ use ruff_python_ast::StmtPass; -use crate::comments::{SourceComment, SuppressionKind}; -use crate::prelude::*; +use crate::comments::SourceComment; +use crate::{has_skip_comment, prelude::*}; #[derive(Default)] pub struct FormatStmtPass; @@ -16,6 +16,6 @@ impl FormatNodeRule for FormatStmtPass { trailing_comments: &[SourceComment], context: &PyFormatContext, ) -> bool { - SuppressionKind::has_skip_comment(trailing_comments, context.source()) + has_skip_comment(trailing_comments, context.source()) } } diff --git a/crates/ruff_python_formatter/src/statement/stmt_raise.rs b/crates/ruff_python_formatter/src/statement/stmt_raise.rs index 18c72e7672cb7..6a855e4418586 100644 --- a/crates/ruff_python_formatter/src/statement/stmt_raise.rs +++ b/crates/ruff_python_formatter/src/statement/stmt_raise.rs @@ -1,10 +1,10 @@ use ruff_formatter::write; use ruff_python_ast::StmtRaise; -use crate::comments::{SourceComment, SuppressionKind}; +use crate::comments::SourceComment; use crate::expression::maybe_parenthesize_expression; use crate::expression::parentheses::Parenthesize; -use crate::prelude::*; +use crate::{has_skip_comment, prelude::*}; #[derive(Default)] pub struct FormatStmtRaise; @@ -48,6 +48,6 @@ impl FormatNodeRule for FormatStmtRaise { trailing_comments: &[SourceComment], context: &PyFormatContext, ) -> bool { - SuppressionKind::has_skip_comment(trailing_comments, context.source()) + has_skip_comment(trailing_comments, context.source()) } } diff --git a/crates/ruff_python_formatter/src/statement/stmt_return.rs b/crates/ruff_python_formatter/src/statement/stmt_return.rs index 4eaa3d92613ab..ab782c02edbf2 100644 --- a/crates/ruff_python_formatter/src/statement/stmt_return.rs +++ b/crates/ruff_python_formatter/src/statement/stmt_return.rs @@ -1,10 +1,10 @@ use ruff_formatter::write; use ruff_python_ast::{Expr, StmtReturn}; -use crate::comments::{SourceComment, SuppressionKind}; +use crate::comments::SourceComment; use crate::expression::expr_tuple::TupleParentheses; -use crate::prelude::*; use crate::statement::stmt_assign::FormatStatementsLastExpression; +use crate::{has_skip_comment, prelude::*}; #[derive(Default)] pub struct FormatStmtReturn; @@ -45,6 +45,6 @@ impl FormatNodeRule for FormatStmtReturn { trailing_comments: &[SourceComment], context: &PyFormatContext, ) -> bool { - SuppressionKind::has_skip_comment(trailing_comments, context.source()) + has_skip_comment(trailing_comments, context.source()) } } diff --git a/crates/ruff_python_formatter/src/statement/stmt_type_alias.rs b/crates/ruff_python_formatter/src/statement/stmt_type_alias.rs index b6ae69b4430af..6926543460761 100644 --- a/crates/ruff_python_formatter/src/statement/stmt_type_alias.rs +++ b/crates/ruff_python_formatter/src/statement/stmt_type_alias.rs @@ -1,12 +1,12 @@ use ruff_formatter::write; use ruff_python_ast::StmtTypeAlias; -use crate::comments::{SourceComment, SuppressionKind}; -use crate::prelude::*; +use crate::comments::SourceComment; use crate::preview::is_prefer_splitting_right_hand_side_of_assignments_enabled; use crate::statement::stmt_assign::{ AnyAssignmentOperator, AnyBeforeOperator, FormatStatementsLastExpression, }; +use crate::{has_skip_comment, prelude::*}; #[derive(Default)] pub struct FormatStmtTypeAlias; @@ -52,6 +52,6 @@ impl FormatNodeRule for FormatStmtTypeAlias { trailing_comments: &[SourceComment], context: &PyFormatContext, ) -> bool { - SuppressionKind::has_skip_comment(trailing_comments, context.source()) + has_skip_comment(trailing_comments, context.source()) } } diff --git a/crates/ruff_python_trivia/src/lib.rs b/crates/ruff_python_trivia/src/lib.rs index 9fed887c17ff4..3aa7039c77397 100644 --- a/crates/ruff_python_trivia/src/lib.rs +++ b/crates/ruff_python_trivia/src/lib.rs @@ -1,6 +1,7 @@ mod comment_ranges; mod cursor; mod pragmas; +mod suppression; pub mod textwrap; mod tokenizer; mod whitespace; @@ -8,5 +9,6 @@ mod whitespace; pub use comment_ranges::CommentRanges; pub use cursor::*; pub use pragmas::*; +pub use suppression::*; pub use tokenizer::*; pub use whitespace::*; diff --git a/crates/ruff_python_trivia/src/suppression.rs b/crates/ruff_python_trivia/src/suppression.rs new file mode 100644 index 0000000000000..f0de8d0bd327d --- /dev/null +++ b/crates/ruff_python_trivia/src/suppression.rs @@ -0,0 +1,98 @@ +use crate::PythonWhitespace; + +#[derive(Copy, Clone, Eq, PartialEq, Debug)] +pub enum SuppressionKind { + /// A `fmt: off` or `yapf: disable` comment + Off, + /// A `fmt: on` or `yapf: enable` comment + On, + /// A `fmt: skip` comment + Skip, +} + +impl SuppressionKind { + /// Given a `slice` + pub fn from_slice(slice: &str) -> Option { + // Match against `# fmt: on`, `# fmt: off`, `# yapf: disable`, and `# yapf: enable`, which + // must be on their own lines. + let trimmed = slice.strip_prefix('#').unwrap_or(slice).trim_whitespace(); + if let Some(command) = trimmed.strip_prefix("fmt:") { + match command.trim_whitespace_start() { + "off" => return Some(Self::Off), + "on" => return Some(Self::On), + "skip" => return Some(Self::Skip), + _ => {} + } + } else if let Some(command) = trimmed.strip_prefix("yapf:") { + match command.trim_whitespace_start() { + "disable" => return Some(Self::Off), + "enable" => return Some(Self::On), + _ => {} + } + } + + // Search for `# fmt: skip` comments, which can be interspersed with other comments (e.g., + // `# fmt: skip # noqa: E501`). + for segment in slice.split('#') { + let trimmed = segment.trim_whitespace(); + if let Some(command) = trimmed.strip_prefix("fmt:") { + if command.trim_whitespace_start() == "skip" { + return Some(SuppressionKind::Skip); + } + } + } + + None + } + + /// Returns true if this comment is a `fmt: off` or `yapf: disable` own line suppression comment. + pub fn is_suppression_on(slice: &str, position: CommentLinePosition) -> bool { + position.is_own_line() && matches!(Self::from_slice(slice), Some(Self::Off)) + } + + /// Returns true if this comment is a `fmt: on` or `yapf: enable` own line suppression comment. + pub fn is_suppression_off(slice: &str, position: CommentLinePosition) -> bool { + position.is_own_line() && matches!(Self::from_slice(slice), Some(Self::On)) + } +} +/// The position of a comment in the source text. +#[derive(Debug, Copy, Clone, Eq, PartialEq)] +pub enum CommentLinePosition { + /// A comment that is on the same line as the preceding token and is separated by at least one line break from the following token. + /// + /// # Examples + /// + /// ## End of line + /// + /// ```python + /// a; # comment + /// b; + /// ``` + /// + /// `# comment` is an end of line comments because it is separated by at least one line break from the following token `b`. + /// Comments that not only end, but also start on a new line are [`OwnLine`](CommentLinePosition::OwnLine) comments. + EndOfLine, + + /// A Comment that is separated by at least one line break from the preceding token. + /// + /// # Examples + /// + /// ```python + /// a; + /// # comment + /// b; + /// ``` + /// + /// `# comment` line comments because they are separated by one line break from the preceding token `a`. + OwnLine, +} + +impl CommentLinePosition { + pub const fn is_own_line(self) -> bool { + matches!(self, Self::OwnLine) + } + + pub const fn is_end_of_line(self) -> bool { + matches!(self, Self::EndOfLine) + } +} From fd65789f1b4caaa0434ff50aa685d2aa9dc9dd75 Mon Sep 17 00:00:00 2001 From: Jane Lewis Date: Tue, 6 Feb 2024 10:00:15 -0500 Subject: [PATCH 02/13] Draft implementation of RUF028. Several edge cases still need to be resolved, particularly with dangling comments. --- .../resources/test/fixtures/ruff/RUF028.py | 21 ++ .../src/checkers/ast/analyze/module.rs | 5 +- crates/ruff_linter/src/checkers/noqa.rs | 13 +- crates/ruff_linter/src/codes.rs | 1 + crates/ruff_linter/src/rules/ruff/mod.rs | 1 + .../ruff_linter/src/rules/ruff/rules/mod.rs | 3 + .../ruff/rules/suppression_comment_visitor.rs | 195 ++++++++++++++++++ .../ruff/rules/useless_formatter_noqa.rs | 164 +++++++++++++++ ..._rules__ruff__tests__RUF028_RUF028.py.snap | 61 ++++++ .../src/comments/visitor.rs | 28 +-- crates/ruff_python_trivia/src/suppression.rs | 19 +- 11 files changed, 485 insertions(+), 26 deletions(-) create mode 100644 crates/ruff_linter/resources/test/fixtures/ruff/RUF028.py create mode 100644 crates/ruff_linter/src/rules/ruff/rules/suppression_comment_visitor.rs create mode 100644 crates/ruff_linter/src/rules/ruff/rules/useless_formatter_noqa.rs create mode 100644 crates/ruff_linter/src/rules/ruff/snapshots/ruff_linter__rules__ruff__tests__RUF028_RUF028.py.snap diff --git a/crates/ruff_linter/resources/test/fixtures/ruff/RUF028.py b/crates/ruff_linter/resources/test/fixtures/ruff/RUF028.py new file mode 100644 index 0000000000000..8ca1d2a11bbc5 --- /dev/null +++ b/crates/ruff_linter/resources/test/fixtures/ruff/RUF028.py @@ -0,0 +1,21 @@ + +def fmt_off_in_elif(): + if True: + pass + elif False: + pass +# fmt: on + +def fmt_off_between_lists(): + test_list = [ + #fmt: off + 1, + 2, + 3 + ] + +@fmt_off_in_elif +@fmt_off_between_lists +def fmt_off_between_decorators(): + # fmt: skip + pass diff --git a/crates/ruff_linter/src/checkers/ast/analyze/module.rs b/crates/ruff_linter/src/checkers/ast/analyze/module.rs index 139d6c5294783..52c72f1afdae4 100644 --- a/crates/ruff_linter/src/checkers/ast/analyze/module.rs +++ b/crates/ruff_linter/src/checkers/ast/analyze/module.rs @@ -2,11 +2,14 @@ use ruff_python_ast::Suite; use crate::checkers::ast::Checker; use crate::codes::Rule; -use crate::rules::flake8_bugbear; +use crate::rules::{flake8_bugbear, ruff}; /// Run lint rules over a module. pub(crate) fn module(suite: &Suite, checker: &mut Checker) { if checker.enabled(Rule::FStringDocstring) { flake8_bugbear::rules::f_string_docstring(checker, suite); } + if checker.enabled(Rule::UselessFormatterNOQA) { + ruff::rules::useless_formatter_noqa(checker, suite); + } } diff --git a/crates/ruff_linter/src/checkers/noqa.rs b/crates/ruff_linter/src/checkers/noqa.rs index d5d4276abb27a..00fbc9885a170 100644 --- a/crates/ruff_linter/src/checkers/noqa.rs +++ b/crates/ruff_linter/src/checkers/noqa.rs @@ -111,7 +111,8 @@ pub(crate) fn check_noqa( if line.matches.is_empty() { let mut diagnostic = Diagnostic::new(UnusedNOQA { codes: None }, directive.range()); - diagnostic.set_fix(Fix::safe_edit(delete_noqa(directive.range(), locator))); + diagnostic + .set_fix(Fix::safe_edit(delete_comment(directive.range(), locator))); diagnostics.push(diagnostic); } @@ -177,8 +178,10 @@ pub(crate) fn check_noqa( directive.range(), ); if valid_codes.is_empty() { - diagnostic - .set_fix(Fix::safe_edit(delete_noqa(directive.range(), locator))); + diagnostic.set_fix(Fix::safe_edit(delete_comment( + directive.range(), + locator, + ))); } else { diagnostic.set_fix(Fix::safe_edit(Edit::range_replacement( format!("# noqa: {}", valid_codes.join(", ")), @@ -196,8 +199,8 @@ pub(crate) fn check_noqa( ignored_diagnostics } -/// Generate a [`Edit`] to delete a `noqa` directive. -fn delete_noqa(range: TextRange, locator: &Locator) -> Edit { +/// Generate a [`Edit`] to delete a comment (for example: a `noqa` directive). +pub(crate) fn delete_comment(range: TextRange, locator: &Locator) -> Edit { let line_range = locator.line_range(range.start()); // Compute the leading space. diff --git a/crates/ruff_linter/src/codes.rs b/crates/ruff_linter/src/codes.rs index 5cf0293d1292e..cbd645df2aa73 100644 --- a/crates/ruff_linter/src/codes.rs +++ b/crates/ruff_linter/src/codes.rs @@ -945,6 +945,7 @@ pub fn code_to_rule(linter: Linter, code: &str) -> Option<(RuleGroup, Rule)> { (Ruff, "025") => (RuleGroup::Preview, rules::ruff::rules::UnnecessaryDictComprehensionForIterable), (Ruff, "026") => (RuleGroup::Preview, rules::ruff::rules::DefaultFactoryKwarg), (Ruff, "027") => (RuleGroup::Preview, rules::ruff::rules::MissingFStringSyntax), + (Ruff, "028") => (RuleGroup::Preview, rules::ruff::rules::UselessFormatterNOQA), (Ruff, "100") => (RuleGroup::Stable, rules::ruff::rules::UnusedNOQA), (Ruff, "200") => (RuleGroup::Stable, rules::ruff::rules::InvalidPyprojectToml), #[cfg(feature = "test-rules")] diff --git a/crates/ruff_linter/src/rules/ruff/mod.rs b/crates/ruff_linter/src/rules/ruff/mod.rs index 7239323bcbddb..adfec1b3cb99d 100644 --- a/crates/ruff_linter/src/rules/ruff/mod.rs +++ b/crates/ruff_linter/src/rules/ruff/mod.rs @@ -49,6 +49,7 @@ mod tests { #[test_case(Rule::MissingFStringSyntax, Path::new("RUF027_0.py"))] #[test_case(Rule::MissingFStringSyntax, Path::new("RUF027_1.py"))] #[test_case(Rule::MissingFStringSyntax, Path::new("RUF027_2.py"))] + #[test_case(Rule::UselessFormatterNOQA, Path::new("RUF028.py"))] fn rules(rule_code: Rule, path: &Path) -> Result<()> { let snapshot = format!("{}_{}", rule_code.noqa_code(), path.to_string_lossy()); let diagnostics = test_path( diff --git a/crates/ruff_linter/src/rules/ruff/rules/mod.rs b/crates/ruff_linter/src/rules/ruff/rules/mod.rs index b8d7fca925135..00eaca20cd150 100644 --- a/crates/ruff_linter/src/rules/ruff/rules/mod.rs +++ b/crates/ruff_linter/src/rules/ruff/rules/mod.rs @@ -25,6 +25,7 @@ pub(crate) use unnecessary_dict_comprehension_for_iterable::*; pub(crate) use unnecessary_iterable_allocation_for_first_element::*; pub(crate) use unnecessary_key_check::*; pub(crate) use unused_noqa::*; +pub(crate) use useless_formatter_noqa::*; mod ambiguous_unicode_character; mod assignment_in_assert; @@ -50,12 +51,14 @@ mod sequence_sorting; mod sort_dunder_all; mod sort_dunder_slots; mod static_key_dict_comprehension; +mod suppression_comment_visitor; #[cfg(feature = "test-rules")] pub(crate) mod test_rules; mod unnecessary_dict_comprehension_for_iterable; mod unnecessary_iterable_allocation_for_first_element; mod unnecessary_key_check; mod unused_noqa; +mod useless_formatter_noqa; #[derive(Clone, Copy)] pub(crate) enum Context { diff --git a/crates/ruff_linter/src/rules/ruff/rules/suppression_comment_visitor.rs b/crates/ruff_linter/src/rules/ruff/rules/suppression_comment_visitor.rs new file mode 100644 index 0000000000000..ffb1a9cbef50d --- /dev/null +++ b/crates/ruff_linter/src/rules/ruff/rules/suppression_comment_visitor.rs @@ -0,0 +1,195 @@ +use std::iter::{FilterMap, Peekable}; + +use ruff_python_ast::{ + visitor::preorder::{self, PreorderVisitor, TraversalSignal}, + AnyNodeRef, Suite, +}; +use ruff_python_trivia::{CommentLinePosition, CommentRanges, SuppressionKind}; +use ruff_text_size::{Ranged, TextRange, TextSize}; + +#[derive(Clone, Copy, Debug)] +struct SuppressionComment { + range: TextRange, + kind: SuppressionKind, +} + +type MapCommentFn<'src> = Box Option + 'src>; +type CommentIter<'src> = Peekable, MapCommentFn<'src>>>; + +/// Visitor that captures AST data for suppression comments. This uses a similar approach +/// to `CommentsVisitor` in the formatter crate. +pub(super) struct SuppressionCommentVisitor<'src, 'builder> { + source: &'src str, + comments: CommentIter<'src>, + + parents: Vec>, + preceding_node: Option>, + + builder: &'builder mut (dyn CaptureSuppressionComment<'src> + 'src), +} + +impl<'src, 'builder> SuppressionCommentVisitor<'src, 'builder> { + pub(super) fn new( + source: &'src str, + comment_ranges: &'src CommentRanges, + builder: &'builder mut (dyn CaptureSuppressionComment<'src> + 'src), + ) -> Self { + let map_fn: MapCommentFn<'_> = Box::new(|range: &'src TextRange| { + Some(SuppressionComment { + range: *range, + kind: SuppressionKind::from_slice(&source[*range])?, + }) + }); + Self { + source, + comments: comment_ranges.iter().filter_map(map_fn).peekable(), + parents: Vec::default(), + preceding_node: Option::default(), + builder, + } + } + + pub(super) fn visit(mut self, suite: &'src Suite) { + self.visit_body(suite.as_slice()); + } + + fn can_skip(&mut self, node_end: TextSize) -> bool { + self.comments + .peek() + .map_or(true, |next| next.range.start() >= node_end) + } +} + +impl<'ast> PreorderVisitor<'ast> for SuppressionCommentVisitor<'ast, '_> { + fn enter_node(&mut self, node: AnyNodeRef<'ast>) -> TraversalSignal { + let node_range = node.range(); + + let enclosing_node = self.parents.last().copied(); + + // Process all remaining comments that end before this node's start position. + // If the `preceding` node is set, then it process all comments ending after the `preceding` node + // and ending before this node's start position + while let Some(SuppressionComment { range, kind }) = self.comments.peek().copied() { + // Exit if the comment is enclosed by this node or comes after it + if range.end() > node_range.start() { + break; + } + + let data = SuppressionCommentData { + enclosing: enclosing_node, + preceding: self.preceding_node, + following: Some(node), + parent: self.parents.iter().rev().nth(0).copied(), + line_position: CommentLinePosition::text_position(range, self.source), + kind, + range, + }; + + self.builder.capture(data); + self.comments.next(); + } + + // From here on, we're inside of `node`, meaning, we're passed the preceding node. + self.preceding_node = None; + self.parents.push(node); + + if self.can_skip(node_range.end()) { + TraversalSignal::Skip + } else { + TraversalSignal::Traverse + } + } + + fn leave_node(&mut self, node: AnyNodeRef<'ast>) { + self.parents.pop(); + + let node_end = node.end(); + + // Process all comments that start after the `preceding` node and end before this node's end. + while let Some(SuppressionComment { range, kind }) = self.comments.peek().copied() { + // If the comment starts after this node, break. + if range.start() >= node_end { + break; + } + + let data = SuppressionCommentData { + enclosing: Some(node), + preceding: self.preceding_node, + following: None, + parent: self.parents.last().copied(), + line_position: CommentLinePosition::text_position(range, self.source), + kind, + range, + }; + + self.builder.capture(data); + self.comments.next(); + } + + self.preceding_node = Some(node); + } + fn visit_body(&mut self, body: &'ast [ruff_python_ast::Stmt]) { + match body { + [] => { + // no-op + } + [only] => { + self.visit_stmt(only); + } + [first, .., last] => { + if self.can_skip(last.end()) { + // Skip traversing the body when there's no comment between the first and last statement. + // It is still necessary to visit the first statement to process all comments between + // the previous node and the first statement. + self.visit_stmt(first); + self.preceding_node = Some(last.into()); + } else { + preorder::walk_body(self, body); + } + } + } + } +} + +#[derive(Clone, Debug)] +#[allow(dead_code)] +pub(super) struct SuppressionCommentData<'src> { + /// If `enclosing` is `None`, this comment is top-level + pub(super) enclosing: Option>, + pub(super) preceding: Option>, + pub(super) following: Option>, + pub(super) parent: Option>, + pub(super) line_position: CommentLinePosition, + pub(super) kind: SuppressionKind, + pub(super) range: TextRange, +} + +impl<'src> PartialEq for SuppressionCommentData<'src> { + fn eq(&self, other: &Self) -> bool { + self.range.start().eq(&other.range.start()) + } +} + +impl<'src> Eq for SuppressionCommentData<'src> {} + +impl<'src> Ord for SuppressionCommentData<'src> { + fn cmp(&self, other: &Self) -> std::cmp::Ordering { + self.range.start().cmp(&other.range.start()) + } +} + +impl<'src> PartialOrd for SuppressionCommentData<'src> { + fn partial_cmp(&self, other: &Self) -> Option { + Some(self.cmp(other)) + } +} + +impl<'src> Ranged for SuppressionCommentData<'src> { + fn range(&self) -> TextRange { + self.range + } +} + +pub(super) trait CaptureSuppressionComment<'src> { + fn capture(&mut self, comment: SuppressionCommentData<'src>); +} diff --git a/crates/ruff_linter/src/rules/ruff/rules/useless_formatter_noqa.rs b/crates/ruff_linter/src/rules/ruff/rules/useless_formatter_noqa.rs new file mode 100644 index 0000000000000..896e4753e3fae --- /dev/null +++ b/crates/ruff_linter/src/rules/ruff/rules/useless_formatter_noqa.rs @@ -0,0 +1,164 @@ +#![allow(dead_code)] +use std::{collections::BTreeMap, fmt::Display}; + +use ruff_diagnostics::{AlwaysFixableViolation, Diagnostic, Fix}; +use ruff_macros::{derive_message_formats, violation}; +use ruff_python_ast::{AnyNodeRef, Suite}; +use ruff_python_trivia::SuppressionKind; +use ruff_text_size::{Ranged, TextRange}; + +use crate::checkers::{ast::Checker, noqa::delete_comment}; + +use super::suppression_comment_visitor::{ + CaptureSuppressionComment, SuppressionCommentData, SuppressionCommentVisitor, +}; + +/// ## What it does +/// Checks for formatter suppression comments that are ineffective or incompatible +/// with Ruff's formatter. +/// +/// ## Why is this bad? +/// Suppression comments that do not actually prevent formatting could cause unintended changes +/// when the formatter is run. +/// +/// ## Examples +/// In the following example, all suppression comments would cause +/// a rule violation. +/// +/// ```python +/// def decorator(): +/// pass +/// +/// @decorator +/// # fmt: off +/// def example(): +/// if True: +/// expression = 1 + \ # fmt: skip +/// # fmt: off +/// 1 +/// # yapf: disable +/// # fmt: on +/// # yapf: enable +/// ``` +#[violation] +pub struct UselessFormatterNOQA { + reason: UselessReason, +} + +impl AlwaysFixableViolation for UselessFormatterNOQA { + #[derive_message_formats] + fn message(&self) -> String { + format!("Supression comment is useless - {}", self.reason) + } + + fn fix_title(&self) -> String { + format!("Remove this supression comment") + } +} + +/// RUF028 +pub(crate) fn useless_formatter_noqa(checker: &mut Checker, suite: &Suite) { + let indexer = checker.indexer(); + let locator = checker.locator(); + let comment_ranges = indexer.comment_ranges(); + let contents = locator.to_source_code().text(); + + let mut comments = UselessSuppressionComments::new(); + + let visitor = SuppressionCommentVisitor::new(contents, comment_ranges, &mut comments); + + visitor.visit(suite); + + for (comment, reason) in comments.useless_comments() { + checker.diagnostics.push( + Diagnostic::new(UselessFormatterNOQA { reason }, comment.range).with_fix( + Fix::unsafe_edit(delete_comment(comment.range, checker.locator())), + ), + ); + } +} + +struct UselessSuppressionComments<'src> { + captured: BTreeMap, Option>, +} + +impl<'src> UselessSuppressionComments<'src> { + fn new() -> Self { + Self { + captured: BTreeMap::default(), + } + } + fn check_suppression_comment(&self, comment: &SuppressionCommentData) -> Option { + // Check if the comment is inside of an expression. + if comment + .enclosing + .map(AnyNodeRef::is_expression) + .unwrap_or_default() + { + return Some(UselessReason::InsideExpression); + } + if comment.kind == SuppressionKind::Skip && !comment.line_position.is_end_of_line() { + return Some(UselessReason::SkipHasToBeTrailing); + } + // If the comment turns off formatting, we need to make sure + // that something follows it which is worth formatting. + if comment.kind == SuppressionKind::Off && comment.following.is_none() { + return Some(UselessReason::NoCodeSuppressed); + } + // If the comment turns on formatting, we need to check if another + // comment turned formatting off within the same scope. + if comment.kind == SuppressionKind::On { + let enclosing_range = comment + .enclosing + .map_or(TextRange::new(0u32.into(), u32::MAX.into()), |e| e.range()); + let has_corresponding_fmt_off = self + .captured + .iter() + .rev() + .filter(|(c, _)| c.enclosing == comment.enclosing) + .take_while(|(c, _)| c.range.start() >= enclosing_range.start()) + .any(|(c, _)| c.kind == SuppressionKind::Off); + if !has_corresponding_fmt_off { + return Some(UselessReason::NoFmtOff); + } + } + None + } + + fn useless_comments( + &self, + ) -> impl Iterator, UselessReason)> { + self.captured.iter().filter_map(|(c, r)| Some((c, (*r)?))) + } +} + +impl<'src> CaptureSuppressionComment<'src> for UselessSuppressionComments<'src> { + fn capture(&mut self, comment: SuppressionCommentData<'src>) { + let possible_reason = self.check_suppression_comment(&comment); + self.captured.insert(comment, possible_reason); + } +} + +#[derive(Clone, Copy, Debug, PartialEq, Eq)] +enum UselessReason { + InsideExpression, + NoFmtOff, + NoCodeSuppressed, + OnOffNotAllowed, + SkipHasToBeTrailing, +} + +impl Display for UselessReason { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + match self { + Self::InsideExpression => write!( + f, + "suppression comments inside expressions are not supported" + ), + Self::NoFmtOff => write!(f, "formatting was already enabled here"), + Self::NoCodeSuppressed => write!(f, "no eligible code is suppressed by this comment"), + Self::OnOffNotAllowed => write!(f, "on/off suppression comments are not allowed here"), + Self::SkipHasToBeTrailing => write!(f, "a skip comment has to be at the end of a line"), + } + } +} diff --git a/crates/ruff_linter/src/rules/ruff/snapshots/ruff_linter__rules__ruff__tests__RUF028_RUF028.py.snap b/crates/ruff_linter/src/rules/ruff/snapshots/ruff_linter__rules__ruff__tests__RUF028_RUF028.py.snap new file mode 100644 index 0000000000000..38bb30b7456e9 --- /dev/null +++ b/crates/ruff_linter/src/rules/ruff/snapshots/ruff_linter__rules__ruff__tests__RUF028_RUF028.py.snap @@ -0,0 +1,61 @@ +--- +source: crates/ruff_linter/src/rules/ruff/mod.rs +--- +RUF028.py:7:1: RUF028 [*] Supression comment is useless - formatting was already enabled here + | +5 | elif False: +6 | pass +7 | # fmt: on + | ^^^^^^^^^ RUF028 +8 | +9 | def fmt_off_between_lists(): + | + = help: Remove this supression comment + +ℹ Unsafe fix +4 4 | pass +5 5 | elif False: +6 6 | pass +7 |-# fmt: on +8 7 | +9 8 | def fmt_off_between_lists(): +10 9 | test_list = [ + +RUF028.py:11:9: RUF028 [*] Supression comment is useless - suppression comments inside expressions are not supported + | + 9 | def fmt_off_between_lists(): +10 | test_list = [ +11 | #fmt: off + | ^^^^^^^^^ RUF028 +12 | 1, +13 | 2, + | + = help: Remove this supression comment + +ℹ Unsafe fix +8 8 | +9 9 | def fmt_off_between_lists(): +10 10 | test_list = [ +11 |- #fmt: off +12 11 | 1, +13 12 | 2, +14 13 | 3 + +RUF028.py:20:5: RUF028 [*] Supression comment is useless - a skip comment has to be at the end of a line + | +18 | @fmt_off_between_lists +19 | def fmt_off_between_decorators(): +20 | # fmt: skip + | ^^^^^^^^^^^ RUF028 +21 | pass + | + = help: Remove this supression comment + +ℹ Unsafe fix +17 17 | @fmt_off_in_elif +18 18 | @fmt_off_between_lists +19 19 | def fmt_off_between_decorators(): +20 |- # fmt: skip +21 20 | pass + + diff --git a/crates/ruff_python_formatter/src/comments/visitor.rs b/crates/ruff_python_formatter/src/comments/visitor.rs index b98fb7180fd96..6f1b78090c358 100644 --- a/crates/ruff_python_formatter/src/comments/visitor.rs +++ b/crates/ruff_python_formatter/src/comments/visitor.rs @@ -8,7 +8,7 @@ use ruff_python_ast::{Mod, Stmt}; // pre-order. #[allow(clippy::wildcard_imports)] use ruff_python_ast::visitor::preorder::*; -use ruff_python_trivia::{is_python_whitespace, CommentLinePosition, CommentRanges}; +use ruff_python_trivia::{CommentLinePosition, CommentRanges}; use ruff_source_file::Locator; use ruff_text_size::{Ranged, TextRange, TextSize}; @@ -90,7 +90,10 @@ impl<'ast> PreorderVisitor<'ast> for CommentsVisitor<'ast, '_> { preceding: self.preceding_node, following: Some(node), parent: self.parents.iter().rev().nth(1).copied(), - line_position: text_position(*comment_range, self.source_code), + line_position: CommentLinePosition::text_position( + *comment_range, + self.source_code.as_str(), + ), slice: self.source_code.slice(*comment_range), }; @@ -130,7 +133,10 @@ impl<'ast> PreorderVisitor<'ast> for CommentsVisitor<'ast, '_> { parent: self.parents.last().copied(), preceding: self.preceding_node, following: None, - line_position: text_position(*comment_range, self.source_code), + line_position: CommentLinePosition::text_position( + *comment_range, + self.source_code.as_str(), + ), slice: self.source_code.slice(*comment_range), }; @@ -163,22 +169,6 @@ impl<'ast> PreorderVisitor<'ast> for CommentsVisitor<'ast, '_> { } } -fn text_position(comment_range: TextRange, source_code: SourceCode) -> CommentLinePosition { - let before = &source_code.as_str()[TextRange::up_to(comment_range.start())]; - - for c in before.chars().rev() { - match c { - '\n' | '\r' => { - break; - } - c if is_python_whitespace(c) => continue, - _ => return CommentLinePosition::EndOfLine, - } - } - - CommentLinePosition::OwnLine -} - /// A comment decorated with additional information about its surrounding context in the source document. /// /// Used by [`CommentStyle::place_comment`] to determine if this should become a [leading](self#leading-comments), [dangling](self#dangling-comments), or [trailing](self#trailing-comments) comment. diff --git a/crates/ruff_python_trivia/src/suppression.rs b/crates/ruff_python_trivia/src/suppression.rs index f0de8d0bd327d..be8a849f0f156 100644 --- a/crates/ruff_python_trivia/src/suppression.rs +++ b/crates/ruff_python_trivia/src/suppression.rs @@ -1,4 +1,6 @@ -use crate::PythonWhitespace; +use ruff_text_size::TextRange; + +use crate::{is_python_whitespace, PythonWhitespace}; #[derive(Copy, Clone, Eq, PartialEq, Debug)] pub enum SuppressionKind { @@ -95,4 +97,19 @@ impl CommentLinePosition { pub const fn is_end_of_line(self) -> bool { matches!(self, Self::EndOfLine) } + + pub fn text_position(comment_range: TextRange, source_code: &str) -> Self { + let before = &source_code[TextRange::up_to(comment_range.start())]; + + for c in before.chars().rev() { + match c { + '\n' | '\r' => { + break; + } + c if is_python_whitespace(c) => continue, + _ => return Self::EndOfLine, + } + } + Self::OwnLine + } } From f413bd50ff6bb735f4979a0e7eb471b41929cab1 Mon Sep 17 00:00:00 2001 From: Jane Lewis Date: Thu, 8 Feb 2024 14:21:33 -0500 Subject: [PATCH 03/13] Continuing implementation of RUF028. Several edge cases have been addressed and several new problems are now reported. Edge cases involving dangling comments still need to be handled --- .../resources/test/fixtures/ruff/RUF028.py | 69 ++++- .../ruff/rules/suppression_comment_visitor.rs | 142 +++++++++- .../ruff/rules/useless_formatter_noqa.rs | 177 +++++++++--- ..._rules__ruff__tests__RUF028_RUF028.py.snap | 257 +++++++++++++++--- 4 files changed, 551 insertions(+), 94 deletions(-) diff --git a/crates/ruff_linter/resources/test/fixtures/ruff/RUF028.py b/crates/ruff_linter/resources/test/fixtures/ruff/RUF028.py index 8ca1d2a11bbc5..894253d9edcd6 100644 --- a/crates/ruff_linter/resources/test/fixtures/ruff/RUF028.py +++ b/crates/ruff_linter/resources/test/fixtures/ruff/RUF028.py @@ -1,21 +1,78 @@ - -def fmt_off_in_elif(): +# fmt: on +def fmt_off_used_earlier(): if True: - pass + a = 5 + with a: + # fmt: off + pass elif False: + # fmt: off pass + else: + pass + # fmt: off + if True: + # fmt: off + pass + # fmt: off + +# fmt: off + # fmt: on + def fmt_off_between_lists(): test_list = [ - #fmt: off + # fmt: off 1, 2, - 3 + 3, ] -@fmt_off_in_elif + +@fmt_on_after_func +# fmt: off @fmt_off_between_lists def fmt_off_between_decorators(): # fmt: skip pass + +def fmt_on_trailing(): + # fmt: off + val = 5 # fmt: on + pass + +def fmt_off_in_else(): + x = [1, 2, 3] + for val in x: + print(x) + # fmt: off + else: + print("done") + while False: + print("while") + # fmt: on + # fmt: off + else: + print("done") + if len(x) > 3: + print("huh?") + # fmt: on + # fmt: off + else: + print("expected") + +def dangling_fmt_off(): + pass + # fmt: off + +def dangling_fmt_off2(): + if True: + if True: + pass + else: + pass + # fmt: off + else: + pass + # fmt: off diff --git a/crates/ruff_linter/src/rules/ruff/rules/suppression_comment_visitor.rs b/crates/ruff_linter/src/rules/ruff/rules/suppression_comment_visitor.rs index ffb1a9cbef50d..0f3cf4194f257 100644 --- a/crates/ruff_linter/src/rules/ruff/rules/suppression_comment_visitor.rs +++ b/crates/ruff_linter/src/rules/ruff/rules/suppression_comment_visitor.rs @@ -4,8 +4,12 @@ use ruff_python_ast::{ visitor::preorder::{self, PreorderVisitor, TraversalSignal}, AnyNodeRef, Suite, }; -use ruff_python_trivia::{CommentLinePosition, CommentRanges, SuppressionKind}; -use ruff_text_size::{Ranged, TextRange, TextSize}; +use ruff_python_trivia::{ + indentation_at_offset, CommentLinePosition, CommentRanges, SimpleTokenKind, SimpleTokenizer, + SuppressionKind, +}; +use ruff_source_file::Locator; +use ruff_text_size::{Ranged, TextLen, TextRange, TextSize}; #[derive(Clone, Copy, Debug)] struct SuppressionComment { @@ -19,33 +23,35 @@ type CommentIter<'src> = Peekable, M /// Visitor that captures AST data for suppression comments. This uses a similar approach /// to `CommentsVisitor` in the formatter crate. pub(super) struct SuppressionCommentVisitor<'src, 'builder> { - source: &'src str, comments: CommentIter<'src>, parents: Vec>, preceding_node: Option>, + comments_in_scope: Vec<(Option>, SuppressionKind)>, builder: &'builder mut (dyn CaptureSuppressionComment<'src> + 'src), + locator: &'src Locator<'src>, } impl<'src, 'builder> SuppressionCommentVisitor<'src, 'builder> { pub(super) fn new( - source: &'src str, comment_ranges: &'src CommentRanges, builder: &'builder mut (dyn CaptureSuppressionComment<'src> + 'src), + locator: &'src Locator<'src>, ) -> Self { let map_fn: MapCommentFn<'_> = Box::new(|range: &'src TextRange| { Some(SuppressionComment { range: *range, - kind: SuppressionKind::from_slice(&source[*range])?, + kind: SuppressionKind::from_slice(locator.slice(range))?, }) }); Self { - source, comments: comment_ranges.iter().filter_map(map_fn).peekable(), parents: Vec::default(), preceding_node: Option::default(), + comments_in_scope: Vec::with_capacity(comment_ranges.len()), builder, + locator, } } @@ -75,17 +81,25 @@ impl<'ast> PreorderVisitor<'ast> for SuppressionCommentVisitor<'ast, '_> { break; } + let line_position = CommentLinePosition::text_position(range, self.locator.contents()); + + let previous_state = self.comments_in_scope.last().map(|(_, s)| s).copied(); + let data = SuppressionCommentData { enclosing: enclosing_node, preceding: self.preceding_node, following: Some(node), - parent: self.parents.iter().rev().nth(0).copied(), - line_position: CommentLinePosition::text_position(range, self.source), + parent: self.parents.iter().rev().nth(1).copied(), + previous_state, + line_position, kind, range, }; self.builder.capture(data); + if line_position.is_own_line() { + self.comments_in_scope.push((enclosing_node, kind)); + } self.comments.next(); } @@ -101,28 +115,59 @@ impl<'ast> PreorderVisitor<'ast> for SuppressionCommentVisitor<'ast, '_> { } fn leave_node(&mut self, node: AnyNodeRef<'ast>) { - self.parents.pop(); + let parent_node = self.parents.pop(); let node_end = node.end(); + for index in (0..self.comments_in_scope.len()).rev() { + if self.comments_in_scope[index].0 == parent_node { + self.comments_in_scope.pop(); + } else { + break; + } + } + // Process all comments that start after the `preceding` node and end before this node's end. while let Some(SuppressionComment { range, kind }) = self.comments.peek().copied() { - // If the comment starts after this node, break. + let line_position = CommentLinePosition::text_position(range, self.locator.contents()); + // TODO(jane): We want to catch any own-line comments that have the same indentation if range.start() >= node_end { break; + /* + if !line_position.is_own_line() { + break; + } + let Some(preceding) = self.preceding_node else { break; }; + // check indent + let comment_ident = + own_line_comment_indentation(preceding, range, self.locator); + let preceding_indentation = + indentation_at_offset(preceding.start(), self.locator) + .unwrap_or_default() + .text_len(); + if comment_ident < preceding_indentation { + break; + } + */ } + let previous_state = self.comments_in_scope.last().map(|(_, s)| s).copied(); + let data = SuppressionCommentData { enclosing: Some(node), preceding: self.preceding_node, following: None, parent: self.parents.last().copied(), - line_position: CommentLinePosition::text_position(range, self.source), + previous_state, + line_position, kind, range, }; self.builder.capture(data); + if line_position.is_own_line() { + self.comments_in_scope.push((Some(node), kind)); + } self.comments.next(); } @@ -159,6 +204,7 @@ pub(super) struct SuppressionCommentData<'src> { pub(super) preceding: Option>, pub(super) following: Option>, pub(super) parent: Option>, + pub(super) previous_state: Option, pub(super) line_position: CommentLinePosition, pub(super) kind: SuppressionKind, pub(super) range: TextRange, @@ -193,3 +239,77 @@ impl<'src> Ranged for SuppressionCommentData<'src> { pub(super) trait CaptureSuppressionComment<'src> { fn capture(&mut self, comment: SuppressionCommentData<'src>); } + +/// Determine the indentation level of an own-line comment, defined as the minimum indentation of +/// all comments between the preceding node and the comment, including the comment itself. In +/// other words, we don't allow successive comments to ident _further_ than any preceding comments. +/// +/// For example, given: +/// ```python +/// if True: +/// pass +/// # comment +/// ``` +/// +/// The indentation would be 4, as the comment is indented by 4 spaces. +/// +/// Given: +/// ```python +/// if True: +/// pass +/// # comment +/// else: +/// pass +/// ``` +/// +/// The indentation would be 0, as the comment is not indented at all. +/// +/// Given: +/// ```python +/// if True: +/// pass +/// # comment +/// # comment +/// ``` +/// +/// Both comments would be marked as indented at 4 spaces, as the indentation of the first comment +/// is used for the second comment. +/// +/// This logic avoids pathological cases like: +/// ```python +/// try: +/// if True: +/// if True: +/// pass +/// +/// # a +/// # b +/// # c +/// except Exception: +/// pass +/// ``` +/// +/// If we don't use the minimum indentation of any preceding comments, we would mark `# b` as +/// indented to the same depth as `pass`, which could in turn lead to us treating it as a trailing +/// comment of `pass`, despite there being a comment between them that "resets" the indentation. +pub(super) fn own_line_comment_indentation( + preceding: AnyNodeRef, + comment_range: TextRange, + locator: &Locator, +) -> TextSize { + let tokenizer = SimpleTokenizer::new( + locator.contents(), + TextRange::new(locator.full_line_end(preceding.end()), comment_range.end()), + ); + + tokenizer + .filter_map(|token| { + if token.kind() == SimpleTokenKind::Comment { + indentation_at_offset(token.start(), locator).map(TextLen::text_len) + } else { + None + } + }) + .min() + .unwrap_or_default() +} diff --git a/crates/ruff_linter/src/rules/ruff/rules/useless_formatter_noqa.rs b/crates/ruff_linter/src/rules/ruff/rules/useless_formatter_noqa.rs index 896e4753e3fae..c7c23ae8b3565 100644 --- a/crates/ruff_linter/src/rules/ruff/rules/useless_formatter_noqa.rs +++ b/crates/ruff_linter/src/rules/ruff/rules/useless_formatter_noqa.rs @@ -3,14 +3,16 @@ use std::{collections::BTreeMap, fmt::Display}; use ruff_diagnostics::{AlwaysFixableViolation, Diagnostic, Fix}; use ruff_macros::{derive_message_formats, violation}; -use ruff_python_ast::{AnyNodeRef, Suite}; -use ruff_python_trivia::SuppressionKind; -use ruff_text_size::{Ranged, TextRange}; +use ruff_python_ast::{self as ast, AnyNodeRef}; +use ruff_python_trivia::{indentation_at_offset, SuppressionKind}; +use ruff_source_file::Locator; +use ruff_text_size::{Ranged, TextLen}; use crate::checkers::{ast::Checker, noqa::delete_comment}; use super::suppression_comment_visitor::{ - CaptureSuppressionComment, SuppressionCommentData, SuppressionCommentVisitor, + own_line_comment_indentation, CaptureSuppressionComment, SuppressionCommentData, + SuppressionCommentVisitor, }; /// ## What it does @@ -48,24 +50,23 @@ pub struct UselessFormatterNOQA { impl AlwaysFixableViolation for UselessFormatterNOQA { #[derive_message_formats] fn message(&self) -> String { - format!("Supression comment is useless - {}", self.reason) + format!("Suppression comment is useless - {}", self.reason) } fn fix_title(&self) -> String { - format!("Remove this supression comment") + format!("Remove this suppression comment") } } /// RUF028 -pub(crate) fn useless_formatter_noqa(checker: &mut Checker, suite: &Suite) { +pub(crate) fn useless_formatter_noqa(checker: &mut Checker, suite: &ast::Suite) { let indexer = checker.indexer(); let locator = checker.locator(); let comment_ranges = indexer.comment_ranges(); - let contents = locator.to_source_code().text(); - let mut comments = UselessSuppressionComments::new(); + let mut comments = UselessSuppressionComments::new(locator); - let visitor = SuppressionCommentVisitor::new(contents, comment_ranges, &mut comments); + let visitor = SuppressionCommentVisitor::new(comment_ranges, &mut comments, checker.locator()); visitor.visit(suite); @@ -78,18 +79,22 @@ pub(crate) fn useless_formatter_noqa(checker: &mut Checker, suite: &Suite) { } } -struct UselessSuppressionComments<'src> { +struct UselessSuppressionComments<'src, 'loc> { captured: BTreeMap, Option>, + locator: &'loc Locator<'src>, } -impl<'src> UselessSuppressionComments<'src> { - fn new() -> Self { +impl<'src, 'loc> UselessSuppressionComments<'src, 'loc> { + fn new(locator: &'loc Locator<'src>) -> Self { Self { captured: BTreeMap::default(), + locator, } } + /// This function determines whether or not `comment` is a useful suppression comment. + /// If it isn't, it will give a reason why the comment is useless. See [`UselessReason`] for more. fn check_suppression_comment(&self, comment: &SuppressionCommentData) -> Option { - // Check if the comment is inside of an expression. + // check if the comment is inside of an expression. if comment .enclosing .map(AnyNodeRef::is_expression) @@ -97,29 +102,72 @@ impl<'src> UselessSuppressionComments<'src> { { return Some(UselessReason::InsideExpression); } + + // check if a skip comment is at the end of a line if comment.kind == SuppressionKind::Skip && !comment.line_position.is_end_of_line() { return Some(UselessReason::SkipHasToBeTrailing); } - // If the comment turns off formatting, we need to make sure - // that something follows it which is worth formatting. - if comment.kind == SuppressionKind::Off && comment.following.is_none() { - return Some(UselessReason::NoCodeSuppressed); + + if comment.kind == SuppressionKind::Off && comment.line_position.is_own_line() { + // check for a previous `fmt: off` + if comment.previous_state == Some(SuppressionKind::Off) { + return Some(UselessReason::FmtOffUsedEarlier); + } + let Some(following) = comment.following else { + return Some(UselessReason::NoCodeSuppressed); + }; + if let Some(enclosing) = comment.enclosing { + // check if this comment is dangling (in other words, in a block with nothing following it) + + // check if this comment is before an alternative body (for example: an `else` or `elif`) + if let Some(preceding) = comment.preceding { + if is_first_statement_in_alternate_body(following, enclosing) { + // check indentation + let comment_indentation = + own_line_comment_indentation(preceding, comment.range, self.locator); + + let preceding_indentation = + indentation_at_offset(preceding.start(), self.locator) + .unwrap_or_default() + .text_len(); + if comment_indentation <= preceding_indentation { + return Some(UselessReason::FmtOffOverElseBlock); + } + } + } + } } - // If the comment turns on formatting, we need to check if another - // comment turned formatting off within the same scope. + if comment.kind == SuppressionKind::On { - let enclosing_range = comment - .enclosing - .map_or(TextRange::new(0u32.into(), u32::MAX.into()), |e| e.range()); - let has_corresponding_fmt_off = self - .captured - .iter() - .rev() - .filter(|(c, _)| c.enclosing == comment.enclosing) - .take_while(|(c, _)| c.range.start() >= enclosing_range.start()) - .any(|(c, _)| c.kind == SuppressionKind::Off); - if !has_corresponding_fmt_off { - return Some(UselessReason::NoFmtOff); + // Ensure the comment is not a trailing comment + if !comment.line_position.is_own_line() { + return Some(UselessReason::FmtOnCannotBeTrailing); + } + + // If the comment turns on formatting, we need to check if another + // comment turned formatting off within the same scope. + match comment.previous_state { + None | Some(SuppressionKind::On) => return Some(UselessReason::NoFmtOff), + _ => {} + } + } + + if comment.kind == SuppressionKind::Off || comment.kind == SuppressionKind::On { + if let Some(enclosing) = comment.enclosing { + match enclosing { + AnyNodeRef::StmtClassDef(class_def) => { + if comment.line_position.is_own_line() + && comment.start() < class_def.name.start() + { + if let Some(decorator) = class_def.decorator_list.last() { + if decorator.end() < comment.start() { + return Some(UselessReason::BetweenDecorators); + } + } + } + } + _ => {} + } } } None @@ -132,20 +180,64 @@ impl<'src> UselessSuppressionComments<'src> { } } -impl<'src> CaptureSuppressionComment<'src> for UselessSuppressionComments<'src> { +impl<'src, 'loc> CaptureSuppressionComment<'src> for UselessSuppressionComments<'src, 'loc> { fn capture(&mut self, comment: SuppressionCommentData<'src>) { let possible_reason = self.check_suppression_comment(&comment); self.captured.insert(comment, possible_reason); } } +/// Returns `true` if `statement` is the first statement in an alternate `body` (e.g. the else of an if statement) +fn is_first_statement_in_alternate_body(statement: AnyNodeRef, has_body: AnyNodeRef) -> bool { + match has_body { + AnyNodeRef::StmtFor(ast::StmtFor { orelse, .. }) + | AnyNodeRef::StmtWhile(ast::StmtWhile { orelse, .. }) => { + are_same_optional(statement, orelse.first()) + } + + AnyNodeRef::StmtTry(ast::StmtTry { + handlers, + orelse, + finalbody, + .. + }) => { + are_same_optional(statement, handlers.first()) + || are_same_optional(statement, orelse.first()) + || are_same_optional(statement, finalbody.first()) + } + + AnyNodeRef::StmtIf(ast::StmtIf { + elif_else_clauses, .. + }) => are_same_optional(statement, elif_else_clauses.first()), + _ => false, + } +} + +/// Returns `true` if the parameters are parenthesized (as in a function definition), or `false` if +/// not (as in a lambda). +fn are_parameters_parenthesized(parameters: &ast::Parameters, contents: &str) -> bool { + // A lambda never has parentheses around its parameters, but a function definition always does. + contents[parameters.range()].starts_with('(') +} + +/// Returns `true` if `right` is `Some` and `left` and `right` are referentially equal. +fn are_same_optional<'a, T>(left: AnyNodeRef, right: Option) -> bool +where + T: Into>, +{ + right.is_some_and(|right| left.ptr_eq(right.into())) +} + #[derive(Clone, Copy, Debug, PartialEq, Eq)] enum UselessReason { InsideExpression, + FmtOffUsedEarlier, NoFmtOff, NoCodeSuppressed, - OnOffNotAllowed, + BetweenDecorators, + FmtOnCannotBeTrailing, SkipHasToBeTrailing, + FmtOffOverElseBlock, } impl Display for UselessReason { @@ -155,10 +247,21 @@ impl Display for UselessReason { f, "suppression comments inside expressions are not supported" ), - Self::NoFmtOff => write!(f, "formatting was already enabled here"), + Self::FmtOffUsedEarlier => write!(f, "formatting is already disabled here"), + Self::NoFmtOff => write!(f, "formatting is already enabled here"), Self::NoCodeSuppressed => write!(f, "no eligible code is suppressed by this comment"), - Self::OnOffNotAllowed => write!(f, "on/off suppression comments are not allowed here"), - Self::SkipHasToBeTrailing => write!(f, "a skip comment has to be at the end of a line"), + Self::BetweenDecorators => { + write!(f, "suppression comment cannot be between decorators") + } + Self::SkipHasToBeTrailing => { + write!(f, "'fmt: skip' has to be at the end of a line") + } + Self::FmtOnCannotBeTrailing => { + write!(f, "'fmt: on' cannot be at the end of a line") + } + Self::FmtOffOverElseBlock => { + write!(f, "'fmt: off' cannot be in front of an else/elif") + } } } } diff --git a/crates/ruff_linter/src/rules/ruff/snapshots/ruff_linter__rules__ruff__tests__RUF028_RUF028.py.snap b/crates/ruff_linter/src/rules/ruff/snapshots/ruff_linter__rules__ruff__tests__RUF028_RUF028.py.snap index 38bb30b7456e9..80204590a5021 100644 --- a/crates/ruff_linter/src/rules/ruff/snapshots/ruff_linter__rules__ruff__tests__RUF028_RUF028.py.snap +++ b/crates/ruff_linter/src/rules/ruff/snapshots/ruff_linter__rules__ruff__tests__RUF028_RUF028.py.snap @@ -1,61 +1,238 @@ --- source: crates/ruff_linter/src/rules/ruff/mod.rs --- -RUF028.py:7:1: RUF028 [*] Supression comment is useless - formatting was already enabled here +RUF028.py:1:1: RUF028 [*] Suppression comment is useless - formatting is already enabled here | -5 | elif False: -6 | pass -7 | # fmt: on +1 | # fmt: on | ^^^^^^^^^ RUF028 -8 | -9 | def fmt_off_between_lists(): +2 | def fmt_off_used_earlier(): +3 | if True: | - = help: Remove this supression comment + = help: Remove this suppression comment ℹ Unsafe fix -4 4 | pass -5 5 | elif False: -6 6 | pass -7 |-# fmt: on -8 7 | -9 8 | def fmt_off_between_lists(): -10 9 | test_list = [ +1 |-# fmt: on +2 1 | def fmt_off_used_earlier(): +3 2 | if True: +4 3 | a = 5 -RUF028.py:11:9: RUF028 [*] Supression comment is useless - suppression comments inside expressions are not supported +RUF028.py:15:9: RUF028 [*] Suppression comment is useless - formatting is already disabled here | - 9 | def fmt_off_between_lists(): -10 | test_list = [ -11 | #fmt: off - | ^^^^^^^^^ RUF028 -12 | 1, -13 | 2, +13 | # fmt: off +14 | if True: +15 | # fmt: off + | ^^^^^^^^^^ RUF028 +16 | pass +17 | # fmt: off | - = help: Remove this supression comment + = help: Remove this suppression comment ℹ Unsafe fix -8 8 | -9 9 | def fmt_off_between_lists(): -10 10 | test_list = [ -11 |- #fmt: off -12 11 | 1, -13 12 | 2, -14 13 | 3 +12 12 | pass +13 13 | # fmt: off +14 14 | if True: +15 |- # fmt: off +16 15 | pass +17 16 | # fmt: off +18 17 | -RUF028.py:20:5: RUF028 [*] Supression comment is useless - a skip comment has to be at the end of a line +RUF028.py:19:1: RUF028 [*] Suppression comment is useless - formatting is already disabled here | -18 | @fmt_off_between_lists -19 | def fmt_off_between_decorators(): -20 | # fmt: skip +17 | # fmt: off +18 | +19 | # fmt: off + | ^^^^^^^^^^ RUF028 +20 | +21 | # fmt: on + | + = help: Remove this suppression comment + +ℹ Unsafe fix +16 16 | pass +17 17 | # fmt: off +18 18 | +19 |-# fmt: off +20 19 | +21 20 | # fmt: on +22 21 | + +RUF028.py:26:9: RUF028 [*] Suppression comment is useless - suppression comments inside expressions are not supported + | +24 | def fmt_off_between_lists(): +25 | test_list = [ +26 | # fmt: off + | ^^^^^^^^^^ RUF028 +27 | 1, +28 | 2, + | + = help: Remove this suppression comment + +ℹ Unsafe fix +23 23 | +24 24 | def fmt_off_between_lists(): +25 25 | test_list = [ +26 |- # fmt: off +27 26 | 1, +28 27 | 2, +29 28 | 3, + +RUF028.py:37:5: RUF028 [*] Suppression comment is useless - 'fmt: skip' has to be at the end of a line + | +35 | @fmt_off_between_lists +36 | def fmt_off_between_decorators(): +37 | # fmt: skip | ^^^^^^^^^^^ RUF028 -21 | pass +38 | pass + | + = help: Remove this suppression comment + +ℹ Unsafe fix +34 34 | # fmt: off +35 35 | @fmt_off_between_lists +36 36 | def fmt_off_between_decorators(): +37 |- # fmt: skip +38 37 | pass +39 38 | +40 39 | def fmt_on_trailing(): + +RUF028.py:42:13: RUF028 [*] Suppression comment is useless - 'fmt: on' cannot be at the end of a line + | +40 | def fmt_on_trailing(): +41 | # fmt: off +42 | val = 5 # fmt: on + | ^^^^^^^^^ RUF028 +43 | pass + | + = help: Remove this suppression comment + +ℹ Unsafe fix +39 39 | +40 40 | def fmt_on_trailing(): +41 41 | # fmt: off +42 |- val = 5 # fmt: on + 42 |+ val = 5 +43 43 | pass +44 44 | +45 45 | def fmt_off_in_else(): + +RUF028.py:49:5: RUF028 [*] Suppression comment is useless - 'fmt: off' cannot be in front of an else/elif + | +47 | for val in x: +48 | print(x) +49 | # fmt: off + | ^^^^^^^^^^ RUF028 +50 | else: +51 | print("done") + | + = help: Remove this suppression comment + +ℹ Unsafe fix +46 46 | x = [1, 2, 3] +47 47 | for val in x: +48 48 | print(x) +49 |- # fmt: off +50 49 | else: +51 50 | print("done") +52 51 | while False: + +RUF028.py:54:5: RUF028 [*] Suppression comment is useless - formatting is already enabled here + | +52 | while False: +53 | print("while") +54 | # fmt: on + | ^^^^^^^^^ RUF028 +55 | # fmt: off +56 | else: + | + = help: Remove this suppression comment + +ℹ Unsafe fix +51 51 | print("done") +52 52 | while False: +53 53 | print("while") +54 |- # fmt: on +55 54 | # fmt: off +56 55 | else: +57 56 | print("done") + +RUF028.py:55:5: RUF028 [*] Suppression comment is useless - 'fmt: off' cannot be in front of an else/elif + | +53 | print("while") +54 | # fmt: on +55 | # fmt: off + | ^^^^^^^^^^ RUF028 +56 | else: +57 | print("done") + | + = help: Remove this suppression comment + +ℹ Unsafe fix +52 52 | while False: +53 53 | print("while") +54 54 | # fmt: on +55 |- # fmt: off +56 55 | else: +57 56 | print("done") +58 57 | if len(x) > 3: + +RUF028.py:60:5: RUF028 [*] Suppression comment is useless - formatting is already enabled here + | +58 | if len(x) > 3: +59 | print("huh?") +60 | # fmt: on + | ^^^^^^^^^ RUF028 +61 | # fmt: off +62 | else: + | + = help: Remove this suppression comment + +ℹ Unsafe fix +57 57 | print("done") +58 58 | if len(x) > 3: +59 59 | print("huh?") +60 |- # fmt: on +61 60 | # fmt: off +62 61 | else: +63 62 | print("expected") + +RUF028.py:61:5: RUF028 [*] Suppression comment is useless - 'fmt: off' cannot be in front of an else/elif + | +59 | print("huh?") +60 | # fmt: on +61 | # fmt: off + | ^^^^^^^^^^ RUF028 +62 | else: +63 | print("expected") + | + = help: Remove this suppression comment + +ℹ Unsafe fix +58 58 | if len(x) > 3: +59 59 | print("huh?") +60 60 | # fmt: on +61 |- # fmt: off +62 61 | else: +63 62 | print("expected") +64 63 | + +RUF028.py:75:9: RUF028 [*] Suppression comment is useless - formatting is already disabled here + | +73 | else: +74 | pass +75 | # fmt: off + | ^^^^^^^^^^ RUF028 +76 | else: +77 | pass | - = help: Remove this supression comment + = help: Remove this suppression comment ℹ Unsafe fix -17 17 | @fmt_off_in_elif -18 18 | @fmt_off_between_lists -19 19 | def fmt_off_between_decorators(): -20 |- # fmt: skip -21 20 | pass +72 72 | pass +73 73 | else: +74 74 | pass +75 |- # fmt: off +76 75 | else: +77 76 | pass +78 77 | # fmt: off From 61d7e1098df36e7a23f902bcaadecf54f73311c7 Mon Sep 17 00:00:00 2001 From: Jane Lewis Date: Wed, 14 Feb 2024 14:40:32 -0800 Subject: [PATCH 04/13] Correctly attach end-of-body comments to the body and ignore invalid comments when determining suppression state --- .../resources/test/fixtures/ruff/RUF028.py | 2 + .../ruff/rules/suppression_comment_visitor.rs | 54 ++++--- .../ruff/rules/useless_formatter_noqa.rs | 59 ++++---- ..._rules__ruff__tests__RUF028_RUF028.py.snap | 138 +++++++++++++++--- 4 files changed, 177 insertions(+), 76 deletions(-) diff --git a/crates/ruff_linter/resources/test/fixtures/ruff/RUF028.py b/crates/ruff_linter/resources/test/fixtures/ruff/RUF028.py index 894253d9edcd6..cc9b09cfee842 100644 --- a/crates/ruff_linter/resources/test/fixtures/ruff/RUF028.py +++ b/crates/ruff_linter/resources/test/fixtures/ruff/RUF028.py @@ -70,8 +70,10 @@ def dangling_fmt_off2(): if True: if True: pass + # fmt: off else: pass + # fmt: off # fmt: off else: pass diff --git a/crates/ruff_linter/src/rules/ruff/rules/suppression_comment_visitor.rs b/crates/ruff_linter/src/rules/ruff/rules/suppression_comment_visitor.rs index 0f3cf4194f257..a2ed26cf6cffe 100644 --- a/crates/ruff_linter/src/rules/ruff/rules/suppression_comment_visitor.rs +++ b/crates/ruff_linter/src/rules/ruff/rules/suppression_comment_visitor.rs @@ -96,9 +96,8 @@ impl<'ast> PreorderVisitor<'ast> for SuppressionCommentVisitor<'ast, '_> { range, }; - self.builder.capture(data); - if line_position.is_own_line() { - self.comments_in_scope.push((enclosing_node, kind)); + if let Some(kind) = self.builder.capture(data) { + self.comments_in_scope.push((Some(node), kind)); } self.comments.next(); } @@ -115,40 +114,28 @@ impl<'ast> PreorderVisitor<'ast> for SuppressionCommentVisitor<'ast, '_> { } fn leave_node(&mut self, node: AnyNodeRef<'ast>) { - let parent_node = self.parents.pop(); + self.parents.pop(); let node_end = node.end(); - for index in (0..self.comments_in_scope.len()).rev() { - if self.comments_in_scope[index].0 == parent_node { - self.comments_in_scope.pop(); - } else { - break; - } - } - // Process all comments that start after the `preceding` node and end before this node's end. while let Some(SuppressionComment { range, kind }) = self.comments.peek().copied() { let line_position = CommentLinePosition::text_position(range, self.locator.contents()); - // TODO(jane): We want to catch any own-line comments that have the same indentation if range.start() >= node_end { - break; - /* if !line_position.is_own_line() { break; } - let Some(preceding) = self.preceding_node else { break; }; - // check indent - let comment_ident = - own_line_comment_indentation(preceding, range, self.locator); - let preceding_indentation = - indentation_at_offset(preceding.start(), self.locator) - .unwrap_or_default() - .text_len(); - if comment_ident < preceding_indentation { + let Some(preceding) = self.preceding_node else { + break; + }; + // check indent of comment against the minimum indentation of a hypothetical body + let comment_indent = own_line_comment_indentation(preceding, range, self.locator); + let min_indentation = indentation_at_offset(node.start(), self.locator) + .unwrap_or_default() + .text_len(); + if comment_indent <= min_indentation { break; } - */ } let previous_state = self.comments_in_scope.last().map(|(_, s)| s).copied(); @@ -164,13 +151,21 @@ impl<'ast> PreorderVisitor<'ast> for SuppressionCommentVisitor<'ast, '_> { range, }; - self.builder.capture(data); - if line_position.is_own_line() { + if let Some(kind) = self.builder.capture(data) { self.comments_in_scope.push((Some(node), kind)); } self.comments.next(); } + // remove comments that are about to become out of scope + for index in (0..self.comments_in_scope.len()).rev() { + if self.comments_in_scope[index].0 == Some(node) { + self.comments_in_scope.pop(); + } else { + break; + } + } + self.preceding_node = Some(node); } fn visit_body(&mut self, body: &'ast [ruff_python_ast::Stmt]) { @@ -237,7 +232,10 @@ impl<'src> Ranged for SuppressionCommentData<'src> { } pub(super) trait CaptureSuppressionComment<'src> { - fn capture(&mut self, comment: SuppressionCommentData<'src>); + /// This is the entrypoint for the capturer to analyze the next comment. + /// Returning a `Some` value will update the suppression state for future comments. + #[must_use] + fn capture(&mut self, comment: SuppressionCommentData<'src>) -> Option; } /// Determine the indentation level of an own-line comment, defined as the minimum indentation of diff --git a/crates/ruff_linter/src/rules/ruff/rules/useless_formatter_noqa.rs b/crates/ruff_linter/src/rules/ruff/rules/useless_formatter_noqa.rs index c7c23ae8b3565..38f74b065fea4 100644 --- a/crates/ruff_linter/src/rules/ruff/rules/useless_formatter_noqa.rs +++ b/crates/ruff_linter/src/rules/ruff/rules/useless_formatter_noqa.rs @@ -80,7 +80,8 @@ pub(crate) fn useless_formatter_noqa(checker: &mut Checker, suite: &ast::Suite) } struct UselessSuppressionComments<'src, 'loc> { - captured: BTreeMap, Option>, + captured: BTreeMap, UselessReason>, + comments_in_scope: Vec<(Option>, SuppressionKind)>, locator: &'loc Locator<'src>, } @@ -88,33 +89,37 @@ impl<'src, 'loc> UselessSuppressionComments<'src, 'loc> { fn new(locator: &'loc Locator<'src>) -> Self { Self { captured: BTreeMap::default(), + comments_in_scope: vec![], locator, } } /// This function determines whether or not `comment` is a useful suppression comment. /// If it isn't, it will give a reason why the comment is useless. See [`UselessReason`] for more. - fn check_suppression_comment(&self, comment: &SuppressionCommentData) -> Option { + fn check_suppression_comment( + &self, + comment: &SuppressionCommentData, + ) -> Result, UselessReason> { // check if the comment is inside of an expression. if comment .enclosing .map(AnyNodeRef::is_expression) .unwrap_or_default() { - return Some(UselessReason::InsideExpression); + return Err(UselessReason::InsideExpression); } // check if a skip comment is at the end of a line if comment.kind == SuppressionKind::Skip && !comment.line_position.is_end_of_line() { - return Some(UselessReason::SkipHasToBeTrailing); + return Err(UselessReason::SkipHasToBeTrailing); } if comment.kind == SuppressionKind::Off && comment.line_position.is_own_line() { // check for a previous `fmt: off` if comment.previous_state == Some(SuppressionKind::Off) { - return Some(UselessReason::FmtOffUsedEarlier); + return Err(UselessReason::FmtOffUsedEarlier); } let Some(following) = comment.following else { - return Some(UselessReason::NoCodeSuppressed); + return Err(UselessReason::NoCodeSuppressed); }; if let Some(enclosing) = comment.enclosing { // check if this comment is dangling (in other words, in a block with nothing following it) @@ -131,7 +136,7 @@ impl<'src, 'loc> UselessSuppressionComments<'src, 'loc> { .unwrap_or_default() .text_len(); if comment_indentation <= preceding_indentation { - return Some(UselessReason::FmtOffOverElseBlock); + return Err(UselessReason::FmtOffOverElseBlock); } } } @@ -141,49 +146,51 @@ impl<'src, 'loc> UselessSuppressionComments<'src, 'loc> { if comment.kind == SuppressionKind::On { // Ensure the comment is not a trailing comment if !comment.line_position.is_own_line() { - return Some(UselessReason::FmtOnCannotBeTrailing); + return Err(UselessReason::FmtOnCannotBeTrailing); } // If the comment turns on formatting, we need to check if another // comment turned formatting off within the same scope. match comment.previous_state { - None | Some(SuppressionKind::On) => return Some(UselessReason::NoFmtOff), + None | Some(SuppressionKind::On) => return Err(UselessReason::NoFmtOff), _ => {} } } if comment.kind == SuppressionKind::Off || comment.kind == SuppressionKind::On { - if let Some(enclosing) = comment.enclosing { - match enclosing { - AnyNodeRef::StmtClassDef(class_def) => { - if comment.line_position.is_own_line() - && comment.start() < class_def.name.start() - { - if let Some(decorator) = class_def.decorator_list.last() { - if decorator.end() < comment.start() { - return Some(UselessReason::BetweenDecorators); - } - } + if let Some(AnyNodeRef::StmtClassDef(class_def)) = comment.enclosing { + if comment.line_position.is_own_line() && comment.start() < class_def.name.start() { + if let Some(decorator) = class_def.decorator_list.last() { + if decorator.end() < comment.start() { + return Err(UselessReason::BetweenDecorators); } } - _ => {} } } + + // at this point, any comment being handled should be considered 'valid'. + // on/off suppression comments should be added to the scope + return Ok(Some(comment.kind)); } - None + Ok(None) } fn useless_comments( &self, ) -> impl Iterator, UselessReason)> { - self.captured.iter().filter_map(|(c, r)| Some((c, (*r)?))) + self.captured.iter().map(|(c, r)| (c, *r)) } } impl<'src, 'loc> CaptureSuppressionComment<'src> for UselessSuppressionComments<'src, 'loc> { - fn capture(&mut self, comment: SuppressionCommentData<'src>) { - let possible_reason = self.check_suppression_comment(&comment); - self.captured.insert(comment, possible_reason); + fn capture(&mut self, comment: SuppressionCommentData<'src>) -> Option { + match self.check_suppression_comment(&comment) { + Ok(kind) => kind, + Err(reason) => { + self.captured.insert(comment, reason); + None + } + } } } diff --git a/crates/ruff_linter/src/rules/ruff/snapshots/ruff_linter__rules__ruff__tests__RUF028_RUF028.py.snap b/crates/ruff_linter/src/rules/ruff/snapshots/ruff_linter__rules__ruff__tests__RUF028_RUF028.py.snap index 80204590a5021..7c655a4878c06 100644 --- a/crates/ruff_linter/src/rules/ruff/snapshots/ruff_linter__rules__ruff__tests__RUF028_RUF028.py.snap +++ b/crates/ruff_linter/src/rules/ruff/snapshots/ruff_linter__rules__ruff__tests__RUF028_RUF028.py.snap @@ -36,25 +36,25 @@ RUF028.py:15:9: RUF028 [*] Suppression comment is useless - formatting is alread 17 16 | # fmt: off 18 17 | -RUF028.py:19:1: RUF028 [*] Suppression comment is useless - formatting is already disabled here +RUF028.py:17:5: RUF028 [*] Suppression comment is useless - no eligible code is suppressed by this comment | +15 | # fmt: off +16 | pass 17 | # fmt: off + | ^^^^^^^^^^^ RUF028 18 | 19 | # fmt: off - | ^^^^^^^^^^ RUF028 -20 | -21 | # fmt: on | = help: Remove this suppression comment ℹ Unsafe fix +14 14 | if True: +15 15 | # fmt: off 16 16 | pass -17 17 | # fmt: off -18 18 | -19 |-# fmt: off +17 |- # fmt: off +18 17 | +19 18 | # fmt: off 20 19 | -21 20 | # fmt: on -22 21 | RUF028.py:26:9: RUF028 [*] Suppression comment is useless - suppression comments inside expressions are not supported | @@ -95,6 +95,25 @@ RUF028.py:37:5: RUF028 [*] Suppression comment is useless - 'fmt: skip' has to b 39 38 | 40 39 | def fmt_on_trailing(): +RUF028.py:41:5: RUF028 [*] Suppression comment is useless - no eligible code is suppressed by this comment + | +40 | def fmt_on_trailing(): +41 | # fmt: off + | ^^^^^^^^^^ RUF028 +42 | val = 5 # fmt: on +43 | pass + | + = help: Remove this suppression comment + +ℹ Unsafe fix +38 38 | pass +39 39 | +40 40 | def fmt_on_trailing(): +41 |- # fmt: off +42 41 | val = 5 # fmt: on +43 42 | pass +44 43 | + RUF028.py:42:13: RUF028 [*] Suppression comment is useless - 'fmt: on' cannot be at the end of a line | 40 | def fmt_on_trailing(): @@ -215,24 +234,99 @@ RUF028.py:61:5: RUF028 [*] Suppression comment is useless - 'fmt: off' cannot be 63 62 | print("expected") 64 63 | -RUF028.py:75:9: RUF028 [*] Suppression comment is useless - formatting is already disabled here +RUF028.py:67:5: RUF028 [*] Suppression comment is useless - no eligible code is suppressed by this comment | -73 | else: -74 | pass -75 | # fmt: off - | ^^^^^^^^^^ RUF028 -76 | else: -77 | pass +65 | def dangling_fmt_off(): +66 | pass +67 | # fmt: off + | ^^^^^^^^^^ RUF028 +68 | +69 | def dangling_fmt_off2(): + | + = help: Remove this suppression comment + +ℹ Unsafe fix +64 64 | +65 65 | def dangling_fmt_off(): +66 66 | pass +67 |- # fmt: off +68 67 | +69 68 | def dangling_fmt_off2(): +70 69 | if True: + +RUF028.py:73:13: RUF028 [*] Suppression comment is useless - no eligible code is suppressed by this comment + | +71 | if True: +72 | pass +73 | # fmt: off + | ^^^^^^^^^^ RUF028 +74 | else: +75 | pass | = help: Remove this suppression comment ℹ Unsafe fix +70 70 | if True: +71 71 | if True: 72 72 | pass -73 73 | else: -74 74 | pass -75 |- # fmt: off -76 75 | else: -77 76 | pass -78 77 | # fmt: off +73 |- # fmt: off +74 73 | else: +75 74 | pass +76 75 | # fmt: off + +RUF028.py:76:13: RUF028 [*] Suppression comment is useless - no eligible code is suppressed by this comment + | +74 | else: +75 | pass +76 | # fmt: off + | ^^^^^^^^^^ RUF028 +77 | # fmt: off +78 | else: + | + = help: Remove this suppression comment + +ℹ Unsafe fix +73 73 | # fmt: off +74 74 | else: +75 75 | pass +76 |- # fmt: off +77 76 | # fmt: off +78 77 | else: +79 78 | pass + +RUF028.py:77:9: RUF028 [*] Suppression comment is useless - no eligible code is suppressed by this comment + | +75 | pass +76 | # fmt: off +77 | # fmt: off + | ^^^^^^^^^^ RUF028 +78 | else: +79 | pass + | + = help: Remove this suppression comment + +ℹ Unsafe fix +74 74 | else: +75 75 | pass +76 76 | # fmt: off +77 |- # fmt: off +78 77 | else: +79 78 | pass +80 79 | # fmt: off + +RUF028.py:80:5: RUF028 [*] Suppression comment is useless - no eligible code is suppressed by this comment + | +78 | else: +79 | pass +80 | # fmt: off + | ^^^^^^^^^^ RUF028 + | + = help: Remove this suppression comment + +ℹ Unsafe fix +77 77 | # fmt: off +78 78 | else: +79 79 | pass +80 |- # fmt: off From dcd9fef6106d7b7fddb084e1c46bf419d23a4cb7 Mon Sep 17 00:00:00 2001 From: Jane Lewis Date: Wed, 14 Feb 2024 14:51:28 -0800 Subject: [PATCH 05/13] Fix parse error in RUF028 docs --- .../src/rules/ruff/rules/useless_formatter_noqa.rs | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/crates/ruff_linter/src/rules/ruff/rules/useless_formatter_noqa.rs b/crates/ruff_linter/src/rules/ruff/rules/useless_formatter_noqa.rs index 38f74b065fea4..4673bb2f80c4d 100644 --- a/crates/ruff_linter/src/rules/ruff/rules/useless_formatter_noqa.rs +++ b/crates/ruff_linter/src/rules/ruff/rules/useless_formatter_noqa.rs @@ -35,9 +35,12 @@ use super::suppression_comment_visitor::{ /// # fmt: off /// def example(): /// if True: -/// expression = 1 + \ # fmt: skip -/// # fmt: off -/// 1 +/// # fmt: skip +/// expression = [ +/// # fmt: off +/// 1, +/// 2 +/// ] /// # yapf: disable /// # fmt: on /// # yapf: enable From b1539afc154eb5eacb42e1d365cb66ec96233cb9 Mon Sep 17 00:00:00 2001 From: Jane Lewis Date: Wed, 14 Feb 2024 14:52:32 -0800 Subject: [PATCH 06/13] Update ruff.schema.json --- ruff.schema.json | 1 + 1 file changed, 1 insertion(+) diff --git a/ruff.schema.json b/ruff.schema.json index ae81665c41985..28d31208b0060 100644 --- a/ruff.schema.json +++ b/ruff.schema.json @@ -3531,6 +3531,7 @@ "RUF025", "RUF026", "RUF027", + "RUF028", "RUF1", "RUF10", "RUF100", From 809d1825f261f072b1e24f4ac7a7dac11099dfd2 Mon Sep 17 00:00:00 2001 From: Jane Lewis Date: Wed, 14 Feb 2024 15:31:53 -0800 Subject: [PATCH 07/13] Revise the language and messages for RUF028 --- .../src/checkers/ast/analyze/module.rs | 4 +- crates/ruff_linter/src/codes.rs | 2 +- crates/ruff_linter/src/rules/ruff/mod.rs | 2 +- ...tter_noqa.rs => ignored_formatter_noqa.rs} | 62 ++++++++--------- .../ruff_linter/src/rules/ruff/rules/mod.rs | 4 +- ..._rules__ruff__tests__RUF028_RUF028.py.snap | 68 +++++++++---------- 6 files changed, 71 insertions(+), 71 deletions(-) rename crates/ruff_linter/src/rules/ruff/rules/{useless_formatter_noqa.rs => ignored_formatter_noqa.rs} (81%) diff --git a/crates/ruff_linter/src/checkers/ast/analyze/module.rs b/crates/ruff_linter/src/checkers/ast/analyze/module.rs index 52c72f1afdae4..62316f280a1de 100644 --- a/crates/ruff_linter/src/checkers/ast/analyze/module.rs +++ b/crates/ruff_linter/src/checkers/ast/analyze/module.rs @@ -9,7 +9,7 @@ pub(crate) fn module(suite: &Suite, checker: &mut Checker) { if checker.enabled(Rule::FStringDocstring) { flake8_bugbear::rules::f_string_docstring(checker, suite); } - if checker.enabled(Rule::UselessFormatterNOQA) { - ruff::rules::useless_formatter_noqa(checker, suite); + if checker.enabled(Rule::IgnoredFormatterNOQA) { + ruff::rules::ignored_formatter_noqa(checker, suite); } } diff --git a/crates/ruff_linter/src/codes.rs b/crates/ruff_linter/src/codes.rs index cbd645df2aa73..874a8051808d2 100644 --- a/crates/ruff_linter/src/codes.rs +++ b/crates/ruff_linter/src/codes.rs @@ -945,7 +945,7 @@ pub fn code_to_rule(linter: Linter, code: &str) -> Option<(RuleGroup, Rule)> { (Ruff, "025") => (RuleGroup::Preview, rules::ruff::rules::UnnecessaryDictComprehensionForIterable), (Ruff, "026") => (RuleGroup::Preview, rules::ruff::rules::DefaultFactoryKwarg), (Ruff, "027") => (RuleGroup::Preview, rules::ruff::rules::MissingFStringSyntax), - (Ruff, "028") => (RuleGroup::Preview, rules::ruff::rules::UselessFormatterNOQA), + (Ruff, "028") => (RuleGroup::Preview, rules::ruff::rules::IgnoredFormatterNOQA), (Ruff, "100") => (RuleGroup::Stable, rules::ruff::rules::UnusedNOQA), (Ruff, "200") => (RuleGroup::Stable, rules::ruff::rules::InvalidPyprojectToml), #[cfg(feature = "test-rules")] diff --git a/crates/ruff_linter/src/rules/ruff/mod.rs b/crates/ruff_linter/src/rules/ruff/mod.rs index adfec1b3cb99d..b295f3e97b8af 100644 --- a/crates/ruff_linter/src/rules/ruff/mod.rs +++ b/crates/ruff_linter/src/rules/ruff/mod.rs @@ -49,7 +49,7 @@ mod tests { #[test_case(Rule::MissingFStringSyntax, Path::new("RUF027_0.py"))] #[test_case(Rule::MissingFStringSyntax, Path::new("RUF027_1.py"))] #[test_case(Rule::MissingFStringSyntax, Path::new("RUF027_2.py"))] - #[test_case(Rule::UselessFormatterNOQA, Path::new("RUF028.py"))] + #[test_case(Rule::IgnoredFormatterNOQA, Path::new("RUF028.py"))] fn rules(rule_code: Rule, path: &Path) -> Result<()> { let snapshot = format!("{}_{}", rule_code.noqa_code(), path.to_string_lossy()); let diagnostics = test_path( diff --git a/crates/ruff_linter/src/rules/ruff/rules/useless_formatter_noqa.rs b/crates/ruff_linter/src/rules/ruff/rules/ignored_formatter_noqa.rs similarity index 81% rename from crates/ruff_linter/src/rules/ruff/rules/useless_formatter_noqa.rs rename to crates/ruff_linter/src/rules/ruff/rules/ignored_formatter_noqa.rs index 4673bb2f80c4d..e2b0cbe3e0330 100644 --- a/crates/ruff_linter/src/rules/ruff/rules/useless_formatter_noqa.rs +++ b/crates/ruff_linter/src/rules/ruff/rules/ignored_formatter_noqa.rs @@ -46,23 +46,23 @@ use super::suppression_comment_visitor::{ /// # yapf: enable /// ``` #[violation] -pub struct UselessFormatterNOQA { - reason: UselessReason, +pub struct IgnoredFormatterNOQA { + reason: IgnoredReason, } -impl AlwaysFixableViolation for UselessFormatterNOQA { +impl AlwaysFixableViolation for IgnoredFormatterNOQA { #[derive_message_formats] fn message(&self) -> String { - format!("Suppression comment is useless - {}", self.reason) + format!("This comment will be ignored by the formatter because {}", self.reason) } fn fix_title(&self) -> String { - format!("Remove this suppression comment") + format!("Remove this comment") } } /// RUF028 -pub(crate) fn useless_formatter_noqa(checker: &mut Checker, suite: &ast::Suite) { +pub(crate) fn ignored_formatter_noqa(checker: &mut Checker, suite: &ast::Suite) { let indexer = checker.indexer(); let locator = checker.locator(); let comment_ranges = indexer.comment_ranges(); @@ -73,9 +73,9 @@ pub(crate) fn useless_formatter_noqa(checker: &mut Checker, suite: &ast::Suite) visitor.visit(suite); - for (comment, reason) in comments.useless_comments() { + for (comment, reason) in comments.ignored_comments() { checker.diagnostics.push( - Diagnostic::new(UselessFormatterNOQA { reason }, comment.range).with_fix( + Diagnostic::new(IgnoredFormatterNOQA { reason }, comment.range).with_fix( Fix::unsafe_edit(delete_comment(comment.range, checker.locator())), ), ); @@ -83,7 +83,7 @@ pub(crate) fn useless_formatter_noqa(checker: &mut Checker, suite: &ast::Suite) } struct UselessSuppressionComments<'src, 'loc> { - captured: BTreeMap, UselessReason>, + captured: BTreeMap, IgnoredReason>, comments_in_scope: Vec<(Option>, SuppressionKind)>, locator: &'loc Locator<'src>, } @@ -97,32 +97,32 @@ impl<'src, 'loc> UselessSuppressionComments<'src, 'loc> { } } /// This function determines whether or not `comment` is a useful suppression comment. - /// If it isn't, it will give a reason why the comment is useless. See [`UselessReason`] for more. + /// If it isn't, it will give a reason why the comment is ignored. See [`IgnoredReason`] for more. fn check_suppression_comment( &self, comment: &SuppressionCommentData, - ) -> Result, UselessReason> { + ) -> Result, IgnoredReason> { // check if the comment is inside of an expression. if comment .enclosing .map(AnyNodeRef::is_expression) .unwrap_or_default() { - return Err(UselessReason::InsideExpression); + return Err(IgnoredReason::InsideExpression); } // check if a skip comment is at the end of a line if comment.kind == SuppressionKind::Skip && !comment.line_position.is_end_of_line() { - return Err(UselessReason::SkipHasToBeTrailing); + return Err(IgnoredReason::SkipHasToBeTrailing); } if comment.kind == SuppressionKind::Off && comment.line_position.is_own_line() { // check for a previous `fmt: off` if comment.previous_state == Some(SuppressionKind::Off) { - return Err(UselessReason::FmtOffUsedEarlier); + return Err(IgnoredReason::FmtOffUsedEarlier); } let Some(following) = comment.following else { - return Err(UselessReason::NoCodeSuppressed); + return Err(IgnoredReason::NoCodeSuppressed); }; if let Some(enclosing) = comment.enclosing { // check if this comment is dangling (in other words, in a block with nothing following it) @@ -139,7 +139,7 @@ impl<'src, 'loc> UselessSuppressionComments<'src, 'loc> { .unwrap_or_default() .text_len(); if comment_indentation <= preceding_indentation { - return Err(UselessReason::FmtOffOverElseBlock); + return Err(IgnoredReason::FmtOffAboveBlock); } } } @@ -149,13 +149,13 @@ impl<'src, 'loc> UselessSuppressionComments<'src, 'loc> { if comment.kind == SuppressionKind::On { // Ensure the comment is not a trailing comment if !comment.line_position.is_own_line() { - return Err(UselessReason::FmtOnCannotBeTrailing); + return Err(IgnoredReason::FmtOnCannotBeTrailing); } // If the comment turns on formatting, we need to check if another // comment turned formatting off within the same scope. match comment.previous_state { - None | Some(SuppressionKind::On) => return Err(UselessReason::NoFmtOff), + None | Some(SuppressionKind::On) => return Err(IgnoredReason::NoFmtOff), _ => {} } } @@ -165,7 +165,7 @@ impl<'src, 'loc> UselessSuppressionComments<'src, 'loc> { if comment.line_position.is_own_line() && comment.start() < class_def.name.start() { if let Some(decorator) = class_def.decorator_list.last() { if decorator.end() < comment.start() { - return Err(UselessReason::BetweenDecorators); + return Err(IgnoredReason::BetweenDecorators); } } } @@ -178,9 +178,9 @@ impl<'src, 'loc> UselessSuppressionComments<'src, 'loc> { Ok(None) } - fn useless_comments( + fn ignored_comments( &self, - ) -> impl Iterator, UselessReason)> { + ) -> impl Iterator, IgnoredReason)> { self.captured.iter().map(|(c, r)| (c, *r)) } } @@ -239,7 +239,7 @@ where } #[derive(Clone, Copy, Debug, PartialEq, Eq)] -enum UselessReason { +enum IgnoredReason { InsideExpression, FmtOffUsedEarlier, NoFmtOff, @@ -247,30 +247,30 @@ enum UselessReason { BetweenDecorators, FmtOnCannotBeTrailing, SkipHasToBeTrailing, - FmtOffOverElseBlock, + FmtOffAboveBlock, } -impl Display for UselessReason { +impl Display for IgnoredReason { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { match self { Self::InsideExpression => write!( f, - "suppression comments inside expressions are not supported" + "it's inside an expression" ), Self::FmtOffUsedEarlier => write!(f, "formatting is already disabled here"), Self::NoFmtOff => write!(f, "formatting is already enabled here"), - Self::NoCodeSuppressed => write!(f, "no eligible code is suppressed by this comment"), + Self::NoCodeSuppressed => write!(f, "it does not suppress formatting for any code"), Self::BetweenDecorators => { - write!(f, "suppression comment cannot be between decorators") + write!(f, "it cannot be between decorators") } Self::SkipHasToBeTrailing => { - write!(f, "'fmt: skip' has to be at the end of a line") + write!(f, "it has to be at the end of a line") } Self::FmtOnCannotBeTrailing => { - write!(f, "'fmt: on' cannot be at the end of a line") + write!(f, "it cannot be at the end of a line") } - Self::FmtOffOverElseBlock => { - write!(f, "'fmt: off' cannot be in front of an else/elif") + Self::FmtOffAboveBlock => { + write!(f, "it suppresses formatting for an ambiguous region") } } } diff --git a/crates/ruff_linter/src/rules/ruff/rules/mod.rs b/crates/ruff_linter/src/rules/ruff/rules/mod.rs index 00eaca20cd150..113e51fc2036c 100644 --- a/crates/ruff_linter/src/rules/ruff/rules/mod.rs +++ b/crates/ruff_linter/src/rules/ruff/rules/mod.rs @@ -25,7 +25,7 @@ pub(crate) use unnecessary_dict_comprehension_for_iterable::*; pub(crate) use unnecessary_iterable_allocation_for_first_element::*; pub(crate) use unnecessary_key_check::*; pub(crate) use unused_noqa::*; -pub(crate) use useless_formatter_noqa::*; +pub(crate) use ignored_formatter_noqa::*; mod ambiguous_unicode_character; mod assignment_in_assert; @@ -58,7 +58,7 @@ mod unnecessary_dict_comprehension_for_iterable; mod unnecessary_iterable_allocation_for_first_element; mod unnecessary_key_check; mod unused_noqa; -mod useless_formatter_noqa; +mod ignored_formatter_noqa; #[derive(Clone, Copy)] pub(crate) enum Context { diff --git a/crates/ruff_linter/src/rules/ruff/snapshots/ruff_linter__rules__ruff__tests__RUF028_RUF028.py.snap b/crates/ruff_linter/src/rules/ruff/snapshots/ruff_linter__rules__ruff__tests__RUF028_RUF028.py.snap index 7c655a4878c06..aba9d73ae0acd 100644 --- a/crates/ruff_linter/src/rules/ruff/snapshots/ruff_linter__rules__ruff__tests__RUF028_RUF028.py.snap +++ b/crates/ruff_linter/src/rules/ruff/snapshots/ruff_linter__rules__ruff__tests__RUF028_RUF028.py.snap @@ -1,14 +1,14 @@ --- source: crates/ruff_linter/src/rules/ruff/mod.rs --- -RUF028.py:1:1: RUF028 [*] Suppression comment is useless - formatting is already enabled here +RUF028.py:1:1: RUF028 [*] This comment will be ignored by the formatter because formatting is already enabled here | 1 | # fmt: on | ^^^^^^^^^ RUF028 2 | def fmt_off_used_earlier(): 3 | if True: | - = help: Remove this suppression comment + = help: Remove this comment ℹ Unsafe fix 1 |-# fmt: on @@ -16,7 +16,7 @@ RUF028.py:1:1: RUF028 [*] Suppression comment is useless - formatting is already 3 2 | if True: 4 3 | a = 5 -RUF028.py:15:9: RUF028 [*] Suppression comment is useless - formatting is already disabled here +RUF028.py:15:9: RUF028 [*] This comment will be ignored by the formatter because formatting is already disabled here | 13 | # fmt: off 14 | if True: @@ -25,7 +25,7 @@ RUF028.py:15:9: RUF028 [*] Suppression comment is useless - formatting is alread 16 | pass 17 | # fmt: off | - = help: Remove this suppression comment + = help: Remove this comment ℹ Unsafe fix 12 12 | pass @@ -36,7 +36,7 @@ RUF028.py:15:9: RUF028 [*] Suppression comment is useless - formatting is alread 17 16 | # fmt: off 18 17 | -RUF028.py:17:5: RUF028 [*] Suppression comment is useless - no eligible code is suppressed by this comment +RUF028.py:17:5: RUF028 [*] This comment will be ignored by the formatter because it does not suppress formatting for any code | 15 | # fmt: off 16 | pass @@ -45,7 +45,7 @@ RUF028.py:17:5: RUF028 [*] Suppression comment is useless - no eligible code is 18 | 19 | # fmt: off | - = help: Remove this suppression comment + = help: Remove this comment ℹ Unsafe fix 14 14 | if True: @@ -56,7 +56,7 @@ RUF028.py:17:5: RUF028 [*] Suppression comment is useless - no eligible code is 19 18 | # fmt: off 20 19 | -RUF028.py:26:9: RUF028 [*] Suppression comment is useless - suppression comments inside expressions are not supported +RUF028.py:26:9: RUF028 [*] This comment will be ignored by the formatter because it's inside an expression | 24 | def fmt_off_between_lists(): 25 | test_list = [ @@ -65,7 +65,7 @@ RUF028.py:26:9: RUF028 [*] Suppression comment is useless - suppression comments 27 | 1, 28 | 2, | - = help: Remove this suppression comment + = help: Remove this comment ℹ Unsafe fix 23 23 | @@ -76,7 +76,7 @@ RUF028.py:26:9: RUF028 [*] Suppression comment is useless - suppression comments 28 27 | 2, 29 28 | 3, -RUF028.py:37:5: RUF028 [*] Suppression comment is useless - 'fmt: skip' has to be at the end of a line +RUF028.py:37:5: RUF028 [*] This comment will be ignored by the formatter because it has to be at the end of a line | 35 | @fmt_off_between_lists 36 | def fmt_off_between_decorators(): @@ -84,7 +84,7 @@ RUF028.py:37:5: RUF028 [*] Suppression comment is useless - 'fmt: skip' has to b | ^^^^^^^^^^^ RUF028 38 | pass | - = help: Remove this suppression comment + = help: Remove this comment ℹ Unsafe fix 34 34 | # fmt: off @@ -95,7 +95,7 @@ RUF028.py:37:5: RUF028 [*] Suppression comment is useless - 'fmt: skip' has to b 39 38 | 40 39 | def fmt_on_trailing(): -RUF028.py:41:5: RUF028 [*] Suppression comment is useless - no eligible code is suppressed by this comment +RUF028.py:41:5: RUF028 [*] This comment will be ignored by the formatter because it does not suppress formatting for any code | 40 | def fmt_on_trailing(): 41 | # fmt: off @@ -103,7 +103,7 @@ RUF028.py:41:5: RUF028 [*] Suppression comment is useless - no eligible code is 42 | val = 5 # fmt: on 43 | pass | - = help: Remove this suppression comment + = help: Remove this comment ℹ Unsafe fix 38 38 | pass @@ -114,7 +114,7 @@ RUF028.py:41:5: RUF028 [*] Suppression comment is useless - no eligible code is 43 42 | pass 44 43 | -RUF028.py:42:13: RUF028 [*] Suppression comment is useless - 'fmt: on' cannot be at the end of a line +RUF028.py:42:13: RUF028 [*] This comment will be ignored by the formatter because it cannot be at the end of a line | 40 | def fmt_on_trailing(): 41 | # fmt: off @@ -122,7 +122,7 @@ RUF028.py:42:13: RUF028 [*] Suppression comment is useless - 'fmt: on' cannot be | ^^^^^^^^^ RUF028 43 | pass | - = help: Remove this suppression comment + = help: Remove this comment ℹ Unsafe fix 39 39 | @@ -134,7 +134,7 @@ RUF028.py:42:13: RUF028 [*] Suppression comment is useless - 'fmt: on' cannot be 44 44 | 45 45 | def fmt_off_in_else(): -RUF028.py:49:5: RUF028 [*] Suppression comment is useless - 'fmt: off' cannot be in front of an else/elif +RUF028.py:49:5: RUF028 [*] This comment will be ignored by the formatter because it suppresses formatting for an ambiguous region | 47 | for val in x: 48 | print(x) @@ -143,7 +143,7 @@ RUF028.py:49:5: RUF028 [*] Suppression comment is useless - 'fmt: off' cannot be 50 | else: 51 | print("done") | - = help: Remove this suppression comment + = help: Remove this comment ℹ Unsafe fix 46 46 | x = [1, 2, 3] @@ -154,7 +154,7 @@ RUF028.py:49:5: RUF028 [*] Suppression comment is useless - 'fmt: off' cannot be 51 50 | print("done") 52 51 | while False: -RUF028.py:54:5: RUF028 [*] Suppression comment is useless - formatting is already enabled here +RUF028.py:54:5: RUF028 [*] This comment will be ignored by the formatter because formatting is already enabled here | 52 | while False: 53 | print("while") @@ -163,7 +163,7 @@ RUF028.py:54:5: RUF028 [*] Suppression comment is useless - formatting is alread 55 | # fmt: off 56 | else: | - = help: Remove this suppression comment + = help: Remove this comment ℹ Unsafe fix 51 51 | print("done") @@ -174,7 +174,7 @@ RUF028.py:54:5: RUF028 [*] Suppression comment is useless - formatting is alread 56 55 | else: 57 56 | print("done") -RUF028.py:55:5: RUF028 [*] Suppression comment is useless - 'fmt: off' cannot be in front of an else/elif +RUF028.py:55:5: RUF028 [*] This comment will be ignored by the formatter because it suppresses formatting for an ambiguous region | 53 | print("while") 54 | # fmt: on @@ -183,7 +183,7 @@ RUF028.py:55:5: RUF028 [*] Suppression comment is useless - 'fmt: off' cannot be 56 | else: 57 | print("done") | - = help: Remove this suppression comment + = help: Remove this comment ℹ Unsafe fix 52 52 | while False: @@ -194,7 +194,7 @@ RUF028.py:55:5: RUF028 [*] Suppression comment is useless - 'fmt: off' cannot be 57 56 | print("done") 58 57 | if len(x) > 3: -RUF028.py:60:5: RUF028 [*] Suppression comment is useless - formatting is already enabled here +RUF028.py:60:5: RUF028 [*] This comment will be ignored by the formatter because formatting is already enabled here | 58 | if len(x) > 3: 59 | print("huh?") @@ -203,7 +203,7 @@ RUF028.py:60:5: RUF028 [*] Suppression comment is useless - formatting is alread 61 | # fmt: off 62 | else: | - = help: Remove this suppression comment + = help: Remove this comment ℹ Unsafe fix 57 57 | print("done") @@ -214,7 +214,7 @@ RUF028.py:60:5: RUF028 [*] Suppression comment is useless - formatting is alread 62 61 | else: 63 62 | print("expected") -RUF028.py:61:5: RUF028 [*] Suppression comment is useless - 'fmt: off' cannot be in front of an else/elif +RUF028.py:61:5: RUF028 [*] This comment will be ignored by the formatter because it suppresses formatting for an ambiguous region | 59 | print("huh?") 60 | # fmt: on @@ -223,7 +223,7 @@ RUF028.py:61:5: RUF028 [*] Suppression comment is useless - 'fmt: off' cannot be 62 | else: 63 | print("expected") | - = help: Remove this suppression comment + = help: Remove this comment ℹ Unsafe fix 58 58 | if len(x) > 3: @@ -234,7 +234,7 @@ RUF028.py:61:5: RUF028 [*] Suppression comment is useless - 'fmt: off' cannot be 63 62 | print("expected") 64 63 | -RUF028.py:67:5: RUF028 [*] Suppression comment is useless - no eligible code is suppressed by this comment +RUF028.py:67:5: RUF028 [*] This comment will be ignored by the formatter because it does not suppress formatting for any code | 65 | def dangling_fmt_off(): 66 | pass @@ -243,7 +243,7 @@ RUF028.py:67:5: RUF028 [*] Suppression comment is useless - no eligible code is 68 | 69 | def dangling_fmt_off2(): | - = help: Remove this suppression comment + = help: Remove this comment ℹ Unsafe fix 64 64 | @@ -254,7 +254,7 @@ RUF028.py:67:5: RUF028 [*] Suppression comment is useless - no eligible code is 69 68 | def dangling_fmt_off2(): 70 69 | if True: -RUF028.py:73:13: RUF028 [*] Suppression comment is useless - no eligible code is suppressed by this comment +RUF028.py:73:13: RUF028 [*] This comment will be ignored by the formatter because it does not suppress formatting for any code | 71 | if True: 72 | pass @@ -263,7 +263,7 @@ RUF028.py:73:13: RUF028 [*] Suppression comment is useless - no eligible code is 74 | else: 75 | pass | - = help: Remove this suppression comment + = help: Remove this comment ℹ Unsafe fix 70 70 | if True: @@ -274,7 +274,7 @@ RUF028.py:73:13: RUF028 [*] Suppression comment is useless - no eligible code is 75 74 | pass 76 75 | # fmt: off -RUF028.py:76:13: RUF028 [*] Suppression comment is useless - no eligible code is suppressed by this comment +RUF028.py:76:13: RUF028 [*] This comment will be ignored by the formatter because it does not suppress formatting for any code | 74 | else: 75 | pass @@ -283,7 +283,7 @@ RUF028.py:76:13: RUF028 [*] Suppression comment is useless - no eligible code is 77 | # fmt: off 78 | else: | - = help: Remove this suppression comment + = help: Remove this comment ℹ Unsafe fix 73 73 | # fmt: off @@ -294,7 +294,7 @@ RUF028.py:76:13: RUF028 [*] Suppression comment is useless - no eligible code is 78 77 | else: 79 78 | pass -RUF028.py:77:9: RUF028 [*] Suppression comment is useless - no eligible code is suppressed by this comment +RUF028.py:77:9: RUF028 [*] This comment will be ignored by the formatter because it does not suppress formatting for any code | 75 | pass 76 | # fmt: off @@ -303,7 +303,7 @@ RUF028.py:77:9: RUF028 [*] Suppression comment is useless - no eligible code is 78 | else: 79 | pass | - = help: Remove this suppression comment + = help: Remove this comment ℹ Unsafe fix 74 74 | else: @@ -314,14 +314,14 @@ RUF028.py:77:9: RUF028 [*] Suppression comment is useless - no eligible code is 79 78 | pass 80 79 | # fmt: off -RUF028.py:80:5: RUF028 [*] Suppression comment is useless - no eligible code is suppressed by this comment +RUF028.py:80:5: RUF028 [*] This comment will be ignored by the formatter because it does not suppress formatting for any code | 78 | else: 79 | pass 80 | # fmt: off | ^^^^^^^^^^ RUF028 | - = help: Remove this suppression comment + = help: Remove this comment ℹ Unsafe fix 77 77 | # fmt: off From 03db7507237180be0ac63084fbe9676083a809f5 Mon Sep 17 00:00:00 2001 From: Jane Lewis Date: Wed, 14 Feb 2024 15:37:55 -0800 Subject: [PATCH 08/13] Fix formatting --- .../src/rules/ruff/rules/ignored_formatter_noqa.rs | 13 +++++++------ crates/ruff_linter/src/rules/ruff/rules/mod.rs | 4 ++-- 2 files changed, 9 insertions(+), 8 deletions(-) diff --git a/crates/ruff_linter/src/rules/ruff/rules/ignored_formatter_noqa.rs b/crates/ruff_linter/src/rules/ruff/rules/ignored_formatter_noqa.rs index e2b0cbe3e0330..9ddf47ccfe243 100644 --- a/crates/ruff_linter/src/rules/ruff/rules/ignored_formatter_noqa.rs +++ b/crates/ruff_linter/src/rules/ruff/rules/ignored_formatter_noqa.rs @@ -31,6 +31,7 @@ use super::suppression_comment_visitor::{ /// def decorator(): /// pass /// +/// /// @decorator /// # fmt: off /// def example(): @@ -39,7 +40,7 @@ use super::suppression_comment_visitor::{ /// expression = [ /// # fmt: off /// 1, -/// 2 +/// 2, /// ] /// # yapf: disable /// # fmt: on @@ -53,7 +54,10 @@ pub struct IgnoredFormatterNOQA { impl AlwaysFixableViolation for IgnoredFormatterNOQA { #[derive_message_formats] fn message(&self) -> String { - format!("This comment will be ignored by the formatter because {}", self.reason) + format!( + "This comment will be ignored by the formatter because {}", + self.reason + ) } fn fix_title(&self) -> String { @@ -253,10 +257,7 @@ enum IgnoredReason { impl Display for IgnoredReason { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { match self { - Self::InsideExpression => write!( - f, - "it's inside an expression" - ), + Self::InsideExpression => write!(f, "it's inside an expression"), Self::FmtOffUsedEarlier => write!(f, "formatting is already disabled here"), Self::NoFmtOff => write!(f, "formatting is already enabled here"), Self::NoCodeSuppressed => write!(f, "it does not suppress formatting for any code"), diff --git a/crates/ruff_linter/src/rules/ruff/rules/mod.rs b/crates/ruff_linter/src/rules/ruff/rules/mod.rs index 113e51fc2036c..1896e6082a4c4 100644 --- a/crates/ruff_linter/src/rules/ruff/rules/mod.rs +++ b/crates/ruff_linter/src/rules/ruff/rules/mod.rs @@ -5,6 +5,7 @@ pub(crate) use collection_literal_concatenation::*; pub(crate) use default_factory_kwarg::*; pub(crate) use explicit_f_string_type_conversion::*; pub(crate) use function_call_in_dataclass_default::*; +pub(crate) use ignored_formatter_noqa::*; pub(crate) use implicit_optional::*; pub(crate) use invalid_index_type::*; pub(crate) use invalid_pyproject_toml::*; @@ -25,7 +26,6 @@ pub(crate) use unnecessary_dict_comprehension_for_iterable::*; pub(crate) use unnecessary_iterable_allocation_for_first_element::*; pub(crate) use unnecessary_key_check::*; pub(crate) use unused_noqa::*; -pub(crate) use ignored_formatter_noqa::*; mod ambiguous_unicode_character; mod assignment_in_assert; @@ -36,6 +36,7 @@ mod default_factory_kwarg; mod explicit_f_string_type_conversion; mod function_call_in_dataclass_default; mod helpers; +mod ignored_formatter_noqa; mod implicit_optional; mod invalid_index_type; mod invalid_pyproject_toml; @@ -58,7 +59,6 @@ mod unnecessary_dict_comprehension_for_iterable; mod unnecessary_iterable_allocation_for_first_element; mod unnecessary_key_check; mod unused_noqa; -mod ignored_formatter_noqa; #[derive(Clone, Copy)] pub(crate) enum Context { From 0271500275f14183891df23a887fea4025d17f0b Mon Sep 17 00:00:00 2001 From: Jane Lewis Date: Fri, 16 Feb 2024 22:03:51 -0800 Subject: [PATCH 09/13] Address some suggestions --- crates/ruff_linter/src/checkers/noqa.rs | 50 ++----------------- crates/ruff_linter/src/fix/edits.rs | 45 +++++++++++++++++ .../ruff/rules/ignored_formatter_noqa.rs | 15 ++---- .../ruff/rules/suppression_comment_visitor.rs | 46 +++++++++-------- .../ruff_python_formatter/src/comments/mod.rs | 22 +++++--- .../src/comments/visitor.rs | 4 +- crates/ruff_python_formatter/src/lib.rs | 15 ++---- .../src/{suppression.rs => comments.rs} | 20 +++++--- crates/ruff_python_trivia/src/lib.rs | 4 +- 9 files changed, 111 insertions(+), 110 deletions(-) rename crates/ruff_python_trivia/src/{suppression.rs => comments.rs} (83%) diff --git a/crates/ruff_linter/src/checkers/noqa.rs b/crates/ruff_linter/src/checkers/noqa.rs index 00fbc9885a170..8e01d671ce601 100644 --- a/crates/ruff_linter/src/checkers/noqa.rs +++ b/crates/ruff_linter/src/checkers/noqa.rs @@ -3,12 +3,13 @@ use std::path::Path; use itertools::Itertools; -use ruff_text_size::{Ranged, TextLen, TextRange}; +use ruff_text_size::Ranged; use ruff_diagnostics::{Diagnostic, Edit, Fix}; -use ruff_python_trivia::{CommentRanges, PythonWhitespace}; +use ruff_python_trivia::CommentRanges; use ruff_source_file::Locator; +use crate::fix::edits::delete_comment; use crate::noqa; use crate::noqa::{Directive, FileExemption, NoqaDirectives, NoqaMapping}; use crate::registry::{AsRule, Rule, RuleSet}; @@ -198,48 +199,3 @@ pub(crate) fn check_noqa( ignored_diagnostics.sort_unstable(); ignored_diagnostics } - -/// Generate a [`Edit`] to delete a comment (for example: a `noqa` directive). -pub(crate) fn delete_comment(range: TextRange, locator: &Locator) -> Edit { - let line_range = locator.line_range(range.start()); - - // Compute the leading space. - let prefix = locator.slice(TextRange::new(line_range.start(), range.start())); - let leading_space_len = prefix.text_len() - prefix.trim_whitespace_end().text_len(); - - // Compute the trailing space. - let suffix = locator.slice(TextRange::new(range.end(), line_range.end())); - let trailing_space_len = suffix.text_len() - suffix.trim_whitespace_start().text_len(); - - // Ex) `# noqa` - if line_range - == TextRange::new( - range.start() - leading_space_len, - range.end() + trailing_space_len, - ) - { - let full_line_end = locator.full_line_end(line_range.end()); - Edit::deletion(line_range.start(), full_line_end) - } - // Ex) `x = 1 # noqa` - else if range.end() + trailing_space_len == line_range.end() { - Edit::deletion(range.start() - leading_space_len, line_range.end()) - } - // Ex) `x = 1 # noqa # type: ignore` - else if locator - .slice(TextRange::new( - range.end() + trailing_space_len, - line_range.end(), - )) - .starts_with('#') - { - Edit::deletion(range.start(), range.end() + trailing_space_len) - } - // Ex) `x = 1 # noqa here` - else { - Edit::deletion( - range.start() + "# ".text_len(), - range.end() + trailing_space_len, - ) - } -} diff --git a/crates/ruff_linter/src/fix/edits.rs b/crates/ruff_linter/src/fix/edits.rs index 69d698fae6219..7adf6b2bfdbc6 100644 --- a/crates/ruff_linter/src/fix/edits.rs +++ b/crates/ruff_linter/src/fix/edits.rs @@ -62,6 +62,51 @@ pub(crate) fn delete_stmt( } } +/// Generate a [`Edit`] to delete a comment (for example: a `noqa` directive). +pub(crate) fn delete_comment(range: TextRange, locator: &Locator) -> Edit { + let line_range = locator.line_range(range.start()); + + // Compute the leading space. + let prefix = locator.slice(TextRange::new(line_range.start(), range.start())); + let leading_space_len = prefix.text_len() - prefix.trim_whitespace_end().text_len(); + + // Compute the trailing space. + let suffix = locator.slice(TextRange::new(range.end(), line_range.end())); + let trailing_space_len = suffix.text_len() - suffix.trim_whitespace_start().text_len(); + + // Ex) `# noqa` + if line_range + == TextRange::new( + range.start() - leading_space_len, + range.end() + trailing_space_len, + ) + { + let full_line_end = locator.full_line_end(line_range.end()); + Edit::deletion(line_range.start(), full_line_end) + } + // Ex) `x = 1 # noqa` + else if range.end() + trailing_space_len == line_range.end() { + Edit::deletion(range.start() - leading_space_len, line_range.end()) + } + // Ex) `x = 1 # noqa # type: ignore` + else if locator + .slice(TextRange::new( + range.end() + trailing_space_len, + line_range.end(), + )) + .starts_with('#') + { + Edit::deletion(range.start(), range.end() + trailing_space_len) + } + // Ex) `x = 1 # noqa here` + else { + Edit::deletion( + range.start() + "# ".text_len(), + range.end() + trailing_space_len, + ) + } +} + /// Generate a `Fix` to remove the specified imports from an `import` statement. pub(crate) fn remove_unused_imports<'a>( member_names: impl Iterator, diff --git a/crates/ruff_linter/src/rules/ruff/rules/ignored_formatter_noqa.rs b/crates/ruff_linter/src/rules/ruff/rules/ignored_formatter_noqa.rs index 9ddf47ccfe243..486994454c29b 100644 --- a/crates/ruff_linter/src/rules/ruff/rules/ignored_formatter_noqa.rs +++ b/crates/ruff_linter/src/rules/ruff/rules/ignored_formatter_noqa.rs @@ -1,4 +1,3 @@ -#![allow(dead_code)] use std::{collections::BTreeMap, fmt::Display}; use ruff_diagnostics::{AlwaysFixableViolation, Diagnostic, Fix}; @@ -8,7 +7,8 @@ use ruff_python_trivia::{indentation_at_offset, SuppressionKind}; use ruff_source_file::Locator; use ruff_text_size::{Ranged, TextLen}; -use crate::checkers::{ast::Checker, noqa::delete_comment}; +use crate::checkers::ast::Checker; +use crate::fix::edits::delete_comment; use super::suppression_comment_visitor::{ own_line_comment_indentation, CaptureSuppressionComment, SuppressionCommentData, @@ -55,7 +55,7 @@ impl AlwaysFixableViolation for IgnoredFormatterNOQA { #[derive_message_formats] fn message(&self) -> String { format!( - "This comment will be ignored by the formatter because {}", + "This suppression comment will be ignored by the formatter because {}", self.reason ) } @@ -88,7 +88,6 @@ pub(crate) fn ignored_formatter_noqa(checker: &mut Checker, suite: &ast::Suite) struct UselessSuppressionComments<'src, 'loc> { captured: BTreeMap, IgnoredReason>, - comments_in_scope: Vec<(Option>, SuppressionKind)>, locator: &'loc Locator<'src>, } @@ -96,7 +95,6 @@ impl<'src, 'loc> UselessSuppressionComments<'src, 'loc> { fn new(locator: &'loc Locator<'src>) -> Self { Self { captured: BTreeMap::default(), - comments_in_scope: vec![], locator, } } @@ -227,13 +225,6 @@ fn is_first_statement_in_alternate_body(statement: AnyNodeRef, has_body: AnyNode } } -/// Returns `true` if the parameters are parenthesized (as in a function definition), or `false` if -/// not (as in a lambda). -fn are_parameters_parenthesized(parameters: &ast::Parameters, contents: &str) -> bool { - // A lambda never has parentheses around its parameters, but a function definition always does. - contents[parameters.range()].starts_with('(') -} - /// Returns `true` if `right` is `Some` and `left` and `right` are referentially equal. fn are_same_optional<'a, T>(left: AnyNodeRef, right: Option) -> bool where diff --git a/crates/ruff_linter/src/rules/ruff/rules/suppression_comment_visitor.rs b/crates/ruff_linter/src/rules/ruff/rules/suppression_comment_visitor.rs index a2ed26cf6cffe..294c4e034ea12 100644 --- a/crates/ruff_linter/src/rules/ruff/rules/suppression_comment_visitor.rs +++ b/crates/ruff_linter/src/rules/ruff/rules/suppression_comment_visitor.rs @@ -1,4 +1,4 @@ -use std::iter::{FilterMap, Peekable}; +use std::iter::Peekable; use ruff_python_ast::{ visitor::preorder::{self, PreorderVisitor, TraversalSignal}, @@ -17,17 +17,29 @@ struct SuppressionComment { kind: SuppressionKind, } -type MapCommentFn<'src> = Box Option + 'src>; -type CommentIter<'src> = Peekable, MapCommentFn<'src>>>; +fn suppression_comments<'src>( + ranges: &'src CommentRanges, + locator: &'src Locator<'src>, +) -> Box + 'src> { + Box::new(ranges.iter().filter_map(|range| { + Some(SuppressionComment { + range: *range, + kind: SuppressionKind::from_comment(locator.slice(range))?, + }) + })) +} /// Visitor that captures AST data for suppression comments. This uses a similar approach /// to `CommentsVisitor` in the formatter crate. pub(super) struct SuppressionCommentVisitor<'src, 'builder> { - comments: CommentIter<'src>, + comments: Peekable + 'src>>, parents: Vec>, preceding_node: Option>, - comments_in_scope: Vec<(Option>, SuppressionKind)>, + // A stack of comment states in scope at the current visited node. + // The last comment state in the list is the top of the stack, + // and is essentially the 'current' state. + comments_in_scope: Vec<(AnyNodeRef<'src>, SuppressionKind)>, builder: &'builder mut (dyn CaptureSuppressionComment<'src> + 'src), locator: &'src Locator<'src>, @@ -39,17 +51,11 @@ impl<'src, 'builder> SuppressionCommentVisitor<'src, 'builder> { builder: &'builder mut (dyn CaptureSuppressionComment<'src> + 'src), locator: &'src Locator<'src>, ) -> Self { - let map_fn: MapCommentFn<'_> = Box::new(|range: &'src TextRange| { - Some(SuppressionComment { - range: *range, - kind: SuppressionKind::from_slice(locator.slice(range))?, - }) - }); Self { - comments: comment_ranges.iter().filter_map(map_fn).peekable(), + comments: suppression_comments(comment_ranges, locator).peekable(), parents: Vec::default(), preceding_node: Option::default(), - comments_in_scope: Vec::with_capacity(comment_ranges.len()), + comments_in_scope: Vec::default(), builder, locator, } @@ -81,7 +87,7 @@ impl<'ast> PreorderVisitor<'ast> for SuppressionCommentVisitor<'ast, '_> { break; } - let line_position = CommentLinePosition::text_position(range, self.locator.contents()); + let line_position = CommentLinePosition::for_range(range, self.locator.contents()); let previous_state = self.comments_in_scope.last().map(|(_, s)| s).copied(); @@ -89,7 +95,6 @@ impl<'ast> PreorderVisitor<'ast> for SuppressionCommentVisitor<'ast, '_> { enclosing: enclosing_node, preceding: self.preceding_node, following: Some(node), - parent: self.parents.iter().rev().nth(1).copied(), previous_state, line_position, kind, @@ -97,7 +102,7 @@ impl<'ast> PreorderVisitor<'ast> for SuppressionCommentVisitor<'ast, '_> { }; if let Some(kind) = self.builder.capture(data) { - self.comments_in_scope.push((Some(node), kind)); + self.comments_in_scope.push((node, kind)); } self.comments.next(); } @@ -120,7 +125,7 @@ impl<'ast> PreorderVisitor<'ast> for SuppressionCommentVisitor<'ast, '_> { // Process all comments that start after the `preceding` node and end before this node's end. while let Some(SuppressionComment { range, kind }) = self.comments.peek().copied() { - let line_position = CommentLinePosition::text_position(range, self.locator.contents()); + let line_position = CommentLinePosition::for_range(range, self.locator.contents()); if range.start() >= node_end { if !line_position.is_own_line() { break; @@ -144,7 +149,6 @@ impl<'ast> PreorderVisitor<'ast> for SuppressionCommentVisitor<'ast, '_> { enclosing: Some(node), preceding: self.preceding_node, following: None, - parent: self.parents.last().copied(), previous_state, line_position, kind, @@ -152,14 +156,14 @@ impl<'ast> PreorderVisitor<'ast> for SuppressionCommentVisitor<'ast, '_> { }; if let Some(kind) = self.builder.capture(data) { - self.comments_in_scope.push((Some(node), kind)); + self.comments_in_scope.push((node, kind)); } self.comments.next(); } // remove comments that are about to become out of scope for index in (0..self.comments_in_scope.len()).rev() { - if self.comments_in_scope[index].0 == Some(node) { + if AnyNodeRef::ptr_eq(self.comments_in_scope[index].0, node) { self.comments_in_scope.pop(); } else { break; @@ -192,13 +196,11 @@ impl<'ast> PreorderVisitor<'ast> for SuppressionCommentVisitor<'ast, '_> { } #[derive(Clone, Debug)] -#[allow(dead_code)] pub(super) struct SuppressionCommentData<'src> { /// If `enclosing` is `None`, this comment is top-level pub(super) enclosing: Option>, pub(super) preceding: Option>, pub(super) following: Option>, - pub(super) parent: Option>, pub(super) previous_state: Option, pub(super) line_position: CommentLinePosition, pub(super) kind: SuppressionKind, diff --git a/crates/ruff_python_formatter/src/comments/mod.rs b/crates/ruff_python_formatter/src/comments/mod.rs index 97939fb9d7706..27127f6ae5a3e 100644 --- a/crates/ruff_python_formatter/src/comments/mod.rs +++ b/crates/ruff_python_formatter/src/comments/mod.rs @@ -168,16 +168,16 @@ impl SourceComment { DebugComment::new(self, source_code) } - pub(crate) fn is_suppression_off_comment(&self, source: &str) -> bool { - SuppressionKind::is_suppression_off(self.slice_source(source), self.line_position) + pub(crate) fn is_suppression_off_comment(&self, text: &str) -> bool { + SuppressionKind::is_suppression_off(self.text(text), self.line_position) } - pub(crate) fn is_suppression_on_comment(&self, source: &str) -> bool { - SuppressionKind::is_suppression_on(self.slice_source(source), self.line_position) + pub(crate) fn is_suppression_on_comment(&self, text: &str) -> bool { + SuppressionKind::is_suppression_on(self.text(text), self.line_position) } - fn slice_source<'a>(&self, source: &'a str) -> &'a str { - self.slice.text(SourceCode::new(source)) + fn text<'a>(&self, text: &'a str) -> &'a str { + self.slice.text(SourceCode::new(text)) } } @@ -464,6 +464,16 @@ impl<'a> PreorderVisitor<'a> for MarkVerbatimCommentsAsFormattedVisitor<'a> { } } +pub(crate) fn has_skip_comment(trailing_comments: &[SourceComment], source: &str) -> bool { + trailing_comments.iter().any(|comment| { + comment.line_position().is_end_of_line() + && matches!( + SuppressionKind::from_comment(comment.slice().text(SourceCode::new(source))), + Some(SuppressionKind::Skip | SuppressionKind::Off) + ) + }) +} + #[cfg(test)] mod tests { use insta::assert_debug_snapshot; diff --git a/crates/ruff_python_formatter/src/comments/visitor.rs b/crates/ruff_python_formatter/src/comments/visitor.rs index 6f1b78090c358..ede7180107a3e 100644 --- a/crates/ruff_python_formatter/src/comments/visitor.rs +++ b/crates/ruff_python_formatter/src/comments/visitor.rs @@ -90,7 +90,7 @@ impl<'ast> PreorderVisitor<'ast> for CommentsVisitor<'ast, '_> { preceding: self.preceding_node, following: Some(node), parent: self.parents.iter().rev().nth(1).copied(), - line_position: CommentLinePosition::text_position( + line_position: CommentLinePosition::for_range( *comment_range, self.source_code.as_str(), ), @@ -133,7 +133,7 @@ impl<'ast> PreorderVisitor<'ast> for CommentsVisitor<'ast, '_> { parent: self.parents.last().copied(), preceding: self.preceding_node, following: None, - line_position: CommentLinePosition::text_position( + line_position: CommentLinePosition::for_range( *comment_range, self.source_code.as_str(), ), diff --git a/crates/ruff_python_formatter/src/lib.rs b/crates/ruff_python_formatter/src/lib.rs index 40a03b63ede0c..c4cf4a16c3d4c 100644 --- a/crates/ruff_python_formatter/src/lib.rs +++ b/crates/ruff_python_formatter/src/lib.rs @@ -8,11 +8,12 @@ use ruff_python_ast::AstNode; use ruff_python_ast::Mod; use ruff_python_index::tokens_and_ranges; use ruff_python_parser::{parse_tokens, AsMode, ParseError, ParseErrorType}; -use ruff_python_trivia::{CommentRanges, SuppressionKind}; +use ruff_python_trivia::CommentRanges; use ruff_source_file::Locator; use crate::comments::{ - dangling_comments, leading_comments, trailing_comments, Comments, SourceComment, + dangling_comments, has_skip_comment, leading_comments, trailing_comments, Comments, + SourceComment, }; pub use crate::context::PyFormatContext; pub use crate::options::{ @@ -116,16 +117,6 @@ where } } -fn has_skip_comment(trailing_comments: &[SourceComment], source: &str) -> bool { - trailing_comments.iter().any(|comment| { - comment.line_position().is_end_of_line() - && matches!( - SuppressionKind::from_slice(comment.slice().text(SourceCode::new(source))), - Some(SuppressionKind::Skip | SuppressionKind::Off) - ) - }) -} - #[derive(Error, Debug)] pub enum FormatModuleError { #[error(transparent)] diff --git a/crates/ruff_python_trivia/src/suppression.rs b/crates/ruff_python_trivia/src/comments.rs similarity index 83% rename from crates/ruff_python_trivia/src/suppression.rs rename to crates/ruff_python_trivia/src/comments.rs index be8a849f0f156..a0b35d330102a 100644 --- a/crates/ruff_python_trivia/src/suppression.rs +++ b/crates/ruff_python_trivia/src/comments.rs @@ -13,11 +13,15 @@ pub enum SuppressionKind { } impl SuppressionKind { - /// Given a `slice` - pub fn from_slice(slice: &str) -> Option { + /// Attempts to identify the `kind` of a `comment`. + /// The comment string should be the full line with the comment on it. + pub fn from_comment(comment: &str) -> Option { // Match against `# fmt: on`, `# fmt: off`, `# yapf: disable`, and `# yapf: enable`, which // must be on their own lines. - let trimmed = slice.strip_prefix('#').unwrap_or(slice).trim_whitespace(); + let trimmed = comment + .strip_prefix('#') + .unwrap_or(comment) + .trim_whitespace(); if let Some(command) = trimmed.strip_prefix("fmt:") { match command.trim_whitespace_start() { "off" => return Some(Self::Off), @@ -35,7 +39,7 @@ impl SuppressionKind { // Search for `# fmt: skip` comments, which can be interspersed with other comments (e.g., // `# fmt: skip # noqa: E501`). - for segment in slice.split('#') { + for segment in comment.split('#') { let trimmed = segment.trim_whitespace(); if let Some(command) = trimmed.strip_prefix("fmt:") { if command.trim_whitespace_start() == "skip" { @@ -49,12 +53,12 @@ impl SuppressionKind { /// Returns true if this comment is a `fmt: off` or `yapf: disable` own line suppression comment. pub fn is_suppression_on(slice: &str, position: CommentLinePosition) -> bool { - position.is_own_line() && matches!(Self::from_slice(slice), Some(Self::Off)) + position.is_own_line() && matches!(Self::from_comment(slice), Some(Self::Off)) } /// Returns true if this comment is a `fmt: on` or `yapf: enable` own line suppression comment. pub fn is_suppression_off(slice: &str, position: CommentLinePosition) -> bool { - position.is_own_line() && matches!(Self::from_slice(slice), Some(Self::On)) + position.is_own_line() && matches!(Self::from_comment(slice), Some(Self::On)) } } /// The position of a comment in the source text. @@ -98,7 +102,9 @@ impl CommentLinePosition { matches!(self, Self::EndOfLine) } - pub fn text_position(comment_range: TextRange, source_code: &str) -> Self { + /// Finds the line position of a comment given a range with + /// + pub fn for_range(comment_range: TextRange, source_code: &str) -> Self { let before = &source_code[TextRange::up_to(comment_range.start())]; for c in before.chars().rev() { diff --git a/crates/ruff_python_trivia/src/lib.rs b/crates/ruff_python_trivia/src/lib.rs index 3aa7039c77397..782662170b6fa 100644 --- a/crates/ruff_python_trivia/src/lib.rs +++ b/crates/ruff_python_trivia/src/lib.rs @@ -1,14 +1,14 @@ mod comment_ranges; +mod comments; mod cursor; mod pragmas; -mod suppression; pub mod textwrap; mod tokenizer; mod whitespace; pub use comment_ranges::CommentRanges; +pub use comments::*; pub use cursor::*; pub use pragmas::*; -pub use suppression::*; pub use tokenizer::*; pub use whitespace::*; From dad765d345ae27b5aded8c94b2a76ad93b36e3cc Mon Sep 17 00:00:00 2001 From: Jane Lewis Date: Fri, 23 Feb 2024 11:06:00 -0800 Subject: [PATCH 10/13] Address remaining suggestions and reduce scope of lint by removing rule checks that need contextual information --- .../resources/test/fixtures/ruff/RUF028.py | 60 +-- .../src/checkers/ast/analyze/module.rs | 4 +- crates/ruff_linter/src/codes.rs | 2 +- crates/ruff_linter/src/rules/ruff/mod.rs | 2 +- ... invalid_formatter_suppression_comment.rs} | 169 ++++---- .../ruff_linter/src/rules/ruff/rules/mod.rs | 4 +- .../ruff/rules/suppression_comment_visitor.rs | 141 +++---- ..._rules__ruff__tests__RUF028_RUF028.py.snap | 382 +++++------------- .../ruff_python_formatter/src/comments/mod.rs | 2 +- crates/ruff_python_trivia/src/comments.rs | 4 +- 10 files changed, 274 insertions(+), 496 deletions(-) rename crates/ruff_linter/src/rules/ruff/rules/{ignored_formatter_noqa.rs => invalid_formatter_suppression_comment.rs} (58%) diff --git a/crates/ruff_linter/resources/test/fixtures/ruff/RUF028.py b/crates/ruff_linter/resources/test/fixtures/ruff/RUF028.py index cc9b09cfee842..1b6cb4297908d 100644 --- a/crates/ruff_linter/resources/test/fixtures/ruff/RUF028.py +++ b/crates/ruff_linter/resources/test/fixtures/ruff/RUF028.py @@ -1,26 +1,3 @@ -# fmt: on -def fmt_off_used_earlier(): - if True: - a = 5 - with a: - # fmt: off - pass - elif False: - # fmt: off - pass - else: - pass - # fmt: off - if True: - # fmt: off - pass - # fmt: off - -# fmt: off - -# fmt: on - - def fmt_off_between_lists(): test_list = [ # fmt: off @@ -29,18 +6,23 @@ def fmt_off_between_lists(): 3, ] +# note: the second `fmt: skip`` should be OK +def fmt_skip_on_own_line(): + # fmt: skip + pass # fmt: skip + -@fmt_on_after_func +@fmt_skip_on_own_line # fmt: off @fmt_off_between_lists def fmt_off_between_decorators(): - # fmt: skip pass -def fmt_on_trailing(): - # fmt: off - val = 5 # fmt: on - pass +@fmt_off_between_decorators +# fmt: off +@fmt_off_between_lists +class FmtOffBetweenClassDecorators: + ... def fmt_off_in_else(): x = [1, 2, 3] @@ -51,7 +33,7 @@ def fmt_off_in_else(): print("done") while False: print("while") - # fmt: on + # fmt: off # fmt: off else: print("done") @@ -62,19 +44,7 @@ def fmt_off_in_else(): else: print("expected") -def dangling_fmt_off(): - pass - # fmt: off - -def dangling_fmt_off2(): - if True: - if True: - pass - # fmt: off - else: - pass - # fmt: off - # fmt: off - else: - pass +def fmt_on_trailing(): # fmt: off + val = 5 # fmt: on + pass # fmt: on diff --git a/crates/ruff_linter/src/checkers/ast/analyze/module.rs b/crates/ruff_linter/src/checkers/ast/analyze/module.rs index 62316f280a1de..e80e052b55b3c 100644 --- a/crates/ruff_linter/src/checkers/ast/analyze/module.rs +++ b/crates/ruff_linter/src/checkers/ast/analyze/module.rs @@ -9,7 +9,7 @@ pub(crate) fn module(suite: &Suite, checker: &mut Checker) { if checker.enabled(Rule::FStringDocstring) { flake8_bugbear::rules::f_string_docstring(checker, suite); } - if checker.enabled(Rule::IgnoredFormatterNOQA) { - ruff::rules::ignored_formatter_noqa(checker, suite); + if checker.enabled(Rule::InvalidFormatterSuppressionComment) { + ruff::rules::ignored_formatter_suppression_comment(checker, suite); } } diff --git a/crates/ruff_linter/src/codes.rs b/crates/ruff_linter/src/codes.rs index 874a8051808d2..3ec7497eb3f89 100644 --- a/crates/ruff_linter/src/codes.rs +++ b/crates/ruff_linter/src/codes.rs @@ -945,7 +945,7 @@ pub fn code_to_rule(linter: Linter, code: &str) -> Option<(RuleGroup, Rule)> { (Ruff, "025") => (RuleGroup::Preview, rules::ruff::rules::UnnecessaryDictComprehensionForIterable), (Ruff, "026") => (RuleGroup::Preview, rules::ruff::rules::DefaultFactoryKwarg), (Ruff, "027") => (RuleGroup::Preview, rules::ruff::rules::MissingFStringSyntax), - (Ruff, "028") => (RuleGroup::Preview, rules::ruff::rules::IgnoredFormatterNOQA), + (Ruff, "028") => (RuleGroup::Preview, rules::ruff::rules::InvalidFormatterSuppressionComment), (Ruff, "100") => (RuleGroup::Stable, rules::ruff::rules::UnusedNOQA), (Ruff, "200") => (RuleGroup::Stable, rules::ruff::rules::InvalidPyprojectToml), #[cfg(feature = "test-rules")] diff --git a/crates/ruff_linter/src/rules/ruff/mod.rs b/crates/ruff_linter/src/rules/ruff/mod.rs index b295f3e97b8af..1b06577295d13 100644 --- a/crates/ruff_linter/src/rules/ruff/mod.rs +++ b/crates/ruff_linter/src/rules/ruff/mod.rs @@ -49,7 +49,7 @@ mod tests { #[test_case(Rule::MissingFStringSyntax, Path::new("RUF027_0.py"))] #[test_case(Rule::MissingFStringSyntax, Path::new("RUF027_1.py"))] #[test_case(Rule::MissingFStringSyntax, Path::new("RUF027_2.py"))] - #[test_case(Rule::IgnoredFormatterNOQA, Path::new("RUF028.py"))] + #[test_case(Rule::InvalidFormatterSuppressionComment, Path::new("RUF028.py"))] fn rules(rule_code: Rule, path: &Path) -> Result<()> { let snapshot = format!("{}_{}", rule_code.noqa_code(), path.to_string_lossy()); let diagnostics = test_path( diff --git a/crates/ruff_linter/src/rules/ruff/rules/ignored_formatter_noqa.rs b/crates/ruff_linter/src/rules/ruff/rules/invalid_formatter_suppression_comment.rs similarity index 58% rename from crates/ruff_linter/src/rules/ruff/rules/ignored_formatter_noqa.rs rename to crates/ruff_linter/src/rules/ruff/rules/invalid_formatter_suppression_comment.rs index 486994454c29b..592ee0505906d 100644 --- a/crates/ruff_linter/src/rules/ruff/rules/ignored_formatter_noqa.rs +++ b/crates/ruff_linter/src/rules/ruff/rules/invalid_formatter_suppression_comment.rs @@ -1,18 +1,20 @@ -use std::{collections::BTreeMap, fmt::Display}; +use std::fmt::Display; +use ast::{StmtClassDef, StmtFunctionDef}; use ruff_diagnostics::{AlwaysFixableViolation, Diagnostic, Fix}; use ruff_macros::{derive_message_formats, violation}; use ruff_python_ast::{self as ast, AnyNodeRef}; use ruff_python_trivia::{indentation_at_offset, SuppressionKind}; use ruff_source_file::Locator; -use ruff_text_size::{Ranged, TextLen}; +use ruff_text_size::{Ranged, TextLen, TextRange}; +use smallvec::SmallVec; use crate::checkers::ast::Checker; use crate::fix::edits::delete_comment; use super::suppression_comment_visitor::{ - own_line_comment_indentation, CaptureSuppressionComment, SuppressionCommentData, - SuppressionCommentVisitor, + own_line_comment_indentation, CaptureSuppressionComment, SuppressionComment, + SuppressionCommentData, SuppressionCommentVisitor, }; /// ## What it does @@ -47,15 +49,15 @@ use super::suppression_comment_visitor::{ /// # yapf: enable /// ``` #[violation] -pub struct IgnoredFormatterNOQA { +pub struct InvalidFormatterSuppressionComment { reason: IgnoredReason, } -impl AlwaysFixableViolation for IgnoredFormatterNOQA { +impl AlwaysFixableViolation for InvalidFormatterSuppressionComment { #[derive_message_formats] fn message(&self) -> String { format!( - "This suppression comment will be ignored by the formatter because {}", + "This suppression comment is invalid because {}", self.reason ) } @@ -66,35 +68,53 @@ impl AlwaysFixableViolation for IgnoredFormatterNOQA { } /// RUF028 -pub(crate) fn ignored_formatter_noqa(checker: &mut Checker, suite: &ast::Suite) { +pub(crate) fn ignored_formatter_suppression_comment(checker: &mut Checker, suite: &ast::Suite) { let indexer = checker.indexer(); let locator = checker.locator(); - let comment_ranges = indexer.comment_ranges(); + let comment_ranges: SmallVec<[SuppressionComment; 8]> = indexer + .comment_ranges() + .into_iter() + .filter_map(|range| { + Some(SuppressionComment { + range: *range, + kind: SuppressionKind::from_comment(locator.slice(range))?, + }) + }) + .collect(); + + if comment_ranges.is_empty() { + return; + } let mut comments = UselessSuppressionComments::new(locator); - let visitor = SuppressionCommentVisitor::new(comment_ranges, &mut comments, checker.locator()); + let visitor = SuppressionCommentVisitor::new( + comment_ranges.into_iter(), + &mut comments, + checker.locator(), + ); visitor.visit(suite); - for (comment, reason) in comments.ignored_comments() { + comments.sort(); + + for (range, reason) in comments.ignored_comments() { checker.diagnostics.push( - Diagnostic::new(IgnoredFormatterNOQA { reason }, comment.range).with_fix( - Fix::unsafe_edit(delete_comment(comment.range, checker.locator())), - ), + Diagnostic::new(InvalidFormatterSuppressionComment { reason }, range) + .with_fix(Fix::unsafe_edit(delete_comment(range, checker.locator()))), ); } } struct UselessSuppressionComments<'src, 'loc> { - captured: BTreeMap, IgnoredReason>, + captured: Vec<(TextRange, IgnoredReason)>, locator: &'loc Locator<'src>, } impl<'src, 'loc> UselessSuppressionComments<'src, 'loc> { fn new(locator: &'loc Locator<'src>) -> Self { Self { - captured: BTreeMap::default(), + captured: vec![], locator, } } @@ -103,7 +123,7 @@ impl<'src, 'loc> UselessSuppressionComments<'src, 'loc> { fn check_suppression_comment( &self, comment: &SuppressionCommentData, - ) -> Result, IgnoredReason> { + ) -> Result<(), IgnoredReason> { // check if the comment is inside of an expression. if comment .enclosing @@ -115,34 +135,50 @@ impl<'src, 'loc> UselessSuppressionComments<'src, 'loc> { // check if a skip comment is at the end of a line if comment.kind == SuppressionKind::Skip && !comment.line_position.is_end_of_line() { - return Err(IgnoredReason::SkipHasToBeTrailing); + if !comment.line_position.is_end_of_line() { + return Err(IgnoredReason::SkipHasToBeTrailing); + } } - if comment.kind == SuppressionKind::Off && comment.line_position.is_own_line() { - // check for a previous `fmt: off` - if comment.previous_state == Some(SuppressionKind::Off) { - return Err(IgnoredReason::FmtOffUsedEarlier); + if comment.kind == SuppressionKind::Off || comment.kind == SuppressionKind::On { + if let Some( + AnyNodeRef::StmtClassDef(StmtClassDef { + name, + decorator_list, + .. + }) + | AnyNodeRef::StmtFunctionDef(StmtFunctionDef { + name, + decorator_list, + .. + }), + ) = comment.enclosing + { + if comment.line_position.is_own_line() && comment.range.start() < name.start() { + if let Some(decorator) = decorator_list.last() { + if decorator.start() > comment.range.end() { + return Err(IgnoredReason::BetweenDecorators); + } + } + } } - let Some(following) = comment.following else { - return Err(IgnoredReason::NoCodeSuppressed); - }; - if let Some(enclosing) = comment.enclosing { - // check if this comment is dangling (in other words, in a block with nothing following it) + } - // check if this comment is before an alternative body (for example: an `else` or `elif`) - if let Some(preceding) = comment.preceding { - if is_first_statement_in_alternate_body(following, enclosing) { - // check indentation - let comment_indentation = - own_line_comment_indentation(preceding, comment.range, self.locator); + if comment.kind == SuppressionKind::Off && comment.line_position.is_own_line() { + if let (Some(enclosing), Some(preceding), Some(following)) = + (comment.enclosing, comment.preceding, comment.following) + { + if is_first_statement_in_alternate_body(following, enclosing) { + // check indentation + let comment_indentation = + own_line_comment_indentation(preceding, comment.range, self.locator); - let preceding_indentation = - indentation_at_offset(preceding.start(), self.locator) - .unwrap_or_default() - .text_len(); - if comment_indentation <= preceding_indentation { - return Err(IgnoredReason::FmtOffAboveBlock); - } + let preceding_indentation = + indentation_at_offset(preceding.start(), self.locator) + .unwrap_or_default() + .text_len(); + if comment_indentation != preceding_indentation { + return Err(IgnoredReason::FmtOffAboveBlock); } } } @@ -153,47 +189,26 @@ impl<'src, 'loc> UselessSuppressionComments<'src, 'loc> { if !comment.line_position.is_own_line() { return Err(IgnoredReason::FmtOnCannotBeTrailing); } - - // If the comment turns on formatting, we need to check if another - // comment turned formatting off within the same scope. - match comment.previous_state { - None | Some(SuppressionKind::On) => return Err(IgnoredReason::NoFmtOff), - _ => {} - } } - if comment.kind == SuppressionKind::Off || comment.kind == SuppressionKind::On { - if let Some(AnyNodeRef::StmtClassDef(class_def)) = comment.enclosing { - if comment.line_position.is_own_line() && comment.start() < class_def.name.start() { - if let Some(decorator) = class_def.decorator_list.last() { - if decorator.end() < comment.start() { - return Err(IgnoredReason::BetweenDecorators); - } - } - } - } + Ok(()) + } - // at this point, any comment being handled should be considered 'valid'. - // on/off suppression comments should be added to the scope - return Ok(Some(comment.kind)); - } - Ok(None) + fn sort(&mut self) { + self.captured.sort_by_key(|(t, _)| t.start()); } - fn ignored_comments( - &self, - ) -> impl Iterator, IgnoredReason)> { - self.captured.iter().map(|(c, r)| (c, *r)) + fn ignored_comments(&self) -> impl Iterator + '_ { + self.captured.iter().map(|(r, i)| (*r, *i)) } } impl<'src, 'loc> CaptureSuppressionComment<'src> for UselessSuppressionComments<'src, 'loc> { - fn capture(&mut self, comment: SuppressionCommentData<'src>) -> Option { + fn capture(&mut self, comment: SuppressionCommentData<'src>) { match self.check_suppression_comment(&comment) { - Ok(kind) => kind, + Ok(()) => {} Err(reason) => { - self.captured.insert(comment, reason); - None + self.captured.push((comment.range, reason)); } } } @@ -236,33 +251,27 @@ where #[derive(Clone, Copy, Debug, PartialEq, Eq)] enum IgnoredReason { InsideExpression, - FmtOffUsedEarlier, - NoFmtOff, - NoCodeSuppressed, BetweenDecorators, - FmtOnCannotBeTrailing, SkipHasToBeTrailing, + FmtOnCannotBeTrailing, FmtOffAboveBlock, } impl Display for IgnoredReason { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { match self { - Self::InsideExpression => write!(f, "it's inside an expression"), - Self::FmtOffUsedEarlier => write!(f, "formatting is already disabled here"), - Self::NoFmtOff => write!(f, "formatting is already enabled here"), - Self::NoCodeSuppressed => write!(f, "it does not suppress formatting for any code"), + Self::InsideExpression => write!(f, "it cannot be inside an expression"), Self::BetweenDecorators => { write!(f, "it cannot be between decorators") } Self::SkipHasToBeTrailing => { - write!(f, "it has to be at the end of a line") + write!(f, "it cannot be on its own line") } Self::FmtOnCannotBeTrailing => { write!(f, "it cannot be at the end of a line") } Self::FmtOffAboveBlock => { - write!(f, "it suppresses formatting for an ambiguous region") + write!(f, "it cannot suppress formatting for an ambiguous region") } } } diff --git a/crates/ruff_linter/src/rules/ruff/rules/mod.rs b/crates/ruff_linter/src/rules/ruff/rules/mod.rs index 1896e6082a4c4..d4461f1f3b6af 100644 --- a/crates/ruff_linter/src/rules/ruff/rules/mod.rs +++ b/crates/ruff_linter/src/rules/ruff/rules/mod.rs @@ -5,8 +5,8 @@ pub(crate) use collection_literal_concatenation::*; pub(crate) use default_factory_kwarg::*; pub(crate) use explicit_f_string_type_conversion::*; pub(crate) use function_call_in_dataclass_default::*; -pub(crate) use ignored_formatter_noqa::*; pub(crate) use implicit_optional::*; +pub(crate) use invalid_formatter_suppression_comment::*; pub(crate) use invalid_index_type::*; pub(crate) use invalid_pyproject_toml::*; pub(crate) use missing_fstring_syntax::*; @@ -36,8 +36,8 @@ mod default_factory_kwarg; mod explicit_f_string_type_conversion; mod function_call_in_dataclass_default; mod helpers; -mod ignored_formatter_noqa; mod implicit_optional; +mod invalid_formatter_suppression_comment; mod invalid_index_type; mod invalid_pyproject_toml; mod missing_fstring_syntax; diff --git a/crates/ruff_linter/src/rules/ruff/rules/suppression_comment_visitor.rs b/crates/ruff_linter/src/rules/ruff/rules/suppression_comment_visitor.rs index 294c4e034ea12..e6a857dddb4a8 100644 --- a/crates/ruff_linter/src/rules/ruff/rules/suppression_comment_visitor.rs +++ b/crates/ruff_linter/src/rules/ruff/rules/suppression_comment_visitor.rs @@ -5,57 +5,46 @@ use ruff_python_ast::{ AnyNodeRef, Suite, }; use ruff_python_trivia::{ - indentation_at_offset, CommentLinePosition, CommentRanges, SimpleTokenKind, SimpleTokenizer, - SuppressionKind, + indentation_at_offset, CommentLinePosition, SimpleTokenKind, SimpleTokenizer, SuppressionKind, }; use ruff_source_file::Locator; use ruff_text_size::{Ranged, TextLen, TextRange, TextSize}; #[derive(Clone, Copy, Debug)] -struct SuppressionComment { - range: TextRange, - kind: SuppressionKind, -} - -fn suppression_comments<'src>( - ranges: &'src CommentRanges, - locator: &'src Locator<'src>, -) -> Box + 'src> { - Box::new(ranges.iter().filter_map(|range| { - Some(SuppressionComment { - range: *range, - kind: SuppressionKind::from_comment(locator.slice(range))?, - }) - })) +pub(super) struct SuppressionComment { + pub(super) range: TextRange, + pub(super) kind: SuppressionKind, } /// Visitor that captures AST data for suppression comments. This uses a similar approach /// to `CommentsVisitor` in the formatter crate. -pub(super) struct SuppressionCommentVisitor<'src, 'builder> { - comments: Peekable + 'src>>, +pub(super) struct SuppressionCommentVisitor< + 'src, + 'builder, + I: Iterator + 'src, +> { + comments: Peekable, parents: Vec>, preceding_node: Option>, - // A stack of comment states in scope at the current visited node. - // The last comment state in the list is the top of the stack, - // and is essentially the 'current' state. - comments_in_scope: Vec<(AnyNodeRef<'src>, SuppressionKind)>, builder: &'builder mut (dyn CaptureSuppressionComment<'src> + 'src), locator: &'src Locator<'src>, } -impl<'src, 'builder> SuppressionCommentVisitor<'src, 'builder> { +impl<'src, 'builder, I> SuppressionCommentVisitor<'src, 'builder, I> +where + I: Iterator + 'src, +{ pub(super) fn new( - comment_ranges: &'src CommentRanges, + comment_ranges: I, builder: &'builder mut (dyn CaptureSuppressionComment<'src> + 'src), locator: &'src Locator<'src>, ) -> Self { Self { - comments: suppression_comments(comment_ranges, locator).peekable(), + comments: comment_ranges.peekable(), parents: Vec::default(), preceding_node: Option::default(), - comments_in_scope: Vec::default(), builder, locator, } @@ -72,7 +61,10 @@ impl<'src, 'builder> SuppressionCommentVisitor<'src, 'builder> { } } -impl<'ast> PreorderVisitor<'ast> for SuppressionCommentVisitor<'ast, '_> { +impl<'ast, I> PreorderVisitor<'ast> for SuppressionCommentVisitor<'ast, '_, I> +where + I: Iterator + 'ast, +{ fn enter_node(&mut self, node: AnyNodeRef<'ast>) -> TraversalSignal { let node_range = node.range(); @@ -89,21 +81,16 @@ impl<'ast> PreorderVisitor<'ast> for SuppressionCommentVisitor<'ast, '_> { let line_position = CommentLinePosition::for_range(range, self.locator.contents()); - let previous_state = self.comments_in_scope.last().map(|(_, s)| s).copied(); - let data = SuppressionCommentData { enclosing: enclosing_node, preceding: self.preceding_node, following: Some(node), - previous_state, line_position, kind, range, }; - if let Some(kind) = self.builder.capture(data) { - self.comments_in_scope.push((node, kind)); - } + self.builder.capture(data); self.comments.next(); } @@ -127,49 +114,47 @@ impl<'ast> PreorderVisitor<'ast> for SuppressionCommentVisitor<'ast, '_> { while let Some(SuppressionComment { range, kind }) = self.comments.peek().copied() { let line_position = CommentLinePosition::for_range(range, self.locator.contents()); if range.start() >= node_end { - if !line_position.is_own_line() { - break; - } - let Some(preceding) = self.preceding_node else { - break; - }; - // check indent of comment against the minimum indentation of a hypothetical body - let comment_indent = own_line_comment_indentation(preceding, range, self.locator); - let min_indentation = indentation_at_offset(node.start(), self.locator) - .unwrap_or_default() - .text_len(); - if comment_indent <= min_indentation { - break; + if line_position.is_own_line() { + if let Some(token) = + SimpleTokenizer::starts_at(node_end, self.locator.contents()) + .skip_trivia() + .next() + { + if token.end() <= range.end() { + break; + } + } + let comment_indent = indentation_at_offset(range.start(), self.locator) + .unwrap_or_default() + .len(); + let node_indent = indentation_at_offset(node.start(), self.locator) + .unwrap_or_default() + .len(); + if node_indent >= comment_indent { + break; + } + } else { + if self.locator.line_start(range.start()) + != self.locator.line_start(node.start()) + { + break; + } } } - let previous_state = self.comments_in_scope.last().map(|(_, s)| s).copied(); - let data = SuppressionCommentData { enclosing: Some(node), preceding: self.preceding_node, following: None, - previous_state, line_position, kind, range, }; - if let Some(kind) = self.builder.capture(data) { - self.comments_in_scope.push((node, kind)); - } + self.builder.capture(data); self.comments.next(); } - // remove comments that are about to become out of scope - for index in (0..self.comments_in_scope.len()).rev() { - if AnyNodeRef::ptr_eq(self.comments_in_scope[index].0, node) { - self.comments_in_scope.pop(); - } else { - break; - } - } - self.preceding_node = Some(node); } fn visit_body(&mut self, body: &'ast [ruff_python_ast::Stmt]) { @@ -201,43 +186,15 @@ pub(super) struct SuppressionCommentData<'src> { pub(super) enclosing: Option>, pub(super) preceding: Option>, pub(super) following: Option>, - pub(super) previous_state: Option, + pub(super) line_position: CommentLinePosition, pub(super) kind: SuppressionKind, pub(super) range: TextRange, } -impl<'src> PartialEq for SuppressionCommentData<'src> { - fn eq(&self, other: &Self) -> bool { - self.range.start().eq(&other.range.start()) - } -} - -impl<'src> Eq for SuppressionCommentData<'src> {} - -impl<'src> Ord for SuppressionCommentData<'src> { - fn cmp(&self, other: &Self) -> std::cmp::Ordering { - self.range.start().cmp(&other.range.start()) - } -} - -impl<'src> PartialOrd for SuppressionCommentData<'src> { - fn partial_cmp(&self, other: &Self) -> Option { - Some(self.cmp(other)) - } -} - -impl<'src> Ranged for SuppressionCommentData<'src> { - fn range(&self) -> TextRange { - self.range - } -} - pub(super) trait CaptureSuppressionComment<'src> { /// This is the entrypoint for the capturer to analyze the next comment. - /// Returning a `Some` value will update the suppression state for future comments. - #[must_use] - fn capture(&mut self, comment: SuppressionCommentData<'src>) -> Option; + fn capture(&mut self, comment: SuppressionCommentData<'src>); } /// Determine the indentation level of an own-line comment, defined as the minimum indentation of diff --git a/crates/ruff_linter/src/rules/ruff/snapshots/ruff_linter__rules__ruff__tests__RUF028_RUF028.py.snap b/crates/ruff_linter/src/rules/ruff/snapshots/ruff_linter__rules__ruff__tests__RUF028_RUF028.py.snap index aba9d73ae0acd..81c2547dc220b 100644 --- a/crates/ruff_linter/src/rules/ruff/snapshots/ruff_linter__rules__ruff__tests__RUF028_RUF028.py.snap +++ b/crates/ruff_linter/src/rules/ruff/snapshots/ruff_linter__rules__ruff__tests__RUF028_RUF028.py.snap @@ -1,332 +1,174 @@ --- source: crates/ruff_linter/src/rules/ruff/mod.rs --- -RUF028.py:1:1: RUF028 [*] This comment will be ignored by the formatter because formatting is already enabled here +RUF028.py:3:9: RUF028 [*] This suppression comment is invalid because it cannot be inside an expression | -1 | # fmt: on - | ^^^^^^^^^ RUF028 -2 | def fmt_off_used_earlier(): -3 | if True: +1 | def fmt_off_between_lists(): +2 | test_list = [ +3 | # fmt: off + | ^^^^^^^^^^ RUF028 +4 | 1, +5 | 2, | = help: Remove this comment ℹ Unsafe fix -1 |-# fmt: on -2 1 | def fmt_off_used_earlier(): -3 2 | if True: -4 3 | a = 5 +1 1 | def fmt_off_between_lists(): +2 2 | test_list = [ +3 |- # fmt: off +4 3 | 1, +5 4 | 2, +6 5 | 3, -RUF028.py:15:9: RUF028 [*] This comment will be ignored by the formatter because formatting is already disabled here +RUF028.py:11:5: RUF028 [*] This suppression comment is invalid because it cannot be on its own line | -13 | # fmt: off -14 | if True: -15 | # fmt: off - | ^^^^^^^^^^ RUF028 -16 | pass -17 | # fmt: off - | - = help: Remove this comment - -ℹ Unsafe fix -12 12 | pass -13 13 | # fmt: off -14 14 | if True: -15 |- # fmt: off -16 15 | pass -17 16 | # fmt: off -18 17 | - -RUF028.py:17:5: RUF028 [*] This comment will be ignored by the formatter because it does not suppress formatting for any code - | -15 | # fmt: off -16 | pass -17 | # fmt: off + 9 | # note: the second `fmt: skip`` should be OK +10 | def fmt_skip_on_own_line(): +11 | # fmt: skip | ^^^^^^^^^^^ RUF028 -18 | -19 | # fmt: off - | - = help: Remove this comment - -ℹ Unsafe fix -14 14 | if True: -15 15 | # fmt: off -16 16 | pass -17 |- # fmt: off -18 17 | -19 18 | # fmt: off -20 19 | - -RUF028.py:26:9: RUF028 [*] This comment will be ignored by the formatter because it's inside an expression - | -24 | def fmt_off_between_lists(): -25 | test_list = [ -26 | # fmt: off - | ^^^^^^^^^^ RUF028 -27 | 1, -28 | 2, - | - = help: Remove this comment - -ℹ Unsafe fix -23 23 | -24 24 | def fmt_off_between_lists(): -25 25 | test_list = [ -26 |- # fmt: off -27 26 | 1, -28 27 | 2, -29 28 | 3, - -RUF028.py:37:5: RUF028 [*] This comment will be ignored by the formatter because it has to be at the end of a line - | -35 | @fmt_off_between_lists -36 | def fmt_off_between_decorators(): -37 | # fmt: skip - | ^^^^^^^^^^^ RUF028 -38 | pass - | - = help: Remove this comment - -ℹ Unsafe fix -34 34 | # fmt: off -35 35 | @fmt_off_between_lists -36 36 | def fmt_off_between_decorators(): -37 |- # fmt: skip -38 37 | pass -39 38 | -40 39 | def fmt_on_trailing(): - -RUF028.py:41:5: RUF028 [*] This comment will be ignored by the formatter because it does not suppress formatting for any code - | -40 | def fmt_on_trailing(): -41 | # fmt: off - | ^^^^^^^^^^ RUF028 -42 | val = 5 # fmt: on -43 | pass - | - = help: Remove this comment - -ℹ Unsafe fix -38 38 | pass -39 39 | -40 40 | def fmt_on_trailing(): -41 |- # fmt: off -42 41 | val = 5 # fmt: on -43 42 | pass -44 43 | - -RUF028.py:42:13: RUF028 [*] This comment will be ignored by the formatter because it cannot be at the end of a line - | -40 | def fmt_on_trailing(): -41 | # fmt: off -42 | val = 5 # fmt: on - | ^^^^^^^^^ RUF028 -43 | pass +12 | pass # fmt: skip | = help: Remove this comment ℹ Unsafe fix -39 39 | -40 40 | def fmt_on_trailing(): -41 41 | # fmt: off -42 |- val = 5 # fmt: on - 42 |+ val = 5 -43 43 | pass -44 44 | -45 45 | def fmt_off_in_else(): +8 8 | +9 9 | # note: the second `fmt: skip`` should be OK +10 10 | def fmt_skip_on_own_line(): +11 |- # fmt: skip +12 11 | pass # fmt: skip +13 12 | +14 13 | -RUF028.py:49:5: RUF028 [*] This comment will be ignored by the formatter because it suppresses formatting for an ambiguous region +RUF028.py:16:1: RUF028 [*] This suppression comment is invalid because it cannot be between decorators | -47 | for val in x: -48 | print(x) -49 | # fmt: off - | ^^^^^^^^^^ RUF028 -50 | else: -51 | print("done") +15 | @fmt_skip_on_own_line +16 | # fmt: off + | ^^^^^^^^^^ RUF028 +17 | @fmt_off_between_lists +18 | def fmt_off_between_decorators(): | = help: Remove this comment ℹ Unsafe fix -46 46 | x = [1, 2, 3] -47 47 | for val in x: -48 48 | print(x) -49 |- # fmt: off -50 49 | else: -51 50 | print("done") -52 51 | while False: +13 13 | +14 14 | +15 15 | @fmt_skip_on_own_line +16 |-# fmt: off +17 16 | @fmt_off_between_lists +18 17 | def fmt_off_between_decorators(): +19 18 | pass -RUF028.py:54:5: RUF028 [*] This comment will be ignored by the formatter because formatting is already enabled here +RUF028.py:22:1: RUF028 [*] This suppression comment is invalid because it cannot be between decorators | -52 | while False: -53 | print("while") -54 | # fmt: on - | ^^^^^^^^^ RUF028 -55 | # fmt: off -56 | else: +21 | @fmt_off_between_decorators +22 | # fmt: off + | ^^^^^^^^^^ RUF028 +23 | @fmt_off_between_lists +24 | class FmtOffBetweenClassDecorators: | = help: Remove this comment ℹ Unsafe fix -51 51 | print("done") -52 52 | while False: -53 53 | print("while") -54 |- # fmt: on -55 54 | # fmt: off -56 55 | else: -57 56 | print("done") +19 19 | pass +20 20 | +21 21 | @fmt_off_between_decorators +22 |-# fmt: off +23 22 | @fmt_off_between_lists +24 23 | class FmtOffBetweenClassDecorators: +25 24 | ... -RUF028.py:55:5: RUF028 [*] This comment will be ignored by the formatter because it suppresses formatting for an ambiguous region +RUF028.py:31:5: RUF028 [*] This suppression comment is invalid because it cannot suppress formatting for an ambiguous region | -53 | print("while") -54 | # fmt: on -55 | # fmt: off +29 | for val in x: +30 | print(x) +31 | # fmt: off | ^^^^^^^^^^ RUF028 -56 | else: -57 | print("done") +32 | else: +33 | print("done") | = help: Remove this comment ℹ Unsafe fix -52 52 | while False: -53 53 | print("while") -54 54 | # fmt: on -55 |- # fmt: off -56 55 | else: -57 56 | print("done") -58 57 | if len(x) > 3: +28 28 | x = [1, 2, 3] +29 29 | for val in x: +30 30 | print(x) +31 |- # fmt: off +32 31 | else: +33 32 | print("done") +34 33 | while False: -RUF028.py:60:5: RUF028 [*] This comment will be ignored by the formatter because formatting is already enabled here +RUF028.py:37:5: RUF028 [*] This suppression comment is invalid because it cannot suppress formatting for an ambiguous region | -58 | if len(x) > 3: -59 | print("huh?") -60 | # fmt: on - | ^^^^^^^^^ RUF028 -61 | # fmt: off -62 | else: - | - = help: Remove this comment - -ℹ Unsafe fix -57 57 | print("done") -58 58 | if len(x) > 3: -59 59 | print("huh?") -60 |- # fmt: on -61 60 | # fmt: off -62 61 | else: -63 62 | print("expected") - -RUF028.py:61:5: RUF028 [*] This comment will be ignored by the formatter because it suppresses formatting for an ambiguous region - | -59 | print("huh?") -60 | # fmt: on -61 | # fmt: off +35 | print("while") +36 | # fmt: off +37 | # fmt: off | ^^^^^^^^^^ RUF028 -62 | else: -63 | print("expected") +38 | else: +39 | print("done") | = help: Remove this comment ℹ Unsafe fix -58 58 | if len(x) > 3: -59 59 | print("huh?") -60 60 | # fmt: on -61 |- # fmt: off -62 61 | else: -63 62 | print("expected") -64 63 | +34 34 | while False: +35 35 | print("while") +36 36 | # fmt: off +37 |- # fmt: off +38 37 | else: +39 38 | print("done") +40 39 | if len(x) > 3: -RUF028.py:67:5: RUF028 [*] This comment will be ignored by the formatter because it does not suppress formatting for any code +RUF028.py:43:5: RUF028 [*] This suppression comment is invalid because it cannot suppress formatting for an ambiguous region | -65 | def dangling_fmt_off(): -66 | pass -67 | # fmt: off +41 | print("huh?") +42 | # fmt: on +43 | # fmt: off | ^^^^^^^^^^ RUF028 -68 | -69 | def dangling_fmt_off2(): +44 | else: +45 | print("expected") | = help: Remove this comment ℹ Unsafe fix -64 64 | -65 65 | def dangling_fmt_off(): -66 66 | pass -67 |- # fmt: off -68 67 | -69 68 | def dangling_fmt_off2(): -70 69 | if True: +40 40 | if len(x) > 3: +41 41 | print("huh?") +42 42 | # fmt: on +43 |- # fmt: off +44 43 | else: +45 44 | print("expected") +46 45 | -RUF028.py:73:13: RUF028 [*] This comment will be ignored by the formatter because it does not suppress formatting for any code - | -71 | if True: -72 | pass -73 | # fmt: off - | ^^^^^^^^^^ RUF028 -74 | else: -75 | pass +RUF028.py:49:13: RUF028 [*] This suppression comment is invalid because it cannot be at the end of a line | - = help: Remove this comment - -ℹ Unsafe fix -70 70 | if True: -71 71 | if True: -72 72 | pass -73 |- # fmt: off -74 73 | else: -75 74 | pass -76 75 | # fmt: off - -RUF028.py:76:13: RUF028 [*] This comment will be ignored by the formatter because it does not suppress formatting for any code - | -74 | else: -75 | pass -76 | # fmt: off - | ^^^^^^^^^^ RUF028 -77 | # fmt: off -78 | else: - | - = help: Remove this comment - -ℹ Unsafe fix -73 73 | # fmt: off -74 74 | else: -75 75 | pass -76 |- # fmt: off -77 76 | # fmt: off -78 77 | else: -79 78 | pass - -RUF028.py:77:9: RUF028 [*] This comment will be ignored by the formatter because it does not suppress formatting for any code - | -75 | pass -76 | # fmt: off -77 | # fmt: off - | ^^^^^^^^^^ RUF028 -78 | else: -79 | pass +47 | def fmt_on_trailing(): +48 | # fmt: off +49 | val = 5 # fmt: on + | ^^^^^^^^^ RUF028 +50 | pass # fmt: on | = help: Remove this comment ℹ Unsafe fix -74 74 | else: -75 75 | pass -76 76 | # fmt: off -77 |- # fmt: off -78 77 | else: -79 78 | pass -80 79 | # fmt: off +46 46 | +47 47 | def fmt_on_trailing(): +48 48 | # fmt: off +49 |- val = 5 # fmt: on + 49 |+ val = 5 +50 50 | pass # fmt: on -RUF028.py:80:5: RUF028 [*] This comment will be ignored by the formatter because it does not suppress formatting for any code +RUF028.py:50:10: RUF028 [*] This suppression comment is invalid because it cannot be at the end of a line | -78 | else: -79 | pass -80 | # fmt: off - | ^^^^^^^^^^ RUF028 +48 | # fmt: off +49 | val = 5 # fmt: on +50 | pass # fmt: on + | ^^^^^^^^^ RUF028 | = help: Remove this comment ℹ Unsafe fix -77 77 | # fmt: off -78 78 | else: -79 79 | pass -80 |- # fmt: off +47 47 | def fmt_on_trailing(): +48 48 | # fmt: off +49 49 | val = 5 # fmt: on +50 |- pass # fmt: on + 50 |+ pass diff --git a/crates/ruff_python_formatter/src/comments/mod.rs b/crates/ruff_python_formatter/src/comments/mod.rs index 27127f6ae5a3e..9ca2134f700d9 100644 --- a/crates/ruff_python_formatter/src/comments/mod.rs +++ b/crates/ruff_python_formatter/src/comments/mod.rs @@ -468,7 +468,7 @@ pub(crate) fn has_skip_comment(trailing_comments: &[SourceComment], source: &str trailing_comments.iter().any(|comment| { comment.line_position().is_end_of_line() && matches!( - SuppressionKind::from_comment(comment.slice().text(SourceCode::new(source))), + SuppressionKind::from_comment(comment.text(source)), Some(SuppressionKind::Skip | SuppressionKind::Off) ) }) diff --git a/crates/ruff_python_trivia/src/comments.rs b/crates/ruff_python_trivia/src/comments.rs index a0b35d330102a..b8d1a58fd00ed 100644 --- a/crates/ruff_python_trivia/src/comments.rs +++ b/crates/ruff_python_trivia/src/comments.rs @@ -102,8 +102,8 @@ impl CommentLinePosition { matches!(self, Self::EndOfLine) } - /// Finds the line position of a comment given a range with - /// + /// Finds the line position of a comment given a range over a valid + /// comment. pub fn for_range(comment_range: TextRange, source_code: &str) -> Self { let before = &source_code[TextRange::up_to(comment_range.start())]; From 5ab92d82fee24fabb22bcef7c291c37634bfa616 Mon Sep 17 00:00:00 2001 From: Jane Lewis Date: Fri, 23 Feb 2024 13:26:44 -0800 Subject: [PATCH 11/13] Fix inverted logic for is_suppression_on/is_suppression_off --- crates/ruff_python_trivia/src/comments.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/crates/ruff_python_trivia/src/comments.rs b/crates/ruff_python_trivia/src/comments.rs index b8d1a58fd00ed..6eb42c26f3074 100644 --- a/crates/ruff_python_trivia/src/comments.rs +++ b/crates/ruff_python_trivia/src/comments.rs @@ -53,12 +53,12 @@ impl SuppressionKind { /// Returns true if this comment is a `fmt: off` or `yapf: disable` own line suppression comment. pub fn is_suppression_on(slice: &str, position: CommentLinePosition) -> bool { - position.is_own_line() && matches!(Self::from_comment(slice), Some(Self::Off)) + position.is_own_line() && matches!(Self::from_comment(slice), Some(Self::On)) } /// Returns true if this comment is a `fmt: on` or `yapf: enable` own line suppression comment. pub fn is_suppression_off(slice: &str, position: CommentLinePosition) -> bool { - position.is_own_line() && matches!(Self::from_comment(slice), Some(Self::On)) + position.is_own_line() && matches!(Self::from_comment(slice), Some(Self::Off)) } } /// The position of a comment in the source text. From 5b80da066f1409eed55cd4a2b7bf447ffcf06457 Mon Sep 17 00:00:00 2001 From: Jane Lewis Date: Tue, 27 Feb 2024 11:37:43 -0800 Subject: [PATCH 12/13] Address suggestions --- .../resources/test/fixtures/ruff/RUF028.py | 18 +- .../invalid_formatter_suppression_comment.rs | 76 ++--- .../ruff/rules/suppression_comment_visitor.rs | 98 +------ ..._rules__ruff__tests__RUF028_RUF028.py.snap | 267 +++++++++++------- crates/ruff_python_ast/src/node.rs | 192 ++++++++++++- .../src/comments/placement.rs | 204 +------------ 6 files changed, 416 insertions(+), 439 deletions(-) diff --git a/crates/ruff_linter/resources/test/fixtures/ruff/RUF028.py b/crates/ruff_linter/resources/test/fixtures/ruff/RUF028.py index 1b6cb4297908d..5409ddf5fbc3e 100644 --- a/crates/ruff_linter/resources/test/fixtures/ruff/RUF028.py +++ b/crates/ruff_linter/resources/test/fixtures/ruff/RUF028.py @@ -6,10 +6,11 @@ def fmt_off_between_lists(): 3, ] + # note: the second `fmt: skip`` should be OK def fmt_skip_on_own_line(): # fmt: skip - pass # fmt: skip + pass # fmt: skip @fmt_skip_on_own_line @@ -18,12 +19,13 @@ def fmt_skip_on_own_line(): def fmt_off_between_decorators(): pass + @fmt_off_between_decorators # fmt: off -@fmt_off_between_lists class FmtOffBetweenClassDecorators: ... + def fmt_off_in_else(): x = [1, 2, 3] for val in x: @@ -44,6 +46,18 @@ def fmt_off_in_else(): else: print("expected") + +class Test: + @classmethod + # fmt: off + def cls_method_a( + # fmt: off + cls, + ) -> None: + # noqa: test # fmt: skip + pass + + def fmt_on_trailing(): # fmt: off val = 5 # fmt: on diff --git a/crates/ruff_linter/src/rules/ruff/rules/invalid_formatter_suppression_comment.rs b/crates/ruff_linter/src/rules/ruff/rules/invalid_formatter_suppression_comment.rs index 592ee0505906d..cbda57903a98a 100644 --- a/crates/ruff_linter/src/rules/ruff/rules/invalid_formatter_suppression_comment.rs +++ b/crates/ruff_linter/src/rules/ruff/rules/invalid_formatter_suppression_comment.rs @@ -13,8 +13,8 @@ use crate::checkers::ast::Checker; use crate::fix::edits::delete_comment; use super::suppression_comment_visitor::{ - own_line_comment_indentation, CaptureSuppressionComment, SuppressionComment, - SuppressionCommentData, SuppressionCommentVisitor, + CaptureSuppressionComment, SuppressionComment, SuppressionCommentData, + SuppressionCommentVisitor, }; /// ## What it does @@ -127,17 +127,15 @@ impl<'src, 'loc> UselessSuppressionComments<'src, 'loc> { // check if the comment is inside of an expression. if comment .enclosing - .map(AnyNodeRef::is_expression) + .map(|n| !AnyNodeRef::is_statement(n)) .unwrap_or_default() { - return Err(IgnoredReason::InsideExpression); + return Err(IgnoredReason::InNonStatement); } // check if a skip comment is at the end of a line if comment.kind == SuppressionKind::Skip && !comment.line_position.is_end_of_line() { - if !comment.line_position.is_end_of_line() { - return Err(IgnoredReason::SkipHasToBeTrailing); - } + return Err(IgnoredReason::SkipHasToBeTrailing); } if comment.kind == SuppressionKind::Off || comment.kind == SuppressionKind::On { @@ -155,9 +153,9 @@ impl<'src, 'loc> UselessSuppressionComments<'src, 'loc> { ) = comment.enclosing { if comment.line_position.is_own_line() && comment.range.start() < name.start() { - if let Some(decorator) = decorator_list.last() { - if decorator.start() > comment.range.end() { - return Err(IgnoredReason::BetweenDecorators); + if let Some(decorator) = decorator_list.first() { + if decorator.end() < comment.range.start() { + return Err(IgnoredReason::AfterDecorator); } } } @@ -168,10 +166,13 @@ impl<'src, 'loc> UselessSuppressionComments<'src, 'loc> { if let (Some(enclosing), Some(preceding), Some(following)) = (comment.enclosing, comment.preceding, comment.following) { - if is_first_statement_in_alternate_body(following, enclosing) { + if following.is_first_statement_in_alternate_body(enclosing) { // check indentation - let comment_indentation = - own_line_comment_indentation(preceding, comment.range, self.locator); + let comment_indentation = AnyNodeRef::comment_indentation_after( + preceding, + comment.range, + self.locator, + ); let preceding_indentation = indentation_at_offset(preceding.start(), self.locator) @@ -214,44 +215,10 @@ impl<'src, 'loc> CaptureSuppressionComment<'src> for UselessSuppressionComments< } } -/// Returns `true` if `statement` is the first statement in an alternate `body` (e.g. the else of an if statement) -fn is_first_statement_in_alternate_body(statement: AnyNodeRef, has_body: AnyNodeRef) -> bool { - match has_body { - AnyNodeRef::StmtFor(ast::StmtFor { orelse, .. }) - | AnyNodeRef::StmtWhile(ast::StmtWhile { orelse, .. }) => { - are_same_optional(statement, orelse.first()) - } - - AnyNodeRef::StmtTry(ast::StmtTry { - handlers, - orelse, - finalbody, - .. - }) => { - are_same_optional(statement, handlers.first()) - || are_same_optional(statement, orelse.first()) - || are_same_optional(statement, finalbody.first()) - } - - AnyNodeRef::StmtIf(ast::StmtIf { - elif_else_clauses, .. - }) => are_same_optional(statement, elif_else_clauses.first()), - _ => false, - } -} - -/// Returns `true` if `right` is `Some` and `left` and `right` are referentially equal. -fn are_same_optional<'a, T>(left: AnyNodeRef, right: Option) -> bool -where - T: Into>, -{ - right.is_some_and(|right| left.ptr_eq(right.into())) -} - #[derive(Clone, Copy, Debug, PartialEq, Eq)] enum IgnoredReason { - InsideExpression, - BetweenDecorators, + InNonStatement, + AfterDecorator, SkipHasToBeTrailing, FmtOnCannotBeTrailing, FmtOffAboveBlock, @@ -260,9 +227,12 @@ enum IgnoredReason { impl Display for IgnoredReason { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { match self { - Self::InsideExpression => write!(f, "it cannot be inside an expression"), - Self::BetweenDecorators => { - write!(f, "it cannot be between decorators") + Self::InNonStatement => write!( + f, + "it cannot be in an expression, pattern, argument list, or other non-statement" + ), + Self::AfterDecorator => { + write!(f, "it cannot be after a decorator") } Self::SkipHasToBeTrailing => { write!(f, "it cannot be on its own line") @@ -271,7 +241,7 @@ impl Display for IgnoredReason { write!(f, "it cannot be at the end of a line") } Self::FmtOffAboveBlock => { - write!(f, "it cannot suppress formatting for an ambiguous region") + write!(f, "it cannot be directly above an alternate body") } } } diff --git a/crates/ruff_linter/src/rules/ruff/rules/suppression_comment_visitor.rs b/crates/ruff_linter/src/rules/ruff/rules/suppression_comment_visitor.rs index e6a857dddb4a8..1dbe4c1a40394 100644 --- a/crates/ruff_linter/src/rules/ruff/rules/suppression_comment_visitor.rs +++ b/crates/ruff_linter/src/rules/ruff/rules/suppression_comment_visitor.rs @@ -5,10 +5,10 @@ use ruff_python_ast::{ AnyNodeRef, Suite, }; use ruff_python_trivia::{ - indentation_at_offset, CommentLinePosition, SimpleTokenKind, SimpleTokenizer, SuppressionKind, + indentation_at_offset, CommentLinePosition, SimpleTokenizer, SuppressionKind, }; use ruff_source_file::Locator; -use ruff_text_size::{Ranged, TextLen, TextRange, TextSize}; +use ruff_text_size::{Ranged, TextRange, TextSize}; #[derive(Clone, Copy, Debug)] pub(super) struct SuppressionComment { @@ -124,16 +124,17 @@ where break; } } - let comment_indent = indentation_at_offset(range.start(), self.locator) - .unwrap_or_default() - .len(); - let node_indent = indentation_at_offset(node.start(), self.locator) - .unwrap_or_default() - .len(); + let comment_indent = + AnyNodeRef::comment_indentation_after(node, range, self.locator); + let node_indent = TextSize::of( + indentation_at_offset(node.start(), self.locator).unwrap_or_default(), + ); if node_indent >= comment_indent { break; } } else { + // If the end-of-line comment is not on the same line, we can assume that there's a node in between + // the end of this node and this comment. if self.locator.line_start(range.start()) != self.locator.line_start(node.start()) { @@ -182,13 +183,18 @@ where #[derive(Clone, Debug)] pub(super) struct SuppressionCommentData<'src> { - /// If `enclosing` is `None`, this comment is top-level + /// The AST node that encloses the comment. If `enclosing` is `None`, this comment is a top-level statement. pub(super) enclosing: Option>, + /// An AST node that comes directly before the comment. A child of `enclosing`. pub(super) preceding: Option>, + /// An AST node that comes directly after the comment. A child of `enclosing`. pub(super) following: Option>, + /// The line position of the comment - it can either be on its own line, or at the end of a line. pub(super) line_position: CommentLinePosition, + /// Whether this comment is `fmt: off`, `fmt: on`, or `fmt: skip` (or `yapf disable` / `yapf enable`) pub(super) kind: SuppressionKind, + /// The range of text that makes up the comment. This includes the `#` prefix. pub(super) range: TextRange, } @@ -196,77 +202,3 @@ pub(super) trait CaptureSuppressionComment<'src> { /// This is the entrypoint for the capturer to analyze the next comment. fn capture(&mut self, comment: SuppressionCommentData<'src>); } - -/// Determine the indentation level of an own-line comment, defined as the minimum indentation of -/// all comments between the preceding node and the comment, including the comment itself. In -/// other words, we don't allow successive comments to ident _further_ than any preceding comments. -/// -/// For example, given: -/// ```python -/// if True: -/// pass -/// # comment -/// ``` -/// -/// The indentation would be 4, as the comment is indented by 4 spaces. -/// -/// Given: -/// ```python -/// if True: -/// pass -/// # comment -/// else: -/// pass -/// ``` -/// -/// The indentation would be 0, as the comment is not indented at all. -/// -/// Given: -/// ```python -/// if True: -/// pass -/// # comment -/// # comment -/// ``` -/// -/// Both comments would be marked as indented at 4 spaces, as the indentation of the first comment -/// is used for the second comment. -/// -/// This logic avoids pathological cases like: -/// ```python -/// try: -/// if True: -/// if True: -/// pass -/// -/// # a -/// # b -/// # c -/// except Exception: -/// pass -/// ``` -/// -/// If we don't use the minimum indentation of any preceding comments, we would mark `# b` as -/// indented to the same depth as `pass`, which could in turn lead to us treating it as a trailing -/// comment of `pass`, despite there being a comment between them that "resets" the indentation. -pub(super) fn own_line_comment_indentation( - preceding: AnyNodeRef, - comment_range: TextRange, - locator: &Locator, -) -> TextSize { - let tokenizer = SimpleTokenizer::new( - locator.contents(), - TextRange::new(locator.full_line_end(preceding.end()), comment_range.end()), - ); - - tokenizer - .filter_map(|token| { - if token.kind() == SimpleTokenKind::Comment { - indentation_at_offset(token.start(), locator).map(TextLen::text_len) - } else { - None - } - }) - .min() - .unwrap_or_default() -} diff --git a/crates/ruff_linter/src/rules/ruff/snapshots/ruff_linter__rules__ruff__tests__RUF028_RUF028.py.snap b/crates/ruff_linter/src/rules/ruff/snapshots/ruff_linter__rules__ruff__tests__RUF028_RUF028.py.snap index 81c2547dc220b..4fd416657b520 100644 --- a/crates/ruff_linter/src/rules/ruff/snapshots/ruff_linter__rules__ruff__tests__RUF028_RUF028.py.snap +++ b/crates/ruff_linter/src/rules/ruff/snapshots/ruff_linter__rules__ruff__tests__RUF028_RUF028.py.snap @@ -1,7 +1,7 @@ --- source: crates/ruff_linter/src/rules/ruff/mod.rs --- -RUF028.py:3:9: RUF028 [*] This suppression comment is invalid because it cannot be inside an expression +RUF028.py:3:9: RUF028 [*] This suppression comment is invalid because it cannot be in an expression, pattern, argument list, or other non-statement | 1 | def fmt_off_between_lists(): 2 | test_list = [ @@ -20,155 +20,214 @@ RUF028.py:3:9: RUF028 [*] This suppression comment is invalid because it cannot 5 4 | 2, 6 5 | 3, -RUF028.py:11:5: RUF028 [*] This suppression comment is invalid because it cannot be on its own line +RUF028.py:12:5: RUF028 [*] This suppression comment is invalid because it cannot be on its own line | - 9 | # note: the second `fmt: skip`` should be OK -10 | def fmt_skip_on_own_line(): -11 | # fmt: skip +10 | # note: the second `fmt: skip`` should be OK +11 | def fmt_skip_on_own_line(): +12 | # fmt: skip | ^^^^^^^^^^^ RUF028 -12 | pass # fmt: skip +13 | pass # fmt: skip | = help: Remove this comment ℹ Unsafe fix -8 8 | -9 9 | # note: the second `fmt: skip`` should be OK -10 10 | def fmt_skip_on_own_line(): -11 |- # fmt: skip -12 11 | pass # fmt: skip -13 12 | +9 9 | +10 10 | # note: the second `fmt: skip`` should be OK +11 11 | def fmt_skip_on_own_line(): +12 |- # fmt: skip +13 12 | pass # fmt: skip 14 13 | +15 14 | -RUF028.py:16:1: RUF028 [*] This suppression comment is invalid because it cannot be between decorators +RUF028.py:17:1: RUF028 [*] This suppression comment is invalid because it cannot be after a decorator | -15 | @fmt_skip_on_own_line -16 | # fmt: off +16 | @fmt_skip_on_own_line +17 | # fmt: off | ^^^^^^^^^^ RUF028 -17 | @fmt_off_between_lists -18 | def fmt_off_between_decorators(): +18 | @fmt_off_between_lists +19 | def fmt_off_between_decorators(): | = help: Remove this comment ℹ Unsafe fix -13 13 | 14 14 | -15 15 | @fmt_skip_on_own_line -16 |-# fmt: off -17 16 | @fmt_off_between_lists -18 17 | def fmt_off_between_decorators(): -19 18 | pass +15 15 | +16 16 | @fmt_skip_on_own_line +17 |-# fmt: off +18 17 | @fmt_off_between_lists +19 18 | def fmt_off_between_decorators(): +20 19 | pass -RUF028.py:22:1: RUF028 [*] This suppression comment is invalid because it cannot be between decorators +RUF028.py:24:1: RUF028 [*] This suppression comment is invalid because it cannot be after a decorator | -21 | @fmt_off_between_decorators -22 | # fmt: off +23 | @fmt_off_between_decorators +24 | # fmt: off | ^^^^^^^^^^ RUF028 -23 | @fmt_off_between_lists -24 | class FmtOffBetweenClassDecorators: +25 | class FmtOffBetweenClassDecorators: +26 | ... | = help: Remove this comment ℹ Unsafe fix -19 19 | pass -20 20 | -21 21 | @fmt_off_between_decorators -22 |-# fmt: off -23 22 | @fmt_off_between_lists -24 23 | class FmtOffBetweenClassDecorators: -25 24 | ... - -RUF028.py:31:5: RUF028 [*] This suppression comment is invalid because it cannot suppress formatting for an ambiguous region - | -29 | for val in x: -30 | print(x) -31 | # fmt: off +21 21 | +22 22 | +23 23 | @fmt_off_between_decorators +24 |-# fmt: off +25 24 | class FmtOffBetweenClassDecorators: +26 25 | ... +27 26 | + +RUF028.py:33:5: RUF028 [*] This suppression comment is invalid because it cannot be directly above an alternate body + | +31 | for val in x: +32 | print(x) +33 | # fmt: off | ^^^^^^^^^^ RUF028 -32 | else: -33 | print("done") +34 | else: +35 | print("done") | = help: Remove this comment ℹ Unsafe fix -28 28 | x = [1, 2, 3] -29 29 | for val in x: -30 30 | print(x) -31 |- # fmt: off -32 31 | else: -33 32 | print("done") -34 33 | while False: - -RUF028.py:37:5: RUF028 [*] This suppression comment is invalid because it cannot suppress formatting for an ambiguous region - | -35 | print("while") -36 | # fmt: off -37 | # fmt: off +30 30 | x = [1, 2, 3] +31 31 | for val in x: +32 32 | print(x) +33 |- # fmt: off +34 33 | else: +35 34 | print("done") +36 35 | while False: + +RUF028.py:39:5: RUF028 [*] This suppression comment is invalid because it cannot be directly above an alternate body + | +37 | print("while") +38 | # fmt: off +39 | # fmt: off | ^^^^^^^^^^ RUF028 -38 | else: -39 | print("done") +40 | else: +41 | print("done") | = help: Remove this comment ℹ Unsafe fix -34 34 | while False: -35 35 | print("while") -36 36 | # fmt: off -37 |- # fmt: off -38 37 | else: -39 38 | print("done") -40 39 | if len(x) > 3: - -RUF028.py:43:5: RUF028 [*] This suppression comment is invalid because it cannot suppress formatting for an ambiguous region - | -41 | print("huh?") -42 | # fmt: on -43 | # fmt: off +36 36 | while False: +37 37 | print("while") +38 38 | # fmt: off +39 |- # fmt: off +40 39 | else: +41 40 | print("done") +42 41 | if len(x) > 3: + +RUF028.py:45:5: RUF028 [*] This suppression comment is invalid because it cannot be directly above an alternate body + | +43 | print("huh?") +44 | # fmt: on +45 | # fmt: off | ^^^^^^^^^^ RUF028 -44 | else: -45 | print("expected") +46 | else: +47 | print("expected") | = help: Remove this comment ℹ Unsafe fix -40 40 | if len(x) > 3: -41 41 | print("huh?") -42 42 | # fmt: on -43 |- # fmt: off -44 43 | else: -45 44 | print("expected") -46 45 | - -RUF028.py:49:13: RUF028 [*] This suppression comment is invalid because it cannot be at the end of a line - | -47 | def fmt_on_trailing(): -48 | # fmt: off -49 | val = 5 # fmt: on +42 42 | if len(x) > 3: +43 43 | print("huh?") +44 44 | # fmt: on +45 |- # fmt: off +46 45 | else: +47 46 | print("expected") +48 47 | + +RUF028.py:52:5: RUF028 [*] This suppression comment is invalid because it cannot be after a decorator + | +50 | class Test: +51 | @classmethod +52 | # fmt: off + | ^^^^^^^^^^ RUF028 +53 | def cls_method_a( +54 | # fmt: off + | + = help: Remove this comment + +ℹ Unsafe fix +49 49 | +50 50 | class Test: +51 51 | @classmethod +52 |- # fmt: off +53 52 | def cls_method_a( +54 53 | # fmt: off +55 54 | cls, + +RUF028.py:54:9: RUF028 [*] This suppression comment is invalid because it cannot be in an expression, pattern, argument list, or other non-statement + | +52 | # fmt: off +53 | def cls_method_a( +54 | # fmt: off + | ^^^^^^^^^^ RUF028 +55 | cls, +56 | ) -> None: + | + = help: Remove this comment + +ℹ Unsafe fix +51 51 | @classmethod +52 52 | # fmt: off +53 53 | def cls_method_a( +54 |- # fmt: off +55 54 | cls, +56 55 | ) -> None: +57 56 | # noqa: test # fmt: skip + +RUF028.py:57:9: RUF028 [*] This suppression comment is invalid because it cannot be on its own line + | +55 | cls, +56 | ) -> None: +57 | # noqa: test # fmt: skip + | ^^^^^^^^^^^^^^^^^^^^^^^^ RUF028 +58 | pass + | + = help: Remove this comment + +ℹ Unsafe fix +54 54 | # fmt: off +55 55 | cls, +56 56 | ) -> None: +57 |- # noqa: test # fmt: skip +58 57 | pass +59 58 | +60 59 | + +RUF028.py:63:13: RUF028 [*] This suppression comment is invalid because it cannot be at the end of a line + | +61 | def fmt_on_trailing(): +62 | # fmt: off +63 | val = 5 # fmt: on | ^^^^^^^^^ RUF028 -50 | pass # fmt: on +64 | pass # fmt: on | = help: Remove this comment ℹ Unsafe fix -46 46 | -47 47 | def fmt_on_trailing(): -48 48 | # fmt: off -49 |- val = 5 # fmt: on - 49 |+ val = 5 -50 50 | pass # fmt: on - -RUF028.py:50:10: RUF028 [*] This suppression comment is invalid because it cannot be at the end of a line - | -48 | # fmt: off -49 | val = 5 # fmt: on -50 | pass # fmt: on +60 60 | +61 61 | def fmt_on_trailing(): +62 62 | # fmt: off +63 |- val = 5 # fmt: on + 63 |+ val = 5 +64 64 | pass # fmt: on + +RUF028.py:64:10: RUF028 [*] This suppression comment is invalid because it cannot be at the end of a line + | +62 | # fmt: off +63 | val = 5 # fmt: on +64 | pass # fmt: on | ^^^^^^^^^ RUF028 | = help: Remove this comment ℹ Unsafe fix -47 47 | def fmt_on_trailing(): -48 48 | # fmt: off -49 49 | val = 5 # fmt: on -50 |- pass # fmt: on - 50 |+ pass +61 61 | def fmt_on_trailing(): +62 62 | # fmt: off +63 63 | val = 5 # fmt: on +64 |- pass # fmt: on + 64 |+ pass diff --git a/crates/ruff_python_ast/src/node.rs b/crates/ruff_python_ast/src/node.rs index 54afd2e040b3e..27528e44de86d 100644 --- a/crates/ruff_python_ast/src/node.rs +++ b/crates/ruff_python_ast/src/node.rs @@ -5,7 +5,9 @@ use crate::{ PatternArguments, PatternKeyword, Stmt, TypeParam, TypeParamParamSpec, TypeParamTypeVar, TypeParamTypeVarTuple, TypeParams, WithItem, }; -use ruff_text_size::{Ranged, TextRange}; +use ruff_python_trivia::{indentation_at_offset, SimpleTokenKind, SimpleTokenizer}; +use ruff_source_file::Locator; +use ruff_text_size::{Ranged, TextLen, TextRange, TextSize}; use std::ptr::NonNull; pub trait AstNode: Ranged { @@ -6196,6 +6198,194 @@ impl<'a> AnyNodeRef<'a> { body.last().map(AnyNodeRef::from) } + + /// Check if the given statement is the first statement after the colon of a branch, be it in if + /// statements, for statements, after each part of a try-except-else-finally or function/class + /// definitions. + /// + /// + /// ```python + /// if True: <- has body + /// a <- first statement + /// b + /// elif b: <- has body + /// c <- first statement + /// d + /// else: <- has body + /// e <- first statement + /// f + /// + /// class: <- has body + /// a: int <- first statement + /// b: int + /// + /// ``` + /// + /// For nodes with multiple bodies, we check all bodies that don't have their own node. For + /// try-except-else-finally, each except branch has it's own node, so for the `StmtTry`, we check + /// the `try:`, `else:` and `finally:`, bodies, while `ExceptHandlerExceptHandler` has it's own + /// check. For for-else and while-else, we check both branches for the whole statement. + /// + /// ```python + /// try: <- has body (a) + /// 6/8 <- first statement (a) + /// 1/0 + /// except: <- has body (b) + /// a <- first statement (b) + /// b + /// else: + /// c <- first statement (a) + /// d + /// finally: + /// e <- first statement (a) + /// f + /// ``` + pub fn is_first_statement_in_body(&self, body: AnyNodeRef) -> bool { + match body { + AnyNodeRef::StmtFor(ast::StmtFor { body, orelse, .. }) + | AnyNodeRef::StmtWhile(ast::StmtWhile { body, orelse, .. }) => { + are_same_optional(*self, body.first()) || are_same_optional(*self, orelse.first()) + } + + AnyNodeRef::StmtTry(ast::StmtTry { + body, + orelse, + finalbody, + .. + }) => { + are_same_optional(*self, body.first()) + || are_same_optional(*self, orelse.first()) + || are_same_optional(*self, finalbody.first()) + } + + AnyNodeRef::StmtIf(ast::StmtIf { body, .. }) + | AnyNodeRef::ElifElseClause(ast::ElifElseClause { body, .. }) + | AnyNodeRef::StmtWith(ast::StmtWith { body, .. }) + | AnyNodeRef::ExceptHandlerExceptHandler(ast::ExceptHandlerExceptHandler { + body, + .. + }) + | AnyNodeRef::MatchCase(MatchCase { body, .. }) + | AnyNodeRef::StmtFunctionDef(ast::StmtFunctionDef { body, .. }) + | AnyNodeRef::StmtClassDef(ast::StmtClassDef { body, .. }) => { + are_same_optional(*self, body.first()) + } + + AnyNodeRef::StmtMatch(ast::StmtMatch { cases, .. }) => { + are_same_optional(*self, cases.first()) + } + + _ => false, + } + } + + /// Returns `true` if `statement` is the first statement in an alternate `body` (e.g. the else of an if statement) + pub fn is_first_statement_in_alternate_body(&self, body: AnyNodeRef) -> bool { + match body { + AnyNodeRef::StmtFor(ast::StmtFor { orelse, .. }) + | AnyNodeRef::StmtWhile(ast::StmtWhile { orelse, .. }) => { + are_same_optional(*self, orelse.first()) + } + + AnyNodeRef::StmtTry(ast::StmtTry { + handlers, + orelse, + finalbody, + .. + }) => { + are_same_optional(*self, handlers.first()) + || are_same_optional(*self, orelse.first()) + || are_same_optional(*self, finalbody.first()) + } + + AnyNodeRef::StmtIf(ast::StmtIf { + elif_else_clauses, .. + }) => are_same_optional(*self, elif_else_clauses.first()), + _ => false, + } + } + + /// Determine the indentation level of an own-line comment, defined as the minimum indentation of + /// all comments between the preceding node and the comment, including the comment itself. In + /// other words, we don't allow successive comments to ident _further_ than any preceding comments. + /// + /// For example, given: + /// ```python + /// if True: + /// pass + /// # comment + /// ``` + /// + /// The indentation would be 4, as the comment is indented by 4 spaces. + /// + /// Given: + /// ```python + /// if True: + /// pass + /// # comment + /// else: + /// pass + /// ``` + /// + /// The indentation would be 0, as the comment is not indented at all. + /// + /// Given: + /// ```python + /// if True: + /// pass + /// # comment + /// # comment + /// ``` + /// + /// Both comments would be marked as indented at 4 spaces, as the indentation of the first comment + /// is used for the second comment. + /// + /// This logic avoids pathological cases like: + /// ```python + /// try: + /// if True: + /// if True: + /// pass + /// + /// # a + /// # b + /// # c + /// except Exception: + /// pass + /// ``` + /// + /// If we don't use the minimum indentation of any preceding comments, we would mark `# b` as + /// indented to the same depth as `pass`, which could in turn lead to us treating it as a trailing + /// comment of `pass`, despite there being a comment between them that "resets" the indentation. + pub fn comment_indentation_after( + preceding: Self, + comment_range: TextRange, + locator: &Locator, + ) -> TextSize { + let tokenizer = SimpleTokenizer::new( + locator.contents(), + TextRange::new(locator.full_line_end(preceding.end()), comment_range.end()), + ); + + tokenizer + .filter_map(|token| { + if token.kind() == SimpleTokenKind::Comment { + indentation_at_offset(token.start(), locator).map(TextLen::text_len) + } else { + None + } + }) + .min() + .unwrap_or_default() + } +} + +/// Returns `true` if `right` is `Some` and `left` and `right` are referentially equal. +fn are_same_optional<'a, T>(left: AnyNodeRef, right: Option) -> bool +where + T: Into>, +{ + right.is_some_and(|right| left.ptr_eq(right.into())) } impl<'a> From<&'a ast::ModModule> for AnyNodeRef<'a> { diff --git a/crates/ruff_python_formatter/src/comments/placement.rs b/crates/ruff_python_formatter/src/comments/placement.rs index 0033d69206e6d..0b50e819760ce 100644 --- a/crates/ruff_python_formatter/src/comments/placement.rs +++ b/crates/ruff_python_formatter/src/comments/placement.rs @@ -1,15 +1,13 @@ use std::cmp::Ordering; use ruff_python_ast::whitespace::indentation; -use ruff_python_ast::{ - self as ast, AnyNodeRef, Comprehension, Expr, MatchCase, ModModule, Parameters, -}; +use ruff_python_ast::{self as ast, AnyNodeRef, Comprehension, Expr, ModModule, Parameters}; use ruff_python_trivia::{ find_only_token_in_range, indentation_at_offset, BackwardsTokenizer, CommentRanges, SimpleToken, SimpleTokenKind, SimpleTokenizer, }; use ruff_source_file::Locator; -use ruff_text_size::{Ranged, TextLen, TextRange, TextSize}; +use ruff_text_size::{Ranged, TextLen, TextRange}; use crate::comments::visitor::{CommentPlacement, DecoratedComment}; use crate::expression::expr_generator_exp::is_generator_parenthesized; @@ -344,7 +342,7 @@ fn handle_end_of_line_comment_around_body<'a>( // pass // ``` if let Some(following) = comment.following_node() { - if is_first_statement_in_body(following, comment.enclosing_node()) + if following.is_first_statement_in_body(comment.enclosing_node()) && SimpleTokenizer::new( locator.contents(), TextRange::new(comment.end(), following.start()), @@ -381,86 +379,6 @@ fn handle_end_of_line_comment_around_body<'a>( CommentPlacement::Default(comment) } -/// Check if the given statement is the first statement after the colon of a branch, be it in if -/// statements, for statements, after each part of a try-except-else-finally or function/class -/// definitions. -/// -/// -/// ```python -/// if True: <- has body -/// a <- first statement -/// b -/// elif b: <- has body -/// c <- first statement -/// d -/// else: <- has body -/// e <- first statement -/// f -/// -/// class: <- has body -/// a: int <- first statement -/// b: int -/// -/// ``` -/// -/// For nodes with multiple bodies, we check all bodies that don't have their own node. For -/// try-except-else-finally, each except branch has it's own node, so for the `StmtTry`, we check -/// the `try:`, `else:` and `finally:`, bodies, while `ExceptHandlerExceptHandler` has it's own -/// check. For for-else and while-else, we check both branches for the whole statement. -/// -/// ```python -/// try: <- has body (a) -/// 6/8 <- first statement (a) -/// 1/0 -/// except: <- has body (b) -/// a <- first statement (b) -/// b -/// else: -/// c <- first statement (a) -/// d -/// finally: -/// e <- first statement (a) -/// f -/// ``` -fn is_first_statement_in_body(statement: AnyNodeRef, has_body: AnyNodeRef) -> bool { - match has_body { - AnyNodeRef::StmtFor(ast::StmtFor { body, orelse, .. }) - | AnyNodeRef::StmtWhile(ast::StmtWhile { body, orelse, .. }) => { - are_same_optional(statement, body.first()) - || are_same_optional(statement, orelse.first()) - } - - AnyNodeRef::StmtTry(ast::StmtTry { - body, - orelse, - finalbody, - .. - }) => { - are_same_optional(statement, body.first()) - || are_same_optional(statement, orelse.first()) - || are_same_optional(statement, finalbody.first()) - } - - AnyNodeRef::StmtIf(ast::StmtIf { body, .. }) - | AnyNodeRef::ElifElseClause(ast::ElifElseClause { body, .. }) - | AnyNodeRef::StmtWith(ast::StmtWith { body, .. }) - | AnyNodeRef::ExceptHandlerExceptHandler(ast::ExceptHandlerExceptHandler { - body, .. - }) - | AnyNodeRef::MatchCase(MatchCase { body, .. }) - | AnyNodeRef::StmtFunctionDef(ast::StmtFunctionDef { body, .. }) - | AnyNodeRef::StmtClassDef(ast::StmtClassDef { body, .. }) => { - are_same_optional(statement, body.first()) - } - - AnyNodeRef::StmtMatch(ast::StmtMatch { cases, .. }) => { - are_same_optional(statement, cases.first()) - } - - _ => false, - } -} - /// Handles own-line comments around a body (at the end of the body, at the end of the header /// preceding the body, or between bodies): /// @@ -612,13 +530,14 @@ fn handle_own_line_comment_between_branches<'a>( let Some(following) = comment.following_node() else { return CommentPlacement::Default(comment); }; - if !is_first_statement_in_alternate_body(following, comment.enclosing_node()) { + if !following.is_first_statement_in_alternate_body(comment.enclosing_node()) { return CommentPlacement::Default(comment); } // It depends on the indentation level of the comment if it is a leading comment for the // following branch or if it a trailing comment of the previous body's last statement. - let comment_indentation = own_line_comment_indentation(preceding, &comment, locator); + let comment_indentation = + AnyNodeRef::comment_indentation_after(preceding, comment.range(), locator); let preceding_indentation = indentation(locator, &preceding) .unwrap_or_default() @@ -702,7 +621,8 @@ fn handle_own_line_comment_after_branch<'a>( // We only care about the length because indentations with mixed spaces and tabs are only valid if // the indent-level doesn't depend on the tab width (the indent level must be the same if the tab width is 1 or 8). - let comment_indentation = own_line_comment_indentation(preceding, &comment, locator); + let comment_indentation = + AnyNodeRef::comment_indentation_after(preceding, comment.range(), locator); // Keep the comment on the entire statement in case it's a trailing comment // ```python @@ -778,80 +698,6 @@ fn handle_own_line_comment_after_branch<'a>( } } -/// Determine the indentation level of an own-line comment, defined as the minimum indentation of -/// all comments between the preceding node and the comment, including the comment itself. In -/// other words, we don't allow successive comments to ident _further_ than any preceding comments. -/// -/// For example, given: -/// ```python -/// if True: -/// pass -/// # comment -/// ``` -/// -/// The indentation would be 4, as the comment is indented by 4 spaces. -/// -/// Given: -/// ```python -/// if True: -/// pass -/// # comment -/// else: -/// pass -/// ``` -/// -/// The indentation would be 0, as the comment is not indented at all. -/// -/// Given: -/// ```python -/// if True: -/// pass -/// # comment -/// # comment -/// ``` -/// -/// Both comments would be marked as indented at 4 spaces, as the indentation of the first comment -/// is used for the second comment. -/// -/// This logic avoids pathological cases like: -/// ```python -/// try: -/// if True: -/// if True: -/// pass -/// -/// # a -/// # b -/// # c -/// except Exception: -/// pass -/// ``` -/// -/// If we don't use the minimum indentation of any preceding comments, we would mark `# b` as -/// indented to the same depth as `pass`, which could in turn lead to us treating it as a trailing -/// comment of `pass`, despite there being a comment between them that "resets" the indentation. -fn own_line_comment_indentation( - preceding: AnyNodeRef, - comment: &DecoratedComment, - locator: &Locator, -) -> TextSize { - let tokenizer = SimpleTokenizer::new( - locator.contents(), - TextRange::new(locator.full_line_end(preceding.end()), comment.end()), - ); - - tokenizer - .filter_map(|token| { - if token.kind() == SimpleTokenKind::Comment { - indentation_at_offset(token.start(), locator).map(TextLen::text_len) - } else { - None - } - }) - .min() - .unwrap_or_default() -} - /// Attaches comments for the positional-only parameters separator `/` or the keywords-only /// parameters separator `*` as dangling comments to the enclosing [`Parameters`] node. /// @@ -2194,40 +2040,6 @@ fn handle_comprehension_comment<'a>( CommentPlacement::Default(comment) } -/// Returns `true` if `right` is `Some` and `left` and `right` are referentially equal. -fn are_same_optional<'a, T>(left: AnyNodeRef, right: Option) -> bool -where - T: Into>, -{ - right.is_some_and(|right| left.ptr_eq(right.into())) -} - -/// Returns `true` if `statement` is the first statement in an alternate `body` (e.g. the else of an if statement) -fn is_first_statement_in_alternate_body(statement: AnyNodeRef, has_body: AnyNodeRef) -> bool { - match has_body { - AnyNodeRef::StmtFor(ast::StmtFor { orelse, .. }) - | AnyNodeRef::StmtWhile(ast::StmtWhile { orelse, .. }) => { - are_same_optional(statement, orelse.first()) - } - - AnyNodeRef::StmtTry(ast::StmtTry { - handlers, - orelse, - finalbody, - .. - }) => { - are_same_optional(statement, handlers.first()) - || are_same_optional(statement, orelse.first()) - || are_same_optional(statement, finalbody.first()) - } - - AnyNodeRef::StmtIf(ast::StmtIf { - elif_else_clauses, .. - }) => are_same_optional(statement, elif_else_clauses.first()), - _ => false, - } -} - /// Returns `true` if the parameters are parenthesized (as in a function definition), or `false` if /// not (as in a lambda). fn are_parameters_parenthesized(parameters: &Parameters, contents: &str) -> bool { From 8fc186ee0cbf6ab745ec1dc20f1b0f9a29563706 Mon Sep 17 00:00:00 2001 From: Jane Lewis Date: Wed, 28 Feb 2024 11:12:05 -0800 Subject: [PATCH 13/13] Fix trailing comment visitation and add coverage for trailing skip comments on function definitions --- .../resources/test/fixtures/ruff/RUF028.py | 3 +- .../invalid_formatter_suppression_comment.rs | 9 +-- .../ruff/rules/suppression_comment_visitor.rs | 51 +++++++----- ..._rules__ruff__tests__RUF028_RUF028.py.snap | 69 ++++++---------- crates/ruff_python_ast/src/helpers.rs | 78 ++++++++++++++++++- crates/ruff_python_ast/src/node.rs | 78 +------------------ .../src/comments/placement.rs | 7 +- 7 files changed, 141 insertions(+), 154 deletions(-) diff --git a/crates/ruff_linter/resources/test/fixtures/ruff/RUF028.py b/crates/ruff_linter/resources/test/fixtures/ruff/RUF028.py index 5409ddf5fbc3e..47100c2b3ef90 100644 --- a/crates/ruff_linter/resources/test/fixtures/ruff/RUF028.py +++ b/crates/ruff_linter/resources/test/fixtures/ruff/RUF028.py @@ -53,8 +53,7 @@ class Test: def cls_method_a( # fmt: off cls, - ) -> None: - # noqa: test # fmt: skip + ) -> None: # noqa: test # fmt: skip pass diff --git a/crates/ruff_linter/src/rules/ruff/rules/invalid_formatter_suppression_comment.rs b/crates/ruff_linter/src/rules/ruff/rules/invalid_formatter_suppression_comment.rs index cbda57903a98a..f72e87a330d74 100644 --- a/crates/ruff_linter/src/rules/ruff/rules/invalid_formatter_suppression_comment.rs +++ b/crates/ruff_linter/src/rules/ruff/rules/invalid_formatter_suppression_comment.rs @@ -3,7 +3,7 @@ use std::fmt::Display; use ast::{StmtClassDef, StmtFunctionDef}; use ruff_diagnostics::{AlwaysFixableViolation, Diagnostic, Fix}; use ruff_macros::{derive_message_formats, violation}; -use ruff_python_ast::{self as ast, AnyNodeRef}; +use ruff_python_ast::{self as ast, helpers::comment_indentation_after, AnyNodeRef}; use ruff_python_trivia::{indentation_at_offset, SuppressionKind}; use ruff_source_file::Locator; use ruff_text_size::{Ranged, TextLen, TextRange}; @@ -168,11 +168,8 @@ impl<'src, 'loc> UselessSuppressionComments<'src, 'loc> { { if following.is_first_statement_in_alternate_body(enclosing) { // check indentation - let comment_indentation = AnyNodeRef::comment_indentation_after( - preceding, - comment.range, - self.locator, - ); + let comment_indentation = + comment_indentation_after(preceding, comment.range, self.locator); let preceding_indentation = indentation_at_offset(preceding.start(), self.locator) diff --git a/crates/ruff_linter/src/rules/ruff/rules/suppression_comment_visitor.rs b/crates/ruff_linter/src/rules/ruff/rules/suppression_comment_visitor.rs index 1dbe4c1a40394..fe7f6ffa61994 100644 --- a/crates/ruff_linter/src/rules/ruff/rules/suppression_comment_visitor.rs +++ b/crates/ruff_linter/src/rules/ruff/rules/suppression_comment_visitor.rs @@ -1,6 +1,7 @@ use std::iter::Peekable; use ruff_python_ast::{ + helpers::comment_indentation_after, visitor::preorder::{self, PreorderVisitor, TraversalSignal}, AnyNodeRef, Suite, }; @@ -114,32 +115,44 @@ where while let Some(SuppressionComment { range, kind }) = self.comments.peek().copied() { let line_position = CommentLinePosition::for_range(range, self.locator.contents()); if range.start() >= node_end { + let between = TextRange::new(node_end, range.start()); + // Check if a non-trivial token exists between the end of this node and the start of the comment. + // If it doesn't, that means this comment could possibly be a trailing comment that comes after the + // end of this node. + // For example: + // ``` + // def func(x): + // pass # fmt: skip + // ``` + // We want to make sure that `# fmt: skip` is associated with the `pass` statement, + // even though it comes after the end of that node. + if SimpleTokenizer::new(self.locator.contents(), between) + .skip_trivia() + .next() + .is_some() + { + break; + } + // If the comment is on its own line, it could still be a trailing comment if it has a greater + // level of indentation compared to this node. For example: + // ``` + // def func(x): + // # fmt: off + // pass + // # fmt: on + // def func2(y): + // pass + // ``` + // We want `# fmt: on` to be considered a trailing comment of `func(x)` instead of a leading comment + // on `func2(y)`. if line_position.is_own_line() { - if let Some(token) = - SimpleTokenizer::starts_at(node_end, self.locator.contents()) - .skip_trivia() - .next() - { - if token.end() <= range.end() { - break; - } - } - let comment_indent = - AnyNodeRef::comment_indentation_after(node, range, self.locator); + let comment_indent = comment_indentation_after(node, range, self.locator); let node_indent = TextSize::of( indentation_at_offset(node.start(), self.locator).unwrap_or_default(), ); if node_indent >= comment_indent { break; } - } else { - // If the end-of-line comment is not on the same line, we can assume that there's a node in between - // the end of this node and this comment. - if self.locator.line_start(range.start()) - != self.locator.line_start(node.start()) - { - break; - } } } diff --git a/crates/ruff_linter/src/rules/ruff/snapshots/ruff_linter__rules__ruff__tests__RUF028_RUF028.py.snap b/crates/ruff_linter/src/rules/ruff/snapshots/ruff_linter__rules__ruff__tests__RUF028_RUF028.py.snap index 4fd416657b520..400fe6637ef52 100644 --- a/crates/ruff_linter/src/rules/ruff/snapshots/ruff_linter__rules__ruff__tests__RUF028_RUF028.py.snap +++ b/crates/ruff_linter/src/rules/ruff/snapshots/ruff_linter__rules__ruff__tests__RUF028_RUF028.py.snap @@ -164,7 +164,7 @@ RUF028.py:54:9: RUF028 [*] This suppression comment is invalid because it cannot 54 | # fmt: off | ^^^^^^^^^^ RUF028 55 | cls, -56 | ) -> None: +56 | ) -> None: # noqa: test # fmt: skip | = help: Remove this comment @@ -174,60 +174,41 @@ RUF028.py:54:9: RUF028 [*] This suppression comment is invalid because it cannot 53 53 | def cls_method_a( 54 |- # fmt: off 55 54 | cls, -56 55 | ) -> None: -57 56 | # noqa: test # fmt: skip +56 55 | ) -> None: # noqa: test # fmt: skip +57 56 | pass -RUF028.py:57:9: RUF028 [*] This suppression comment is invalid because it cannot be on its own line +RUF028.py:62:13: RUF028 [*] This suppression comment is invalid because it cannot be at the end of a line | -55 | cls, -56 | ) -> None: -57 | # noqa: test # fmt: skip - | ^^^^^^^^^^^^^^^^^^^^^^^^ RUF028 -58 | pass - | - = help: Remove this comment - -ℹ Unsafe fix -54 54 | # fmt: off -55 55 | cls, -56 56 | ) -> None: -57 |- # noqa: test # fmt: skip -58 57 | pass -59 58 | -60 59 | - -RUF028.py:63:13: RUF028 [*] This suppression comment is invalid because it cannot be at the end of a line - | -61 | def fmt_on_trailing(): -62 | # fmt: off -63 | val = 5 # fmt: on +60 | def fmt_on_trailing(): +61 | # fmt: off +62 | val = 5 # fmt: on | ^^^^^^^^^ RUF028 -64 | pass # fmt: on +63 | pass # fmt: on | = help: Remove this comment ℹ Unsafe fix -60 60 | -61 61 | def fmt_on_trailing(): -62 62 | # fmt: off -63 |- val = 5 # fmt: on - 63 |+ val = 5 -64 64 | pass # fmt: on - -RUF028.py:64:10: RUF028 [*] This suppression comment is invalid because it cannot be at the end of a line - | -62 | # fmt: off -63 | val = 5 # fmt: on -64 | pass # fmt: on +59 59 | +60 60 | def fmt_on_trailing(): +61 61 | # fmt: off +62 |- val = 5 # fmt: on + 62 |+ val = 5 +63 63 | pass # fmt: on + +RUF028.py:63:10: RUF028 [*] This suppression comment is invalid because it cannot be at the end of a line + | +61 | # fmt: off +62 | val = 5 # fmt: on +63 | pass # fmt: on | ^^^^^^^^^ RUF028 | = help: Remove this comment ℹ Unsafe fix -61 61 | def fmt_on_trailing(): -62 62 | # fmt: off -63 63 | val = 5 # fmt: on -64 |- pass # fmt: on - 64 |+ pass +60 60 | def fmt_on_trailing(): +61 61 | # fmt: off +62 62 | val = 5 # fmt: on +63 |- pass # fmt: on + 63 |+ pass diff --git a/crates/ruff_python_ast/src/helpers.rs b/crates/ruff_python_ast/src/helpers.rs index 154be660d37ae..d80bfef769197 100644 --- a/crates/ruff_python_ast/src/helpers.rs +++ b/crates/ruff_python_ast/src/helpers.rs @@ -4,9 +4,9 @@ use std::path::Path; use rustc_hash::FxHashMap; use smallvec::SmallVec; -use ruff_python_trivia::CommentRanges; +use ruff_python_trivia::{indentation_at_offset, CommentRanges, SimpleTokenKind, SimpleTokenizer}; use ruff_source_file::Locator; -use ruff_text_size::{Ranged, TextRange}; +use ruff_text_size::{Ranged, TextLen, TextRange, TextSize}; use crate::call_path::CallPath; use crate::parenthesize::parenthesized_range; @@ -1481,6 +1481,80 @@ pub fn typing_union(elts: &[Expr], binding: String) -> Expr { }) } +/// Determine the indentation level of an own-line comment, defined as the minimum indentation of +/// all comments between the preceding node and the comment, including the comment itself. In +/// other words, we don't allow successive comments to ident _further_ than any preceding comments. +/// +/// For example, given: +/// ```python +/// if True: +/// pass +/// # comment +/// ``` +/// +/// The indentation would be 4, as the comment is indented by 4 spaces. +/// +/// Given: +/// ```python +/// if True: +/// pass +/// # comment +/// else: +/// pass +/// ``` +/// +/// The indentation would be 0, as the comment is not indented at all. +/// +/// Given: +/// ```python +/// if True: +/// pass +/// # comment +/// # comment +/// ``` +/// +/// Both comments would be marked as indented at 4 spaces, as the indentation of the first comment +/// is used for the second comment. +/// +/// This logic avoids pathological cases like: +/// ```python +/// try: +/// if True: +/// if True: +/// pass +/// +/// # a +/// # b +/// # c +/// except Exception: +/// pass +/// ``` +/// +/// If we don't use the minimum indentation of any preceding comments, we would mark `# b` as +/// indented to the same depth as `pass`, which could in turn lead to us treating it as a trailing +/// comment of `pass`, despite there being a comment between them that "resets" the indentation. +pub fn comment_indentation_after( + preceding: AnyNodeRef, + comment_range: TextRange, + locator: &Locator, +) -> TextSize { + let tokenizer = SimpleTokenizer::new( + locator.contents(), + TextRange::new(locator.full_line_end(preceding.end()), comment_range.end()), + ); + + tokenizer + .filter_map(|token| { + if token.kind() == SimpleTokenKind::Comment { + indentation_at_offset(token.start(), locator).map(TextLen::text_len) + } else { + None + } + }) + .min() + .unwrap_or_default() +} + #[cfg(test)] mod tests { use std::borrow::Cow; diff --git a/crates/ruff_python_ast/src/node.rs b/crates/ruff_python_ast/src/node.rs index 27528e44de86d..16b19622c36d8 100644 --- a/crates/ruff_python_ast/src/node.rs +++ b/crates/ruff_python_ast/src/node.rs @@ -5,9 +5,7 @@ use crate::{ PatternArguments, PatternKeyword, Stmt, TypeParam, TypeParamParamSpec, TypeParamTypeVar, TypeParamTypeVarTuple, TypeParams, WithItem, }; -use ruff_python_trivia::{indentation_at_offset, SimpleTokenKind, SimpleTokenizer}; -use ruff_source_file::Locator; -use ruff_text_size::{Ranged, TextLen, TextRange, TextSize}; +use ruff_text_size::{Ranged, TextRange}; use std::ptr::NonNull; pub trait AstNode: Ranged { @@ -6304,80 +6302,6 @@ impl<'a> AnyNodeRef<'a> { _ => false, } } - - /// Determine the indentation level of an own-line comment, defined as the minimum indentation of - /// all comments between the preceding node and the comment, including the comment itself. In - /// other words, we don't allow successive comments to ident _further_ than any preceding comments. - /// - /// For example, given: - /// ```python - /// if True: - /// pass - /// # comment - /// ``` - /// - /// The indentation would be 4, as the comment is indented by 4 spaces. - /// - /// Given: - /// ```python - /// if True: - /// pass - /// # comment - /// else: - /// pass - /// ``` - /// - /// The indentation would be 0, as the comment is not indented at all. - /// - /// Given: - /// ```python - /// if True: - /// pass - /// # comment - /// # comment - /// ``` - /// - /// Both comments would be marked as indented at 4 spaces, as the indentation of the first comment - /// is used for the second comment. - /// - /// This logic avoids pathological cases like: - /// ```python - /// try: - /// if True: - /// if True: - /// pass - /// - /// # a - /// # b - /// # c - /// except Exception: - /// pass - /// ``` - /// - /// If we don't use the minimum indentation of any preceding comments, we would mark `# b` as - /// indented to the same depth as `pass`, which could in turn lead to us treating it as a trailing - /// comment of `pass`, despite there being a comment between them that "resets" the indentation. - pub fn comment_indentation_after( - preceding: Self, - comment_range: TextRange, - locator: &Locator, - ) -> TextSize { - let tokenizer = SimpleTokenizer::new( - locator.contents(), - TextRange::new(locator.full_line_end(preceding.end()), comment_range.end()), - ); - - tokenizer - .filter_map(|token| { - if token.kind() == SimpleTokenKind::Comment { - indentation_at_offset(token.start(), locator).map(TextLen::text_len) - } else { - None - } - }) - .min() - .unwrap_or_default() - } } /// Returns `true` if `right` is `Some` and `left` and `right` are referentially equal. diff --git a/crates/ruff_python_formatter/src/comments/placement.rs b/crates/ruff_python_formatter/src/comments/placement.rs index 0b50e819760ce..c8b5af6533f1a 100644 --- a/crates/ruff_python_formatter/src/comments/placement.rs +++ b/crates/ruff_python_formatter/src/comments/placement.rs @@ -1,5 +1,6 @@ use std::cmp::Ordering; +use ast::helpers::comment_indentation_after; use ruff_python_ast::whitespace::indentation; use ruff_python_ast::{self as ast, AnyNodeRef, Comprehension, Expr, ModModule, Parameters}; use ruff_python_trivia::{ @@ -536,8 +537,7 @@ fn handle_own_line_comment_between_branches<'a>( // It depends on the indentation level of the comment if it is a leading comment for the // following branch or if it a trailing comment of the previous body's last statement. - let comment_indentation = - AnyNodeRef::comment_indentation_after(preceding, comment.range(), locator); + let comment_indentation = comment_indentation_after(preceding, comment.range(), locator); let preceding_indentation = indentation(locator, &preceding) .unwrap_or_default() @@ -621,8 +621,7 @@ fn handle_own_line_comment_after_branch<'a>( // We only care about the length because indentations with mixed spaces and tabs are only valid if // the indent-level doesn't depend on the tab width (the indent level must be the same if the tab width is 1 or 8). - let comment_indentation = - AnyNodeRef::comment_indentation_after(preceding, comment.range(), locator); + let comment_indentation = comment_indentation_after(preceding, comment.range(), locator); // Keep the comment on the entire statement in case it's a trailing comment // ```python