-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
refactor(ast_node): Make AST enums non_exhaustive
#11115
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
Conversation
🦋 Changeset detectedLatest commit: da866fd 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 |
Do you think this should go into the main branch? |
Sure, I can change it. |
7e9b1f3
to
5c7ab17
Compare
CodSpeed Performance ReportMerging #11115 will not alter performanceComparing Summary
Footnotes |
58133f1
to
0c129c5
Compare
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
Warning Review the following alerts detected in dependencies. According to your organization's Security Policy, it is recommended to resolve "Warn" alerts. Learn more about Socket for GitHub.
|
69ffe4b
to
9ee1a15
Compare
There was a problem hiding this 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 implements the first step of adding unknown variants to AST enums by making all AST enums non_exhaustive
. The changes enable the introduction of unknown variants in a future PR while ensuring that match statements are properly handled.
- Adds
#[cfg(swc_ast_unknown)]
conditional branches to handle future unknown variants - Updates the visitor code generator to conditionally generate fallback arms for
swc_ecma_ast
- Makes AST enums non-exhaustive except for specific ones (HTML, XML, regexp) that opt out
Reviewed Changes
Copilot reviewed 185 out of 186 changed files in this pull request and generated no comments.
Show a summary per file
File | Description |
---|---|
tools/generate-code/src/main.rs | Updates visitor generator to accept string parameter and detect swc_ecma_ast |
tools/generate-code/src/generators/visitor.rs | Conditionally generates fallback arms for unknown variants in match statements |
crates//src/.rs | Adds #[cfg(swc_ast_unknown)] fallback branches to existing match statements |
crates/*/Cargo.toml | Adds lint configuration to recognize the swc_ast_unknown cfg |
crates/swc_*_ast/src/base.rs | Adds no_unknown attribute to specific AST enums that shouldn't be non-exhaustive |
Comments suppressed due to low confidence (2)
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
I've switched to using a cfg flag instead of a feature to handle unknown variants, which makes it easier to ensure consistent handle. Also, since all changes are behind the |
I updated your branch to use patch commands |
d8eb351
to
909addf
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.
@@ -0,0 +1,5 @@ | |||
--- | |||
ast_node: major |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added ast_node: major
because I don't want to see complains about breaking changes published wrongly
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you! Nice work!
non_exhaustive
non_exhaustive
Description:
This is the split of #11100 . Since we need to add unknown variants to the ast enum, this requires a lot of code changes to handle non-exhaustive match. I'm doing this in a separate PR for review.
BREAKING CHANGE:
Related issue (if exists):