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

transformer: non-type-only import is only referenced by the type, but is not eliminated. #4423

Closed
Dunqing opened this issue Jul 23, 2024 · 8 comments · Fixed by #4550
Closed
Assignees
Labels
A-transformer Area - Transformer / Transpiler C-bug Category - Bug good first issue Experience Level - Good for newcomers

Comments

@Dunqing
Copy link
Member

Dunqing commented Jul 23, 2024

esbuild handles this case correctly. esbuild repl

OXC Playground

In

import { T } from 'mod'
let T: T = 0;
export { T }

Out

import { T } from "mod";
let T = 0;
export { T };

Expected

let T = 0;
export { T };
@Dunqing Dunqing added C-bug Category - Bug A-transformer Area - Transformer / Transpiler labels Jul 23, 2024
@DonIsaac DonIsaac added the good first issue Experience Level - Good for newcomers label Jul 23, 2024
@7086cmd
Copy link
Contributor

7086cmd commented Jul 27, 2024

May I try it? I am going to investigate this issue to enhance the understanding and functionality of oxc. Nevermind for better solutions :)

@Dunqing
Copy link
Member Author

Dunqing commented Jul 28, 2024

May I try it? I am going to investigate this issue to enhance the understanding and functionality of oxc. Nevermind for better solutions :)

Go for it!

@Dunqing
Copy link
Member Author

Dunqing commented Jul 30, 2024

I came up with a solution to this problem. We need to check if the next value binding definition has a value reference before defining it. If it doesn't, we can safely remove the import.

To make it possible, we need to add SymbolFlags in

redeclarations: IndexVec<SymbolId, Option<RedeclarationId>>,
redeclaration_spans: IndexVec<RedeclarationId, Vec<Span>>,

This could help us to find if it is a value symbol or a type symbol from redeclarations

Also, Since the transformer doesn't have a whole Semantic, so we can't get the reference's span by Semantic. So I'm going to revert #4464.

@overlookmotel Can you confirm this for me?

@overlookmotel
Copy link
Collaborator

I'm not familiar with the changes you've made over past few weeks to resolve TS type references. But I'd have assumed could figure out that can remove import { T } from 'mod'; without needing to figure out Span.

But, given that I don't understand how it's working now, I could well be wrong.

I don't object if you want to revert #4464. Really I didn't want to be making changes to Semantic until I've finished my "review". Mainly I made that PR just to inform that review - to research how much perf boost we can get by reducing the size of Semantic's structures.

@magic-akari
Copy link
Collaborator

magic-akari commented Jul 30, 2024

It's hard to verify; consider this situation.

// mod.ts
export class T {}

In this situation, the value of T and its type have been exported.
I believe that only a comprehensive type analyzer like TSC can solve this problem.
Or a bundle tool like esbuild, which can analyze across files, thereby eliminating intermediate values.

@magic-akari
Copy link
Collaborator

Oh, I misunderstood, this is about the issue of name resolution.

@Dunqing
Copy link
Member Author

Dunqing commented Jul 30, 2024

After digging deeper into how TypeScript handles this, I realized that we don't need to make any changes to what I said above! Sorry to bother you @overlookmotel

@DonIsaac
Copy link
Collaborator

DonIsaac commented Aug 5, 2024

@Dunqing can we close this?

@Dunqing Dunqing closed this as completed Aug 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-transformer Area - Transformer / Transpiler C-bug Category - Bug good first issue Experience Level - Good for newcomers
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants