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

feat(traverse): add scope flags to TraverseCtx #3229

Merged

Conversation

overlookmotel
Copy link
Collaborator

@overlookmotel overlookmotel commented May 10, 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.

Copy link
Collaborator Author

overlookmotel commented May 10, 2024

Copy link

codspeed-hq bot commented May 10, 2024

CodSpeed Performance Report

Merging #3229 will not alter performance

Comparing 05-10-feat_traverse_add_scope_flags_to_traversectx_ (46c02ae) with main (132db7d)

Summary

✅ 27 untouched benchmarks

Copy link
Member

@Boshen Boshen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I may have missed this, where is the logic for checking "use strict" inside a function?

Edit: oh it's in another PR.

Copy link
Member

Boshen commented May 11, 2024

Merge activity

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 Boshen force-pushed the 05-10-refactor_ast_order_ast_type_fields_in_visitation_order branch from 122b361 to 4208733 Compare May 11, 2024 04:40
@Boshen Boshen force-pushed the 05-10-feat_traverse_add_scope_flags_to_traversectx_ branch from 365a12f to 46c02ae Compare May 11, 2024 04:41
@Boshen Boshen changed the base branch from 05-10-refactor_ast_order_ast_type_fields_in_visitation_order to main May 11, 2024 04:45
@graphite-app graphite-app bot merged commit 46c02ae into main May 11, 2024
26 checks passed
@graphite-app graphite-app bot deleted the 05-10-feat_traverse_add_scope_flags_to_traversectx_ branch May 11, 2024 04:46
@overlookmotel
Copy link
Collaborator Author

@Boshen FYI The logic for checking for "use strict" is in the #[visited_node] attr on Function (strict_if clause):

#[visited_node(
scope(ScopeFlags::Function),
// Don't create a 2nd scope if `MethodDefinition` already created one
scope_if((ctx.scope() & ScopeFlags::Modifiers).is_empty()),
strict_if(self.body.as_ref().is_some_and(|body| body.has_use_strict_directive()))
)]
#[derive(Debug, Hash)]
#[cfg_attr(feature = "serialize", derive(Serialize, Tsify))]
#[cfg_attr(feature = "serialize", serde(rename_all = "camelCase"))]
pub struct Function<'a> {

The additional is_use_strict and has_use_strict_directive methods added in PR further down the stack were just to make the code in #[visited_node] less verbose.

Boshen pushed a commit that referenced this pull request May 11, 2024
Fixes a bug in #3229.

The logic to prevent a duplicate scope being created for a `Function` which is a `MethodDefinition` would also stop a scope being created for inner function in:

```rs
class X {
  foo() {
    function bar() {}
  }
}
```

or

```rs
class X {
  foo( bar = function() {} ) {}
}
```

This PR fixes that. This change also allows removing `ScopeFlags::Method` which #3229 added.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-ast Area - AST
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add scopes support to Traverse / transformer
2 participants