-
-
Notifications
You must be signed in to change notification settings - Fork 753
refactor: use Pattern enum instead of Cow<Pat> in javascript parser
#12625
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
base: main
Are you sure you want to change the base?
Conversation
✅ Deploy Preview for rspack canceled.
|
Summary of ChangesHello @CPunisher, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly refactors the JavaScript parser by introducing a dedicated Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
Cow<Pat> in javascript parser
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 refactors the JavaScript parser to replace Cow<Pat> with a new Pattern<'ast> enum for representing AST patterns. The refactoring aims to work with AST references more directly, reducing cloning and improving performance.
Key Changes:
- Introduced
Pattern<'ast>enum with variants for Identifier, ObjectPattern, ArrayPattern, RestElement, AssignmentPattern, and Expression - Removed
Cow<Pat>usage and the deprecatedwarp_ident_to_pathelper function - Updated pattern walking and entering methods to use the new
Patterntype with proper lifetime annotations
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
crates/rspack_plugin_javascript/src/visitors/dependency/parser/estree.rs |
Defines new Pattern<'ast> enum and From<&'ast Pat> implementation |
crates/rspack_plugin_javascript/src/visitors/dependency/parser/walk_pre.rs |
Removes Cow import and updates to use Pattern::from |
crates/rspack_plugin_javascript/src/visitors/dependency/parser/walk.rs |
Removes warp_ident_to_pat, updates walk_pattern and related methods to use Pattern, replaces Cow::Borrowed/Cow::Owned with Pattern variants |
crates/rspack_plugin_javascript/src/visitors/dependency/parser/mod.rs |
Updates enter_pattern signature and removes Pat import |
crates/rspack_plugin_javascript/src/parser_plugin/import_parser_plugin.rs |
Updates to use Pattern instead of Cow<Pat> in scope parameters |
crates/rspack_plugin_javascript/src/parser_plugin/amd/amd_require_dependencies_block_parser_plugin.rs |
Removes Cow import and converts to Pattern |
crates/rspack_plugin_javascript/src/parser_plugin/amd/amd_define_dependency_parser_plugin.rs |
Updates to use Pattern and refactors get_ident_name to accept Pattern |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
crates/rspack_plugin_javascript/src/visitors/dependency/parser/estree.rs
Outdated
Show resolved
Hide resolved
crates/rspack_plugin_javascript/src/visitors/dependency/parser/mod.rs
Outdated
Show resolved
Hide resolved
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.
Code Review
This pull request is a great refactoring that replaces Cow<Pat> with a new Pattern enum, which should improve performance and code clarity by reducing cloning. The changes are well-executed and consistent across the codebase. I've found one potential issue regarding the handling of invalid AST patterns that could lead to a panic, and I've provided suggestions to improve robustness.
crates/rspack_plugin_javascript/src/visitors/dependency/parser/estree.rs
Show resolved
Hide resolved
crates/rspack_plugin_javascript/src/visitors/dependency/parser/estree.rs
Outdated
Show resolved
Hide resolved
Cow<Pat> in javascript parserPattern enum instead of Cow<Pat> in javascript parser
Rsdoctor Bundle Diff AnalysisFound 5 projects in monorepo, 0 projects with changes. 📊 Quick Summary
Generated by Rsdoctor GitHub Action |
📦 Binary Size-limit
❌ Size increased by 5.50KB from 47.87MB to 47.87MB (⬆️0.01%) |
CodSpeed Performance ReportMerging #12625 will not alter performanceComparing Summary
Footnotes
|
Summary
This PR refactors the JavaScript parser to use a new
Patternenum instead ofCow<Pat>for representing AST patterns.Key Changes:
Pattern<'ast>enum inestree.rsto wrap various SWC AST pattern types.Cow<Pat>usage withPattern<'ast>acrossJavascriptParserand its plugins.warp_ident_to_pathelper.walk_pattern,enter_pattern, and related methods to handle the newPatternenum.Rationale:
Using
Pattern<'ast>allows us to work with references to the AST more directly, reducing the need forCowand unnecessary cloning ofPatnodes, which should improve performance and simplify the code.Related links
None.
Checklist