Address some suggestions

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,
-        )
-    }
-}

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<Item = &'a str>,

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,15 +88,13 @@ pub(crate) fn ignored_formatter_noqa(checker: &mut Checker, suite: &ast::Suite)
 
 struct UselessSuppressionComments<'src, 'loc> {
     captured: BTreeMap<SuppressionCommentData<'src>, IgnoredReason>,
-    comments_in_scope: Vec<(Option<AnyNodeRef<'src>>, SuppressionKind)>,
     locator: &'loc Locator<'src>,
 }
 
 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<T>) -> bool
 where

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<dyn Fn(&'src TextRange) -> Option<SuppressionComment> + 'src>;
-type CommentIter<'src> = Peekable<FilterMap<std::slice::Iter<'src, TextRange>, MapCommentFn<'src>>>;
+fn suppression_comments<'src>(
+    ranges: &'src CommentRanges,
+    locator: &'src Locator<'src>,
+) -> Box<dyn Iterator<Item = SuppressionComment> + '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<Box<dyn Iterator<Item = SuppressionComment> + 'src>>,
 
     parents: Vec<AnyNodeRef<'src>>,
     preceding_node: Option<AnyNodeRef<'src>>,
-    comments_in_scope: Vec<(Option<AnyNodeRef<'src>>, 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,23 +87,22 @@ 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();
 
             let data = SuppressionCommentData {
                 enclosing: enclosing_node,
                 preceding: self.preceding_node,
                 following: Some(node),
-                parent: self.parents.iter().rev().nth(1).copied(),
                 previous_state,
                 line_position,
                 kind,
                 range,
             };
 
             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,22 +149,21 @@ 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,
                 range,
             };
 
             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<AnyNodeRef<'src>>,
     pub(super) preceding: Option<AnyNodeRef<'src>>,
     pub(super) following: Option<AnyNodeRef<'src>>,
-    pub(super) parent: Option<AnyNodeRef<'src>>,
     pub(super) previous_state: Option<SuppressionKind>,
     pub(super) line_position: CommentLinePosition,
     pub(super) kind: SuppressionKind,

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;

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(),
                 ),

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)]