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

No SymbolId for declare class C {} #7278

Open
overlookmotel opened this issue Nov 14, 2024 · 8 comments · May be fixed by #8038 or #8039
Open

No SymbolId for declare class C {} #7278

overlookmotel opened this issue Nov 14, 2024 · 8 comments · May be fixed by #8038 or #8039
Assignees
Labels
A-semantic Area - Semantic C-bug Category - Bug

Comments

@overlookmotel
Copy link
Contributor

overlookmotel commented Nov 14, 2024

Semantic does not produce a SymbolId for C in declare class C {}.

I believe it should - all BindingIdentifiers should have a SymbolId, and we do produce SymbolIds for other TS-only identifiers e.g. type N = number;.

In this example, only A gets assigned a SymbolId:

class A {}
declare class B {}
export declare class C {}

Playground

Relevant code:

fn bind(&self, builder: &mut SemanticBuilder) {
if !self.declare {
let Some(ident) = &self.id else { return };
let symbol_id = builder.declare_symbol(
ident.span,
&ident.name,
SymbolFlags::Class,
if self.is_declaration() {
SymbolFlags::ClassExcludes
} else {
SymbolFlags::empty()
},
);
ident.symbol_id.set(Some(symbol_id));
}
}

@overlookmotel overlookmotel added C-bug Category - Bug A-semantic Area - Semantic labels Nov 14, 2024
@Boshen
Copy link
Member

Boshen commented Nov 28, 2024

I couldn't figure out where in binder.ts permits

declare class F {}
function F() {}

but not

class F {}
function F() {}

I need to debug binder.ts :-/

@Dunqing
Copy link
Member

Dunqing commented Dec 5, 2024

We need this to remove declare binding from export {}. See rolldown/rolldown#3048 (comment)

@Dunqing
Copy link
Member

Dunqing commented Dec 17, 2024

Ditto for the declare function

@ShuiRuTian ShuiRuTian linked a pull request Dec 20, 2024 that will close this issue
@ShuiRuTian
Copy link
Contributor

I had a try to fix declare class by #8038

Here is how binder.ts handles this(binder is rare to be changed, should be fine to assume the line is not changed):
image

The logic is not special. In fact, oxc chose to skip bind declare class somehow, delete this related logic.

Will fix function in another PR if no one is interested/has time for it :)

@ShuiRuTian ShuiRuTian linked a pull request Dec 20, 2024 that will close this issue
@ShuiRuTian
Copy link
Contributor

Another try to bind for declare function is #8039

However, there is still a missed function type called TSEmptyBodyFunctionExpression, which confused me. What is this? I do not quite understand why it's only needed in eslint-typescript....

@ShuiRuTian
Copy link
Contributor

Not sure whether this is the right place to discuss detials...

  1. feels like Ambient should not be a member of SymbolFlags, a symbol might be decalred through many declarations, which might be ambient or not. I think it's why Typescript add such flag to just Node(a term used by tsc, which is similar to AST node)
  2. A symbol might have multi valid declarations in TS, but current SymbolTable seems to be designed for JS.

In my opinion, they are problems need to be resolved for supporting TS, waiting for options from core team members...

also cc @EliLichtblau, for who introduced "Ambient" as a member of "SymbolFlags"

@EliLichtblau
Copy link
Contributor

Yea - I wonder if ambient was in the definition of symbolFlags at the time I made the PR in tsc but I agree ambient should be associated with a node rather than a symbol

@Dunqing
Copy link
Member

Dunqing commented Dec 23, 2024

I am not deep into this, so I don't know how to fix this, but we respect one principle is aligned with TypeScript.

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
5 participants