oxc-project / backlog

backlog for collborators only
1 stars 0 forks source link

Add `Scoping` structure combining `ScopeTree` and `SymbolTable` #92

Open overlookmotel opened 3 months ago

overlookmotel commented 3 months ago

To my knowledge, there is nowhere where we need either ScopeTree or SymbolTable by itself.

There are various methods on TraverseCtx which need access to both ScopeTree and SymbolTable. These would be better located in oxc_semantic so that they can be used more widely beyond the transformer. But we can't implement them on Semantic, as Traverse does not have access to a full Semantic, only ScopeTree and SymbolTable.

2 possible solutions:

1. New struct

We should probably also move references field out of SymbolTable and put it in its own struct References. It seems arbitrary that symbols and references are stored separately.

2. New trait

Add a trait Scopes which can be implemented on any struct which has access to both ScopeTree and SymbolTable. Then implement all the methods from ScopeTree, SymbolTable and TraverseCtx on this trait.

trait Scopes {
    // Methods Implemented in `impl Scopes for Semantic` and `impl Scopes for TraverseCtx`
    fn scopes(&self) -> &ScopeTree;
    fn scopes_mut(&mut self) -> &mut ScopeTree;
    fn symbols(&self) -> &SymbolTable;
    fn symbols_mut(&mut self) -> &mut SymbolTable;

    // Methods which use both implemented here
    fn get_symbol_flags_for_reference(&self, reference_id: ReferenceId) -> Option<SymbolFlags> {
        let reference = self.get_reference(reference_id);
        match reference.symbol_id() {
            Some(symbol_id) => Some(self.symbols().get_flags(symbol_id)),
            None => None,
        }
    }
}

Could also implement another trait for any object which has access to ScopeTree, SymbolTable and AstBuilder, which could contain methods which use scoping info to create AST nodes. e.g. generate a UID Expression::Identifier (that requires AstBuilder to allocate it into arena).

Which?

Personally I prefer the struct approach. That'd be a large breaking change, but it will also greatly simplify (a) passing ScopeTree + SymbolTable around and (b) method calls - they're all on Scopes.

I think we'll need to change ScopeTree and SymbolTable APIs quite a bit anyway to optimize them (#32, #11), so we're likely to have to make breaking changes anyway.

overlookmotel commented 3 months ago

@Boshen Are you aware of Rolldown ever retaining only ScopeTree or SymbolTable by itself? Or is it a case of "where there is one, there is the other"? I suspect the latter, but just checking...

Boshen commented 3 months ago

@Boshen Are you aware of Rolldown ever retaining only ScopeTree or SymbolTable by itself? Or is it a case of "where there is one, there is the other"? I suspect the latter, but just checking...

Yeah they always come in pairs.