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

Remove dead code from AstNode trait #15479

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Conversation

dcreager
Copy link
Member

While looking into potential AST optimizations, I noticed that there were several methods in the AstNode trait that aren't used anywhere in Ruff or Red Knot. It looks like they might be historical artifacts of previous ways of consuming AST nodes?

  • cast, cast_ref, and can_cast are not used anywhere.
  • Since cast_ref isn't needed anymore, the Ref associated type and StatementRef and TypeParamRef types aren't either.
  • We can move visit_source_order out of the trait, since it was never used through a dyn reference anyway.

I had also considered inlining visit_source_order directly into its call sites, but it's used both in the walk_foo functions and within AnyNodeRef::visit_preorder, so I kept them factored out into separate methods.

This is a pure refactoring, with no intended behavior changes.

@MichaReiser MichaReiser self-requested a review January 14, 2025 20:40
Copy link
Contributor

ruff-ecosystem results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

✅ ecosystem check detected no linter changes.

Formatter (stable)

ℹ️ ecosystem check encountered format errors. (no format changes; 1 project error)

openai/openai-cookbook (error)

warning: Detected debug build without --no-cache.
error: Failed to read examples/Assistants_API_overview_python.ipynb: Expected a Jupyter Notebook, which must be internally stored as JSON, but this file isn't valid JSON: expected `,` or `]` at line 197 column 8

Formatter (preview)

ℹ️ ecosystem check encountered format errors. (no format changes; 1 project error)

openai/openai-cookbook (error)

ruff format --preview

warning: Detected debug build without --no-cache.
error: Failed to read examples/Assistants_API_overview_python.ipynb: Expected a Jupyter Notebook, which must be internally stored as JSON, but this file isn't valid JSON: expected `,` or `]` at line 197 column 8

@AlexWaygood AlexWaygood added the internal An internal refactor or improvement label Jan 14, 2025
@MichaReiser
Copy link
Member

MichaReiser commented Jan 15, 2025

It looks like they might be historical artifacts of previous ways of consuming AST nodes?

They're mainly experimentations on my end but I never really had time to fully persuade them. The overall design with AstNode is inspired by rust analyzers/BiomeJS AST facade over a generic syntax node. But I don't think the design works as well for our AST. That's why removing cast, cast_ref, and Ref is probably a good idea. I'm also open to removing the AstNode.

I do think it might be valuable to keep StatementRef around:

  • We have and use ExpressionRef, AnyNodeRef. StatementRef is needed for a "complete" API
  • The main motivation for StatementRef (and ExpressionRef) is that we have methods that should accept any statement node. Historically, we used &Stmt for those. The problem with using &Stmt is that you're screwed if you only have a &StmtIf because there's no way to go from &StmtIf to &Stmt::If. Our previous solution was to change the call-site to get access to the &Stmt node. Unfortunately, that method now accepts redundant arguments or we just weakened its signature (with a very likely unwrap call in the method itself). A possible solution for this problem is to use StatementRef over &Stmt because you can convert a &Stmt and a specific statement node to StatementRef. I've never had the time to refactor some code to use StatementRef because it's hard to identify the problematic methods now where they all use &Stmt instead of a specific node. It's also something that needs to be done at a larger scale because mixing StatementRef and &Stmt is problematic again whenever you have to call from a function taking a StatementRef into a function that takes a &Stmt.

@dcreager
Copy link
Member Author

dcreager commented Jan 15, 2025

I do think it might be valuable to keep StatementRef around:

  • We have and use ExpressionRef, AnyNodeRef. StatementRef is needed for a "complete" API

  • The main motivation for StatementRef (and ExpressionRef) is that we have methods that should accept any statement node. Historically, we used &Stmt for those. The problem with using &Stmt is that you're screwed if you only have a &StmtIf because there's no way to go from &StmtIf to &Stmt::If. Our previous solution was to change the call-site to get access to the &Stmt node. Unfortunately, that method now accepts redundant arguments or we just weakened its signature (with a very likely unwrap call in the method itself). A possible solution for this problem is to use StatementRef over &Stmt because you can convert a &Stmt and a specific statement node to StatementRef. I've never had the time to refactor some code to use StatementRef because it's hard to identify the problematic methods now where they all use &Stmt instead of a specific node. It's also something that needs to be done at a larger scale because mixing StatementRef and &Stmt is problematic again whenever you have to call from a function taking a StatementRef into a function that takes a &Stmt.

What I'm eventually aiming for (WIP branch here) would be to use IndexVec to store all of the AST variants, and Stmt/Expr/etc would be enums wrapping the id, not the actual data. I think that would make Expr work in places where we're currently using ExpressionRef. And a method that only operates on StmtIf would take in StmtIfId instead of the enum wrapper.

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

Successfully merging this pull request may close these issues.

3 participants