Visit Identifier node as part of the SourceOrderVisitor (#17110)

## Summary

I don't remember exactly when we made `Identifier` a node but it is now
considered a node (it implements `AnyNodeRef`, it has a range). However,
we never updated
the `SourceOrderVisitor` to visit identifiers because we never had a use
case for it and visiting new nodes can change how the formatter
associates comments (breaking change!).
This PR updates the `SourceOrderVisitor` to visit identifiers and
changes the formatter comment visitor to skip identifiers (updating the
visitor might be desired because it could help simplifying some comment
placement logic but this is out of scope for this PR).

## Test Plan

Tests, updated snapshot tests

crates/ruff_linter/src/rules/ruff/rules/suppression_comment_visitor.rs

@@ -195,6 +195,10 @@ where
             }
         }
     }
+
+    fn visit_identifier(&mut self, _identifier: &'ast ruff_python_ast::Identifier) {
+        // Skip identifiers, matching the formatter comment extraction
+    }
 }
 
 #[derive(Clone, Debug)]

crates/ruff_python_ast/src/node.rs

@@ -1,3 +1,5 @@
+use ruff_text_size::Ranged;
+
 use crate::visitor::source_order::SourceOrderVisitor;
 use crate::{
     self as ast, Alias, AnyNodeRef, AnyParameterRef, ArgOrKeyword, MatchCase, PatternArguments,
@@ -35,13 +37,17 @@ impl ast::StmtFunctionDef {
             decorator_list,
             returns,
             type_params,
-            ..
+            range: _,
+            is_async: _,
+            name,
         } = self;
 
         for decorator in decorator_list {
             visitor.visit_decorator(decorator);
         }
 
+        visitor.visit_identifier(name);
+
         if let Some(type_params) = type_params {
             visitor.visit_type_params(type_params);
         }
@@ -66,13 +72,16 @@ impl ast::StmtClassDef {
             body,
             decorator_list,
             type_params,
-            ..
+            name,
+            range: _,
         } = self;
 
         for decorator in decorator_list {
             visitor.visit_decorator(decorator);
         }
 
+        visitor.visit_identifier(name);
+
         if let Some(type_params) = type_params {
             visitor.visit_type_params(type_params);
         }
@@ -197,7 +206,8 @@ impl ast::StmtFor {
             iter,
             body,
             orelse,
-            ..
+            range: _,
+            is_async: _,
         } = self;
 
         visitor.visit_expr(target);
@@ -379,11 +389,15 @@ impl ast::StmtImportFrom {
     {
         let ast::StmtImportFrom {
             range: _,
-            module: _,
+            module,
             names,
             level: _,
         } = self;
 
+        if let Some(module) = module {
+            visitor.visit_identifier(module);
+        }
+
         for alias in names {
             visitor.visit_alias(alias);
         }
@@ -391,22 +405,28 @@ impl ast::StmtImportFrom {
 }
 
 impl ast::StmtGlobal {
-    #[inline]
-    pub(crate) fn visit_source_order<'a, V>(&'a self, _visitor: &mut V)
+    pub(crate) fn visit_source_order<'a, V>(&'a self, visitor: &mut V)
     where
         V: SourceOrderVisitor<'a> + ?Sized,
     {
-        let ast::StmtGlobal { range: _, names: _ } = self;
+        let ast::StmtGlobal { range: _, names } = self;
+
+        for name in names {
+            visitor.visit_identifier(name);
+        }
     }
 }
 
 impl ast::StmtNonlocal {
-    #[inline]
-    pub(crate) fn visit_source_order<'a, V>(&'a self, _visitor: &mut V)
+    pub(crate) fn visit_source_order<'a, V>(&'a self, visitor: &mut V)
     where
         V: SourceOrderVisitor<'a> + ?Sized,
     {
-        let ast::StmtNonlocal { range: _, names: _ } = self;
+        let ast::StmtNonlocal { range: _, names } = self;
+
+        for name in names {
+            visitor.visit_identifier(name);
+        }
     }
 }
 
@@ -879,12 +899,13 @@ impl ast::ExprAttribute {
     {
         let ast::ExprAttribute {
             value,
-            attr: _,
+            attr,
             ctx: _,
             range: _,
         } = self;
 
         visitor.visit_expr(value);
+        visitor.visit_identifier(attr);
     }
 }
 
@@ -1014,12 +1035,17 @@ impl ast::ExceptHandlerExceptHandler {
         let ast::ExceptHandlerExceptHandler {
             range: _,
             type_,
-            name: _,
+            name,
             body,
         } = self;
         if let Some(expr) = type_ {
             visitor.visit_expr(expr);
         }
+
+        if let Some(name) = name {
+            visitor.visit_identifier(name);
+        }
+
         visitor.visit_body(body);
     }
 }
@@ -1064,13 +1090,26 @@ impl ast::PatternMatchMapping {
         let ast::PatternMatchMapping {
             keys,
             patterns,
+            rest,
             range: _,
-            rest: _,
         } = self;
+
+        let mut rest = rest.as_ref();
+
         for (key, pattern) in keys.iter().zip(patterns) {
+            if let Some(rest_identifier) = rest {
+                if rest_identifier.start() < key.start() {
+                    visitor.visit_identifier(rest_identifier);
+                    rest = None;
+                }
+            }
             visitor.visit_expr(key);
             visitor.visit_pattern(pattern);
         }
+
+        if let Some(rest) = rest {
+            visitor.visit_identifier(rest);
+        }
     }
 }
 
@@ -1090,12 +1129,15 @@ impl ast::PatternMatchClass {
 }
 
 impl ast::PatternMatchStar {
-    #[inline]
-    pub(crate) fn visit_source_order<'a, V>(&'a self, _visitor: &mut V)
+    pub(crate) fn visit_source_order<'a, V>(&'a self, visitor: &mut V)
     where
         V: SourceOrderVisitor<'a> + ?Sized,
     {
-        let ast::PatternMatchStar { range: _, name: _ } = self;
+        let ast::PatternMatchStar { range: _, name } = self;
+
+        if let Some(name) = name {
+            visitor.visit_identifier(name);
+        }
     }
 }
 
@@ -1107,11 +1149,15 @@ impl ast::PatternMatchAs {
         let ast::PatternMatchAs {
             pattern,
             range: _,
-            name: _,
+            name,
         } = self;
         if let Some(pattern) = pattern {
             visitor.visit_pattern(pattern);
         }
+
+        if let Some(name) = name {
+            visitor.visit_identifier(name);
+        }
     }
 }
 
@@ -1155,10 +1201,11 @@ impl ast::PatternKeyword {
     {
         let PatternKeyword {
             range: _,
-            attr: _,
+            attr,
             pattern,
         } = self;
 
+        visitor.visit_identifier(attr);
         visitor.visit_pattern(pattern);
     }
 }
@@ -1221,10 +1268,11 @@ impl ast::Parameter {
     {
         let ast::Parameter {
             range: _,
-            name: _,
+            name,
             annotation,
         } = self;
 
+        visitor.visit_identifier(name);
         if let Some(expr) = annotation {
             visitor.visit_annotation(expr);
         }
@@ -1255,25 +1303,32 @@ impl ast::Keyword {
     {
         let ast::Keyword {
             range: _,
-            arg: _,
+            arg,
             value,
         } = self;
 
+        if let Some(arg) = arg {
+            visitor.visit_identifier(arg);
+        }
         visitor.visit_expr(value);
     }
 }
 
 impl Alias {
-    #[inline]
-    pub(crate) fn visit_source_order<'a, V>(&'a self, _visitor: &mut V)
+    pub(crate) fn visit_source_order<'a, V>(&'a self, visitor: &mut V)
     where
         V: SourceOrderVisitor<'a> + ?Sized,
     {
         let ast::Alias {
             range: _,
-            name: _,
-            asname: _,
+            name,
+            asname,
         } = self;
+
+        visitor.visit_identifier(name);
+        if let Some(asname) = asname {
+            visitor.visit_identifier(asname);
+        }
     }
 }
 
@@ -1354,10 +1409,11 @@ impl ast::TypeParamTypeVar {
         let ast::TypeParamTypeVar {
             bound,
             default,
-            name: _,
+            name,
             range: _,
         } = self;
 
+        visitor.visit_identifier(name);
         if let Some(expr) = bound {
             visitor.visit_expr(expr);
         }
@@ -1375,9 +1431,10 @@ impl ast::TypeParamTypeVarTuple {
     {
         let ast::TypeParamTypeVarTuple {
             range: _,
-            name: _,
+            name,
             default,
         } = self;
+        visitor.visit_identifier(name);
         if let Some(expr) = default {
             visitor.visit_expr(expr);
         }
@@ -1392,9 +1449,10 @@ impl ast::TypeParamParamSpec {
     {
         let ast::TypeParamParamSpec {
             range: _,
-            name: _,
+            name,
             default,
         } = self;
+        visitor.visit_identifier(name);
         if let Some(expr) = default {
             visitor.visit_expr(expr);
         }

crates/ruff_python_ast/src/visitor/source_order.rs

@@ -1,10 +1,10 @@
-use crate::AnyNodeRef;
 use crate::{
     Alias, Arguments, BoolOp, BytesLiteral, CmpOp, Comprehension, Decorator, ElifElseClause,
     ExceptHandler, Expr, FString, FStringElement, Keyword, MatchCase, Mod, Operator, Parameter,
     ParameterWithDefault, Parameters, Pattern, PatternArguments, PatternKeyword, Singleton, Stmt,
     StringLiteral, TypeParam, TypeParams, UnaryOp, WithItem,
 };
+use crate::{AnyNodeRef, Identifier};
 
 /// Visitor that traverses all nodes recursively in the order they appear in the source.
 ///
@@ -170,6 +170,11 @@ pub trait SourceOrderVisitor<'a> {
     fn visit_bytes_literal(&mut self, bytes_literal: &'a BytesLiteral) {
         walk_bytes_literal(self, bytes_literal);
     }
+
+    #[inline]
+    fn visit_identifier(&mut self, identifier: &'a Identifier) {
+        walk_identifier(self, identifier);
+    }
 }
 
 pub fn walk_module<'a, V>(visitor: &mut V, module: &'a Mod)
@@ -580,3 +585,15 @@ where
     }
     visitor.leave_node(node);
 }
+
+#[inline]
+pub fn walk_identifier<'a, V: SourceOrderVisitor<'a> + ?Sized>(
+    visitor: &mut V,
+    identifier: &'a Identifier,
+) {
+    let node = AnyNodeRef::from(identifier);
+    if visitor.enter_node(node).is_traverse() {
+        identifier.visit_source_order(visitor);
+    }
+    visitor.leave_node(node);
+}

crates/ruff_python_ast_integration_tests/tests/snapshots/source_order__class_type_parameters.snap

@@ -1,15 +1,19 @@
 ---
 source: crates/ruff_python_ast_integration_tests/tests/source_order.rs
 expression: trace
-snapshot_kind: text
 ---
 - ModModule
   - StmtClassDef
+    - Identifier
     - TypeParams
       - TypeParamTypeVar
+        - Identifier
         - ExprName
       - TypeParamTypeVar
+        - Identifier
       - TypeParamTypeVarTuple
+        - Identifier
       - TypeParamParamSpec
+        - Identifier
     - StmtExpr
       - ExprEllipsisLiteral

crates/ruff_python_ast_integration_tests/tests/snapshots/source_order__decorators.snap

@@ -1,15 +1,16 @@
 ---
 source: crates/ruff_python_ast_integration_tests/tests/source_order.rs
 expression: trace
-snapshot_kind: text
 ---
 - ModModule
   - StmtFunctionDef
     - Decorator
       - ExprName
+    - Identifier
     - Parameters
     - StmtPass
   - StmtClassDef
     - Decorator
       - ExprName
+    - Identifier
     - StmtPass

crates/ruff_python_ast_integration_tests/tests/snapshots/source_order__function_arguments.snap

@@ -1,26 +1,34 @@
 ---
 source: crates/ruff_python_ast_integration_tests/tests/source_order.rs
 expression: trace
-snapshot_kind: text
 ---
 - ModModule
   - StmtFunctionDef
+    - Identifier
     - Parameters
       - ParameterWithDefault
         - Parameter
+          - Identifier
       - ParameterWithDefault
         - Parameter
+          - Identifier
       - ParameterWithDefault
         - Parameter
+          - Identifier
       - ParameterWithDefault
         - Parameter
+          - Identifier
         - ExprNumberLiteral
       - Parameter
+        - Identifier
       - ParameterWithDefault
         - Parameter
+          - Identifier
         - ExprNumberLiteral
       - ParameterWithDefault
         - Parameter
+          - Identifier
         - ExprNumberLiteral
       - Parameter
+        - Identifier
     - StmtPass

crates/ruff_python_ast_integration_tests/tests/snapshots/source_order__function_positional_only_with_default.snap

@@ -1,18 +1,22 @@
 ---
 source: crates/ruff_python_ast_integration_tests/tests/source_order.rs
 expression: trace
-snapshot_kind: text
 ---
 - ModModule
   - StmtFunctionDef
+    - Identifier
     - Parameters
       - ParameterWithDefault
         - Parameter
+          - Identifier
       - ParameterWithDefault
         - Parameter
+          - Identifier
         - ExprNumberLiteral
       - ParameterWithDefault
         - Parameter
+          - Identifier
         - ExprNumberLiteral
       - Parameter
+        - Identifier
     - StmtPass

crates/ruff_python_ast_integration_tests/tests/snapshots/source_order__function_type_parameters.snap

@@ -1,16 +1,20 @@
 ---
 source: crates/ruff_python_ast_integration_tests/tests/source_order.rs
 expression: trace
-snapshot_kind: text
 ---
 - ModModule
   - StmtFunctionDef
+    - Identifier
     - TypeParams
       - TypeParamTypeVar
+        - Identifier
         - ExprName
       - TypeParamTypeVar
+        - Identifier
       - TypeParamTypeVarTuple
+        - Identifier
       - TypeParamParamSpec
+        - Identifier
     - Parameters
     - StmtExpr
       - ExprEllipsisLiteral

crates/ruff_python_ast_integration_tests/tests/snapshots/source_order__type_aliases.snap

@@ -1,17 +1,20 @@
 ---
 source: crates/ruff_python_ast_integration_tests/tests/source_order.rs
 expression: trace
-snapshot_kind: text
 ---
 - ModModule
   - StmtTypeAlias
     - ExprName
     - TypeParams
       - TypeParamTypeVar
+        - Identifier
         - ExprName
       - TypeParamTypeVar
+        - Identifier
       - TypeParamTypeVarTuple
+        - Identifier
       - TypeParamParamSpec
+        - Identifier
     - ExprSubscript
       - ExprName
       - ExprName

crates/ruff_python_formatter/src/comments/visitor.rs

@@ -2,7 +2,7 @@ use std::fmt::Debug;
 use std::iter::Peekable;
 
 use ruff_formatter::{SourceCode, SourceCodeSlice};
-use ruff_python_ast::AnyNodeRef;
+use ruff_python_ast::{AnyNodeRef, Identifier};
 use ruff_python_ast::{Mod, Stmt};
 // The interface is designed to only export the members relevant for iterating nodes in
 // pre-order.
@@ -166,6 +166,10 @@ impl<'ast> SourceOrderVisitor<'ast> for CommentsVisitor<'ast, '_> {
             }
         }
     }
+
+    fn visit_identifier(&mut self, _identifier: &'ast Identifier) {
+        // TODO: Visit and associate comments with identifiers
+    }
 }
 
 /// A comment decorated with additional information about its surrounding context in the source document.