fix(transformer/legacy-decorator): emit correct metadata types for enum#13327
Conversation
How to use the Graphite Merge QueueAdd either label to this PR to merge it via the merge queue:
You must have a Graphite account in order to use the merge queue. Sign up using this link. An organization admin has enabled the Graphite Merge Queue in this repository. Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue. This stack of pull requests is managed by Graphite. Learn more about stacking. |
There was a problem hiding this comment.
Pull Request Overview
This PR fixes the metadata emission for enum types in legacy decorators by ensuring that enum references in decorator metadata emit the correct primitive types (String, Number, or Object) instead of defaulting to Object.
- Adds enum type inference to determine whether an enum is string-based, number-based, or mixed
- Implements optimization to only analyze enums that are actually referenced as types in decorators
- Updates the type serialization logic to emit appropriate primitive types for enum references
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
tasks/transform_conformance/tests/legacy-decorators/test/fixtures/oxc/metadata/enum-types/output.js |
Test output showing correct metadata emission with String/Number types for enum properties |
tasks/transform_conformance/tests/legacy-decorators/test/fixtures/oxc/metadata/enum-types/input.ts |
Test input defining various enum types (string, number, bigint, mixed, computed) with decorated properties |
tasks/transform_conformance/snapshots/oxc.snap.md |
Updated test results showing the new test passing |
crates/oxc_transformer/src/lib.rs |
Adds enum statement processing to the main transformer |
crates/oxc_transformer/src/decorator/mod.rs |
Forwards enum declaration processing to legacy decorator handler |
crates/oxc_transformer/src/decorator/legacy/mod.rs |
Delegates statement processing to metadata collection |
crates/oxc_transformer/src/decorator/legacy/metadata.rs |
Core implementation of enum type inference and metadata emission |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
787f328 to
8deb807
Compare
CodSpeed Instrumentation Performance ReportMerging #13327 will not alter performanceComparing Summary
Footnotes |
a8ba87a to
f9a7350
Compare
f9a7350 to
4b431b2
Compare
| Expression::UnaryExpression(expr) | ||
| if expr.argument.is_number_literal() && !has_string => | ||
| { | ||
| has_number = true; | ||
| } |
There was a problem hiding this comment.
Not all UnaryExpressions evaluate to a number. It depends on the operator:
!123-> booleantypeof 123-> stringvoid 123-> undefineddelete 123-> boolean
The first one is not entirely stupid - !0 and !1 are sometimes used as shortcuts for true / false.
There was a problem hiding this comment.
Good catch, I forgot the importance of the operator. In this case, we only care about +, - and ~. For the typeof 123, it isn't allowed. See TypeScript Playground
There was a problem hiding this comment.
For the
typeof 123, it isn't allowed. See TypeScript Playground
Ah ha. Does this mean we can assume any computed expression (e.g. foo()) evaluates to a number?
But maybe the gain of that is not worth the trouble. Some more complex expressions that evaluate to strings seem to be allowed e.g. enum E { X = 'foo' + 'bar' + 'qux' }. So we'd need to handle those to accurately determine whether string or number.
4b431b2 to
cc3b0cf
Compare
overlookmotel
left a comment
There was a problem hiding this comment.
Pushed a commit just to update the comment about #[inline] - hope you don't mind, I thought it'd save time vs another round of review. Will merge as soon as CI passes.
Follow-on after #13327. It seems that TS infers the type of `+x`, `-x`, and `~x` as `Number`, no matter what `x` is. All other `UnaryOperator`s (`!x`, `void x`, `typeof x`, `delete x`) are illegal as enum initializers, so we can ignore those cases and just say all `UnaryExpression`s are `Number`s. [TS Playground](https://www.typescriptlang.org/play/?experimentalDecorators=true&emitDecoratorMetadata=true&target=99#code/GYVwdgxgLglg9mABAcwKZQMpQE4zMgCgEpEBvRbdEbJAcgGcc9laBuRAXwCgvUwQAtogCqYAIbYAngHkoAC1TYAovyGkuiRGFTIxsAG6pEAXkQBaNJib5iAGg2IADnHowDR0wGpLWXDaL2mgBGbgDuMPQeiAB+PtaEAVzcXKCQsAiIACaoEHDYeqgEAPpQEpYAXIhiYJK2iEWO2HCOilCSlYx+yCSkyagAHs7YUIgQADZi9PSIAGJwcGQOAALZuflQqA7gEjLyigAKTY6VojuyCsqqJiLiUueKKoIAdNq67qxJXEA) Additionally, `BigInt`s are illegal as enum initializers, so we can ignore them too. [TS Playground](https://www.typescriptlang.org/play/?experimentalDecorators=true&emitDecoratorMetadata=true&target=99#code/KYOwrgtgBAQglgcwJIgC4FFzQN4CgpQBGiUAvFAIwAMVIANPkYgsAE5mU0324C+uQA) We can therefore simplify type inference for decorators.

emitDecoratorMetadata#13319