From 9b5fb8f38f837eeb4e92f03eef741bddd5861cb6 Mon Sep 17 00:00:00 2001
From: James Berry <jgbcode@gmail.com>
Date: Wed, 21 Jun 2023 14:40:58 -0400
Subject: [PATCH] Fix AST visitor traversal order (#5221)

## 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.
---
 crates/ruff_python_ast/src/visitor.rs | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/crates/ruff_python_ast/src/visitor.rs b/crates/ruff_python_ast/src/visitor.rs
index e9f506fc8a..5830b8e09d 100644
--- a/crates/ruff_python_ast/src/visitor.rs
+++ b/crates/ruff_python_ast/src/visitor.rs
@@ -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);
             }
@@ -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);
             }
@@ -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 {
@@ -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 {
@@ -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) {