fix(es/typescript): Handle TypeScript expressions in enum transformation#11769
fix(es/typescript): Handle TypeScript expressions in enum transformation#11769magic-akari wants to merge 15 commits intoswc-project:mainfrom
Conversation
|
Binary Sizes
Commit: 2093266 |
75b4daa to
c9ea557
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c9ea557187
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
There was a problem hiding this comment.
Pull request overview
Fixes a crash when transforming TypeScript enums whose member initializers include TS-only expression wrappers (e.g. as any), by unwrapping/stripping those wrappers during enum value computation and enum emission.
Changes:
- Unwrap TS expression wrappers (
as, non-null, type assertions,satisfies, etc.) when computing enum member values. - Visit/transform opaque enum initializer expressions during enum emission so TS-only syntax is stripped before codegen.
- Add regression tests for TS-wrapped and complex enum initializers; update existing fixture outputs to match new behavior.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| crates/swc_ecma_transforms_typescript/src/ts_enum.rs | Unwraps TS expression wrappers when computing enum member values. |
| crates/swc_ecma_transforms_typescript/src/transform.rs | Visits opaque enum initializer expressions and tracks opacity during enum emission. |
| crates/swc_ecma_transforms_typescript/tests/strip.rs | Adds new enum-related regression tests. |
| crates/swc_ecma_transforms_typescript/tests/swc_snapshots/tests/strip.rs/* | Adds snapshots for new regression tests. |
| crates/swc_ecma_transforms_typescript/tests/fixture/issue-3001/*/output.js | Updates expected fixture output due to new enum-expression processing/inlining. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
...cma_transforms_typescript/tests/__swc_snapshots__/tests/strip.rs/ts_enum_with_nested_enum.js
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 64335328f7
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 12 out of 12 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Intentionally do not predeclare enum members in the resolver. | ||
| // Enum initializers may reference other members, including quoted names whose | ||
| // text is a valid identifier: | ||
| // | ||
| // ```TypeScript | ||
| // enum E { | ||
| // A = "A", | ||
| // "B" = "B", | ||
| // C = (() => { console.log(A, B); })(), | ||
| // } | ||
| // ``` | ||
| // | ||
| // We keep those references unresolved here so the TypeScript enum transform | ||
| // can rewrite them using semantic.enum_record. |
There was a problem hiding this comment.
Another approach would be to mark all of them with contexts, like this:
enum E#2 {
A#4 = "A",
"B"#4 = "B",
C = (() => { console#1.log(A#4, B#4); })(),
}This way, during the subsequent enum transformation process, we could directly compare IDs to perform correct replacements.
However, SWC's current design doesn't allocate contexts for string enum members, so this approach doesn't work. We have to settle for the alternative: not marking any of them.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fd53909b1e
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 20 out of 20 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // | ||
| // We keep those references unresolved here so the TypeScript enum transform | ||
| // can rewrite them using semantic.enum_record. | ||
| child.current.mark = self.config.unresolved_mark; |
There was a problem hiding this comment.
Please review this part carefully.
I am unsure if this implementation represents the correct direction for the fix.
@swc-project/core
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 22 out of 22 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 930359506d
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3d75291b7c
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 00d05585e9
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| let TsEnumRecordValue::Opaque(mut e) = enum_computer.compute(e) else { | ||
| unreachable!(); |
There was a problem hiding this comment.
Avoid panicking when recomputed enum value becomes const
This unreachable!() can be hit for validly parsed enums where the semantic pass stored an initializer as Opaque, but the second pass recomputes it as a constant using the full enum record. For example, enum E { A = B, B = 1 } records A as opaque during semantic analysis (because B is not known yet), then enum_computer.compute returns Number(1) here, so the destructuring fails and the transform panics instead of emitting code. The same crash pattern applies to forward references across merged enum declarations.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 22 out of 22 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| let member_names = self | ||
| .semantic | ||
| .enum_record | ||
| .keys() | ||
| .filter(|k| k.enum_id == id.to_id()) | ||
| .map(|k| k.member_name.clone()) | ||
| .collect(); | ||
|
|
||
| let enum_computer = EnumValueComputer { | ||
| enum_id: &id.to_id(), | ||
| unresolved_ctxt: self.unresolved_ctxt, | ||
| record: &self.semantic.enum_record, | ||
| }; |
There was a problem hiding this comment.
EnumValueComputer { enum_id: &id.to_id(), .. } takes a reference to a temporary Id produced by to_id(). This will not live long enough and is expected to fail to compile (temporary dropped while borrowed). Bind let enum_id = id.to_id(); once and pass enum_id: &enum_id (and reuse enum_id in the nearby .filter(|k| k.enum_id == ...) / TsEnumRecordKey construction to avoid repeated to_id() calls).
| }; | ||
| e.visit_mut_with(&mut RefRewriter { | ||
| query: EnumMemberRefQuery { | ||
| enum_id: &id.to_id(), | ||
| member_names: &member_names, | ||
| unresolved_ctxt: self.unresolved_ctxt, | ||
| }, | ||
| }); |
There was a problem hiding this comment.
EnumMemberRefQuery { enum_id: &id.to_id(), .. } also borrows a temporary Id. After introducing a local enum_id: Id (e.g. let enum_id = id.to_id();), pass enum_id: &enum_id here as well to avoid borrowing a temporary and to ensure the reference outlives the rewriter.
Description:
BREAKING CHANGE:
Related issue (if exists):
assyntax cause swc crash #11761