oxc-project / oxc-project/backlog
Better way to replace Statements + make `AstBuilder::move_*` cheaper
Nobody has claimed this yet.
- Dominant language
- No language data
- Stars
- 7
- Forks
- 0
- PR merge metrics
- No merged PRs in 30d
Description
Originally opened as https://github.com/oxc-project/oxc/issues/5359. I tried to transfer that issue to backlog repo, but it's not working for some reason, so copying the content into this new issue instead.
I said:
I wonder if we should add Expression::None and Statement::None variants to AST?
Use cases
1. Removing Statements
Removing a Statement from a Vec<Statement> (as various transforms do) would be very easy and would not require "shuffling up" the Vec. Just replace the statement with Statement::None.
Make AstBuilder::move_expression cheaper
Currently move_expression substitutes a dummy Box<NullExpression> into the AST. This involves allocating space for it in the arena and writing it.
In contrast, Expression::None would be an enum variant with no "payload" and so would not require allocation. Probably then the compiler will be able to see code like this:
let owned_expr = move_expression(expr);
*expr = transform(owned_expr);
fn move_expression(expr: &mut Expression) {
std::mem::replace(expr, Expression::None)
}
and understand that it can skip writing Expression::None completely, because it's pointless - it's overwritten again shortly after. So likely it'll be as efficient as AstBuilder::copy, while being completely safe.
Right now it can't do that, because move_expression currently has an observable side effect of advancing the bump allocator's pointer.
Codegen
Codegen would need to skip over Statement::None as if its not there.
It would panic on Expression::None. If any transforms are malfunctioning and leaving Expression::Nones in the AST, we'd discover that very quickly as we'd get panics in CI.
Alternatives
I've tried to dream up other APIs to replace move_expression. e.g. some kind of AstBuilder::replace_expression which uses unsafe code to avoid writing anything to the AST until it gets the replacement to put back in, and uses the type system to ensure you can't commit UB. But it seems pretty intractable to me. Simple cases like a one-for-one replacement are easy enough, but once you get into pulling out child nodes, combining them in a new struct, and pushing that back into the AST (which transformer does a lot), it gets... hard.
So, while I'm not that keen on adding "fake" types into the AST, I can't see a more viable solution to alleviating these pain points at present.
Contributor guide
No contributing guide indexed for this repository
First steps
- Read the whole issue, then the project's contributing guide.
- Comment on the issue to say you are picking it up — it saves two people doing the same work.
- Fork the repository and make your change on a branch.
- Open a pull request that references the issue number.
Research direction
Start with AstBuilder::move_expression and the Expression and Statement AST variants described in the issue, then trace how transforms and codegen handle moved, removed, and invalid nodes. Done requires an agreed design for the proposed None variants or an alternative, including how codegen skips statements and rejects invalid expressions.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- rust
- Domain
- compilers
- Issue type
- Feature
- Difficulty
- 5/5
- Estimated time
- Over a week
- Activity status
- Stale
- Clarity
- Needs clarification
- Newbie friendliness
- 30/100