feat(parser)!: use BindingIdentifier for namespace declaration names

[DonIsaac]

Use BindingIdentifier instead of IdentifierName so that AST visitors can access the bound symbol id for the namespace's name. This makes namespace consistent with other named declarations, such as Class, Function, and TSInterfaceDeclaration.

We should consider modifying TSModuleDeclarationName::StringLiteral(...) to also store a symbol_id, since one gets bound in semantic for it as well.

[graphite-app]

Your org has enabled the Graphite merge queue for merging into main

Add the label “0-merge” to the PR and Graphite will automatically add it to the merge queue when it’s ready to merge. Or use the label “hotfix” to add to the merge queue as a hot fix.

You must have a Graphite account and log in to Graphite in order to use the merge queue. Sign up using this link.

[DonIsaac]

• feat(ast): add TSModuleDeclarationName::binding_identifier() #6005
• fix(semantic): do not create a global symbol for declare global {} #6004
• feat(parser)!: use BindingIdentifier for namespace declaration names #6003 👈
• main

This stack of pull requests is managed by Graphite. Learn more about stacking.

Join @DonIsaac and the rest of your teammates on Graphite

[codspeed-hq]

CodSpeed Performance Report

Merging #6003 will not alter performance

_{Comparing don/09-23-feat_parser_use_bindingidentifier_for_namespace_declaration_names (01b878e) with main (87f700f)}

Summary

✅ 29 untouched benchmarks

[Boshen]

Let me check the spec.

[Boshen]

This is incorrect, it's not a binding identifier. Although the "global" fix is correct in the next PR.

[DonIsaac]

How should we make a namespace's symbol is available from the AST?

[Boshen]

How should we make a namespace's symbol is available from the AST?

Can you elaborate?

[DonIsaac]

How should we make a namespace's symbol is available from the AST?

Can you elaborate?

BindingIdentifier has a symbol_id property, but IdentifierName does not. In this snippet, a symbol Foo is bound and put into SymbolTable

namespace Foo {}

but since TSModuleDeclarationName is an IdentifierReference, Foo's symbol id is not available in the AST.

[Boshen]

How should we make a namespace's symbol is available from the AST?

Can you elaborate?

BindingIdentifier has a symbol_id property, but IdentifierName does not. In this snippet, a symbol Foo is bound and put into SymbolTable

namespace Foo {}

but since TSModuleDeclarationName is an IdentifierReference, Foo's symbol id is not available in the AST.

Let me double check the archived tsc reference...

[overlookmotel]

In my view, regardless of what TS's spec says, in our scheme, anything which has a record in the SymbolTable must have a symbol_id field. Without that field, our transform checker is not able to check the correctness of SymbolTable. I was planning to open a PR doing exactly this when I come to fix the scopes/symbols manipulation in TS transform.

I assume it being IdentifierName is a leftover from before we started resolving TS type references.

I suppose we could add symbol_id field to TSModuleDeclaration instead, but in my view it's much simpler to make the id a BindingIdentifier, as this PR does. Doing it this way also means that visit_identifier will get all `

So am re-opening this and, if you're willing @Boshen, would like to merge it.

[Boshen]

I need proof and reference from tsc to confirm this is a BindingIdentifier.

[DonIsaac]

@Boshen from section 10.1 Module Declarations in the TypeScript v1.4 spec:

ModuleDeclaration:
  module IdentifierPath { ModuleBody }
IdentifierPath:
  Identifier
  IdentifierPath . Identifier

The TypeScript spec uses Identifier for all other declarations. For example, here is the grammar for ClassDeclaration in section 8.1 Class Declarations:

ClassDeclaration:
  class Identifier TypeParametersopt ClassHeritage { ClassBody }

Our Class node uses a BindingIdentifier, meaning Identifier in TypeScript is the same as BindingIdentifier in oxc.

[Boshen]

I checked binder.ts

It calls function bindModuleDeclaration(node: ModuleDeclaration) {

and then

    function declareModuleSymbol(node: ModuleDeclaration): ModuleInstanceState {
        const state = getModuleInstanceState(node);
        const instantiated = state !== ModuleInstanceState.NonInstantiated;
        declareSymbolAndAddToSymbolTable(
            node,
            instantiated ? SymbolFlags.ValueModule : SymbolFlags.NamespaceModule,
            instantiated ? SymbolFlags.ValueModuleExcludes : SymbolFlags.NamespaceModuleExcludes,
        );
        return state;
    }

[graphite-app]

Merge activity

• Oct 8, 4:34 AM EDT: The merge label '0-merge' was detected. This PR will be added to the Graphite merge queue once it meets the requirements.
• Oct 8, 4:34 AM EDT: The merge label '0-merge' was detected. This PR will be added to the Graphite merge queue once it meets the requirements.
• Oct 8, 4:39 AM EDT: Boshen added this pull request to the Graphite merge queue.
• Oct 8, 4:47 AM EDT: Boshen merged this pull request with the Graphite merge queue.