Combine Statement and Declaration types

[overlookmotel]

Would it be a pain if we merged AST Statement and Declaration types into one?

Currently:

pub enum Statement<'a> {
    BlockStatement(Box<'a, BlockStatement<'a>>),
    /* ... */
    ModuleDeclaration(Box<'a, ModuleDeclaration<'a>>),

    Declaration(Declaration<'a>),
}

pub enum Declaration<'a> {
    VariableDeclaration(Box<'a, VariableDeclaration<'a>>),
    /* ... */
    TSImportEqualsDeclaration(Box<'a, TSImportEqualsDeclaration<'a>>),
}

Could we make it just 1 enum combining all the variants:

pub enum Statement<'a> {
    // From `Statement`
    BlockStatement(Box<'a, BlockStatement<'a>>),
    /* ... */
    ModuleDeclaration(Box<'a, ModuleDeclaration<'a>>),

    // From `Declaration`
    VariableDeclaration(Box<'a, VariableDeclaration<'a>>),
    /* ... */
    TSImportEqualsDeclaration(Box<'a, TSImportEqualsDeclaration<'a>>),
}

I'm asking because this would simplify #2409. It is still possible to make it work without this change, but it's a bit more brittle.

To explain:

For AST transfer, ideally we want all enums to be #[repr(u8)] so the byte value for each option's discriminant can be specified explicitly. Then they're completely locked and you can read the bytes representing a Statement in memory, and reliably interpret them.

But in this case, the compiler is cleverly merging the discriminants for Statement and Declaration using niche optimization, which means Statement is 16 bytes. If we make Statement #[repr(u8)], it disables the niche optimization, and Statement becomes 24 bytes. Not good!

There is a way around this without changing the types: Don't make Statement #[repr(u8)], and the compiler in practice is predictable in what discriminants it assigns. But the compiler makes no promise not to change that at any minute without warning, so while I don't think likely that it will break, it could break at any time.

To get to the point... It'd be simpler to sidestep the issue entirely by folding Declaration's variants into Statement. Question is how much of a pain would that be? So is it worth it?

NB: The same problem applies to a few other types (only ExportDefaultDeclarationKind and AssignmentTargetMaybeDefault that I'm aware of, but maybe there's a few more). But of course Statement is the one which gets used absolutely everywhere.

[Boshen]

Let me give this a try and see if the APIs around this part of the AST become difficult to use.

[Boshen]

🤔 The node is reused here

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

Lines 2498 to 2501 in 0f86333

	pub struct ExportNamedDeclaration<'a> {
	#[serde(flatten)]
	pub span: Span,
	pub declaration: Option<Declaration<'a>>,

Edit:

jsparagus:

#[derive(Debug, PartialEq)]
pub enum Export<'alloc> {
    FunctionDeclaration(Function<'alloc>),
    ClassDeclaration(ClassDeclaration<'alloc>),
    VariableDeclaration(VariableDeclaration<'alloc>),
}

#[derive(Debug, PartialEq)]
pub enum ExportDefault<'alloc> {
    FunctionDeclaration(Function<'alloc>),
    ClassDeclaration(ClassDeclaration<'alloc>),
    Expression(arena::Box<'alloc, Expression<'alloc>>),
}

🤔

let me try this tomorrow.

[overlookmotel]

🤔 The node is reused here

Ah. That does complicate matters.

Just to repeat, we don't have to do this. There is another way around it, it's just a bit more brittle.

[Boshen]

After some experimentation (2 hours 😅 ), I conclude the current structure is better than everything flattened.

[overlookmotel]

OK cool. Thanks for trying it out. I'll switch to plan B.