Use visit_annotation for type alias value

[Glyphack]

Summary

This was a follow up task from #17180 (comment) separated into this PR.

Test Plan

The tests run. There was already a test for type alias that I updated.
TBH I don't understand why in the representation the ExprSubscript is repeated.

But I created a sample test to check this behavior and it happens everytime visit_annotation is used:

- ModModule
  - StmtFunctionDef
    - Identifier
    - Parameters
      - ParameterWithDefault
        - Parameter
          - Identifier
          - ExprSubscript
            - ExprSubscript
              - ExprName
              - ExprName
    - StmtPass

for:

def f(x: list[T]): pass

[github-actions]

ruff-ecosystem results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

✅ ecosystem check detected no linter changes.

Formatter (stable)

✅ ecosystem check detected no format changes.

Formatter (preview)

✅ ecosystem check detected no format changes.

[dhruvmanila]

TBH I don't understand why in the representation the ExprSubscript is repeated.

Yeah, I think this is happening because we enter_node twice (1) for the visit_annotation (via walk_annotation) and (2) for the visit_expr that's inside visit_annotation so I think this is an expected behavior.

          - ExprSubscript (visit_annotation)
            - ExprSubscript (visit_expr in visit_annotation)

[dhruvmanila]

We'll need to update other visitors as well i.e., Visitor and Transformer

[MichaReiser]

I'm a bit worried about this change, specifically that enter_node is now called twice. This can break existing visitiors that override enter_node and assume that it is only called exactly once (Which I think is true for the formatter).

It seems to me that SourceOrderVisitor::visit_annotation isn't overriden anywhere. That makes me wonder if we should just remove the method for now and simply call visit_expr.

I think we should also update the Visitor implementation to call visit_annotation

ruff/crates/ruff_python_ast/src/visitor.rs

Line 185 in 8a4158c

visitor.visit_expr(value);

[Glyphack]

Thanks for the link I somehow didn't find that.

Do you mean we can remove the SourceOrderVisitor::visit_annotation? Could it be useful in the future? Although with generator script it's easy to add it back.

We are not overriding Transformer::visit_annotation Shall we remove that as well?
https://github.com/Glyphack/ruff/blob/c3c799802740e497ceed7ea7cd62d467e6f22217/crates/ruff_python_ast/src/visitor/transformer.rs#L14

About the breaking change you mentioned I don't understand in what situation it can break(Should I look for an enter_node override in formatter that gives an error if it visits the node twice?) If you can give me an example I can add it to one of the test cases for future.

[MichaReiser]

Here's an example of an visitor that I'm worried that would break:

ruff/crates/ruff_python_formatter/src/comments/visitor.rs

Lines 73 to 112 in 8a4158c

	fn enter_node(&mut self, node: AnyNodeRef<'ast>) -> TraversalSignal {
	let node_range = node.range();
	let enclosing_node = self.parents.last().copied().unwrap_or(node);
	// 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(comment_range) = self.comment_ranges.peek().copied() {
	// Exit if the comment is enclosed by this node or comes after it
	if comment_range.end() > node_range.start() {
	break;
	}
	let comment = DecoratedComment {
	enclosing: enclosing_node,
	preceding: self.preceding_node,
	following: Some(node),
	parent: self.parents.iter().rev().nth(1).copied(),
	line_position: CommentLinePosition::for_range(
	*comment_range,
	self.source_code.as_str(),
	),
	slice: self.source_code.slice(*comment_range),
	};
	self.builder.push_comment(comment);
	self.comment_ranges.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
	}
	}

But there are probably others. Any visitor that internally maintains a stack of "parents" is affected by this because we may end up with a situation where the subscript becomes its own parent which, obviously, is nonsensical

Do you mean we can remove the SourceOrderVisitor::visit_annotation? Could it be useful in the future? Although with generator script it's easy to add it back.

Yes, I think we can simply remove it entirely because it's unused (I think)

We are not overriding Transformer::visit_annotation Shall we remove that as well?

If that's the case, yes, I'd remove it too

[MichaReiser]

I'll close this due to inactivity. If anyone is interested in picking this up again, feel free to submit a new PR and reference this PR in the summary.

Thanks @Glyphack for exploring this.