refactor(ast): rework enum of structures

[rzvxa]

• ForStatementInit
• MemberExpression
• PropertyKey
• Argument
• AssignmentTarget
• SimpleAssignmentTarget
• ArrayExpressionElement
• Expression
• ModuleDeclaration
• JSXElementName
• JSXAttributeItem
• JSXMemberExpressionObject
• TSTypeName
• Statement (not needed but we might also refactor it for the good measure)

[rzvxa]

• refactor(ast): rework enum of structures #2876 👈
• main

This stack of pull requests is managed by Graphite. Learn more about stacking.

Join @rzvxa and the rest of your teammates on Graphite

[codspeed-hq]

CodSpeed Performance Report

Merging #2876 will degrade performances by 3.62%

_{Comparing 03-31-refactor_ast_rework_enum_of_structures_ast_nodes (1d18bd7) with main (7034bcc)}

Summary

❌ 2 regressions
✅ 34 untouched benchmarks

⚠️ Please fix the performance issues or acknowledge them on CodSpeed.

Benchmarks breakdown

	Benchmark	main	03-31-refactor_ast_rework_enum_of_structures_ast_nodes	Change
❌	parser_napi[RadixUIAdoptionSection.jsx]	230.1 µs	237.5 µs	-3.13%
❌	parser_napi[cal.com.tsx]	115.4 ms	119.7 ms	-3.62%

[rzvxa]

@Dunqing @Boshen @milesj please take a look at this so I discard it early and find another way if we don't want to go down this road.

[Dunqing]

Personally, it looks good to me. It's the same as how BindingPattern is handled.

[Dunqing]

We may need to consider this #2854

[rzvxa]

Personally, it looks good to me. It's the same as how BindingPattern is handled.

That's a good point; I didn't think of BindingPattern, I guess it's a good thing that it isn't a foreign thing but a reoccurring pattern.

[rzvxa]

We may need to consider this #2854

I'll give it a read.

Since @overlookmotel is also currently working regularly on AST for better napi performance we might want to also wait for him to give some feedback.

We may have to take things like #2457 and #2847 into consideration.

[Boshen]

cross post from discord:

Can we somehow validate whether node ids are going to work before going down this rabbit hole?

Can we somehow reduce the surface area for validation, find a small real example where we need the id and do all sorts of operations?

So rustc doesn't have any concrete AST nodes that are enums https://doc.rust-lang.org/beta/nightly-rustc/rustc_ast/ast/index.html#enums, I think this is the right direction, but I don't want us getting burned out with these large changes and then finding out it's not going to work

[rzvxa]

cross post from discord:

Can we somehow validate whether node ids are going to work before going down this rabbit hole?

Can we somehow reduce the surface area for validation, find a small real example where we need the id and do all sorts of operations?

So rustc doesn't have any concrete AST nodes that are enums https://doc.rust-lang.org/beta/nightly-rustc/rustc_ast/ast/index.html#enums, I think this is the right direction, but I don't want us getting burned out with these large changes and then finding out it's not going to work

I'll continue this conversation over here for historical reasons, Yes it is possible, I'll do it for a single enum type to make sure it works.

[overlookmotel]

@rzvxa Thanks for looping me in. I'm not aware of the context, so having a hard time understanding the motivation for this change. Would you be able to point me in direction to the Discord conversation so I can read up?

FYI, the problem with enums I've been having in #2457 is:

Need to be able to determine what niche value is used for Option<T>::None where T is an enum.

You can make this deterministic by making enums #[repr(u8)] and defining 0 as discriminant for 1st variant, and 254 for last variant. Rust only creates niches at start or end of the range, so that only leaves 1 possible niche value - 255. Sorted.

However, when you have an enum within an enum e.g. enum MyEnum { Another(AnotherEnum) } then Rust cleverly "squashes" the 2 discriminants into 1 to reduce the size of the outer enum type. How it does this is less predictable than the simple case, and therefore it becomes difficult predicting the what the niche is. I think I've figured out the rules, but it's unspecified, and so Rust lang makes no promises not to change it at any time. So relying on Rust's internal logic as it is today makes it a bit fragile.

I think #2847 is likely a dead end, so I'll have to find another way, and maybe we just have to accept that while it could break in a new Rust release, in practice it probably won't.

But upshot is... if in the process of the changes you're planning to make, it'd be possible/easy to remove all nested enums, that would solve a problem for AST transfer.

[overlookmotel]

And... just to throw one more thing into the mix:

If we're planning to make changes to enums, we could also consider trying to reduce the type sizes at the same time. This is a fairly common pattern in Oxc's AST:

oxc/crates/oxc_ast/src/ast/js.rs

Lines 466 to 469 in 619fc61

	pub enum ObjectPropertyKind<'a> {
	ObjectProperty(Box<'a, ObjectProperty<'a>>),
	SpreadProperty(Box<'a, SpreadElement<'a>>),
	}

Box<ObjectProperty> and Box<SpreadElement> are both 8 bytes, with alignment 8. The enum discriminant only needs 1 bit, but because size must be a multiple of of alignment, that 1 bit increases the size of ObjectPropertyKind to 16 bytes.

ObjectProperty and SpreadElement both have alignment of 8 and therefore the bottom 3 bits of both Box pointers are always 0. So could instead put the enum discriminant in one of these unused bits, using pointer tagging, and reduce ObjectPropertyKind to 8 bytes. The same trick could be used on any other enums with 8 variants or less.

I imagine this would improve performance somewhat, but question is whether it can be made ergonomic - you then can't use match kind { ObjectPropertyKind::ObjectProperty(prop) => ... } in the usual way.

This may not be worth doing at this stage (or maybe at any stage), but I just thought I'd mention it in case it's useful to have in mind.

[rzvxa]

Would you be able to point me in direction to the Discord conversation so I can read up?

Gladly, this PR aims to resolve some of the roadblocks in the progress of #2818.

when you have an enum within an enum e.g. enum MyEnum { Another(AnotherEnum) } then Rust cleverly "squashes" the 2 discriminants into 1 to reduce the size of the outer enum type

Does this also get squashed or does it stay intact?

Enum1::VariantA(StructA { foo: Enum2::VariantB(something) })

Because with this change we eliminate most if not all of the nested enums to something more akin to the pseudocode mentioned above.

[rzvxa]

And... just to throw one more thing into the mix:

If we're planning to make changes to enums, we could also consider trying to reduce the type sizes at the same time. This is a fairly common pattern in Oxc's AST:

oxc/crates/oxc_ast/src/ast/js.rs

Lines 466 to 469 in 619fc61

	pub enum ObjectPropertyKind<'a> {
	ObjectProperty(Box<'a, ObjectProperty<'a>>),
	SpreadProperty(Box<'a, SpreadElement<'a>>),
	}

Box<ObjectProperty> and Box<SpreadElement> are both 8 bytes, with alignment 8. The enum discriminant only needs 1 bit, but because size must be a multiple of of alignment, that 1 bit increases the size of ObjectPropertyKind to 16 bytes.

ObjectProperty and SpreadElement both have alignment of 8 and therefore the bottom 3 bits of both Box pointers are always 0. So could instead put the enum discriminant in one of these unused bits, using pointer tagging, and reduce ObjectPropertyKind to 8 bytes. The same trick could be used on any other enums with 8 variants or less.

I imagine this would improve performance somewhat, but question is whether it can be made ergonomic - you then can't use match kind { ObjectPropertyKind::ObjectProperty(prop) => ... } in the usual way.

This may not be worth doing at this stage (or maybe at any stage), but I just thought I'd mention it in case it's useful to have in mind.

What is it going to look like? Do we essentially have to implement our own tagged union? Or is it about making a custom Box that would handle the inner with raw pointers? I'm having a hard time imagining the end result.

[overlookmotel]

Gladly, this PR aims to resolve some of the roadblocks in the progress of #2818.

Thanks. I'm on holiday at the moment, but will read up soon as I get a chance.

Does this also get squashed or does it stay intact?
Enum1::VariantA(StructA { foo: Enum2::VariantB(something) })

It appears it does get squashed. Rust playground

In general the squashing is good, as it minimizes type sizes. But, for my purposes, this:

enum Foo {
  A(Box<A>),
  B(Box<B>),
  C(Box<C>),
  D(Box<D>),
}

is preferable to:

enum Foo {
  A(Box<A>),
  B(Box<B>),
  Bar(Bar),
}

enum Bar {
  C(Box<C>),
  D(Box<D>),
}

i.e. do the squashing explicitly, rather than leave it to the compiler.

But like I said, I'm kind of resigned at this point to a more hacky/fragile solution to this problem than I'd ideally like. We shouldn't bend everything around the needs of AST transfer. I just mention it because if the new design was moving in that direction anyway, it'd be great to solve this problem at the same time. But if it's not, don't worry about it.

What is it going to look like? Do we essentially have to implement our own tagged union? Or is it about making a custom Box that would handle the inner with raw pointers? I'm having a hard time imagining the end result.

AST types don't have Drop impls (because they're in arena), so I think it could be implemented as a tagged union, rather than having to work with raw pointers. Not sure exactly what the API would look like, and the big missing element is that you can't use T::VariantX(x) as a match arm, as it's not an enum any more (though maybe some API like match foo.as_enum() { ... } is possible). This is quite a vague idea, and very possibly impractical. But we could look into it further if it seems worthwhile.

[rzvxa]

Thanks. I'm on holiday at the moment, but will read up soon as I get a chance.

Wow! then thanks for answering on your holidays.

But like I said, I'm kind of resigned at this point to a more hacky/fragile solution to this problem than I'd ideally like. We shouldn't bend everything around the needs of AST transfer. I just mention it because if the new design was moving in that direction anyway, it'd be great to solve this problem at the same time. But if it's not, don't worry about it.

Well, It's literally the solution to another issue(which is having concrete AST nodes so we can add their IDs to them). So if we come up with no alternative solution it is going to hit 2 birds with one stone, At least in theory we have to see how squashing behaves in more advanced situations.

AST types don't have Drop impls (because they're in arena), so I think it could be implemented as a tagged union, rather than having to work with raw pointers. Not sure exactly what the API would look like, and the big missing element is that you can't use T::VariantX(x) as a match arm, as it's not an enum any more (though maybe some API like match foo.as_enum() { ... } is possible). This is quite a vague idea, and very possibly impractical. But we could look into it further if it seems worthwhile.

I also like the idea of making our types more compact since it may help with cache misses in our native Rust world so its effect might not be limited to the transfer. However, Implementing as_enum safely needs 2 checks for each match expression so that it would defeat its purpose.

Maybe we can find another way to pack some of our types without going full-on tagged unions? I mean on a case-to-case basis there might be some room for improvement.

[overlookmotel]

I also like the idea of making our types more compact since it may help with cache misses in our native Rust world so its effect might not be limited to the transfer. However, Implementing as_enum safely needs 2 checks for each match expression so that it would defeat its purpose.

Maybe... but I suspect the compiler might be smart enough strip that out again when inlining as_enum. Edit: yes, it seems it is: https://godbolt.org/z/9fzo5ne1a

By the way, my point about reducing type sizes is not related to AST transfer - it was a separate thought. Anyway, perhaps I've lead this too far off topic...

---

Concerning this PR: There is one downside I can see.

If you later add ast_node_id into ForStatementInit, then it will increase to 24 bytes. As there's 7 bytes unused already in ForStatementInitKind, this seems a waste.

There's a hacky way to get around that: Playground But ooof! Not nice.

I'm a bit unclear why the more obvious solution of storing Node IDs inside the variants' types themselves isn't workable. If problem is needing to access the ID cheaply, you could make the structs #[repr(C)] and ensure the ID field is in the same place in every variant, then just pull it out with a pointer read without a branch.

[rzvxa]

Edit: yes, it seems it is: https://godbolt.org/z/9fzo5ne1a

Nice! It was one of my concerns however, Doing it with repr(C) is a great idea that I haven't thought of when writing this, I got away with it via a derive macro that matches all variants.

---

If you later add ast_node_id into ForStatementInit, then it will increase to 24 bytes. As there's 7 bytes unused already in ForStatementInitKind, this seems a waste.

That's exactly what I was hoping to do, Sadly both in oxc and babel which we are porting a bunch of things from there, are assumptions about enum types. let's take the ForStatementInit as an example.

pub enum ForStatementInit<'a> {
    VariableDeclaration(Box<'a, VariableDeclaration<'a>>),
    Expression(Expression<'a>),
    UsingDeclaration(Box<'a, UsingDeclaration<'a>>),
}

When visiting our nodes we might visit the init expression as an Expression and want to see its parent. If we make enum types transparent(which means they just pass the node_id through, not the transparent attribute) We can't access ForStatementInit as our parent and we would directly get the ForStatement, which won't give us enough information about whether this expression is evaluated as the initial value, the condition or the update assignment.

Babel gets away with this since they are using the Path (find out more), But it has way more performance cost than having a few more bits in the memory. Since we actually would need a hashmap to figure this out without adding IDs to our types(which need concrete AST types so no enums as ASTNode).

We still have transparent enums for example AstKind enum which can contain any type of AstNode. These would pass their inner ast_node_id thorough.

---

I like the way you think about problems, Please if you have any other approach to this let me know. They say when all you have is a hammer you see everything as nails, And in the world of compilers written in rust I'm most familiar with rustc so that's why I usually gravitate toward what is done there, Which might not be the most optimized way out there hence the compile times for rust projects(Although to be fair rust does a lot of work compared to js/ts compilers).

[overlookmotel]

won't give us enough information about whether this expression is evaluated as the initial value, the condition or the update assignment.

Thanks for explaining. Now I understand the problem.

But... don't you have a similar problem in other places e.g.:

oxc/crates/oxc_ast/src/ast/js.rs

Lines 1262 to 1266 in e76d70f

	pub struct BlockStatement<'a> {
	#[cfg_attr(feature = "serialize", serde(flatten))]
	pub span: Span,
	pub body: Vec<'a, Statement<'a>>,
	}

If you are visiting a Statement and you want to replace it, don't you need to know both the parent (which BlockStatement) and also the index in body Vec? (maybe you can get away without index by replacing in place, but what if you want to replace a statement with two statements?)

Or:

oxc/crates/oxc_ast/src/ast/js.rs

Lines 888 to 894 in e76d70f

	pub struct BinaryExpression<'a> {
	#[cfg_attr(feature = "serialize", serde(flatten))]
	pub span: Span,
	pub left: Expression<'a>,
	pub operator: BinaryOperator,
	pub right: Expression<'a>,
	}

If you are visiting an Expression and want to replace it, the parent ID gives you the BinaryExpression, but is the expression on the left or the right?

Babel's Path handles these by including both "parent" and "key on parent".

---

On the problem of type sizes increasing, this is better than my previous hacky suggestion:

// 16 bytes - Node ID fits after the discriminant
enum EnumWithId {
    A { ast_node_id: u32, node: Box<NodeA> },
    B { ast_node_id: u32, node: Box<NodeB> },
    C { ast_node_id: u32, node: Box<NodeC> },
}

Playground

But... if you nest the enums the way they are in the current AST, you don't get the "squashed" discriminant optimization, and the type increases to 24 bytes. You have to flatten into a single enum to keep the size down.

[rzvxa]

If you are visiting a Statement and you want to replace it, don't you need to know both the parent (which BlockStatement) and also the index in body Vec? (maybe you can get away without index by replacing in place, but what if you want to replace a statement with two statements?)

You are absolutely right, This approach is only there to let us infer information about neighbors, and reference a particular node in our semantic, scope, and other data structures as SOA. For replacing nodes in the tree I was going to refactor the VisitMut, So it would always return a result, Then the result can be set in the implementor to be the unit but if a Visitor wants it can return the replacement nodes when visiting a single node. This way if we return a SmallVec we can substitute one node with one or many without keeping track of its index.

At least that's the plan, Have to see how it works out.

---

If you are visiting an Expression and want to replace it, the parent ID gives you the BinaryExpression, but is the expression on the left or the right?

Babel's Path handles these by including both "parent" and "key on parent".

It is a known issue with my proposal, For this, we have to visit the BinaryExpression so we know about the lhs and rhs. Much better for performance and may eliminate some revisiting when changing one side of the expression. Although, doing it for everything makes porting from Babel difficult. Edit: But thankfully we only need it a handful of times since most information is held within AstNodeId, And in the future, if we really need this index we can make an SOA holding key in parent information for each AstNodeId.

---

Playground

But... if you nest the enums the way they are in the current AST, you don't get the "squashed" discriminant optimization, and the type increases to 24 bytes. You have to flatten into a single enum to keep the size down.

I'll take this into consideration, Some nested enums are impossible to flatten for example we need some enum types that point to an expression, and bringing everything from an expression is an unreasonable solution for not much difference in the memory footprint, It can be used to make a few types compact tho.

To be honest I won't go too hard on packing our structures right now, Since I prefer to bring it to a working state and then worry about this optimization. I would love to do it at some point and I need your help for doing so, since you clearly have thought about this for a while.

[overlookmotel]

You are absolutely right

Actually, I realized I am absolutely wrong! If you have a reference to the Statement and the parent BlockStatement, you can deduce the index:

fn statement_index(stmt: &Statement, parent: &BlockStatement) -> usize {
  (stmt as *const _ as usize - parent.body.as_ptr() as usize) / std::mem::size_of::<Statement>()
}

Ditto deducing left or right of BinaryExpression:

fn expr_is_left(expr: &Expression, parent: &BinaryExpression) {
  expr as *const _ as usize
    == parent as *const _ as usize + std::mem::offset_of!(BinaryExpression, left)
}

---

I'll take this into consideration, Some nested enums are impossible to flatten for example we need some enum types that point to an expression, and bringing everything from an expression is an unreasonable solution for not much difference in the memory footprint

OK. That's fair. But in my opinion, this pattern is preferable to enum-within-struct, as it saves 8 bytes on most enums with little penalty in terms of usability:

// 16 bytes
enum EnumWithId {
  A { ast_node_id: u32, node: Box<NodeA> },
  B { ast_node_id: u32, node: Box<NodeB> },
  C { ast_node_id: u32, node: Box<NodeC> },
}

impl EnumWithId {
  fn ast_node_id(&self) -> u32 {
    // Compiler will compress this to `(self as *const _ as *const u32).add(1).read()`
    // i.e. single instruction, no branch
    match self {
      Self::A { ast_node_id, .. } => *ast_node_id,
      Self::B { ast_node_id, .. } => *ast_node_id,
      Self::C { ast_node_id, .. } => *ast_node_id,
    }
}

https://godbolt.org/z/r9nEq8KPP

In my view it is worthwhile avoiding an extra 8 bytes on every Statement and Expression. That'd be a lot of bytes in total in an entire AST.

What do you think?

[rzvxa]

You are absolutely right

Actually, I realized I am absolutely wrong! If you have a reference to the Statement and the parent BlockStatement, you can deduce the index:

fn statement_index(stmt: &Statement, parent: &BlockStatement) -> usize {
  (stmt as *const _ as usize - parent.body.as_ptr() as usize) / std::mem::size_of::<Statement>()
}

Ditto deducing left or right of BinaryExpression:

fn expr_is_left(expr: &Expression, parent: &BinaryExpression) {
  expr as *const _ as usize
    == parent as *const _ as usize + std::mem::offset_of!(BinaryExpression, left)
}

I can't wrap my head around this one, How does it work?

[rzvxa]

OK. That's fair. But in my opinion, this pattern is preferable to enum-within-struct, as it saves 8 bytes on most enums with little penalty in terms of usability:

// 16 bytes
enum EnumWithId {
  A { ast_node_id: u32, node: Box<NodeA> },
  B { ast_node_id: u32, node: Box<NodeB> },
  C { ast_node_id: u32, node: Box<NodeC> },
}

impl EnumWithId {
  fn ast_node_id(&self) -> u32 {
    // Compiler will compress this to `(self as *const _ as *const u32).add(1).read()`
    // i.e. single instruction, no branch
    match self {
      Self::A { ast_node_id, .. } => *ast_node_id,
      Self::B { ast_node_id, .. } => *ast_node_id,
      Self::C { ast_node_id, .. } => *ast_node_id,
    }
}

https://godbolt.org/z/r9nEq8KPP

In my view it is worthwhile avoiding an extra 8 bytes on every Statement and Expression. That'd be a lot of bytes in total in an entire AST.

What do you think?

Well, I think this is approachable, Have to let @Boshen make the final call here but I'm with you on this one, I think it is a cleaver solution and the ugliness is mild. It doesn't even need a macro to hide the implementation since it is understandable and reasonable to work with.

We still might want to use a macro like ast_match to hide the struct pattern so people can use it similarly to normal enums.

And are we sure about the predictability of this solution when it comes to how the rust compiler packs the types? What happens to nested enums with this?

[rzvxa]

You are absolutely right

Actually, I realized I am absolutely wrong! If you have a reference to the Statement and the parent BlockStatement, you can deduce the index:

fn statement_index(stmt: &Statement, parent: &BlockStatement) -> usize {
  (stmt as *const _ as usize - parent.body.as_ptr() as usize) / std::mem::size_of::<Statement>()
}

Ditto deducing left or right of BinaryExpression:

fn expr_is_left(expr: &Expression, parent: &BinaryExpression) {
  expr as *const _ as usize
    == parent as *const _ as usize + std::mem::offset_of!(BinaryExpression, left)
}

I can't wrap my head around this one, How does it work?

I think I'm getting it, For the statement example I can see what are you doing but the BinaryExpression one is still an enigma to me since they are values and not boxed references or pointers.

[rzvxa]

@overlookmotel Oh boy, You are a knowledgeable person, I love talking with you. You just shine a new light through every problem. Thanks for the suggestion on finding the leaf position in the parent, It is remarkable!

[overlookmotel]

And are we sure about the predictability of this solution when it comes to how the rust compiler packs the types?

It's predictable enough. From first principles:

enum EnumWithId {
  A { ast_node_id: u32, node: Box<NodeA> },
  B { ast_node_id: u32, node: Box<NodeB> },
  C { ast_node_id: u32, node: Box<NodeC> },
}

• EnumWithId is size 16, align 8.
• Box<NodeA>, Box<NodeB> and Box<NodeC> are size 8, align 8.
• u32 is size 4, align 4.
• The discriminant must be in same location for all variants (otherwise, how can you look it up without already knowing which variant?)

So, to maintain the alignment constraints, the compiler has limited options:

Firstly, Box must be aligned on 8, so can only go in first 8 bytes, or last 8 bytes:

|0 1 2 3 4 5 6 7 8 9 A B C D E F|
|???????????????|Box<Node>......|

|0 1 2 3 4 5 6 7 8 9 A B C D E F|
|Box<Node>......|???????????????|

Then, of the remaining 8 bytes, u32 is aligned on 4, so has to go in either the first 4 or last 4:

|0 1 2 3 4 5 6 7 8 9 A B C D E F|
|???????|u32....|Box<Node>......|

|0 1 2 3 4 5 6 7 8 9 A B C D E F|
|u32....|???????|Box<Node>......|

|0 1 2 3 4 5 6 7 8 9 A B C D E F|
|Box<Node>......|???????|u32....|

|0 1 2 3 4 5 6 7 8 9 A B C D E F|
|Box<Node>......|u32....|???????|

The compiler is free to put the discriminant anywhere in the ?????.

But the point is: Whichever of the 4 possible representations above the compiler chooses, the u32 always has to be next to the discriminant. The discriminant has to be in the same position for all variants, therefore the u32 always has to be in same position for all variants too.

That's all theoretical. In practice the compiler always chooses this repr (D = discriminant):

|0 1 2 3 4 5 6 7 8 9 A B C D E F|
|D|empty|u32....|Box<Node>......|

But even if the compiler chose to do something different in future, all the possible options display the predictability we want.

Important proviso: All this holds true as long as the "payload" has alignment 8 for all variants. This is true for almost all AST types because they pretty much all contain at least one Box, Vec, Atom or &str. But if any types do have a lower alignment, we easily can fix that by making them #[repr(align(8)].

What happens to nested enums with this?

The compiler is not clever enough to combine the discriminants of the inner and outer enums, so it costs 8 bytes (for the inner enum's discriminant). But at worst, that cost just negates the saving you make from the optimization above. So in the most common cases, it saves you bytes, and in the nested enums it doesn't save you anything, but it doesn't cost you anything either.

@overlookmotel Oh boy, You are a knowledgeable person

That's really kind of you to say! But, just to be honest: Yes, I do know this specific area very well - because implementing AST transfer is all about understanding Rust's type layouts. But in other areas I still have a lot to learn. And I'm sure you can teach me!

[overlookmotel]

Actually, a correction: All the above assumes a 64-bit architecture where pointers are aligned on 8. But on WASM32, pointers are aligned on 4 I think, so theoretically the compiler could put u32 and Box in any order. It would be really weird if it did that, but we could const assert that it doesn't if we wanted to be sure.

[overlookmotel]

Sorry, I just realized something: AstNodeId is currently a usize. I had been assuming it's u32.

As far as I'm aware it's not necessary for it to be as big as usize. The parser has a 4 GiB limit on size of source text (i.e. Span::start and Span::end are u32). Presumably it's not possible (or at least reasonable) for a source text of N bytes to produce an AST with more than N x nodes. And therefore AstNodeId can be u32 as well.

If it is technically possible for some bonkers JS input to be u32::MAX bytes and lead to an AST containing more than u32::MAX nodes, I'd suggest we reduce the source text size limit to e.g. 2 GiB so it's impossible for number of nodes to exceed u32::MAX, and then AstNodeId can be u32. 2 GiB is still an insanely large JS file!

[rzvxa]

First off thanks for the thorough explanation.

For architecture, I think it is a sane thing to optimize for 64-bit since nowadays it is tough to find a workstation that isn't running on a 64-bit processor. WASM isn't the right environment for processing really huge JavaScript files to begin with.

---

Sorry, I just realized something: AstNodeId is currently a usize. I had been assuming it's u32.

As far as I'm aware it's not necessary for it to be as big as usize. The parser has a 4 GiB limit on size of source text (i.e. Span::start and Span::end are u32). Presumably it's not possible (or at least reasonable) for a source text of N bytes to produce an AST with more than N x nodes. And therefore AstNodeId can be u32 as well.

If it is technically possible for some bonkers JS input to be u32::MAX bytes and lead to an AST containing more than u32::MAX nodes, I'd suggest we reduce the source text size limit to e.g. 2 GiB so it's impossible for number of nodes to exceed u32::MAX, and then AstNodeId can be u32. 2 GiB is still an insanely large JS file!

It isn't a deal breaker, The Project is heading toward having everything feature-gated so it isn't so far fetch to add some compact-ast flag which would lower the individual file limits to make things more compact and maybe cache-friendly. But lowering the overall limit is something that I'm not fund of, And there are 2 points against this in my opinion.

1. Generated Javascript code bases exist and as you can imagine you can get a lot of boilerplate code trying to express something alien to the language without supervision and using automated tools.
2. We need to be able to parse large bundled codes which can exceed 2 GiB

[rzvxa]

Right now I've shifted my focus toward figuring out the tree mutation with @milesj, I'll revisit this issue in the next week or so. I'll let you know when I have a PR containing any changes related to this subject.