You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
fix(javascript): keep !!!x and writes arguments reads in the one pass - #22958
This fixes two bugs that #22949 brought into the one-pass minifier (modules.onePass, off by default). CodeRabbit reported both on #22956.
!!!x read for its truth printed nothing. The inner !! fold already dropped two !, and the outer fold dropped one more, which left an empty if().
A write to a parameter was dropped as unread even when the function reads its own arguments. In sloppy code, arguments sees that write.
// inputfunctionf(a){a=1;returnarguments[0]}if(!!!x)g();// beforefunctionf(n){returnarguments[0]}if()g();// f(5) returns 5, and the program no longer parses// afterfunctionf(n){n=1;returnarguments[0]}if(!x)g();
side-effects.js still treats comparisons such as < and == as pure, like terser's has_side_effects and the default path. Gzip on the six fixtures is unchanged.
What kind of change does this PR introduce?
fix
Did you add tests for your changes?
Yes: two rows in the one-pass case table in test/unitCases/javascript/printer.unittest.js.
Does this PR introduce a breaking change?
No.
If relevant, what needs to be documented once your changes are merged or what have you already documented?
n/a
Use of AI
Written with Claude Code: it reproduced both CodeRabbit findings, fixed them, and checked printer tests and terser's 672 expect_stdout cases.
…pass
In the one pass, `!!!x` read for its truth printed nothing: the outer
`!!` fold dropped the `!` its inner fold had already dropped. A write
to a parameter of a function reading its own `arguments` went as unread
while `arguments` reads it in sloppy code.
Claude-Session: https://claude.ai/code/session_01Tgxgyb4N35ndP6cC4KcrXC
Reviewing files that changed from the base of the PR and between c494697 and f79c119.
Files selected for processing: 3
.changeset/071-one-pass-negation-arguments.md
lib/javascript/syntax-printer.js
test/unitCases/javascript/printer.unittest.js
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 7 remain after this review.
Walkthrough
The one-pass JavaScript minifier now preserves parameter writes that can be observed through arguments and avoids simplifying double negation when the inner operand is itself negated. Unit cases cover both changes.
Changes: One-pass minifier corrections
Layer / File(s)
Summary
Preserve writes observed through arguments lib/javascript/syntax-printer.js, test/unitCases/javascript/printer.unittest.js, .changeset/071-one-pass-negation-arguments.md
The syntax printer detects arguments reads in a function and nested arrows. It retains parameter assignments when the function reads arguments, and clears the detection cache for each unused-code analysis. Unit cases cover reads through arguments[0] and a returned arrow.
The !!-to-truthiness simplification now excludes operands that are themselves negations. Unit cases expect !!!x to reduce to !x and !!!!x to reduce to x.
Possibly related PRs
webpack/webpack#22956: Both changes modify one-pass compression in lib/javascript/syntax-printer.js; this PR adds call inlining.
webpack/webpack#22914: Both changes preserve semantics involving arguments reads during minification.
The one-pass fixes are ready to merge after normal checks.
Pre-merge checks: 3 · 1
Failed checks: 1 inconclusive
Check name
Status
Explanation
Resolution
Title check
The title uses valid Conventional Commit syntax, uses the allowed type fix, and accurately describes the two one-pass minifier fixes. The branch name is not provided, so the required match between t…
Provide the branch name or confirm that its prefix is fix. If confirmed, this check passes.
Passed checks: 3 passed
Check name
Status
Explanation
Description Check
Check skipped - CodeRabbit’s high-level summary is enabled.
Linked Issues check
Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check
Check skipped because no linked issues were found for this pull request.
Full details: Title check
Explanation
The title uses valid Conventional Commit syntax, uses the allowed type fix, and accurately describes the two one-pass minifier fixes. The branch name is not provided, so the required match between the title type and branch prefix cannot be verified.
Resolution
Provide the branch name or confirm that its prefix is fix. If confirmed, this check passes.
Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Warning
Some tools did not complete. Review the errors below.
ast-grep (0.45.3)
test/unitCases/javascript/printer.unittest.js
ast-grep timed out on this file
lib/javascript/syntax-printer.js
ast-grep timed out on this file
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
Comparing base (c494697) to head (f79c119). Merging this pull request will not change the code webpack generates.
What moved
Tests
Assets
Gzip
Raw
Test edit
Changed, test untouched
0
0
—
—
—
Changed, test edited
0
0
—
—
—
New
0
0
—
—
—
Deleted
0
0
—
—
—
Unchanged
—
9447
—
—
—
Read Changed, test untouched first: the test is byte-identical on both sides, so webpack generated the difference. Changed, test edited moved partly because the test did — Test edit is how many bytes the test gained, and a bundle that grew by less is not a regression. New and Deleted are whole files, not deltas — a new asset was created, not grown, so it reads gray (⚪ added) — and adding tests cannot bury a real change. Gzip decides, because that is what users download; raw is the tiebreak, and brotli and zstd are per asset below.
No asset changed size.
No runtime gained or lost a runtime module.
Built test/configCases with the defaults a user gets: 2315 test(s), 9447 asset(s), 103 emitted nothing.
System calls cannot be simulated consistently and are left out of the measure. The reported value understates the real cost. Switch to the Walltime instrument to measure them.
Some benchmarks with significant performance changes were compared across different runner types, so the changes might not come from the code. Run base and head on the same runner type, or use CodSpeed Macro Runners for more stable results.
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
area: parserJavaScript parsing and dependency analysis (lib/javascript, lib/dependencies)
1 participant
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 fixes two bugs that #22949 brought into the one-pass minifier (
modules.onePass, off by default). CodeRabbit reported both on #22956.!!!xread for its truth printed nothing. The inner!!fold already dropped two!, and the outer fold dropped one more, which left an emptyif().arguments. In sloppy code,argumentssees that write.side-effects.jsstill treats comparisons such as<and==as pure, like terser'shas_side_effectsand the default path. Gzip on the six fixtures is unchanged.What kind of change does this PR introduce?
fix
Did you add tests for your changes?
Yes: two rows in the one-pass case table in
test/unitCases/javascript/printer.unittest.js.Does this PR introduce a breaking change?
No.
If relevant, what needs to be documented once your changes are merged or what have you already documented?
n/a
Use of AI
Written with Claude Code: it reproduced both CodeRabbit findings, fixed them, and checked printer tests and terser's 672
expect_stdoutcases.Generated by Claude Code
Summary by CodeRabbit
arguments.