From 5a876ed25e26d77e5d89670226456cbc69666d2c Mon Sep 17 00:00:00 2001 From: Micha Reiser Date: Tue, 1 Apr 2025 16:58:09 +0200 Subject: [PATCH] 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 --- .../ruff/rules/suppression_comment_visitor.rs | 4 + crates/ruff_python_ast/src/node.rs | 112 +++++++++++++----- .../src/visitor/source_order.rs | 19 ++- .../source_order__class_type_parameters.snap | 6 +- .../snapshots/source_order__decorators.snap | 3 +- .../source_order__function_arguments.snap | 10 +- ...function_positional_only_with_default.snap | 6 +- ...ource_order__function_type_parameters.snap | 6 +- .../snapshots/source_order__type_aliases.snap | 5 +- .../src/comments/visitor.rs | 6 +- 10 files changed, 142 insertions(+), 35 deletions(-) diff --git a/crates/ruff_linter/src/rules/ruff/rules/suppression_comment_visitor.rs b/crates/ruff_linter/src/rules/ruff/rules/suppression_comment_visitor.rs index 24a1423260..19ee1372d1 100644 --- a/crates/ruff_linter/src/rules/ruff/rules/suppression_comment_visitor.rs +++ b/crates/ruff_linter/src/rules/ruff/rules/suppression_comment_visitor.rs @@ -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)] diff --git a/crates/ruff_python_ast/src/node.rs b/crates/ruff_python_ast/src/node.rs index a3bbb669d6..0b0acd6c36 100644 --- a/crates/ruff_python_ast/src/node.rs +++ b/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); } diff --git a/crates/ruff_python_ast/src/visitor/source_order.rs b/crates/ruff_python_ast/src/visitor/source_order.rs index 9092cbc2af..5e6ca022a0 100644 --- a/crates/ruff_python_ast/src/visitor/source_order.rs +++ b/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); +} diff --git a/crates/ruff_python_ast_integration_tests/tests/snapshots/source_order__class_type_parameters.snap b/crates/ruff_python_ast_integration_tests/tests/snapshots/source_order__class_type_parameters.snap index 9e777ff2ae..078564f390 100644 --- a/crates/ruff_python_ast_integration_tests/tests/snapshots/source_order__class_type_parameters.snap +++ b/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 diff --git a/crates/ruff_python_ast_integration_tests/tests/snapshots/source_order__decorators.snap b/crates/ruff_python_ast_integration_tests/tests/snapshots/source_order__decorators.snap index 977336ebd1..fd002ba7e1 100644 --- a/crates/ruff_python_ast_integration_tests/tests/snapshots/source_order__decorators.snap +++ b/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 diff --git a/crates/ruff_python_ast_integration_tests/tests/snapshots/source_order__function_arguments.snap b/crates/ruff_python_ast_integration_tests/tests/snapshots/source_order__function_arguments.snap index 0079337323..57611d34cd 100644 --- a/crates/ruff_python_ast_integration_tests/tests/snapshots/source_order__function_arguments.snap +++ b/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 diff --git a/crates/ruff_python_ast_integration_tests/tests/snapshots/source_order__function_positional_only_with_default.snap b/crates/ruff_python_ast_integration_tests/tests/snapshots/source_order__function_positional_only_with_default.snap index d482b22231..c46edad75c 100644 --- a/crates/ruff_python_ast_integration_tests/tests/snapshots/source_order__function_positional_only_with_default.snap +++ b/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 diff --git a/crates/ruff_python_ast_integration_tests/tests/snapshots/source_order__function_type_parameters.snap b/crates/ruff_python_ast_integration_tests/tests/snapshots/source_order__function_type_parameters.snap index 1bd6284199..3d02f25958 100644 --- a/crates/ruff_python_ast_integration_tests/tests/snapshots/source_order__function_type_parameters.snap +++ b/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 diff --git a/crates/ruff_python_ast_integration_tests/tests/snapshots/source_order__type_aliases.snap b/crates/ruff_python_ast_integration_tests/tests/snapshots/source_order__type_aliases.snap index 2e2e76044c..f21ecb2147 100644 --- a/crates/ruff_python_ast_integration_tests/tests/snapshots/source_order__type_aliases.snap +++ b/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 diff --git a/crates/ruff_python_formatter/src/comments/visitor.rs b/crates/ruff_python_formatter/src/comments/visitor.rs index bf70f37281..d0f146a3f7 100644 --- a/crates/ruff_python_formatter/src/comments/visitor.rs +++ b/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.