Skip to content

Commit

Permalink
Fix AST visitor traversal order (#5221)
Browse files Browse the repository at this point in the history
## Summary

According to the AST visitor documentation, the AST visitor "visits all
nodes in the AST recursively in evaluation-order". However, the current
traversal fails to meet this specification in a few places.

### Function traversal

```python
order = []
@(order.append("decorator") or (lambda x: x))
def f(
    posonly: order.append("posonly annotation") = order.append("posonly default"),
    /,
    arg: order.append("arg annotation") = order.append("arg default"),
    *args: order.append("vararg annotation"),
    kwarg: order.append("kwarg annotation") = order.append("kwarg default"),
    **kwargs: order.append("kwarg annotation")
) -> order.append("return annotation"):
    pass
print(order)
```

Executing the above snippet using CPython 3.10.6 prints the following
result (formatted for readability):
```python
[
    'decorator',
    'posonly default',
    'arg default',
    'kwarg default',
    'arg annotation',
    'posonly annotation',
    'vararg annotation',
    'kwarg annotation',
    'kwarg annotation',
    'return annotation',
]
```

Here we can see that decorators are evaluated first, followed by
argument defaults, and annotations are last. The current traversal of a
function's AST does not align with this order.

### Annotated assignment traversal
```python
order = []
x: order.append("annotation") = order.append("expression")
print(order)
```

Executing the above snippet using CPython 3.10.6 prints the following
result:
```python
['expression', 'annotation']
```

Here we can see that an annotated assignments annotation gets evaluated
after the assignment's expression. The current traversal of an annotated
assignment's AST does not align with this order.

## Why?

I'm slowly working on #3946 and porting over some of the logic and tests
from ssort. ssort is very sensitive to AST traversal order, so ensuring
the utmost correctness here is important.

## Test Plan

There doesn't seem to be existing tests for the AST visitor, so I didn't
bother adding tests for these very subtle changes. However, this
behavior will be captured in the tests for the PR which addresses #3946.
  • Loading branch information
jgberry committed Jun 21, 2023
1 parent d7c7484 commit 9b5fb8f
Showing 1 changed file with 7 additions and 7 deletions.
14 changes: 7 additions & 7 deletions crates/ruff_python_ast/src/visitor.rs
Original file line number Diff line number Diff line change
Expand Up @@ -99,10 +99,10 @@ pub fn walk_stmt<'a, V: Visitor<'a> + ?Sized>(visitor: &mut V, stmt: &'a Stmt) {
returns,
..
}) => {
visitor.visit_arguments(args);
for decorator in decorator_list {
visitor.visit_decorator(decorator);
}
visitor.visit_arguments(args);
for expr in returns {
visitor.visit_annotation(expr);
}
Expand All @@ -115,10 +115,10 @@ pub fn walk_stmt<'a, V: Visitor<'a> + ?Sized>(visitor: &mut V, stmt: &'a Stmt) {
returns,
..
}) => {
visitor.visit_arguments(args);
for decorator in decorator_list {
visitor.visit_decorator(decorator);
}
visitor.visit_arguments(args);
for expr in returns {
visitor.visit_annotation(expr);
}
Expand All @@ -131,15 +131,15 @@ pub fn walk_stmt<'a, V: Visitor<'a> + ?Sized>(visitor: &mut V, stmt: &'a Stmt) {
decorator_list,
..
}) => {
for decorator in decorator_list {
visitor.visit_decorator(decorator);
}
for expr in bases {
visitor.visit_expr(expr);
}
for keyword in keywords {
visitor.visit_keyword(keyword);
}
for decorator in decorator_list {
visitor.visit_decorator(decorator);
}
visitor.visit_body(body);
}
Stmt::Return(ast::StmtReturn {
Expand Down Expand Up @@ -180,10 +180,10 @@ pub fn walk_stmt<'a, V: Visitor<'a> + ?Sized>(visitor: &mut V, stmt: &'a Stmt) {
value,
..
}) => {
visitor.visit_annotation(annotation);
if let Some(expr) = value {
visitor.visit_expr(expr);
}
visitor.visit_annotation(annotation);
visitor.visit_expr(target);
}
Stmt::For(ast::StmtFor {
Expand Down Expand Up @@ -633,10 +633,10 @@ pub fn walk_arg_with_default<'a, V: Visitor<'a> + ?Sized>(
visitor: &mut V,
arg_with_default: &'a ArgWithDefault,
) {
visitor.visit_arg(&arg_with_default.def);
if let Some(expr) = &arg_with_default.default {
visitor.visit_expr(expr);
}
visitor.visit_arg(&arg_with_default.def);
}

pub fn walk_keyword<'a, V: Visitor<'a> + ?Sized>(visitor: &mut V, keyword: &'a Keyword) {
Expand Down

0 comments on commit 9b5fb8f

Please sign in to comment.