Skip to content

Commit 9be968b

Browse files
sapphi-redsapphi-red
committed
fix(minifier): avoid merging side effectful expressions to next assignment statement if the side effect may change the left hand side reference
1 parent e8b6169 commit 9be968b

File tree

7 files changed

+124
-55
lines changed

7 files changed

+124
-55
lines changed

‎apps/oxlint/src-js/generated/deserialize.js‎

Lines changed: 16 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -4991,21 +4991,22 @@ function deserializeTSModuleDeclaration(pos) {
49914991
} else {
49924992
let innerId = body.id;
49934993
if (innerId.type === "Identifier") {
4994-
let start, end;
4995-
id.parent =
4996-
innerId.parent =
4997-
node.id =
4998-
parent =
4999-
{
5000-
__proto__: NodeProto,
5001-
type: "TSQualifiedName",
5002-
left: id,
5003-
right: innerId,
5004-
start: (start = id.start),
5005-
end: (end = innerId.end),
5006-
range: [start, end],
5007-
parent: node,
5008-
};
4994+
let start,
4995+
end,
4996+
outerId =
4997+
(node.id =
4998+
parent =
4999+
{
5000+
__proto__: NodeProto,
5001+
type: "TSQualifiedName",
5002+
left: id,
5003+
right: innerId,
5004+
start: (start = id.start),
5005+
end: (end = innerId.end),
5006+
range: [start, end],
5007+
parent: node,
5008+
});
5009+
id.parent = innerId.parent = outerId;
50095010
} else {
50105011
// Replace `left` of innermost `TSQualifiedName` with a nested `TSQualifiedName` with `id` of
50115012
// this module on left, and previous `left` of innermost `TSQualifiedName` on right

‎crates/oxc_minifier/src/peephole/minimize_statements.rs‎

Lines changed: 47 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1384,6 +1384,38 @@ impl<'a> PeepholeOptimizations {
13841384
// ```
13851385
return Some(false);
13861386
}
1387+
if replacement_has_side_effect {
1388+
// If the assignment target may depend on side effects of the replacement,
1389+
// don't reorder it past the assignment target. The non-last part of the
1390+
// assignment target is evaluated before the assignment evaluation so that
1391+
// part may be changed by the side effect. For example, "fn()" may change
1392+
// "foo" here:
1393+
// ```js
1394+
// let a = fn();
1395+
// foo.bar = a;
1396+
// ```
1397+
let may_depend_on_side_effect = match &assign_expr.left {
1398+
AssignmentTarget::AssignmentTargetIdentifier(_) => false,
1399+
AssignmentTarget::ComputedMemberExpression(member_expr) => {
1400+
Self::is_expression_that_reference_may_change(&member_expr.object, ctx)
1401+
}
1402+
AssignmentTarget::PrivateFieldExpression(member_expr) => {
1403+
Self::is_expression_that_reference_may_change(&member_expr.object, ctx)
1404+
}
1405+
AssignmentTarget::StaticMemberExpression(member_expr) => {
1406+
Self::is_expression_that_reference_may_change(&member_expr.object, ctx)
1407+
}
1408+
AssignmentTarget::ArrayAssignmentTarget(_)
1409+
| AssignmentTarget::ObjectAssignmentTarget(_)
1410+
| AssignmentTarget::TSAsExpression(_)
1411+
| AssignmentTarget::TSNonNullExpression(_)
1412+
| AssignmentTarget::TSSatisfiesExpression(_)
1413+
| AssignmentTarget::TSTypeAssertion(_) => true,
1414+
};
1415+
if may_depend_on_side_effect {
1416+
return Some(false);
1417+
}
1418+
}
13871419
// If we get here then it should be safe to attempt to substitute the
13881420
// replacement past the left operand into the right operand.
13891421
if let Some(changed) = Self::substitute_single_use_symbol_in_expression(
@@ -1799,6 +1831,21 @@ impl<'a> PeepholeOptimizations {
17991831
// Otherwise we should stop trying to substitute past this point
18001832
Some(false)
18011833
}
1834+
1835+
fn is_expression_that_reference_may_change(expr: &Expression<'a>, ctx: &Ctx<'a, '_>) -> bool {
1836+
match expr {
1837+
Expression::Identifier(id) => {
1838+
if let Some(symbol_id) = ctx.scoping().get_reference(id.reference_id()).symbol_id()
1839+
{
1840+
ctx.scoping().symbol_is_mutated(symbol_id)
1841+
} else {
1842+
true
1843+
}
1844+
}
1845+
Expression::ThisExpression(_) => false,
1846+
_ => true,
1847+
}
1848+
}
18021849
}
18031850

18041851
#[cfg(test)]

‎crates/oxc_minifier/tests/peephole/inline_single_use_variable.rs‎

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -222,6 +222,25 @@ fn test_inline_single_use_variable() {
222222
"function wrapper() { var __proto__ = []; return { __proto__ } }",
223223
"function wrapper() { return { ['__proto__']: [] } }",
224224
);
225+
226+
// cannot be compressed to `function wrapper() { var foo; globalThis[console.log(foo)] = (() => { foo = 1 })() }`
227+
test(
228+
"function wrapper() { var foo; var bar = (() => { foo = 1 })(); globalThis[console.log(foo)] = bar }",
229+
"function wrapper() { var foo, bar = (() => { foo = 1 })(); globalThis[console.log(foo)] = bar }",
230+
);
231+
// cannot be compressed to `function wrapper() { let foo; return foo.bar = (() => { foo = {} })(), foo }`
232+
test(
233+
"function wrapper() { let foo; const bar = (() => { foo = {} })(); foo.bar = bar; return foo }",
234+
"function wrapper() { let foo, bar = (() => { foo = {} })(); return foo.bar = bar, foo }",
235+
);
236+
test(
237+
"function wrapper() { let foo = {}; const bar = (() => { console.log() })(); foo.bar = bar; return foo }",
238+
"function wrapper() { let foo = {}; return foo.bar = (() => { console.log() })(), foo }",
239+
);
240+
test(
241+
"function wrapper() { const bar = (() => { console.log() })(); this.bar = bar; return this }",
242+
"function wrapper() { return this.bar = (() => { console.log() })(), this }",
243+
);
225244
}
226245

227246
#[test]

‎napi/parser/generated/deserialize/js_parent.js‎

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -4346,10 +4346,9 @@ function deserializeTSModuleDeclaration(pos) {
43464346
body.parent = node;
43474347
} else {
43484348
let innerId = body.id;
4349-
if (innerId.type === "Identifier")
4350-
id.parent =
4351-
innerId.parent =
4352-
node.id =
4349+
if (innerId.type === "Identifier") {
4350+
let outerId =
4351+
(node.id =
43534352
parent =
43544353
{
43554354
type: "TSQualifiedName",
@@ -4358,8 +4357,9 @@ function deserializeTSModuleDeclaration(pos) {
43584357
start: id.start,
43594358
end: innerId.end,
43604359
parent: node,
4361-
};
4362-
else {
4360+
});
4361+
id.parent = innerId.parent = outerId;
4362+
} else {
43634363
// Replace `left` of innermost `TSQualifiedName` with a nested `TSQualifiedName` with `id` of
43644364
// this module on left, and previous `left` of innermost `TSQualifiedName` on right
43654365
node.id = innerId;

‎napi/parser/generated/deserialize/js_range_parent.js‎

Lines changed: 15 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -4560,20 +4560,21 @@ function deserializeTSModuleDeclaration(pos) {
45604560
} else {
45614561
let innerId = body.id;
45624562
if (innerId.type === "Identifier") {
4563-
let start, end;
4564-
id.parent =
4565-
innerId.parent =
4566-
node.id =
4567-
parent =
4568-
{
4569-
type: "TSQualifiedName",
4570-
left: id,
4571-
right: innerId,
4572-
start: (start = id.start),
4573-
end: (end = innerId.end),
4574-
range: [start, end],
4575-
parent: node,
4576-
};
4563+
let start,
4564+
end,
4565+
outerId =
4566+
(node.id =
4567+
parent =
4568+
{
4569+
type: "TSQualifiedName",
4570+
left: id,
4571+
right: innerId,
4572+
start: (start = id.start),
4573+
end: (end = innerId.end),
4574+
range: [start, end],
4575+
parent: node,
4576+
});
4577+
id.parent = innerId.parent = outerId;
45774578
} else {
45784579
// Replace `left` of innermost `TSQualifiedName` with a nested `TSQualifiedName` with `id` of
45794580
// this module on left, and previous `left` of innermost `TSQualifiedName` on right

‎napi/parser/generated/deserialize/ts_parent.js‎

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -4604,10 +4604,9 @@ function deserializeTSModuleDeclaration(pos) {
46044604
body.parent = node;
46054605
} else {
46064606
let innerId = body.id;
4607-
if (innerId.type === "Identifier")
4608-
id.parent =
4609-
innerId.parent =
4610-
node.id =
4607+
if (innerId.type === "Identifier") {
4608+
let outerId =
4609+
(node.id =
46114610
parent =
46124611
{
46134612
type: "TSQualifiedName",
@@ -4616,8 +4615,9 @@ function deserializeTSModuleDeclaration(pos) {
46164615
start: id.start,
46174616
end: innerId.end,
46184617
parent: node,
4619-
};
4620-
else {
4618+
});
4619+
id.parent = innerId.parent = outerId;
4620+
} else {
46214621
// Replace `left` of innermost `TSQualifiedName` with a nested `TSQualifiedName` with `id` of
46224622
// this module on left, and previous `left` of innermost `TSQualifiedName` on right
46234623
node.id = innerId;

‎napi/parser/generated/deserialize/ts_range_parent.js‎

Lines changed: 15 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -4825,20 +4825,21 @@ function deserializeTSModuleDeclaration(pos) {
48254825
} else {
48264826
let innerId = body.id;
48274827
if (innerId.type === "Identifier") {
4828-
let start, end;
4829-
id.parent =
4830-
innerId.parent =
4831-
node.id =
4832-
parent =
4833-
{
4834-
type: "TSQualifiedName",
4835-
left: id,
4836-
right: innerId,
4837-
start: (start = id.start),
4838-
end: (end = innerId.end),
4839-
range: [start, end],
4840-
parent: node,
4841-
};
4828+
let start,
4829+
end,
4830+
outerId =
4831+
(node.id =
4832+
parent =
4833+
{
4834+
type: "TSQualifiedName",
4835+
left: id,
4836+
right: innerId,
4837+
start: (start = id.start),
4838+
end: (end = innerId.end),
4839+
range: [start, end],
4840+
parent: node,
4841+
});
4842+
id.parent = innerId.parent = outerId;
48424843
} else {
48434844
// Replace `left` of innermost `TSQualifiedName` with a nested `TSQualifiedName` with `id` of
48444845
// this module on left, and previous `left` of innermost `TSQualifiedName` on right

0 commit comments

Comments
 (0)