From 0293908b71eeaf610d8ce77d2950f0602fd88dc5 Mon Sep 17 00:00:00 2001 From: Jane Lewis Date: Wed, 28 Feb 2024 11:21:06 -0800 Subject: [PATCH] Implement RUF028 to detect useless formatter suppression comments (#9899) Fixes #6611 ## Summary This lint rule spots comments that are _intended_ to suppress or enable the formatter, but will be ignored by the Ruff formatter. We borrow some functions the formatter uses for determining comment placement / putting them in context within an AST. The analysis function uses an AST visitor to visit each comment and attach it to the AST. It then uses that context to check: 1. Is this comment in an expression? 2. Does this comment have bad placement? (e.g. a `# fmt: skip` above a function instead of at the end of a line) 3. Is this comment redundant? 4. Does this comment actually suppress any code? 5. Does this comment have ambiguous placement? (e.g. a `# fmt: off` above an `else:` block) If any of these are true, a violation is thrown. The reported reason depends on the order of the above check-list: in other words, a `# fmt: skip` comment on its own line within a list expression will be reported as being in an expression, since that reason takes priority. The lint suggests removing the comment as an unsafe fix, regardless of the reason. ## Test Plan A snapshot test has been created. --- .../resources/test/fixtures/ruff/RUF028.py | 63 +++++ .../src/checkers/ast/analyze/module.rs | 5 +- crates/ruff_linter/src/checkers/noqa.rs | 59 +---- crates/ruff_linter/src/codes.rs | 1 + crates/ruff_linter/src/fix/edits.rs | 45 ++++ crates/ruff_linter/src/rules/ruff/mod.rs | 1 + .../invalid_formatter_suppression_comment.rs | 245 ++++++++++++++++++ .../ruff_linter/src/rules/ruff/rules/mod.rs | 3 + .../ruff/rules/suppression_comment_visitor.rs | 217 ++++++++++++++++ ..._rules__ruff__tests__RUF028_RUF028.py.snap | 214 +++++++++++++++ crates/ruff_python_ast/src/helpers.rs | 78 +++++- crates/ruff_python_ast/src/node.rs | 114 ++++++++ .../src/comments/debug.rs | 4 +- .../src/comments/format.rs | 4 +- .../ruff_python_formatter/src/comments/mod.rs | 123 ++------- .../src/comments/placement.rs | 203 +-------------- .../src/comments/visitor.rs | 30 +-- crates/ruff_python_formatter/src/lib.rs | 3 +- .../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/comments.rs | 121 +++++++++ crates/ruff_python_trivia/src/lib.rs | 2 + ruff.schema.json | 1 + 41 files changed, 1215 insertions(+), 438 deletions(-) create mode 100644 crates/ruff_linter/resources/test/fixtures/ruff/RUF028.py create mode 100644 crates/ruff_linter/src/rules/ruff/rules/invalid_formatter_suppression_comment.rs create mode 100644 crates/ruff_linter/src/rules/ruff/rules/suppression_comment_visitor.rs create mode 100644 crates/ruff_linter/src/rules/ruff/snapshots/ruff_linter__rules__ruff__tests__RUF028_RUF028.py.snap create mode 100644 crates/ruff_python_trivia/src/comments.rs 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..47100c2b3ef90 --- /dev/null +++ b/crates/ruff_linter/resources/test/fixtures/ruff/RUF028.py @@ -0,0 +1,63 @@ +def fmt_off_between_lists(): + test_list = [ + # fmt: off + 1, + 2, + 3, + ] + + +# note: the second `fmt: skip`` should be OK +def fmt_skip_on_own_line(): + # fmt: skip + pass # fmt: skip + + +@fmt_skip_on_own_line +# fmt: off +@fmt_off_between_lists +def fmt_off_between_decorators(): + pass + + +@fmt_off_between_decorators +# fmt: off +class FmtOffBetweenClassDecorators: + ... + + +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: off + # fmt: off + else: + print("done") + if len(x) > 3: + print("huh?") + # fmt: on + # fmt: off + 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 + 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 139d6c5294783..e80e052b55b3c 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::InvalidFormatterSuppressionComment) { + ruff::rules::ignored_formatter_suppression_comment(checker, suite); + } } diff --git a/crates/ruff_linter/src/checkers/noqa.rs b/crates/ruff_linter/src/checkers/noqa.rs index d5d4276abb27a..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}; @@ -111,7 +112,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 +179,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(", ")), @@ -195,48 +199,3 @@ pub(crate) fn check_noqa( ignored_diagnostics.sort_unstable(); ignored_diagnostics } - -/// Generate a [`Edit`] to delete a `noqa` directive. -fn delete_noqa(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/codes.rs b/crates/ruff_linter/src/codes.rs index 06c9cff0dfd95..d313c6816b800 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::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/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/mod.rs b/crates/ruff_linter/src/rules/ruff/mod.rs index 7239323bcbddb..1b06577295d13 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::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/invalid_formatter_suppression_comment.rs b/crates/ruff_linter/src/rules/ruff/rules/invalid_formatter_suppression_comment.rs new file mode 100644 index 0000000000000..f72e87a330d74 --- /dev/null +++ b/crates/ruff_linter/src/rules/ruff/rules/invalid_formatter_suppression_comment.rs @@ -0,0 +1,245 @@ +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, 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}; +use smallvec::SmallVec; + +use crate::checkers::ast::Checker; +use crate::fix::edits::delete_comment; + +use super::suppression_comment_visitor::{ + CaptureSuppressionComment, SuppressionComment, 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: +/// # fmt: skip +/// expression = [ +/// # fmt: off +/// 1, +/// 2, +/// ] +/// # yapf: disable +/// # fmt: on +/// # yapf: enable +/// ``` +#[violation] +pub struct InvalidFormatterSuppressionComment { + reason: IgnoredReason, +} + +impl AlwaysFixableViolation for InvalidFormatterSuppressionComment { + #[derive_message_formats] + fn message(&self) -> String { + format!( + "This suppression comment is invalid because {}", + self.reason + ) + } + + fn fix_title(&self) -> String { + format!("Remove this comment") + } +} + +/// RUF028 +pub(crate) fn ignored_formatter_suppression_comment(checker: &mut Checker, suite: &ast::Suite) { + let indexer = checker.indexer(); + let locator = checker.locator(); + 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.into_iter(), + &mut comments, + checker.locator(), + ); + + visitor.visit(suite); + + comments.sort(); + + for (range, reason) in comments.ignored_comments() { + checker.diagnostics.push( + Diagnostic::new(InvalidFormatterSuppressionComment { reason }, range) + .with_fix(Fix::unsafe_edit(delete_comment(range, checker.locator()))), + ); + } +} + +struct UselessSuppressionComments<'src, 'loc> { + captured: Vec<(TextRange, IgnoredReason)>, + locator: &'loc Locator<'src>, +} + +impl<'src, 'loc> UselessSuppressionComments<'src, 'loc> { + fn new(locator: &'loc Locator<'src>) -> Self { + Self { + captured: 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 ignored. See [`IgnoredReason`] for more. + fn check_suppression_comment( + &self, + comment: &SuppressionCommentData, + ) -> Result<(), IgnoredReason> { + // check if the comment is inside of an expression. + if comment + .enclosing + .map(|n| !AnyNodeRef::is_statement(n)) + .unwrap_or_default() + { + 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() { + return Err(IgnoredReason::SkipHasToBeTrailing); + } + + 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.first() { + if decorator.end() < comment.range.start() { + return Err(IgnoredReason::AfterDecorator); + } + } + } + } + } + + 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 following.is_first_statement_in_alternate_body(enclosing) { + // check indentation + let comment_indentation = + comment_indentation_after(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); + } + } + } + } + + if comment.kind == SuppressionKind::On { + // Ensure the comment is not a trailing comment + if !comment.line_position.is_own_line() { + return Err(IgnoredReason::FmtOnCannotBeTrailing); + } + } + + Ok(()) + } + + fn sort(&mut self) { + self.captured.sort_by_key(|(t, _)| t.start()); + } + + 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>) { + match self.check_suppression_comment(&comment) { + Ok(()) => {} + Err(reason) => { + self.captured.push((comment.range, reason)); + } + } + } +} + +#[derive(Clone, Copy, Debug, PartialEq, Eq)] +enum IgnoredReason { + InNonStatement, + AfterDecorator, + SkipHasToBeTrailing, + FmtOnCannotBeTrailing, + FmtOffAboveBlock, +} + +impl Display for IgnoredReason { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + match self { + 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") + } + Self::FmtOnCannotBeTrailing => { + write!(f, "it cannot be at the end of a line") + } + Self::FmtOffAboveBlock => { + write!(f, "it cannot be directly above an alternate body") + } + } + } +} diff --git a/crates/ruff_linter/src/rules/ruff/rules/mod.rs b/crates/ruff_linter/src/rules/ruff/rules/mod.rs index b8d7fca925135..d4461f1f3b6af 100644 --- a/crates/ruff_linter/src/rules/ruff/rules/mod.rs +++ b/crates/ruff_linter/src/rules/ruff/rules/mod.rs @@ -6,6 +6,7 @@ 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 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,6 +37,7 @@ mod explicit_f_string_type_conversion; mod function_call_in_dataclass_default; mod helpers; mod implicit_optional; +mod invalid_formatter_suppression_comment; mod invalid_index_type; mod invalid_pyproject_toml; mod missing_fstring_syntax; @@ -50,6 +52,7 @@ 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; 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..fe7f6ffa61994 --- /dev/null +++ b/crates/ruff_linter/src/rules/ruff/rules/suppression_comment_visitor.rs @@ -0,0 +1,217 @@ +use std::iter::Peekable; + +use ruff_python_ast::{ + helpers::comment_indentation_after, + visitor::preorder::{self, PreorderVisitor, TraversalSignal}, + AnyNodeRef, Suite, +}; +use ruff_python_trivia::{ + indentation_at_offset, CommentLinePosition, SimpleTokenizer, SuppressionKind, +}; +use ruff_source_file::Locator; +use ruff_text_size::{Ranged, TextRange, TextSize}; + +#[derive(Clone, Copy, Debug)] +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, + I: Iterator + 'src, +> { + comments: Peekable, + + parents: Vec>, + preceding_node: Option>, + + builder: &'builder mut (dyn CaptureSuppressionComment<'src> + 'src), + locator: &'src Locator<'src>, +} + +impl<'src, 'builder, I> SuppressionCommentVisitor<'src, 'builder, I> +where + I: Iterator + 'src, +{ + pub(super) fn new( + comment_ranges: I, + builder: &'builder mut (dyn CaptureSuppressionComment<'src> + 'src), + locator: &'src Locator<'src>, + ) -> Self { + Self { + comments: comment_ranges.peekable(), + parents: Vec::default(), + preceding_node: Option::default(), + builder, + locator, + } + } + + 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, 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(); + + 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 line_position = CommentLinePosition::for_range(range, self.locator.contents()); + + let data = SuppressionCommentData { + enclosing: enclosing_node, + preceding: self.preceding_node, + following: Some(node), + line_position, + 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() { + 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() { + 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; + } + } + } + + let data = SuppressionCommentData { + enclosing: Some(node), + preceding: self.preceding_node, + following: None, + line_position, + 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)] +pub(super) struct SuppressionCommentData<'src> { + /// 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, +} + +pub(super) trait CaptureSuppressionComment<'src> { + /// This is the entrypoint for the capturer to analyze the next comment. + fn capture(&mut self, comment: SuppressionCommentData<'src>); +} 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..400fe6637ef52 --- /dev/null +++ b/crates/ruff_linter/src/rules/ruff/snapshots/ruff_linter__rules__ruff__tests__RUF028_RUF028.py.snap @@ -0,0 +1,214 @@ +--- +source: crates/ruff_linter/src/rules/ruff/mod.rs +--- +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 = [ +3 | # fmt: off + | ^^^^^^^^^^ RUF028 +4 | 1, +5 | 2, + | + = help: Remove this comment + +ℹ Unsafe fix +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:12:5: RUF028 [*] This suppression comment is invalid because it cannot be on its own line + | +10 | # note: the second `fmt: skip`` should be OK +11 | def fmt_skip_on_own_line(): +12 | # fmt: skip + | ^^^^^^^^^^^ RUF028 +13 | pass # fmt: skip + | + = help: Remove this comment + +ℹ Unsafe fix +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:17:1: RUF028 [*] This suppression comment is invalid because it cannot be after a decorator + | +16 | @fmt_skip_on_own_line +17 | # fmt: off + | ^^^^^^^^^^ RUF028 +18 | @fmt_off_between_lists +19 | def fmt_off_between_decorators(): + | + = help: Remove this comment + +ℹ Unsafe fix +14 14 | +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:24:1: RUF028 [*] This suppression comment is invalid because it cannot be after a decorator + | +23 | @fmt_off_between_decorators +24 | # fmt: off + | ^^^^^^^^^^ RUF028 +25 | class FmtOffBetweenClassDecorators: +26 | ... + | + = help: Remove this comment + +ℹ Unsafe fix +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 +34 | else: +35 | print("done") + | + = help: Remove this comment + +ℹ Unsafe fix +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 +40 | else: +41 | print("done") + | + = help: Remove this comment + +ℹ Unsafe fix +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 +46 | else: +47 | print("expected") + | + = help: Remove this comment + +ℹ Unsafe fix +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: # noqa: test # fmt: skip + | + = 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: # noqa: test # fmt: skip +57 56 | pass + +RUF028.py:62:13: RUF028 [*] This suppression comment is invalid because it cannot be at the end of a line + | +60 | def fmt_on_trailing(): +61 | # fmt: off +62 | val = 5 # fmt: on + | ^^^^^^^^^ RUF028 +63 | pass # fmt: on + | + = help: Remove this comment + +ℹ Unsafe fix +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 +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 844da603902c4..174be5f987bea 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; @@ -1484,6 +1484,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 90e4b61c545ec..5b727bcf37703 100644 --- a/crates/ruff_python_ast/src/node.rs +++ b/crates/ruff_python_ast/src/node.rs @@ -6198,6 +6198,120 @@ 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, + } + } +} + +/// 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/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..9ca2134f700d9 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)) + pub(crate) fn is_suppression_off_comment(&self, text: &str) -> bool { + SuppressionKind::is_suppression_off(self.text(text), 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)) + pub(crate) fn is_suppression_on_comment(&self, text: &str) -> bool { + SuppressionKind::is_suppression_on(self.text(text), 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 text<'a>(&self, text: &'a str) -> &'a str { + self.slice.text(SourceCode::new(text)) } } @@ -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. @@ -563,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.text(source)), + Some(SuppressionKind::Skip | SuppressionKind::Off) + ) + }) +} + #[cfg(test)] mod tests { use insta::assert_debug_snapshot; diff --git a/crates/ruff_python_formatter/src/comments/placement.rs b/crates/ruff_python_formatter/src/comments/placement.rs index 44698b0056880..babd7459f9249 100644 --- a/crates/ruff_python_formatter/src/comments/placement.rs +++ b/crates/ruff_python_formatter/src/comments/placement.rs @@ -1,15 +1,14 @@ 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, 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_slice::{assign_comment_in_slice, ExprSliceCommentSection}; @@ -342,7 +341,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()), @@ -379,86 +378,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): /// @@ -610,13 +529,13 @@ 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 = comment_indentation_after(preceding, comment.range(), locator); let preceding_indentation = indentation(locator, &preceding) .unwrap_or_default() @@ -700,7 +619,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 = own_line_comment_indentation(preceding, &comment, 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 @@ -776,80 +695,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. /// @@ -2192,40 +2037,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 { diff --git a/crates/ruff_python_formatter/src/comments/visitor.rs b/crates/ruff_python_formatter/src/comments/visitor.rs index f98aaabb169ef..ede7180107a3e 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::{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. @@ -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::for_range( + *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::for_range( + *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_formatter/src/lib.rs b/crates/ruff_python_formatter/src/lib.rs index 011ba245a91eb..c4cf4a16c3d4c 100644 --- a/crates/ruff_python_formatter/src/lib.rs +++ b/crates/ruff_python_formatter/src/lib.rs @@ -12,7 +12,8 @@ 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::{ 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/comments.rs b/crates/ruff_python_trivia/src/comments.rs new file mode 100644 index 0000000000000..6eb42c26f3074 --- /dev/null +++ b/crates/ruff_python_trivia/src/comments.rs @@ -0,0 +1,121 @@ +use ruff_text_size::TextRange; + +use crate::{is_python_whitespace, 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 { + /// 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 = 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), + "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 comment.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_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::Off)) + } +} +/// 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) + } + + /// 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())]; + + for c in before.chars().rev() { + match c { + '\n' | '\r' => { + break; + } + c if is_python_whitespace(c) => continue, + _ => return Self::EndOfLine, + } + } + Self::OwnLine + } +} diff --git a/crates/ruff_python_trivia/src/lib.rs b/crates/ruff_python_trivia/src/lib.rs index 9fed887c17ff4..782662170b6fa 100644 --- a/crates/ruff_python_trivia/src/lib.rs +++ b/crates/ruff_python_trivia/src/lib.rs @@ -1,4 +1,5 @@ mod comment_ranges; +mod comments; mod cursor; mod pragmas; pub mod textwrap; @@ -6,6 +7,7 @@ mod tokenizer; mod whitespace; pub use comment_ranges::CommentRanges; +pub use comments::*; pub use cursor::*; pub use pragmas::*; pub use tokenizer::*; diff --git a/ruff.schema.json b/ruff.schema.json index c0b1d6bb61502..8e17f813722a0 100644 --- a/ruff.schema.json +++ b/ruff.schema.json @@ -3531,6 +3531,7 @@ "RUF025", "RUF026", "RUF027", + "RUF028", "RUF1", "RUF10", "RUF100",