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

Codegen AST related code #3819

Open
4 of 7 tasks
Boshen opened this issue Jun 22, 2024 · 5 comments
Open
4 of 7 tasks

Codegen AST related code #3819

Boshen opened this issue Jun 22, 2024 · 5 comments
Assignees
Labels
C-enhancement Category - New feature or request

Comments

@Boshen
Copy link
Member

Boshen commented Jun 22, 2024

Background

For keeping compilation speed down for ourselves and our downstream crate users, I have avoided using macros to generate boilerplate code at all cost.

All performance issues (runtime and compilation speed) are considered as bugs in this project.

But due to the amount of AST related supporting code has expanded over the last few months, and our desire to experiment with different ideas with the AST, our current approach of manually updating everything is no longer functional.

The goal of this task is to provide code generation for all AST node related code.

Requirements

  • no build.rs published to the user
  • all generated code are checked into git
  • no nightly
  • Rust code residing in crates/oxc_ast should be the source of truth

Workflow

  • a user changes the oxc_ast crate
  • a watch change picks it up
  • parse all #[visited_node]
  • saves them into a model, e.g. struct ASTModel { name: String, attributes: Vec<Attributes> }
  • generate code and save file

Code to generate

  • ASTKind (Milestone 1)
  • GetSpan (Milestone 2)
  • Hash, PartialEq implementations (Milestone 3)
  • ASTBuilder (Milestone 4)
  • Visit (Milestone 5)
  • VisitMut (Milestone 5)
  • Traverse, remove current js code (Milestone 6)
  • typescript definition for napi and wasm moving the scope to AST transfer.

Every milestone should be a stack of PRs, since they will involve changing other files as well.

@Boshen Boshen added the C-enhancement Category - New feature or request label Jun 22, 2024
@overlookmotel
Copy link
Collaborator

The AST also includes some types which are in oxc_syntax e.g. AssignmentOperator, BinaryOperator etc. Should we move those types into oxc_ast to simplify which files the codegen needs to read?

(I've never been clear why these types are in oxc_syntax, since they are part of AST)

@Boshen
Copy link
Member Author

Boshen commented Jun 22, 2024

Should we move

No, let's not scope creep.

There are other traits and functions that depend on these common operators.

@Boshen
Copy link
Member Author

Boshen commented Jul 2, 2024

Are we able to manually inline @inherit MemberExpression and generate all the other utilities for these inherit_variants! enums?

@rzvxa
Copy link
Collaborator

rzvxa commented Jul 2, 2024

Are we able to manually inline @inherit MemberExpression and generate all the other utilities for these inherit_variants! enums?

Can you elaborate on it? Wouldn't we lose some information if we don't express the inheritance in the type system? While the actual code would behave the same, It might be useful for the user to see this as the definition:

inherit_variants! {
/// Argument
///
/// Inherits variants from [`Expression`]. See [`ast` module docs] for explanation of inheritance.
///
/// [`ast` module docs]: `super`
#[visited_node]
#[repr(C, u8)]
#[derive(Debug, Hash)]
#[cfg_attr(feature = "serialize", derive(Serialize, Tsify))]
#[cfg_attr(feature = "serialize", serde(untagged))]
pub enum Argument<'a> {
    SpreadElement(Box<'a, SpreadElement<'a>>) = 64,
    // `Expression` variants added here by `inherit_variants!` macro
    @inherit Expression
}
}

Instead of this:

/// Argument
///
/// Inherits variants from [`Expression`]. See [`ast` module docs] for explanation of inheritance.
///
/// [`ast` module docs]: `super`
#[visited_node]
#[repr(C, u8)]
#[derive(Debug, Hash)]
#[cfg_attr(feature = "serialize", derive(Serialize, Tsify))]
#[cfg_attr(feature = "serialize", serde(untagged))]
pub enum Argument<'a> {
    SpreadElement(Box<'a, SpreadElement<'a>>) = 64,

    BooleanLiteral(Box<'a, BooleanLiteral>) = 0,
    NullLiteral(Box<'a, NullLiteral>) = 1,
    NumericLiteral(Box<'a, NumericLiteral<'a>>) = 2,
    BigIntLiteral(Box<'a, BigIntLiteral<'a>>) = 3,
    RegExpLiteral(Box<'a, RegExpLiteral<'a>>) = 4,
    StringLiteral(Box<'a, StringLiteral<'a>>) = 5,
    TemplateLiteral(Box<'a, TemplateLiteral<'a>>) = 6,

    Identifier(Box<'a, IdentifierReference<'a>>) = 7,

    MetaProperty(Box<'a, MetaProperty<'a>>) = 8,
    Super(Box<'a, Super>) = 9,

    ArrayExpression(Box<'a, ArrayExpression<'a>>) = 10,
    ArrowFunctionExpression(Box<'a, ArrowFunctionExpression<'a>>) = 11,
    AssignmentExpression(Box<'a, AssignmentExpression<'a>>) = 12,
    AwaitExpression(Box<'a, AwaitExpression<'a>>) = 13,
    BinaryExpression(Box<'a, BinaryExpression<'a>>) = 14,
    CallExpression(Box<'a, CallExpression<'a>>) = 15,
    ChainExpression(Box<'a, ChainExpression<'a>>) = 16,
    // .
    // .
    // .
}

On another note, if we get the macro caching to work (related to this) we can have this without any compilation penalty which I think is much superior(And I guess @overlookmotel would agree).

/// Argument
///
/// Inherits variants from [`Expression`]. See [`ast` module docs] for explanation of inheritance.
///
/// [`ast` module docs]: `super`
#[inheritance]
#[visited_node]
#[repr(C, u8)]
#[derive(Debug, Hash)]
#[cfg_attr(feature = "serialize", derive(Serialize, Tsify))]
#[cfg_attr(feature = "serialize", serde(untagged))]
pub enum Argument<'a> {
    SpreadElement(Box<'a, SpreadElement<'a>>) = 64,
    #[inherit] // or maybe `#[inline]` since it is more rust-like?
    Expression(Expression<'a>),
}

@rzvxa
Copy link
Collaborator

rzvxa commented Jul 2, 2024

If we want to have the inheritance expanded we should do the expansion with the codegen to keep it consistent and schema close to the ECMAspec. But IMHO, manually inlining the variants is going to be a maintenance nightmare. because they would easily drift apart with some time and careless reviewing(which always happens no matter how many people review the same piece of code).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-enhancement Category - New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants