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(semantic): add SymbolId and SymbolFlags to ModuleRecord #5816

Open
Tracked by #5814
DonIsaac opened this issue Sep 16, 2024 · 1 comment
Open
Tracked by #5814

feat(semantic): add SymbolId and SymbolFlags to ModuleRecord #5816

DonIsaac opened this issue Sep 16, 2024 · 1 comment
Labels
A-semantic Area - Semantic C-enhancement Category - New feature or request

Comments

@DonIsaac
Copy link
Collaborator

DonIsaac commented Sep 16, 2024

This allows exporting modules to provide symbol information to modules importing them.

I'm currently thinking we could add it to NameSpan, kind of like this:

pub struct NameSpan {
  name: CompactStr,
  span: Span,
  symbol_id: OnceLock<SymbolId>, // NameSpan must be Send + Sync
  symbol_flags: OnceLock<SymbolFlags>,
}

Binding this data to NameSpan will require a change to when/how ModuleRecordBuilder is used. it currently runs before SemanticBuilder::build. Should we:

  1. Bind symbol info during SemanticBuilder's traversal? Pros: fewer AST iterations. Cons: var hoisting and visit order of export declarations (e.g. export const = 5) may make this challenging
  2. Run ModuleRecordBuilder after Semantic has been built. Pros: much simpler. Cons: requires 2 AST traversals.

Other Questions

  • Is there a better place to put this information?
  • Should we include anything else besides id and flags?
@DonIsaac DonIsaac added C-enhancement Category - New feature or request A-semantic Area - Semantic labels Sep 16, 2024
@overlookmotel
Copy link
Collaborator

overlookmotel commented Sep 17, 2024

API change

We would lose one thing with this change: Currently you can build ModuleRecord without also building Semantic, but after this change you won't be able to, as it needs to run semantic analysis to generate SymbolIds.

But is that an important ability to preserve? (I doubt it, oxc_module_lexer doesn't use oxc_semantic). @Boshen?

We'd need to change API to something like one of these:

let semantic = SemanticBuilder::new(&source_text)
        .with_module_record(path)
        .build(&program);
let module_record = &semantic.module_record;
let semantic = SemanticBuilder::new(&source_text).build(&program);
let module_record = semantic.build_module_record(path, &program);
let semantic = SemanticBuilder::new(&source_text).build(&program);
let module_record = ModuleRecordBuilder::new(path, &semantic).build(&program);

Personally, I think any of these is an improvement. I find it unintuitive that SemanticBuilder uses the with_* pattern for everything else, but build_module_record behaves differently.

Implementation

  1. Run ModuleRecordBuilder after Semantic has been built. Pros: much simpler. Cons: requires 2 AST traversals.

ModuleRecordBuilder doesn't do a full traversal, does it? It just iterates through the Vec of top-level statements. So a 2nd pass is not super-costly.

But if you want to avoid that, this should work:

Imports: You can get the SymbolId of x in import x from 'foo'; during the main traversal, as it binds itself.

Exports: You cannot get the SymbolId of x in export {x}; when that statement is first visited, as x is not yet resolved to a symbol. Resolution has to happen in a 2nd pass once all the SymbolIds have been created and references resolved to them. But... can avoid a full 2nd pass through top-level statements by either:

  1. During main traversal, record statement indexes for export statements, then re-visit just those statements at the end.

or:

  1. During main traversal, fill a temp Vec with ReferenceIds used in export statements. At the end, loop through those ReferenceIds and resolve them to SymbolIds.

Either way, this would require ModuleRecord to be built by SemanticBuilder, so it'd need to be this API:

let semantic = SemanticBuilder::new(&source_text)
        .with_module_record(path)
        .build(&program);
let module_record = &semantic.module_record;

PS: I think you can avoid the OnceLocks too.

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

No branches or pull requests

2 participants