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.

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

Add Ast as top level enum #58

Merged
merged 1 commit into from
May 18, 2023
Merged

Conversation

youknowone
Copy link
Member

One of the possible solution of #48

@youknowone
Copy link
Member Author

cc @MichaReiser

@youknowone youknowone merged commit 3654cf0 into RustPython:main May 18, 2023
@youknowone youknowone deleted the ast-enum branch May 18, 2023 16:53
@MichaReiser
Copy link
Contributor

Thanks for working on this and sorry for my late reply. I've been busy with other work and haven't had time to think more about the problem.

I still think that I would favor the flat structure over the nested one because:

  • The nested structure requires two marker bits to distinguish the variants. One at the top level, and another for the sub-union. That means, the enum is three bytes larger than a flat structure.
  • In my view, the most common access patterns are that we don't care about what the node is OR that we want to test if it is a few very specific nodes. The nested structure requires a nested match except for, in my view, the rare case where you want to match on all statements.
  • I think we can accommodate the "matching on all statements" by introducing the helpers is_stmt(), as_stmt() and stmt() on the flat structure.

Would you mind if I change the enum to a flat structure? I will also introduce a AstRef as part of that work because that's actually the enum that I need first.

Comment on lines +27 to +30
impl<R> Node for Ast<R> {
const NAME: &'static str = "AST";
const FIELD_NAMES: &'static [&'static str] = &[];
}
Copy link
Contributor

Choose a reason for hiding this comment

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

What's the reason that the enums implement the Node trait. They aren't really nodes and have no fields.

@youknowone
Copy link
Member Author

Because current Ast enum is the counter part of python side ast.AST(each rust enum matches to inheritance from Python), adding a new enum for flat structure seems better if you need one. Before adding the Ast type, the top level ast.AST was the only missing type from Rust side.
On the other hand, adding is_stmt_if() to current Ast seems also reasonable if it fits your requirements. (probably not if you need to use match)

@youknowone
Copy link
Member Author

I wish conversion between Ast and other types like Stmt and Expr doesn't require much cost. If we can achieve it with flat structure, that's also fine.

@MichaReiser
Copy link
Contributor

Because current Ast enum is the counter part of python side ast.AST(each rust enum matches to inheritance from Python), adding a new enum for flat structure seems better if you need one. Before adding the Ast type, the top level ast.AST was the only missing type from Rust side. On the other hand, adding is_stmt_if() to current Ast seems also reasonable if it fits your requirements. (probably not if you need to use match)

I'll add it on the Ruff side if you want to keep using the Ast node for CPython compatibility

@youknowone
Copy link
Member Author

Yes, that will be a good option.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants