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(typescript): Align isolatedDeclaration implementation with tsc #9715

Merged
merged 49 commits into from
Dec 2, 2024

Conversation

CPunisher
Copy link
Contributor

@CPunisher CPunisher commented Nov 5, 2024

Description:

  • Basic implementation. This also includes diagnostics and type inference.
    • Function
    • Class
    • Variable declaration
    • Enum
  • Remove unused imports and declarations. Based on the result ast of fast dts transformation, we collect the usage of ids which also contain syntax context. Then we prune the ast to cut off the unused imports and declarations.
  • Port tests and bug fixes. To ensure correctness, I'd like to port the fixtures as oxc does https://github.com/oxc-project/oxc/tree/main/crates/oxc_isolated_declarations/tests/fixtures.
  • Benmarks
  • Strip internal

Implementation:
Most code is direct translation of https://github.com/oxc-project/oxc/tree/main/crates/oxc_isolated_declarations. The differences come from:

  1. swc and oxc use different ast.
  2. The transformation of swc is mutation-based, which directly mutates the whole cloned ast, while the transformation of oxc is immutation-based, which constructs and copies the ast nodes on demand. Maybe mutation-based transformation is not better, but I think it's also annoyed to construct ast nodes and it could be easy to refactor if there is some demands in the future.
  3. oxc transforms while collecting references information to prune unused idents. However, the ast nodes of swc contain syntax context, we don't need to collect the information again. So I build a reference graph and calculate the reachability of identifiers.
  4. I think oxc could have bugs. I manually compare the result with tsc --declaration --isolatedDeclarations --emitDeclarationOnly --isolatedModules --noResolve --noCheck --strictNullChecks test.ts. I also reguard those cases where tsc can't compile as undefined behaviors.

😅 Actually I find my implementation of point 2 is not too good either. I meet many bad cases after my basic implementation and write many ugly patches. So issues are welcome.

Benchmarks:
Roughly test with https://raw.githubusercontent.com/oxc-project/benchmark-files/main/vue-id.ts under M3Pro.

parse without syntax context resolution isolated declarations
swc 282.68ms 335.44ms
oxc 53.286ms 63.917ms
tsc - 3.875s

Known problem:

  • Due to the comment emitting algorithm in swc, isolated declaration may also generate comments with wrong positions.

Related issue (if exists):
closes #9705
closes #9718

Copy link

changeset-bot bot commented Nov 5, 2024

🦋 Changeset detected

Latest commit: 858d762

The changes in this PR will be included in the next version bump.

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@CPunisher CPunisher changed the title feat(typescript): remove unused import in isolatedDeclaration feat(typescript): remove unused imports and declarations in isolatedDeclaration Nov 5, 2024
@SoonIter
Copy link

SoonIter commented Nov 5, 2024

thanks for your help❤️️

😂 got another blocking point of isolatedDeclaration

should generate the same error as tsc

src/App.tsx(8,10): error TS9007: Function must have an explicit return type annotation with --isolatedDeclarations.

I'll open another issue

Copy link

codspeed-hq bot commented Nov 5, 2024

CodSpeed Performance Report

Merging #9715 will degrade performances by 3.5%

Comparing CPunisher:feat/isolated-decl-minify (858d762) with main (ed65eee)

Summary

❌ 1 regressions
✅ 193 untouched benchmarks

⚠️ Please fix the performance issues or acknowledge them on CodSpeed.

Benchmarks breakdown

Benchmark main CPunisher:feat/isolated-decl-minify Change
es/visitor/base-perf/boxing_boxed_clone 2.4 µs 2.5 µs -3.5%

@CPunisher CPunisher changed the title feat(typescript): remove unused imports and declarations in isolatedDeclaration feat(typescript): align tsc isolatedDeclaration Nov 12, 2024
@CPunisher CPunisher changed the title feat(typescript): align tsc isolatedDeclaration feat(typescript): align isolatedDeclaration implementation with tsc Nov 12, 2024
@CPunisher CPunisher changed the title feat(typescript): align isolatedDeclaration implementation with tsc feat(typescript): Align isolatedDeclaration implementation with tsc Nov 12, 2024
@CPunisher CPunisher force-pushed the feat/isolated-decl-minify branch 2 times, most recently from 49c2be0 to 533853e Compare November 13, 2024 12:10
@CPunisher CPunisher force-pushed the feat/isolated-decl-minify branch from 3c501b0 to bf3c9b6 Compare November 23, 2024 06:08
@CPunisher CPunisher marked this pull request as ready for review November 25, 2024 03:05
@CPunisher CPunisher requested review from a team as code owners November 25, 2024 03:05
@kdy1 kdy1 added this to the Planned milestone Nov 25, 2024
@Boshen
Copy link
Contributor

Boshen commented Nov 25, 2024

FYI oxc uses immutable AST for a single transform pipeline.

@CPunisher
Copy link
Contributor Author

CPunisher commented Nov 25, 2024

FYI oxc uses immutable AST for a single transform pipeline.

I see. And I believe swc can also get better performance and other engineering benefits with immutable AST in isolated declaration transformation. After finishing, I have a little bit of regret... But maybe in the next pr 😁.

@kdy1 kdy1 self-assigned this Nov 25, 2024
crates/swc/Cargo.toml Outdated Show resolved Hide resolved
@kdy1
Copy link
Member

kdy1 commented Nov 28, 2024

Sorry for the delay. I'm still reviewing this.

Copy link
Member

@kdy1 kdy1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you so much for a great work!

@kdy1 kdy1 enabled auto-merge (squash) December 2, 2024 01:02
@kdy1 kdy1 disabled auto-merge December 2, 2024 01:38
@kdy1 kdy1 merged commit 0adad25 into swc-project:main Dec 2, 2024
149 of 152 checks passed
@kdy1 kdy1 modified the milestones: Planned, v1.10.0 Dec 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
4 participants