refact(es/minifier): Inline into shorthand prop early#11766
refact(es/minifier): Inline into shorthand prop early#11766Austaras wants to merge 3 commits intoswc-project:mainfrom
Conversation
|
There was a problem hiding this comment.
Pull request overview
Refactors the minifier’s inlining logic to support inlining into object-literal shorthand properties earlier, while preserving correct __proto__ semantics.
Changes:
- Exposes
prop_name_from_identso other optimizer passes can safely expand shorthand props (handling__proto__via computed keys). - Adds a
visit_mut_prophook to inline identifiers insideProp::Shorthandand convert them toKeyValueProp. - Extracts identifier inlining logic into a reusable
inline_identhelper and reuses it frominline().
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| crates/swc_ecma_minifier/src/compress/optimize/util.rs | Makes prop_name_from_ident available for shorthand→key-value expansion while preserving __proto__ behavior. |
| crates/swc_ecma_minifier/src/compress/optimize/mod.rs | Adds shorthand property inlining during AST visitation and expands shorthand into key-value props. |
| crates/swc_ecma_minifier/src/compress/optimize/inline.rs | Refactors identifier inlining into inline_ident and calls it from expression inlining. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| *n = Prop::KeyValue(KeyValueProp { | ||
| key, | ||
| value: expr.clone(), | ||
| }); |
There was a problem hiding this comment.
expr is owned here (returned from inline_ident), but it’s cloned when building KeyValueProp. This adds avoidable allocation/AST cloning; move expr into value instead of cloning it.
| *n = Prop::KeyValue(KeyValueProp { | |
| key, | |
| value: expr.clone(), | |
| }); | |
| *n = Prop::KeyValue(KeyValueProp { key, value: expr }); |
|
|
||
| if let Some(value) = self.vars.vars_for_inlining.remove(&id) { | ||
| self.changed = true; | ||
| report_change!("inline: Replacing '{}' with an expression", i); |
There was a problem hiding this comment.
Same issue as above: this report_change! call uses i, which is not defined in inline_ident. Use ident (or an Id derived from it) so this compiles and logs the correct identifier.
| report_change!("inline: Replacing '{}' with an expression", i); | |
| report_change!("inline: Replacing '{}' with an expression", ident.sym); |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d5419cf6aa
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| self.changed = true; | ||
| report_change!("inline: Replacing '{}' with an expression", i); | ||
| self.changed = true; | ||
| report_change!("inline: Replacing a variable `{}` with cheap expression", i); |
There was a problem hiding this comment.
Replace stale
i reference in inline_ident logging
After extracting inline_ident, the logging calls still format {} with i, but this function only has ident; when the debug feature is enabled, report_change! expands to tracing::debug! and this becomes an unresolved identifier compile error. This breaks swc_ecma_minifier builds in debug-feature workflows (e.g. cargo check -p swc_ecma_minifier --features debug).
Useful? React with 👍 / 👎.
Binary Sizes
Commit: 0d8ba38 |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| /// `__proto__`. When the property name is `__proto__`, it must be converted to | ||
| /// a computed property to preserve JavaScript semantics. | ||
| fn prop_name_from_ident(ident: Ident) -> PropName { | ||
| pub(crate) fn prop_name_from_ident(ident: Ident) -> PropName { |
There was a problem hiding this comment.
prop_name_from_ident is only referenced within the compress::optimize module (currently optimize/mod.rs and this file). Exposing it as pub(crate) increases the crate-wide API surface unnecessarily; consider reducing visibility to pub(super) (or pub(in crate::compress::optimize)) to keep the helper scoped to this module.
| pub(crate) fn prop_name_from_ident(ident: Ident) -> PropName { | |
| pub(super) fn prop_name_from_ident(ident: Ident) -> PropName { |
|
Can you make tests I added pass? |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 50 out of 50 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Description:
BREAKING CHANGE:
Related issue (if exists):