Skip to content

Traverse: Fix and improve design of ChildScopeCollector #9702

@overlookmotel

Description

@overlookmotel

ChildScopeCollector in oxc_traverse has a bug and is poorly designed (both my fault).

ChildScopeCollector traverses the AST until it hits an AST node which has a scope. It records the ScopeId of that scope in a Vec<ScopeId>, and then does not traverse any further from that node.

Result is that it only collects the IDs of direct child scopes of the node you start from (and not more deeply nested scopes - grandchild scopes etc).

The bug

Some nodes enter the scope late or exit it early e.g. Class.

pub struct Class<'a> {
pub span: Span,
pub r#type: ClassType,
/// Decorators applied to the class.
///
/// Decorators are currently a stage 3 proposal. Oxc handles both TC39 and
/// legacy TypeScript decorators.
///
/// ## Example
/// ```ts
/// @Bar() // <-- Decorator
/// class Foo {}
/// ```
#[ts]
pub decorators: Vec<'a, Decorator<'a>>,
/// Class identifier, AKA the name
pub id: Option<BindingIdentifier<'a>>,
#[scope(enter_before)]
#[ts]
pub type_parameters: Option<Box<'a, TSTypeParameterDeclaration<'a>>>,
/// Super class. When present, this will usually be an [`IdentifierReference`].
///
/// ## Example
/// ```ts
/// class Foo extends Bar {}
/// // ^^^
/// ```
pub super_class: Option<Expression<'a>>,
/// Type parameters passed to super class.
///
/// ## Example
/// ```ts
/// class Foo<T> extends Bar<T> {}
/// // ^
/// ```
#[ts]
pub super_type_parameters: Option<Box<'a, TSTypeParameterInstantiation<'a>>>,
/// Interface implementation clause for TypeScript classes.
///
/// ## Example
/// ```ts
/// interface Bar {}
/// class Foo implements Bar {}
/// // ^^^
/// ```
#[ts]
pub implements: Option<Vec<'a, TSClassImplements<'a>>>,
pub body: Box<'a, ClassBody<'a>>,
/// Whether the class is abstract
///
/// ## Example
/// ```ts
/// class Foo {} // true
/// abstract class Bar {} // false
/// ```
#[ts]
pub r#abstract: bool,
/// Whether the class was `declare`ed
///
/// ## Example
/// ```ts
/// declare class Foo {}
/// ```
#[ts]
pub declare: bool,
/// Id of the scope created by the [`Class`], including type parameters and
/// statements within the [`ClassBody`].
pub scope_id: Cell<Option<ScopeId>>,
}

The current implementation in ChildScopeCollector for Class is:

fn visit_class(&mut self, it: &Class<'a>) {
self.add_scope(&it.scope_id);
}

The problem is that it's assuming that all child nodes within Class are within Class's scope. This is wrong - decorators field is outside of the Class's scope.

So in this example the scope of the arrow function within the decorator is a direct child scope, and should be collected, but currently it's not:

@((...args) => decorateIt(args))
class C {}

It should be:

    fn visit_class(&mut self, it: &Class<'a>) {
        self.visit_decorators(&it.decorators);
        self.add_scope(&it.scope_id);
    } 

There are probably other similar bugs for other node types where the node's scope does not enclose all its fields.

The bad design

Currently ChildScopeCollector collects child scope IDs into a Vec.

This temporary allocation is unnecessary. It would be better if it took a callback function which is called on each scope, to avoid constructing a Vec.

Or it could be a trait:

trait ChildScopesVisitor {
    fn visit_child_scope(&mut self, scope_id: ScopeId);
}

impl<'a, V: ChildScopesVisitor> Visit<'a> for V {
    fn visit_block_statement(&mut self, block: &BlockStatement<'a>) {
        self.visit_child_scope(block.scope_id());
    }

    // ... visitors for all other types which have scopes ...
}

Then user would implement as:

struct MyVisitor;

impl ChildScopesVisitor for MyVisitor {
    fn visit_child_scope(&mut self, scope_id: ScopeId) {
        // Do whatever with `scope_id`
    }
}

let mut visitor = MyVisitor;
visitor::visit_statement(&stmt);

Re-implement in oxc_ast_tools

The code which generates ChildScopeCollector is currently implemented in JS in oxc_traverse. I did this for speed of implementation, but it's not ideal. We should re-implement the generator in Rust in oxc_ast_tools.

Probably we should do that first, before making the other changes described above.

Metadata

Metadata

Assignees

Labels

A-transformerArea - Transformer / TranspilerC-bugCategory - BugC-cleanupCategory - technical debt or refactoring. Solution not expected to change behavior

Type

No type

Projects

No projects

Milestone

No milestone

Relationships

None yet

Development

No branches or pull requests

Issue actions