Skip to content

Commit ae7f670

Browse files
committed
fix(minifier): avoid minifying +void unknown to NaN and fix typo (#8784)
fixes #8782 This bug appears when we arbitrarily eliminate potential side-effective elements in unary expression.
1 parent b3a5769 commit ae7f670

File tree

5 files changed

+50
-44
lines changed

5 files changed

+50
-44
lines changed

crates/oxc_ecmascript/src/constant_evaluation/mod.rs

+21-19
Original file line numberDiff line numberDiff line change
@@ -32,7 +32,7 @@ pub trait ConstantEvaluation<'a>: MayHaveSideEffects {
3232
// and there are only a very few cases where we can compute a number value, but there could
3333
// also be side effects. e.g. `void doSomething()` has value NaN, regardless of the behavior
3434
// of `doSomething()`
35-
if value.is_some() && self.expression_may_have_side_efffects(expr) {
35+
if value.is_some() && self.expression_may_have_side_effects(expr) {
3636
None
3737
} else {
3838
value
@@ -41,23 +41,23 @@ pub trait ConstantEvaluation<'a>: MayHaveSideEffects {
4141

4242
fn get_side_free_string_value(&self, expr: &Expression<'a>) -> Option<Cow<'a, str>> {
4343
let value = expr.to_js_string();
44-
if value.is_some() && !self.expression_may_have_side_efffects(expr) {
44+
if value.is_some() && !self.expression_may_have_side_effects(expr) {
4545
return value;
4646
}
4747
None
4848
}
4949

5050
fn get_side_free_boolean_value(&self, expr: &Expression<'a>) -> Option<bool> {
5151
let value = self.get_boolean_value(expr);
52-
if value.is_some() && !self.expression_may_have_side_efffects(expr) {
52+
if value.is_some() && !self.expression_may_have_side_effects(expr) {
5353
return value;
5454
}
5555
None
5656
}
5757

5858
fn get_side_free_bigint_value(&self, expr: &Expression<'a>) -> Option<BigInt> {
5959
let value = expr.to_big_int();
60-
if value.is_some() && self.expression_may_have_side_efffects(expr) {
60+
if value.is_some() && self.expression_may_have_side_effects(expr) {
6161
None
6262
} else {
6363
value
@@ -201,8 +201,8 @@ pub trait ConstantEvaluation<'a>: MayHaveSideEffects {
201201
) -> Option<ConstantValue<'a>> {
202202
match operator {
203203
BinaryOperator::Addition => {
204-
if self.expression_may_have_side_efffects(left)
205-
|| self.expression_may_have_side_efffects(right)
204+
if self.expression_may_have_side_effects(left)
205+
|| self.expression_may_have_side_effects(right)
206206
{
207207
return None;
208208
}
@@ -319,7 +319,7 @@ pub trait ConstantEvaluation<'a>: MayHaveSideEffects {
319319
None
320320
}
321321
BinaryOperator::Instanceof => {
322-
if self.expression_may_have_side_efffects(left) {
322+
if self.expression_may_have_side_effects(left) {
323323
return None;
324324
}
325325
if let Expression::Identifier(right_ident) = right {
@@ -379,34 +379,36 @@ pub trait ConstantEvaluation<'a>: MayHaveSideEffects {
379379
Some(ConstantValue::String(Cow::Borrowed(s)))
380380
}
381381
UnaryOperator::Void => (expr.argument.is_literal()
382-
|| !self.expression_may_have_side_efffects(&expr.argument))
382+
|| !self.expression_may_have_side_effects(&expr.argument))
383383
.then_some(ConstantValue::Undefined),
384384
UnaryOperator::LogicalNot => self
385385
.get_side_free_boolean_value(&expr.argument)
386386
.map(|b| !b)
387387
.map(ConstantValue::Boolean),
388388
UnaryOperator::UnaryPlus => {
389-
self.eval_to_number(&expr.argument).map(ConstantValue::Number)
389+
self.get_side_free_number_value(&expr.argument).map(ConstantValue::Number)
390390
}
391391
UnaryOperator::UnaryNegation => match ValueType::from(&expr.argument) {
392-
ValueType::BigInt => {
393-
self.eval_to_big_int(&expr.argument).map(|v| -v).map(ConstantValue::BigInt)
394-
}
392+
ValueType::BigInt => self
393+
.get_side_free_bigint_value(&expr.argument)
394+
.map(|v| -v)
395+
.map(ConstantValue::BigInt),
395396
ValueType::Number => self
396-
.eval_to_number(&expr.argument)
397+
.get_side_free_number_value(&expr.argument)
397398
.map(|v| if v.is_nan() { v } else { -v })
398399
.map(ConstantValue::Number),
399400
ValueType::Undefined => Some(ConstantValue::Number(f64::NAN)),
400401
ValueType::Null => Some(ConstantValue::Number(-0.0)),
401402
_ => None,
402403
},
403404
UnaryOperator::BitwiseNot => match ValueType::from(&expr.argument) {
404-
ValueType::BigInt => {
405-
self.eval_to_big_int(&expr.argument).map(|v| !v).map(ConstantValue::BigInt)
406-
}
405+
ValueType::BigInt => self
406+
.get_side_free_bigint_value(&expr.argument)
407+
.map(|v| !v)
408+
.map(ConstantValue::BigInt),
407409
#[expect(clippy::cast_lossless)]
408410
_ => self
409-
.eval_to_number(&expr.argument)
411+
.get_side_free_number_value(&expr.argument)
410412
.map(|v| (!v.to_int_32()) as f64)
411413
.map(ConstantValue::Number),
412414
},
@@ -423,7 +425,7 @@ pub trait ConstantEvaluation<'a>: MayHaveSideEffects {
423425
if let Some(ConstantValue::String(s)) = self.eval_expression(&expr.object) {
424426
Some(ConstantValue::Number(s.encode_utf16().count().to_f64().unwrap()))
425427
} else {
426-
if self.expression_may_have_side_efffects(&expr.object) {
428+
if self.expression_may_have_side_effects(&expr.object) {
427429
return None;
428430
}
429431
if let Expression::ArrayExpression(arr) = &expr.object {
@@ -446,7 +448,7 @@ pub trait ConstantEvaluation<'a>: MayHaveSideEffects {
446448
if let Some(ConstantValue::String(s)) = self.eval_expression(&expr.object) {
447449
Some(ConstantValue::Number(s.encode_utf16().count().to_f64().unwrap()))
448450
} else {
449-
if self.expression_may_have_side_efffects(&expr.object) {
451+
if self.expression_may_have_side_effects(&expr.object) {
450452
return None;
451453
}
452454
if let Expression::ArrayExpression(arr) = &expr.object {

crates/oxc_ecmascript/src/side_effects/may_have_side_effects.rs

+15-15
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,7 @@ use oxc_ast::ast::*;
66
pub trait MayHaveSideEffects {
77
fn is_global_reference(&self, ident: &IdentifierReference<'_>) -> bool;
88

9-
fn expression_may_have_side_efffects(&self, e: &Expression<'_>) -> bool {
9+
fn expression_may_have_side_effects(&self, e: &Expression<'_>) -> bool {
1010
match e {
1111
// Reference read can have a side effect.
1212
Expression::Identifier(ident) => match ident.name.as_str() {
@@ -24,19 +24,19 @@ pub trait MayHaveSideEffects {
2424
| Expression::ArrowFunctionExpression(_)
2525
| Expression::FunctionExpression(_) => false,
2626
Expression::TemplateLiteral(template) => {
27-
template.expressions.iter().any(|e| self.expression_may_have_side_efffects(e))
27+
template.expressions.iter().any(|e| self.expression_may_have_side_effects(e))
2828
}
2929
Expression::UnaryExpression(e) => self.unary_expression_may_have_side_effects(e),
3030
Expression::ParenthesizedExpression(e) => {
31-
self.expression_may_have_side_efffects(&e.expression)
31+
self.expression_may_have_side_effects(&e.expression)
3232
}
3333
Expression::ConditionalExpression(e) => {
34-
self.expression_may_have_side_efffects(&e.test)
35-
|| self.expression_may_have_side_efffects(&e.consequent)
36-
|| self.expression_may_have_side_efffects(&e.alternate)
34+
self.expression_may_have_side_effects(&e.test)
35+
|| self.expression_may_have_side_effects(&e.consequent)
36+
|| self.expression_may_have_side_effects(&e.alternate)
3737
}
3838
Expression::SequenceExpression(e) => {
39-
e.expressions.iter().any(|e| self.expression_may_have_side_efffects(e))
39+
e.expressions.iter().any(|e| self.expression_may_have_side_effects(e))
4040
}
4141
Expression::BinaryExpression(e) => self.binary_expression_may_have_side_effects(e),
4242
Expression::ObjectExpression(object_expr) => object_expr
@@ -57,7 +57,7 @@ pub trait MayHaveSideEffects {
5757
operator != UnaryOperator::Delete
5858
}
5959
if is_simple_unary_operator(e.operator) {
60-
return self.expression_may_have_side_efffects(&e.argument);
60+
return self.expression_may_have_side_effects(&e.argument);
6161
}
6262
true
6363
}
@@ -67,8 +67,8 @@ pub trait MayHaveSideEffects {
6767
if matches!(e.operator, BinaryOperator::In | BinaryOperator::Instanceof) {
6868
return true;
6969
}
70-
self.expression_may_have_side_efffects(&e.left)
71-
|| self.expression_may_have_side_efffects(&e.right)
70+
self.expression_may_have_side_effects(&e.left)
71+
|| self.expression_may_have_side_effects(&e.right)
7272
}
7373

7474
fn array_expression_element_may_have_side_effects(
@@ -77,10 +77,10 @@ pub trait MayHaveSideEffects {
7777
) -> bool {
7878
match e {
7979
ArrayExpressionElement::SpreadElement(e) => {
80-
self.expression_may_have_side_efffects(&e.argument)
80+
self.expression_may_have_side_effects(&e.argument)
8181
}
8282
match_expression!(ArrayExpressionElement) => {
83-
self.expression_may_have_side_efffects(e.to_expression())
83+
self.expression_may_have_side_effects(e.to_expression())
8484
}
8585
ArrayExpressionElement::Elision(_) => false,
8686
}
@@ -90,21 +90,21 @@ pub trait MayHaveSideEffects {
9090
match e {
9191
ObjectPropertyKind::ObjectProperty(o) => self.object_property_may_have_side_effects(o),
9292
ObjectPropertyKind::SpreadProperty(e) => {
93-
self.expression_may_have_side_efffects(&e.argument)
93+
self.expression_may_have_side_effects(&e.argument)
9494
}
9595
}
9696
}
9797

9898
fn object_property_may_have_side_effects(&self, e: &ObjectProperty<'_>) -> bool {
9999
self.property_key_may_have_side_effects(&e.key)
100-
|| self.expression_may_have_side_efffects(&e.value)
100+
|| self.expression_may_have_side_effects(&e.value)
101101
}
102102

103103
fn property_key_may_have_side_effects(&self, key: &PropertyKey<'_>) -> bool {
104104
match key {
105105
PropertyKey::StaticIdentifier(_) | PropertyKey::PrivateIdentifier(_) => false,
106106
match_expression!(PropertyKey) => {
107-
self.expression_may_have_side_efffects(key.to_expression())
107+
self.expression_may_have_side_effects(key.to_expression())
108108
}
109109
}
110110
}

crates/oxc_minifier/src/peephole/fold_constants.rs

+9-5
Original file line numberDiff line numberDiff line change
@@ -120,7 +120,7 @@ impl<'a, 'b> PeepholeOptimizations {
120120
// (FALSE && x) => FALSE
121121
if if lval { op == LogicalOperator::Or } else { op == LogicalOperator::And } {
122122
return Some(ctx.ast.move_expression(&mut logical_expr.left));
123-
} else if !ctx.expression_may_have_side_efffects(left) {
123+
} else if !ctx.expression_may_have_side_effects(left) {
124124
let parent = ctx.ancestry.parent();
125125
// Bail `let o = { f() { assert.ok(this !== o); } }; (true && o.f)(); (true && o.f)``;`
126126
if parent.is_tagged_template_expression()
@@ -145,7 +145,7 @@ impl<'a, 'b> PeepholeOptimizations {
145145
let left_child_right_boolean = ctx.get_boolean_value(&left_child.right);
146146
let left_child_op = left_child.operator;
147147
if let Some(right_boolean) = left_child_right_boolean {
148-
if !ctx.expression_may_have_side_efffects(&left_child.right) {
148+
if !ctx.expression_may_have_side_effects(&left_child.right) {
149149
// a || false || b => a || b
150150
// a && true && b => a && b
151151
if !right_boolean && left_child_op == LogicalOperator::Or
@@ -178,7 +178,7 @@ impl<'a, 'b> PeepholeOptimizations {
178178
let left_val = ValueType::from(left);
179179
match left_val {
180180
ValueType::Null | ValueType::Undefined => {
181-
Some(if ctx.expression_may_have_side_efffects(left) {
181+
Some(if ctx.expression_may_have_side_effects(left) {
182182
// e.g. `(a(), null) ?? 1` => `(a(), null, 1)`
183183
let expressions = ctx.ast.vec_from_array([
184184
ctx.ast.move_expression(&mut logical_expr.left),
@@ -381,8 +381,7 @@ impl<'a, 'b> PeepholeOptimizations {
381381
let left = &e.left;
382382
let right = &e.right;
383383
let op = e.operator;
384-
if ctx.expression_may_have_side_efffects(left)
385-
|| ctx.expression_may_have_side_efffects(right)
384+
if ctx.expression_may_have_side_effects(left) || ctx.expression_may_have_side_effects(right)
386385
{
387386
return None;
388387
}
@@ -1828,6 +1827,11 @@ mod test {
18281827
fold("typeof foo === 'number'", "typeof foo == 'number'");
18291828
}
18301829

1830+
#[test]
1831+
fn test_issue_8782() {
1832+
fold("+(void unknown())", "+void unknown()");
1833+
}
1834+
18311835
// TODO: All big ints are rare and difficult to handle.
18321836
mod bigint {
18331837
use super::{

crates/oxc_minifier/src/peephole/remove_dead_code.rs

+4-4
Original file line numberDiff line numberDiff line change
@@ -389,7 +389,7 @@ impl<'a, 'b> PeepholeOptimizations {
389389
| ValueType::Boolean
390390
| ValueType::Number
391391
| ValueType::String
392-
) && !ctx.expression_may_have_side_efffects(arg)
392+
) && !ctx.expression_may_have_side_effects(arg)
393393
}
394394
_ => false,
395395
},
@@ -526,7 +526,7 @@ impl<'a, 'b> PeepholeOptimizations {
526526

527527
match ctx.get_boolean_value(&expr.test) {
528528
Some(v) => {
529-
if ctx.expression_may_have_side_efffects(&expr.test) {
529+
if ctx.expression_may_have_side_effects(&expr.test) {
530530
let mut exprs = ctx.ast.vec_with_capacity(2);
531531
exprs.push(ctx.ast.move_expression(&mut expr.test));
532532
exprs.push(ctx.ast.move_expression(if v {
@@ -564,7 +564,7 @@ impl<'a, 'b> PeepholeOptimizations {
564564
(false, 0),
565565
|(mut should_fold, mut new_len), (i, expr)| {
566566
if i == sequence_expr.expressions.len() - 1
567-
|| ctx.expression_may_have_side_efffects(expr)
567+
|| ctx.expression_may_have_side_effects(expr)
568568
{
569569
new_len += 1;
570570
} else {
@@ -582,7 +582,7 @@ impl<'a, 'b> PeepholeOptimizations {
582582
let mut new_exprs = ctx.ast.vec_with_capacity(new_len);
583583
let len = sequence_expr.expressions.len();
584584
for (i, expr) in sequence_expr.expressions.iter_mut().enumerate() {
585-
if i == len - 1 || ctx.expression_may_have_side_efffects(expr) {
585+
if i == len - 1 || ctx.expression_may_have_side_effects(expr) {
586586
new_exprs.push(ctx.ast.move_expression(expr));
587587
}
588588
}

crates/oxc_minifier/src/peephole/substitute_alternate_syntax.rs

+1-1
Original file line numberDiff line numberDiff line change
@@ -433,7 +433,7 @@ impl<'a> PeepholeOptimizations {
433433
if !match argument {
434434
Expression::Identifier(ident) => ctx.is_identifier_undefined(ident),
435435
Expression::UnaryExpression(e) => {
436-
e.operator.is_void() && !ctx.expression_may_have_side_efffects(argument)
436+
e.operator.is_void() && !ctx.expression_may_have_side_effects(argument)
437437
}
438438
_ => false,
439439
} {

0 commit comments

Comments
 (0)