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

Add scopes support to Traverse / transformer #3189

Closed
overlookmotel opened this issue May 7, 2024 · 6 comments · Fixed by #3229 or #3314
Closed

Add scopes support to Traverse / transformer #3189

overlookmotel opened this issue May 7, 2024 · 6 comments · Fixed by #3229 or #3314
Assignees
Labels
A-transformer Area - Transformer / Transpiler

Comments

@overlookmotel
Copy link
Collaborator

The intent is to replace VisitMut in transformer with the new oxc_traverse::Traverse. #3182 is a starting point for that.

However, transformers require scope information, which is not yet implemented.

VisitMut provides only fairly minimal information - just ScopeFlags indicating:

  • What kind of environment visitor is in (top level / function / method / class static block / etc).
  • Whether strict mode or not.

This would be fairly easy to replicate in Traverse.

However, I assume transformers actually need more info than this, probably more like the info available from semantic. Opening this issue to discuss:

  1. What scope info transformers need.
  2. How to implement it.
@overlookmotel overlookmotel added the C-bug Category - Bug label May 7, 2024
@overlookmotel overlookmotel added A-transformer Area - Transformer / Transpiler and removed C-bug Category - Bug labels May 7, 2024
@Boshen
Copy link
Member

Boshen commented May 7, 2024

What scope info transformers need.

All we need are the scope flags.

During traversing, we keep a stack of scopes, and manage binding information while entering / leaving scopes.

How to implement it.

The coolest approach is to treat scope as an individual AST node, something like

struct Scope<T> {
  inner: T
}

pub struct TryStatement<'a> {
    pub block: Box<'a, Scope<BlockStatement<'a>>>,
    pub handler: Option<Box<'a, Scope<CatchClause<'a>>>>,
    pub finalizer: Option<Box<'a, Scope<BlockStatement<'a>>>>,
}

But actually I've got no idea on how to implement it until I get my hands dirty on the new traverse API.

@overlookmotel
Copy link
Collaborator Author

overlookmotel commented May 7, 2024

If that's all we need, how about TraverseCtx containing stack of ScopeFlags which it pushes to when entering a scope, and pops off when leaving it?

Traverse can handle updating whether context is strict mode or not itself, including inheriting previous state. No need for enter_scope and exit_scope to do that.

You'd use it as follows:

fn enter_binary_expression(&mut self, fn: &mut BinaryExpression<'a>, ctx: &TraverseCtx<'a>) {
  if ctx.scope.is_strict_mode() {
    // ...
  }
}

We could signal to the codegen when to update the scope stack with something like:

#[visited_node(scope = "Function")]
pub struct Function<'a> {
    pub r#type: FunctionType,
    // ...
}

NB: #[visited_node] macro is a no-op. It passes through the input unchanged without even parsing it, so is very cheap. It's just a way to convey information to the codegen which generates Traverse trait and walk_* functions etc.

@Boshen
Copy link
Member

Boshen commented May 7, 2024

I need to store things associated with a scope, but I think I can work with what you proposed to get things started.

@overlookmotel
Copy link
Collaborator Author

overlookmotel commented May 7, 2024

Actually you don't need ScopeFlags to answer the question "am I in a function?".

Something like this would do the same:

let in_function = ctx.find_ancestor(
  |ancestor| if ancestor.is_function() || ancestor.is_arrow_function_expression() {
    FinderRet::Found(())
  } else {
    FinderRet::Continue
  }
).is_some();

ScopeFlags is worth implementing to make this cheaper if it's a question we need to ask a lot in visitors. I assume it is, right?

@overlookmotel
Copy link
Collaborator Author

Related to #2859.

Boshen pushed a commit that referenced this issue May 11, 2024
Add scope flags to `TraverseCtx`.

Closes #3189.

`walk_*` functions build a stack of `ScopeFlags` as AST is traversed, and they can be queried from within visitors with `ctx.scope()`, `ctx.ancestor_scope()` and `ctx.find_scope()`.

The codegen which generates `walk_*` functions gets the info about which AST types have scopes, and how to check for strict mode from the `#[visited_node]` attrs on AST type definitions in `oxc_ast`.

A few notes:

Each scope inherits the strict mode flag from the level before it in the stack, so if you need to know "am I in strict mode context here?", `ctx.scope().is_strict_mode()` will tell you - no need to travel back up the stack to find out.

Scopes do *not* inherit any other flags from level before it. So `ctx.scope()` in a block nested in a function will return `ScopeFlags::empty()` not `ScopeFlags::Function`.

I had to add an extra flag `ScopeFlags::Method`. The reason for this is to deal with when a `Function` is actually a `MethodDefinition`, and to avoid creating 2 scopes in this case. The principle I'm trying to follow is to encode as little logic in the codegen as possible, as it's rather hidden away. Instead the codegen follows a standard logic for every node, guided by attributes which are visible next to the types in `oxc_ast`. This hopefully makes how `Traverse`'s visitors are generated less mysterious, and easier to change.

The case of `Function` within `MethodDefinition` is a weird one and would not be possible to implement without encoding a magic "special case" within the codegen without this extra `ScopeFlags::Method` variant. Its existence does not alter the operation of any other code in Oxc which uses `ScopeFlags`.

In my view `ScopeFlags` might benefit from a little bit of an overhaul anyway. I believe we could pack more information into the bits and make it more useful.
Boshen pushed a commit that referenced this issue May 16, 2024
Allow mutable access to scopes tree and symbol table.

Closes #3189.

This completes the v1 scopes-in-traverse implementation, and provides all the primitives required to implement the missing APIs listed in #3251.

Performance is abysmal, as noted in #3304, but we can fix that later on by taking `Semantic` out of the picture, or optimizing it.
@overlookmotel
Copy link
Collaborator Author

#3314 gave Traverse full scopes support.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-transformer Area - Transformer / Transpiler
Projects
None yet
2 participants