refactor: introduce ExprRef to avoid clone Expr ast nodes
#12622
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
✅ Deploy Preview for rspack canceled.
|
Contributor
Rsdoctor Bundle Diff AnalysisFound 5 projects in monorepo, 0 projects with changes. 📊 Quick Summary
Generated by Rsdoctor GitHub Action |
Contributor
|
📝 Benchmark detail: Open
|
Contributor
📦 Binary Size-limit
🎉 Size decreased by 2.75KB from 47.87MB to 47.87MB (⬇️0.01%) |
CodSpeed Performance ReportMerging #12622 will not alter performanceComparing Summary
Footnotes
|
Contributor
There was a problem hiding this comment.
Pull request overview
This PR optimizes the JavaScript parser by eliminating expensive cloning operations on AST nodes. It introduces a reference-oriented approach using a new ExprRef<'ast> type that holds references to SWC AST expression types instead of taking ownership, significantly reducing memory allocations and improving performance during AST traversal and dependency analysis.
Key Changes
- Introduced
ExprRef<'ast>enum to hold references to various SWC AST expression types - Refactored parser methods to accept references instead of owned values, eliminating
clone()andtake()calls - Updated data structures with lifetime parameters to support reference-based operations
Reviewed changes
Copilot reviewed 8 out of 8 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/ast.rs |
New file introducing the ExprRef<'ast> enum with all expression variants and From trait implementation for conversion from &Expr |
crates/rspack_plugin_javascript/src/visitors/dependency/parser/mod.rs |
Updated method signatures to use ExprRef<'ast> and lifetime parameters; modified data structures to hold references; removed util::take::Take import; added RootName trait implementation for ExprRef |
crates/rspack_plugin_javascript/src/visitors/dependency/parser/walk.rs |
Replaced expensive clone() calls with ExprRef references in hot path methods; updated function call sites to pass references instead of clones |
crates/rspack_plugin_javascript/src/visitors/dependency/parser/call_hooks_name.rs |
Updated to use ExprRef::Member instead of cloning expressions |
crates/rspack_plugin_javascript/src/visitors/dependency/mod.rs |
Added export of ast::* to make ExprRef publicly available |
crates/rspack_plugin_javascript/src/utils/eval/eval_member_expr.rs |
Updated to use ExprRef::Member instead of cloning member expressions |
crates/rspack_plugin_javascript/src/parser_plugin/url_plugin.rs |
Refactored is_meta_url to use ExprRef and pattern matching instead of method chaining |
crates/rspack_plugin_javascript/src/parser_plugin/import_meta_plugin.rs |
Updated to use ExprRef::Member instead of cloning expressions |
Comments suppressed due to low confidence (1)
crates/rspack_plugin_javascript/src/visitors/dependency/parser/mod.rs:90
- The updated
ExtractedMemberExpressionChainDatastruct now includes a lifetime parameter but lacks updated documentation explaining the lifetime and how it relates to the new reference-based approach. Since this is a public API that has changed significantly, the documentation should clarify that theobjectfield now holds a reference to the AST rather than an owned copy.
#[derive(Debug)]
pub struct ExtractedMemberExpressionChainData<'ast> {
pub object: ExprRef<'ast>,
pub members: Vec<Atom>,
pub members_optionals: Vec<bool>,
pub member_ranges: Vec<Span>,
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
ahabhgk
approved these changes
Jan 5, 2026
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
This PR optimizes the JavaScript parser by reducing unnecessary cloning of AST nodes, specifically during member expression extraction and info gathering. It introduces a reference-oriented approach using a new
ExprReftype to handle AST nodes without taking ownership or cloning.Key Changes
ExprRef<'ast>: Added a new enum incrates/rspack_plugin_javascript/src/visitors/dependency/parser/ast.rsthat holds references to various SWC AST expression types.get_member_expression_info,extract_member_expression_chain, andget_member_expression_info_from_exprto acceptExprRef<'ast>or&'ast Exprinstead of ownedExpr.'ast) that reference the original AST.ExtractedMemberExpressionChainData,MemberExpressionInfo, andCallExpressionInfonow use lifetime parameters to hold references to AST nodes (e.g.,&'ast CallExpr) instead of owned copies.clone()andtake(): Replaced expensiveexpr.clone()andexpr.obj.take()calls with reference-based lookups in the visitor's hot path (walk.rs).RootNameimplementation forExprRef<'_>to support root name extraction from references.Checklist