Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix AST visitor traversal order #5221

Merged
merged 4 commits into from Jun 21, 2023
Merged

Fix AST visitor traversal order #5221

Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
9338be2
Traverse argument default before argument annotation
jgberry Jun 20, 2023
7d5caba
Visit decorators before other function and class children
jgberry Jun 20, 2023
78a8eca
Fix variable annotations after expression
jgberry Jun 20, 2023
ad3b630
Move annotated assignment target traversal after annotation traversal
jgberry Jun 20, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
14 changes: 7 additions & 7 deletions crates/ruff_python_ast/src/visitor.rs
View file
Open in desktop
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);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if we instead need to change how we walk over these such that, in walk_arguments, we walk over the defaults, then the arguments... We may want to remove visit_arg_with_default from the interface entirely. 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, that's the way I originally implemented it, but after resolving the conflicts from #5192, this seemed to be the simplest way to do things. I'm open to restructuring, but it would make this PR more involved.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This won't necessarily visit in the described order though. Given:

def f(
  x: int = 1,
  y : float = 2.0,
):
  ...

This would visit 1, then int, then 2.0, then float. I thought you were looking to enforce 1, then 2.0, then int, then float. Is this change sufficient? I'm fine with either but the latter seems "more correct".

Copy link
Contributor Author

@jgberry jgberry Jun 21, 2023 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, you're right. This doesn't completely solve the issue. But, given as given as function arguments are now walked one by one, it wouldn't be possible to traverse them in the correct order without removing or avoiding calling visit_arg_with_default. I think the simplest solution would be to have walk_arguments handle all argument traversal and never call visit_arg_with_default, but this isn't the cleanest in my opinion. I'll throw this change in though and you can let me know what you think.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, this became unbearably messy for my tastes. The problem is we need to call visit_arg with a ArgWithDefault, so we have to convert an ArgWithDefault to just a plain Arg... but then there are lifetime issues to resolve and the whole thing blows up. I'm inclined to leave this as is for now. Yes, it's not completely correct, but it's so subtle that I don't anticipate it causing issues.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hah, ok -- that's fine, lets see how it goes, this is clearly an improvement.

}

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