Skip to content

Comments

fix(transformer): improve legacy decorator handling and fix constructor parameter decorators#13632

Merged
graphite-app[bot] merged 1 commit intomainfrom
09-10-fix_transformer_improve_legacy_decorator_handling_and_fix_constructor_parameter_decorators
Sep 11, 2025
Merged

fix(transformer): improve legacy decorator handling and fix constructor parameter decorators#13632
graphite-app[bot] merged 1 commit intomainfrom
09-10-fix_transformer_improve_legacy_decorator_handling_and_fix_constructor_parameter_decorators

Conversation

@Dunqing
Copy link
Member

@Dunqing Dunqing commented Sep 10, 2025 •
edited
Loading

Fixes rolldown/rolldown#6079. Regressed introduced by #13227.

The root cause is that the class plugin will insert a constructor method in some cases, but metadata handling is processed before the constructor is inserted, which causes the method count not to be exactly the same as the method metadata count. Thus, decorator metadata is inserted in the incorrect place.

This PR refactors the core transformation logic to ensure that the new methods and properties inserted by other plugins won't affect the decorator transformation.

Transformation Flow Changes

Before: Complex upfront analysis with separate processing paths

  • Used check_class_has_decorated() to analyze entire class tree
  • Separate handling for class vs element decorators
  • Multiple disconnected stacks for state tracking

After: Unified incremental processing

  • Process decorators as we traverse (exit_method_definition,
    exit_property_definition, etc.)
  • Accumulate decoration statements in single stack during traversal
  • Unified processing in transform_class() using consolidated state

@Dunqing Graphite App
Copy link
Member Author

Dunqing commented Sep 10, 2025

How to use the Graphite Merge Queue

Add either label to this PR to merge it via the merge queue:

  • 0-merge - adds this PR to the back of the merge queue
  • hotfix - for urgent hot fixes, skip the queue and merge this PR next

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.

@github-actions github-actions bot added A-transformer Area - Transformer / Transpiler C-bug Category - Bug labels Sep 10, 2025
@codspeed-hq
Copy link

codspeed-hq bot commented Sep 10, 2025 •
edited
Loading

CodSpeed Instrumentation Performance Report

Merging #13632 will not alter performance

Comparing 09-10-fix_transformer_improve_legacy_decorator_handling_and_fix_constructor_parameter_decorators (bb2bcf0) with main (d4608f1)1

Summary

✅ 37 untouched benchmarks

Footnotes

  1. No successful run was found on main (bb2bcf0) during the generation of this report, so d4608f1 was used instead as the comparison base. There might be some changes unrelated to this pull request in this report. ↩

@Dunqing Dunqing marked this pull request as ready for review September 10, 2025 02:45
Copilot AI review requested due to automatic review settings September 10, 2025 02:45
Copilot AI reviewed Sep 10, 2025
View reviewed changes
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR fixes an issue in the legacy decorator transformation where decorated classes without names weren't being handled correctly, and improves the overall handling of decorator metadata by refactoring the transformation flow from complex upfront analysis to incremental processing.

  • Refactored decorator transformation to use a stack-based approach for processing nested classes
  • Fixed constructor parameter decorator handling by ensuring metadata is processed in the correct order
  • Improved handling of classes without names by generating appropriate bindings

Reviewed Changes

Copilot reviewed 17 out of 19 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/private-in-expression-in-decorator/* New test case for private expressions in decorators
tasks/transform_conformance/tests/legacy-decorators/test/fixtures/oxc/metadata/class-and-method-decorators/* New test case for combined class and method decorators
tasks/transform_conformance/tests/legacy-decorators/test/fixtures/oxc/class-without-name-with-decorated_* New test cases for unnamed decorated classes
tasks/transform_conformance/snapshots/oxc.snap.md Updated snapshot reflecting improved test coverage
crates/oxc_transformer/src/lib.rs Added exit methods for decorator processing
crates/oxc_transformer/src/decorator/mod.rs Added exit method hooks for incremental decorator processing
crates/oxc_transformer/src/decorator/legacy/mod.rs Major refactor from upfront analysis to stack-based incremental processing
crates/oxc_transformer/src/decorator/legacy/metadata.rs Simplified metadata handling with separate stacks for methods and constructors

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

overlookmotel
overlookmotel reviewed Sep 10, 2025
View reviewed changes
Copy link
Member

@overlookmotel overlookmotel left a comment •
edited
Loading

Choose a reason for hiding this comment

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

This PR changes quite a lot, and I'm not that familiar with decorators transform to start with, so it's hard to review properly without spending hours on it.

There's one bug related to stacks (see below).

Also, a couple of comments/questions:

  1. Are you sure that decorators always get transformed by other transforms before they're removed from AST? (e.g. in a case like @(x ?? y))

  2. How method_metadata_stack and constructor_metadata_stack are pushed/popped looks a little fragile. Is it guaranteed that every push has a corresponding pop, even if some other transform e.g. adds/removes a constructor or method in between, or if some classes are class expressions?

This decorators transform is pretty damn tricky!

(I hope you don't mind I pushed 1 unimportant commit, just to reformat the test cases. Conformance CI fail is not related to that commit - it passes locally. Something must have changed on main since PR was originally submitted.)

@overlookmotel overlookmotel force-pushed the 09-10-fix_transformer_improve_legacy_decorator_handling_and_fix_constructor_parameter_decorators branch from 49c39d7 to f7526d6 Compare September 10, 2025 08:38
@Dunqing Dunqing force-pushed the 09-10-fix_transformer_improve_legacy_decorator_handling_and_fix_constructor_parameter_decorators branch from f7526d6 to 54d9766 Compare September 11, 2025 03:20
@Dunqing
Copy link
Member Author

Dunqing commented Sep 11, 2025

  1. Are you sure that decorators always get transformed by other transforms before they're removed from AST? (e.g. in a case like @(x ?? y))

Ideally, yes, the decorators are transformed in the exit_** phase like exit_method_definition and exit_property_definition, so before entering these methods, the expressions are already transformed by other plugins in enter_expression or exit_expression.

However, there is a weird situation that will break this. For example, if expressions are handled by the sub-visitor and it is called in the exit_class or something else that is even later, then this will probably be overlooked.

@Dunqing
Copy link
Member Author

Dunqing commented Sep 11, 2025

@overlookmotel I've resolved your requested changes. Can you take another look? I am happy if you want to make a change directly in this PR or in the follow-up PRs.

@overlookmotel
Copy link
Member

overlookmotel commented Sep 11, 2025

decorators are transformed in the exit_** phase like exit_method_definition and exit_property_definition, so before entering these methods, the expressions are already transformed by other plugins in enter_expression or exit_expression.

OK great!

However, there is a weird situation that will break this. For example, if expressions are handled by the sub-visitor and it is called in the exit_class or something else that is even later, then this will probably be overlooked.

Hmm. Yes, you're right. But I'm not sure what we could do to solve this. There is a period in time between when the decorators are extracted from the AST, and when they're inserted again. Any code examining the AST during that window of time won't see them. But in practice, I don't think this will present a problem.

@overlookmotel overlookmotel mentioned this pull request Sep 11, 2025
Merged
graphite-app bot pushed a commit that referenced this pull request Sep 11, 2025
@overlookmotel
feat(data_structures): add is_exhausted method to NonEmptyStack a…
1a58e99 
…nd `SparseStack` (#13672)

When using stacks, typically you create a stack at start of AST traversal, and then `debug_assert!` that the stack has been emptied again at the end of traversal, to make sure that the stack has not got out of sync - that every `push` is followed by a corresponding `pop`.

With `NonEmptyStack` and `SparseStack` it's a little confusing how to do this, because these stacks can never be empty. `stack.is_empty()` is not correct. `stack.len() == 1` is correct, but it's unclear when reading that code what exactly this is testing.

Add `is_exhausted` methods to `NonEmptyStack` and `SparseStack` to fulfil this role.

Naming: `is_exhausted` is perhaps not the best name, but I couldn't come up with a better one. I considered `is_emptied`, which is more descriptive, but I think it's too close to `is_empty`.

(prompted by #13632)
@overlookmotel
Copy link
Member

overlookmotel commented Sep 11, 2025

This PR made me realize that it's confusing how to check that stacks are back to initial state at end of AST traversal, because it's different depending on the stack type:

  • Stack -> stack.is_empty()
  • NonEmptyStack -> stack.len() == 1
  • SparseStack -> stack.len() == 1

This is not obvious, and it's really error-prone (bad design on my part).

#13672 adds an is_exhausted method to NonEmptyStack and SparseStack, which I think makes the intent clearer.

Once that PR is merged, I suggest using is_exhausted for the exit_program stack checks in this PR.

@Dunqing Dunqing force-pushed the 09-10-fix_transformer_improve_legacy_decorator_handling_and_fix_constructor_parameter_decorators branch from 454c379 to fa907f4 Compare September 11, 2025 06:18
@Dunqing
Copy link
Member Author

Dunqing commented Sep 11, 2025

This PR made me realize that it's confusing how to check that stacks are back to initial state at end of AST traversal, because it's different depending on the stack type:

  • Stack -> stack.is_empty()
  • NonEmptyStack -> stack.len() == 1
  • SparseStack -> stack.len() == 1

This is not obvious, and it's really error-prone (bad design on my part).

#13672 adds an is_exhausted method to NonEmptyStack and SparseStack, which I think makes the intent clearer.

Once that PR is merged, I suggest using is_exhausted for the exit_program stack checks in this PR.

Thank you for adding this method. I had incorrectly used is_empty a few times 🥲.

graphite-app[bot]
graphite-app bot reviewed Sep 11, 2025
View reviewed changes
overlookmotel
overlookmotel approved these changes Sep 11, 2025
View reviewed changes
Copy link
Member

@overlookmotel overlookmotel left a comment

Choose a reason for hiding this comment

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

I added a commit just to rename a var to be more accurate description of what it is.

One nit comment below. If you're willing to make that change, please go ahead and merge afterwards. Or if you can't be bothered, just merge it now!

@Dunqing
Copy link
Member Author

Dunqing commented Sep 11, 2025

Thank you for reviewing. Merging now!

@Dunqing Dunqing added the 0-merge Merge with Graphite Merge Queue label Sep 11, 2025
@Dunqing Graphite App
Copy link
Member Author

Dunqing commented Sep 11, 2025 •
edited by graphite-app bot
Loading

Merge activity

@Dunqing
fix(transformer): improve legacy decorator handling and fix construct…
bb2bcf0 
…or parameter decorators (#13632)

Fixes rolldown/rolldown#6079. Regressed introduced by #13227.

The root cause is that the class plugin will insert a constructor method in some cases, but metadata handling is processed before the constructor is inserted, which causes the method count not to be exactly the same as the method metadata count. Thus, decorator metadata is inserted in the incorrect place.

This PR refactors the core transformation logic to ensure that the new methods and properties inserted by other plugins won't affect the decorator transformation.

### Transformation Flow Changes

  Before: Complex upfront analysis with separate processing paths
  - Used check_class_has_decorated() to analyze entire class tree
  - Separate handling for class vs element decorators
  - Multiple disconnected stacks for state tracking

  After: Unified incremental processing
  - Process decorators as we traverse (exit_method_definition,
  exit_property_definition, etc.)
  - Accumulate decoration statements in single stack during traversal
  - Unified processing in transform_class() using consolidated state
@graphite-app graphite-app bot force-pushed the 09-10-fix_transformer_improve_legacy_decorator_handling_and_fix_constructor_parameter_decorators branch from b488892 to bb2bcf0 Compare September 11, 2025 08:07
@graphite-app graphite-app bot merged commit bb2bcf0 into main Sep 11, 2025
25 checks passed
@graphite-app graphite-app bot deleted the 09-10-fix_transformer_improve_legacy_decorator_handling_and_fix_constructor_parameter_decorators branch September 11, 2025 08:12
@graphite-app graphite-app bot removed the 0-merge Merge with Graphite Merge Queue label Sep 11, 2025
@Boshen Boshen mentioned this pull request Sep 15, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@overlookmotel overlookmotel overlookmotel approved these changes

@graphite-app graphite-app[bot] graphite-app[bot] left review comments

Copilot code review Copilot Copilot left review comments

Assignees

@Dunqing Dunqing

Labels

A-transformer Area - Transformer / Transpiler C-bug Category - Bug

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[Bug]: Decorators/Constructor: method metadata leaks to class beta.36

2 participants

@Dunqing @overlookmotel