Skip to content

Commit e9b4aa5

Browse files
committed
fix(minifier): keep top level variable declarations in script mode
In script mode: ```js var x; ``` must be preserved. fixes #12341
1 parent 98b17e9 commit e9b4aa5

14 files changed

+236
-121
lines changed

crates/oxc_minifier/src/compressor.rs

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -32,7 +32,7 @@ impl<'a> Compressor<'a> {
3232
scoping: Scoping,
3333
options: CompressOptions,
3434
) {
35-
let state = MinifierState::new(options);
35+
let state = MinifierState::new(program.source_type, options);
3636
let mut ctx = ReusableTraverseCtx::new(state, scoping, self.allocator);
3737
let normalize_options =
3838
NormalizeOptions { convert_while_to_fors: true, convert_const_to_let: true };
@@ -52,7 +52,7 @@ impl<'a> Compressor<'a> {
5252
scoping: Scoping,
5353
options: CompressOptions,
5454
) {
55-
let state = MinifierState::new(options);
55+
let state = MinifierState::new(program.source_type, options);
5656
let mut ctx = ReusableTraverseCtx::new(state, scoping, self.allocator);
5757
let normalize_options =
5858
NormalizeOptions { convert_while_to_fors: false, convert_const_to_let: false };

crates/oxc_minifier/src/ctx.rs

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -101,6 +101,10 @@ impl<'a> Ctx<'a, '_> {
101101
&self.0.state.options
102102
}
103103

104+
pub fn source_type(&self) -> SourceType {
105+
self.0.state.source_type
106+
}
107+
104108
pub fn is_global_reference(&self, ident: &IdentifierReference<'a>) -> bool {
105109
ident.is_global_reference(self.0.scoping())
106110
}

crates/oxc_minifier/src/peephole/minimize_conditional_expression.rs

Lines changed: 3 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -589,15 +589,13 @@ mod test {
589589

590590
use crate::{
591591
CompressOptions,
592-
tester::{run, test, test_same},
592+
tester::{test, test_options, test_same},
593593
};
594594

595595
fn test_es2019(source_text: &str, expected: &str) {
596596
let target = ESTarget::ES2019;
597-
assert_eq!(
598-
run(source_text, Some(CompressOptions { target, ..CompressOptions::default() })),
599-
run(expected, None)
600-
);
597+
let options = CompressOptions { target, ..CompressOptions::default() };
598+
test_options(source_text, expected, &options);
601599
}
602600

603601
#[test]

crates/oxc_minifier/src/peephole/minimize_conditions.rs

Lines changed: 5 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -303,7 +303,7 @@ impl<'a> PeepholeOptimizations {
303303
mod test {
304304
use crate::{
305305
CompressOptions,
306-
tester::{run, test, test_same},
306+
tester::{test, test_same, test_same_options},
307307
};
308308
use oxc_syntax::es_target::ESTarget;
309309

@@ -1429,11 +1429,8 @@ mod test {
14291429
test_same("foo().a || (foo().a = 3)");
14301430

14311431
let target = ESTarget::ES2019;
1432-
let code = "x || (x = 3)";
1433-
assert_eq!(
1434-
run(code, Some(CompressOptions { target, ..CompressOptions::default() })),
1435-
run(code, None)
1436-
);
1432+
let options = CompressOptions { target, ..CompressOptions::default() };
1433+
test_same_options("x || (x = 3)", &options);
14371434
}
14381435

14391436
#[test]
@@ -1456,11 +1453,8 @@ mod test {
14561453
test("var x; x = x || (() => 'a')", "var x; x ||= (() => 'a')");
14571454

14581455
let target = ESTarget::ES2019;
1459-
let code = "var x; x = x || 1";
1460-
assert_eq!(
1461-
run(code, Some(CompressOptions { target, ..CompressOptions::default() })),
1462-
run(code, None)
1463-
);
1456+
let options = CompressOptions { target, ..CompressOptions::default() };
1457+
test_same_options("var x; x = x || 1", &options);
14641458
}
14651459

14661460
#[test]

crates/oxc_minifier/src/peephole/minimize_statements.rs

Lines changed: 2 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,7 @@ use oxc_semantic::ScopeId;
88
use oxc_span::{ContentEq, GetSpan};
99
use oxc_traverse::Ancestor;
1010

11-
use crate::{CompressOptionsUnused, ctx::Ctx, keep_var::KeepVar};
11+
use crate::{ctx::Ctx, keep_var::KeepVar};
1212

1313
use super::{PeepholeOptimizations, State};
1414

@@ -385,7 +385,7 @@ impl<'a> PeepholeOptimizations {
385385
}
386386
let VariableDeclaration { span, kind, declarations, declare } = var_decl.unbox();
387387
for mut decl in declarations {
388-
if Self::is_declarator_unused(&decl, ctx) {
388+
if Self::should_remove_unused_declarator(&decl, ctx) {
389389
state.changed = true;
390390
if let Some(init) = decl.init.take() {
391391
if init.may_have_side_effects(ctx) {
@@ -406,25 +406,6 @@ impl<'a> PeepholeOptimizations {
406406
}
407407
}
408408

409-
fn is_declarator_unused(decl: &VariableDeclarator<'a>, ctx: &mut Ctx<'a, '_>) -> bool {
410-
if ctx.state.options.unused == CompressOptionsUnused::Keep {
411-
return false;
412-
}
413-
// It is unsafe to remove if direct eval is involved.
414-
if ctx.scoping().root_scope_flags().contains_direct_eval() {
415-
return false;
416-
}
417-
if let BindingPatternKind::BindingIdentifier(ident) = &decl.id.kind {
418-
if let Some(symbol_id) = ident.symbol_id.get() {
419-
return ctx
420-
.scoping()
421-
.get_resolved_references(symbol_id)
422-
.all(|r| !r.flags().is_read());
423-
}
424-
}
425-
false
426-
}
427-
428409
fn handle_expression_statement(
429410
&self,
430411
mut expr_stmt: Box<'a, ExpressionStatement<'a>>,

crates/oxc_minifier/src/peephole/mod.rs

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,7 @@ mod minimize_statements;
1313
mod normalize;
1414
mod remove_dead_code;
1515
mod remove_unused_expression;
16+
mod remove_unused_variable_declaration;
1617
mod replace_known_methods;
1718
mod substitute_alternate_syntax;
1819

crates/oxc_minifier/src/peephole/remove_dead_code.rs

Lines changed: 2 additions & 55 deletions
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,7 @@ use oxc_ecmascript::{constant_evaluation::ConstantEvaluation, side_effects::MayH
55
use oxc_span::GetSpan;
66
use oxc_traverse::Ancestor;
77

8-
use crate::{CompressOptionsUnused, ctx::Ctx, keep_var::KeepVar};
8+
use crate::{ctx::Ctx, keep_var::KeepVar};
99

1010
use super::{LatePeepholeOptimizations, PeepholeOptimizations, State};
1111

@@ -49,42 +49,13 @@ impl<'a> PeepholeOptimizations {
4949
self.try_fold_conditional_expression(e, state, ctx)
5050
}
5151
Expression::SequenceExpression(e) => self.try_fold_sequence_expression(e, state, ctx),
52-
Expression::AssignmentExpression(e) => self.remove_dead_assignment_expression(e, ctx),
5352
_ => None,
5453
} {
5554
*expr = folded_expr;
5655
state.changed = true;
5756
}
5857
}
5958

60-
fn remove_dead_assignment_expression(
61-
&self,
62-
e: &mut AssignmentExpression<'a>,
63-
ctx: &mut Ctx<'a, '_>,
64-
) -> Option<Expression<'a>> {
65-
if matches!(
66-
ctx.state.options.unused,
67-
CompressOptionsUnused::Keep | CompressOptionsUnused::KeepAssign
68-
) {
69-
return None;
70-
}
71-
let SimpleAssignmentTarget::AssignmentTargetIdentifier(ident) =
72-
e.left.as_simple_assignment_target()?
73-
else {
74-
return None;
75-
};
76-
let reference_id = ident.reference_id.get()?;
77-
let symbol_id = ctx.scoping().get_reference(reference_id).symbol_id()?;
78-
// Keep error for assigning to `const foo = 1; foo = 2`.
79-
if ctx.scoping().symbol_flags(symbol_id).is_const_variable() {
80-
return None;
81-
}
82-
if !ctx.scoping().get_resolved_references(symbol_id).all(|r| !r.flags().is_read()) {
83-
return None;
84-
}
85-
Some(e.right.take_in(ctx.ast))
86-
}
87-
8859
/// Removes dead code thats comes after `return`, `throw`, `continue` and `break` statements.
8960
pub fn remove_dead_code_exit_statements(
9061
&self,
@@ -566,21 +537,6 @@ impl<'a> PeepholeOptimizations {
566537
_ => false,
567538
}
568539
}
569-
570-
fn remove_unused_function_declaration(
571-
f: &Function<'a>,
572-
ctx: &mut Ctx<'a, '_>,
573-
) -> Option<Statement<'a>> {
574-
if ctx.state.options.unused == CompressOptionsUnused::Keep {
575-
return None;
576-
}
577-
let id = f.id.as_ref()?;
578-
let symbol_id = id.symbol_id.get()?;
579-
if ctx.scoping().symbol_is_unused(symbol_id) {
580-
return Some(ctx.ast.statement_empty(f.span));
581-
}
582-
None
583-
}
584540
}
585541

586542
impl<'a> LatePeepholeOptimizations {
@@ -603,10 +559,7 @@ impl<'a> LatePeepholeOptimizations {
603559
/// <https://github.com/google/closure-compiler/blob/v20240609/test/com/google/javascript/jscomp/PeepholeRemoveDeadCodeTest.java>
604560
#[cfg(test)]
605561
mod test {
606-
use crate::{
607-
CompressOptions,
608-
tester::{test, test_options, test_same},
609-
};
562+
use crate::tester::{test, test_same};
610563

611564
#[test]
612565
fn test_fold_block() {
@@ -813,10 +766,4 @@ mod test {
813766
fn remove_constant_value() {
814767
test("const foo = false; if (foo) { console.log('foo') }", "const foo = !1;");
815768
}
816-
817-
#[test]
818-
fn remove_unused_function_declaration() {
819-
let options = CompressOptions::smallest();
820-
test_options("function foo() {}", "", &options);
821-
}
822769
}

crates/oxc_minifier/src/peephole/remove_unused_expression.rs

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -32,6 +32,9 @@ impl<'a> PeepholeOptimizations {
3232
Expression::ConditionalExpression(_) => self.fold_conditional_expression(e, state, ctx),
3333
Expression::BinaryExpression(_) => self.fold_binary_expression(e, state, ctx),
3434
Expression::CallExpression(_) => self.fold_call_expression(e, state, ctx),
35+
Expression::AssignmentExpression(_) => {
36+
self.remove_unused_assignment_expression(e, state, ctx)
37+
}
3538
_ => !e.may_have_side_effects(ctx),
3639
}
3740
}
Lines changed: 152 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,152 @@
1+
use oxc_allocator::TakeIn;
2+
use oxc_ast::ast::*;
3+
use oxc_ecmascript::side_effects::MayHaveSideEffects;
4+
5+
use crate::{CompressOptionsUnused, ctx::Ctx};
6+
7+
use super::{PeepholeOptimizations, State};
8+
9+
impl<'a> PeepholeOptimizations {
10+
pub fn should_remove_unused_declarator(
11+
decl: &VariableDeclarator<'a>,
12+
ctx: &mut Ctx<'a, '_>,
13+
) -> bool {
14+
if ctx.state.options.unused == CompressOptionsUnused::Keep {
15+
return false;
16+
}
17+
if let BindingPatternKind::BindingIdentifier(ident) = &decl.id.kind {
18+
if Self::keep_top_level_var_in_script_mode(ctx) {
19+
return false;
20+
}
21+
// It is unsafe to remove if direct eval is involved.
22+
if ctx.scoping().root_scope_flags().contains_direct_eval() {
23+
return false;
24+
}
25+
if let Some(symbol_id) = ident.symbol_id.get() {
26+
return ctx.scoping().symbol_is_unused(symbol_id);
27+
}
28+
}
29+
false
30+
}
31+
32+
pub fn remove_unused_function_declaration(
33+
f: &Function<'a>,
34+
ctx: &mut Ctx<'a, '_>,
35+
) -> Option<Statement<'a>> {
36+
if ctx.state.options.unused == CompressOptionsUnused::Keep {
37+
return None;
38+
}
39+
if Self::keep_top_level_var_in_script_mode(ctx) {
40+
return None;
41+
}
42+
let id = f.id.as_ref()?;
43+
let symbol_id = id.symbol_id.get()?;
44+
if ctx.scoping().symbol_is_unused(symbol_id) {
45+
return Some(ctx.ast.statement_empty(f.span));
46+
}
47+
None
48+
}
49+
50+
pub fn remove_unused_assignment_expression(
51+
&self,
52+
e: &mut Expression<'a>,
53+
state: &mut State,
54+
ctx: &mut Ctx<'a, '_>,
55+
) -> bool {
56+
let Expression::AssignmentExpression(assign_expr) = e else { return false };
57+
if matches!(
58+
ctx.state.options.unused,
59+
CompressOptionsUnused::Keep | CompressOptionsUnused::KeepAssign
60+
) {
61+
return false;
62+
}
63+
let Some(SimpleAssignmentTarget::AssignmentTargetIdentifier(ident)) =
64+
assign_expr.left.as_simple_assignment_target()
65+
else {
66+
return false;
67+
};
68+
if Self::keep_top_level_var_in_script_mode(ctx) {
69+
return false;
70+
}
71+
let Some(reference_id) = ident.reference_id.get() else { return false };
72+
let Some(symbol_id) = ctx.scoping().get_reference(reference_id).symbol_id() else {
73+
return false;
74+
};
75+
// Keep error for assigning to `const foo = 1; foo = 2`.
76+
if ctx.scoping().symbol_flags(symbol_id).is_const_variable() {
77+
return false;
78+
}
79+
if !ctx.scoping().get_resolved_references(symbol_id).all(|r| !r.flags().is_read()) {
80+
return false;
81+
}
82+
state.changed = true;
83+
if assign_expr.right.may_have_side_effects(ctx) {
84+
*e = assign_expr.right.take_in(ctx.ast);
85+
false
86+
} else {
87+
true
88+
}
89+
}
90+
91+
/// Do remove top level vars in script mode.
92+
fn keep_top_level_var_in_script_mode(ctx: &Ctx<'a, '_>) -> bool {
93+
ctx.scoping.current_scope_id() == ctx.scoping().root_scope_id()
94+
&& ctx.source_type().is_script()
95+
}
96+
}
97+
98+
#[cfg(test)]
99+
mod test {
100+
use oxc_span::SourceType;
101+
102+
use crate::{
103+
CompressOptions,
104+
tester::{
105+
test_options, test_options_source_type, test_same_options,
106+
test_same_options_source_type,
107+
},
108+
};
109+
110+
#[test]
111+
fn remove_unused_variable_declaration() {
112+
let options = CompressOptions::smallest();
113+
test_options("var x", "", &options);
114+
test_options("var x = 1", "", &options);
115+
test_options("var x = foo", "foo", &options);
116+
test_same_options("var x; foo(x)", &options);
117+
test_same_options("export var x", &options);
118+
}
119+
120+
#[test]
121+
fn remove_unused_function_declaration() {
122+
let options = CompressOptions::smallest();
123+
test_options("function foo() {}", "", &options);
124+
test_same_options("function foo() {} foo()", &options);
125+
test_same_options("export function foo() {} foo()", &options);
126+
}
127+
128+
#[test]
129+
fn remove_unused_assignment_expression() {
130+
let options = CompressOptions::smallest();
131+
test_options("var x = 1; x = 2;", "", &options);
132+
test_options("var x = 1; x = 2;", "", &options);
133+
test_options("var x = 1; x = foo();", "foo()", &options);
134+
test_same_options("var x = 1; x = 2, foo(x)", &options);
135+
test_same_options("function foo() { var t; return t = x(); } foo();", &options);
136+
}
137+
138+
#[test]
139+
fn keep_in_script_mode() {
140+
let options = CompressOptions::smallest();
141+
let source_type = SourceType::cjs();
142+
test_same_options_source_type("var x = 1; x = 2;", source_type, &options);
143+
test_same_options_source_type("var x = 1; x = 2, foo(x)", source_type, &options);
144+
145+
test_options_source_type(
146+
"function foo() { var x = 1; x = 2; bar() } foo()",
147+
"function foo() { bar() } foo()",
148+
source_type,
149+
&options,
150+
);
151+
}
152+
}

0 commit comments

Comments
 (0)