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

Incorrect data in SymbolTable for TS enums #8350

Open
overlookmotel opened this issue Jan 8, 2025 · 1 comment
Open

Incorrect data in SymbolTable for TS enums #8350

overlookmotel opened this issue Jan 8, 2025 · 1 comment
Labels
A-semantic Area - Semantic C-bug Category - Bug

Comments

@overlookmotel
Copy link
Contributor

overlookmotel commented Jan 8, 2025

Possibly relevant to #8342. The way we record references between TS enum fields in SymbolTable is wonky.

The problem

This is legal:

enum Foo {
  A = 123,
  B = A, // `A` evaluates to 123
}

enum Foo {
  C = A, // `A` evaluates to 123
}

Playground

i.e. it's essentially the same as:

enum Foo {
  A = 123,
  B = A, // `A` evaluates to 123
  C = A, // `A` evaluates to 123
}

Our output from transformer for this case is correct, but the data in SymbolTable is not:

  • A here gets a Symbol. But it only has 1 reference, not 2.
  • A in 2nd enum block has a ReferenceId, but that reference has no SymbolId (i.e. SymbolTable says it resolves to global.A.

If you add let A = 'hello'; at top level, then Reference for A in 2nd enum block has SymbolId pointing to let A (similar to the problem in #8342).

Why this matters

  1. If we can correctly resolve identifiers in Semantic it'd make solving transformer: tsc vs babel on enum transformation #8342 trivial.
  2. Correct symbol resolution matters in the linter for e.g. no-unused-vars rule.
  3. We want our semantic data to be correct!

Why it's hard to solve

The binding of enum fields does not follow our model for symbols/reference. It's unlike anything else.

Currently the Symbol for A is bound in the scope for 1st enum's body block. When there's only 1 enum block for an enum, this makes sense. That binding is not visible outside the block. But when there are 2 or more enum {} blocks comprising the enum, it breaks down.

So we need to hoist the binding up to enclosing scope (top level scope in this case) so it is in scope of both enum blocks.

But then how do we deal with this?

enum Foo {
  A = 1,
}

enum Bar {
  A = 2,
}

enum Qux {
  A = 3,
}

Here we have 3 x A bindings which are not the same. We can't put them all in top level scope as they'd all occupy the same "slot", and get conflated.

We have a similar problem with separate namespaces for types and values:

type A = number;
let A = 'hello';

We handle that by making A only one Symbol, and distinguish whether an IdentifierReference is referring to type A or let A by setting/not setting ReferenceFlags::Type.

But we can't use that trick for enums, because there can be any number of bindings with the same name. An on/off flag doesn't cover it.

Possible solution

I can only think of one solution, which unfortunately feels quite hacky.

Ultimately the problem is: An enum field binding is uniquely identified by the field name plus enum name.

Solution:

  • Record enum field bindings with pseudo-name <enum name>.<field name> i.e. Foo.A, Bar.A etc.
  • . is illegal in real identifiers so these pseudo-names cannot conflict with any other bindings.
  • Give these bindings SymbolFlags::EnumField.
  • In SemanticBuilder, resolve IdentifierReferences inside enums using these pseudo-symbols.
  • We can then rely on correctness of the data in SymbolTable in the TS enums transform, and simplify it considerably.
  • Everywhere else, following Reference -> Symbol will work as expected with no "special case" workarounds for enum fields.

Like I said, this feels unsatisfyingly hacky. Can anyone see a better way?

@overlookmotel overlookmotel added C-bug Category - Bug A-semantic Area - Semantic labels Jan 8, 2025
@Dunqing
Copy link
Member

Dunqing commented Jan 9, 2025

Just a quick look, I think It would be easy if they had the same scope_id and symbol_id in the multiple enum with the same binding name. I will look at how TypeScript handles when I finish my work.

In the meantime, I seem to have thought of how to transform const enum in Rolldown. We can build an EnumTable to store all enum data (including non-const, which we can refactor our enum transformation by using this data), then Rolldown can use the data like they use ScopeTree and SymbolTable. I have a rough picture of this, which needs to verify later.

EDIT: Just found out @Boshen has the same thought in #6073.

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

No branches or pull requests

2 participants