Skip to content
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.

Sign up for GitHub

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

Jump to bottom

feat(minifier): replace const with let for non-exported read-only variables #8733

Merged
merged 1 commit into from
Jan 26, 2025
Merged

feat(minifier): replace const with let for non-exported read-only variables #8733

merged 1 commit into from
Jan 26, 2025

Conversation

sapphi-red
Copy link
Contributor

@sapphi-red sapphi-red commented Jan 26, 2025 •
edited
Loading

Replace const with let when that value does not have any assignments and not exposed.

@github-actions github-actions bot added A-minifier Area - Minifier A-ast Area - AST C-enhancement Category - New feature or request labels Jan 26, 2025
This was referenced Jan 26, 2025
Merged
Merged
@sapphi-red Graphite App
Copy link
Contributor Author

sapphi-red commented Jan 26, 2025 •
edited
Loading

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.

@sapphi-red sapphi-red mentioned this pull request Jan 26, 2025
Merged
@sapphi-red sapphi-red marked this pull request as ready for review January 26, 2025 10:50
@sapphi-red sapphi-red marked this pull request as draft January 26, 2025 10:51
@Boshen
Copy link
Member

Boshen commented Jan 26, 2025

esbuild does const replacements, I'd like to do that at some point too.

So this should go to late pass instead of pre pass?

@sapphi-red
Copy link
Contributor Author

esbuild does const replacements, I'd like to do that at some point too.

So this should go to late pass instead of pre pass?

I first implemented in the late pass and it wasn't idempotent because collapse_variable_declarations can merge const a = 1; let b = 2; into let a = 1, b = 2.
The information that it was a const can be obtained from SymbolFlags::ConstVariable. Would that work for const replacements?

@codspeed-hq CodSpeed HQ
Copy link

codspeed-hq bot commented Jan 26, 2025 •
edited
Loading

CodSpeed Performance Report

Merging #8733 will not alter performance

Comparing 01-26-feat_minifier_replace_const_with_let_for_non-exported_read-only_variables (e0117db) with main (dc0b0f2)

Summary

✅ 33 untouched benchmarks

@sapphi-red sapphi-red force-pushed the 01-26-feat_minifier_evaluate_math.sqrt_and_math.cbrt_ branch from 9556f95 to 3378661 Compare January 26, 2025 11:08
@sapphi-red sapphi-red force-pushed the 01-26-feat_minifier_replace_const_with_let_for_non-exported_read-only_variables branch from e554c5c to 655a300 Compare January 26, 2025 11:08
@sapphi-red sapphi-red force-pushed the 01-26-feat_minifier_evaluate_math.sqrt_and_math.cbrt_ branch from 3378661 to 899d108 Compare January 26, 2025 11:12
@sapphi-red sapphi-red force-pushed the 01-26-feat_minifier_replace_const_with_let_for_non-exported_read-only_variables branch from 655a300 to a40e368 Compare January 26, 2025 11:12
@Boshen Boshen force-pushed the 01-26-feat_minifier_evaluate_math.sqrt_and_math.cbrt_ branch from 899d108 to 2f3b1d4 Compare January 26, 2025 11:18
@Boshen Boshen force-pushed the 01-26-feat_minifier_replace_const_with_let_for_non-exported_read-only_variables branch from a40e368 to f8f8355 Compare January 26, 2025 11:18
@sapphi-red sapphi-red force-pushed the 01-26-feat_minifier_evaluate_math.sqrt_and_math.cbrt_ branch from 2f3b1d4 to ad8ec4a Compare January 26, 2025 11:21
@sapphi-red sapphi-red force-pushed the 01-26-feat_minifier_replace_const_with_let_for_non-exported_read-only_variables branch from f8f8355 to 4ff54aa Compare January 26, 2025 11:21
@sapphi-red sapphi-red marked this pull request as ready for review January 26, 2025 11:24
@sapphi-red sapphi-red force-pushed the 01-26-feat_minifier_evaluate_math.sqrt_and_math.cbrt_ branch from ad8ec4a to dda3b76 Compare January 26, 2025 12:08
@sapphi-red sapphi-red force-pushed the 01-26-feat_minifier_replace_const_with_let_for_non-exported_read-only_variables branch from 4ff54aa to 413c513 Compare January 26, 2025 12:08
@sapphi-red sapphi-red changed the base branch from 01-26-feat_minifier_evaluate_math.sqrt_and_math.cbrt_ to graphite-base/8733 January 26, 2025 12:14
@sapphi-red sapphi-red force-pushed the 01-26-feat_minifier_replace_const_with_let_for_non-exported_read-only_variables branch from 413c513 to 2a1f336 Compare January 26, 2025 12:19
@sapphi-red sapphi-red changed the base branch from graphite-base/8733 to main January 26, 2025 12:20
@sapphi-red sapphi-red force-pushed the 01-26-feat_minifier_replace_const_with_let_for_non-exported_read-only_variables branch from 2a1f336 to cf091dd Compare January 26, 2025 12:20
@Boshen
Copy link
Member

Boshen commented Jan 26, 2025

esbuild does const replacements, I'd like to do that at some point too.
So this should go to late pass instead of pre pass?

I first implemented in the late pass and it wasn't idempotent because collapse_variable_declarations can merge const a = 1; let b = 2; into let a = 1, b = 2. The information that it was a const can be obtained from SymbolFlags::ConstVariable. Would that work for const replacements?

Yes, we can merge this PR regardless.

@Boshen Boshen added the 0-merge Merge with Graphite Merge Queue label Jan 26, 2025
@Boshen Graphite App
Copy link
Member

Boshen commented Jan 26, 2025 •
edited by graphite-app bot
Loading

Merge activity

  • Jan 26, 7:35 AM EST: The merge label '0-merge' was detected. This PR will be added to the Graphite merge queue once it meets the requirements.
  • Jan 26, 7:35 AM EST: A user added this pull request to the Graphite merge queue.
  • Jan 26, 7:51 AM EST: A user merged this pull request with the Graphite merge queue.

@sapphi-red
feat(minifier): replace const with let for non-exported read-only…
e0117db 
… variables (#8733)

Replace `const` with `let` when that value does not have any assignments and not exposed.
@Boshen Boshen force-pushed the 01-26-feat_minifier_replace_const_with_let_for_non-exported_read-only_variables branch from cf091dd to e0117db Compare January 26, 2025 12:44
@graphite-app graphite-app bot merged commit e0117db into main Jan 26, 2025
25 checks passed
@graphite-app graphite-app bot deleted the 01-26-feat_minifier_replace_const_with_let_for_non-exported_read-only_variables branch January 26, 2025 12:51
@oxc-bot oxc-bot mentioned this pull request Jan 26, 2025
Merged
Boshen added a commit that referenced this pull request Jan 26, 2025
@oxc-bot @Boshen
release(crates): v0.48.1 (#8738)
2fb08b9 
## [0.48.1] - 2025-01-26

### Features

- b7f13e6 ast: Implement utf8 to utf16 span converter (#8687) (Boshen)
- 6589c3b mangler: Reuse variable names (#8562) (翠 / green)
- 29bd215 minifier: Minimize `Infinity.toString(radix)` to `'Infinity'`
(#8732) (Boshen)
- e0117db minifier: Replace `const` with `let` for non-exported
read-only variables (#8733) (sapphi-red)
- 9e32f55 minifier: Evaluate `Math.sqrt` and `Math.cbrt` (#8731)
(sapphi-red)
- 360d49e minifier: Replace `Math.pow` with `**` (#8730) (sapphi-red)
- 2e9a560 minifier: `NaN.toString(radix)` is always `NaN` (#8727)
(Boshen)
- cbe0e82 minifier: Minimize `foo(...[])` -> `foo()` (#8726) (Boshen)
- e9fb5fe minifier: Dce pure expressions such as `new Map()` (#8725)
(Boshen)

### Bug Fixes

- 0944758 codegen: Remove parens from `new (import(''), function() {})`
(#8707) (Boshen)
- 33de70a mangler: Handle cases where a var is declared in a block scope
(#8706) (翠 / green)
- d982cdb minifier: `Unknown.fromCharCode` should not be treated as
`String.fromCharCode` (#8709) (sapphi-red)
- e7ab96c transformer/jsx: Incorrect `isStaticChildren` argument for
`Fragment` with multiple children (#8713) (Dunqing)
- 3e509e1 transformer/typescript: Enum merging when same name declared
in outer scope (#8691) (branchseer)

### Performance

- dc0b0f2 manger: Remove useless `tmp_bindings` (#8735) (Dunqing)
- e472ced mangler: Optimize handling of collecting lived scope ids
(#8724) (Dunqing)
- 8587965 minifier: Normalize `undefined` to `void 0` before everything
else (#8699) (Boshen)

### Refactor

- 58002e2 ecmascript: Remove the lifetime annotation on
`MayHaveSideEffects` (#8717) (Boshen)
- 10e5920 linter: Move finishing default diagnostic message to
`GraphicalReporter` (#8683) (Sysix)
- 52a37d0 mangler: Simplify initialization of `slots` (#8734) (Dunqing)
- 6bc906c minifier: Allow mutating arguments in methods called from
`try_fold_known_string_methods` (#8729) (sapphi-red)
- bf8be23 minifier: Use `Ctx` (#8716) (Boshen)
- 0af0267 minifier: Side effect detection needs symbols resolution
(#8715) (Boshen)
- 32e0e47 minifier: Clean up `Normalize` (#8700) (Boshen)
- c792068 semantic: Simplify `ScopeTree::iter_bindings` (#8723)
(Dunqing)

### Testing

- 03229c5 minifier: Fix broken tests (#8722) (Boshen)

Co-authored-by: Boshen <1430279+Boshen@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
0-merge Merge with Graphite Merge Queue A-ast Area - AST A-minifier Area - Minifier C-enhancement Category - New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@sapphi-red @Boshen