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.

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

Turbopack: Don't replace constant conditions with sideeffects #72608

Merged
merged 5 commits into from
Nov 18, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 9 additions & 12 deletions turbopack/crates/turbopack-ecmascript/src/analyzer/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2076,18 +2076,15 @@ impl JsValue {
ObjectPart::KeyValue(k, v) => k.has_side_effects() || v.has_side_effects(),
ObjectPart::Spread(v) => v.has_side_effects(),
}),
JsValue::New(_, callee, args) => {
callee.has_side_effects() || args.iter().any(JsValue::has_side_effects)
}
JsValue::Call(_, callee, args) => {
callee.has_side_effects() || args.iter().any(JsValue::has_side_effects)
}
JsValue::SuperCall(_, args) => args.iter().any(JsValue::has_side_effects),
JsValue::MemberCall(_, obj, prop, args) => {
obj.has_side_effects()
|| prop.has_side_effects()
|| args.iter().any(JsValue::has_side_effects)
}
// As function bodies aren't analyzed for side-effects, we have to assume every call can
// have sideeffects as well.
// Otherwise it would be
// `func_body(callee).has_side_effects() ||
// callee.has_side_effects() || args.iter().any(JsValue::has_side_effects`
JsValue::New(_, _callee, _args) => true,
JsValue::Call(_, _callee, _args) => true,
JsValue::SuperCall(_, _args) => true,
JsValue::MemberCall(_, _obj, _prop, _args) => true,
JsValue::Member(_, obj, prop) => obj.has_side_effects() || prop.has_side_effects(),
JsValue::Function(_, _, _) => false,
JsValue::Url(_, _) => false,
Expand Down
14 changes: 10 additions & 4 deletions turbopack/crates/turbopack-ecmascript/src/references/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -900,6 +900,10 @@ pub(crate) async fn analyse_ecmascript_module_internal(
span: _,
in_try: _,
} => {
// Don't replace condition with it's truth-y value, if it has side effects (e.g.
// function calls)
let condition_has_side_effects = condition.has_side_effects();

let condition = analysis_state
.link_value(condition, ImportAttributes::empty_ref())
.await?;
Expand All @@ -911,10 +915,12 @@ pub(crate) async fn analyse_ecmascript_module_internal(
}
macro_rules! condition {
($expr:expr) => {
analysis.add_code_gen(ConstantCondition::new(
Value::new($expr),
Vc::cell(condition_ast_path.to_vec()),
));
if !condition_has_side_effects {
analysis.add_code_gen(ConstantCondition::new(
Value::new($expr),
Vc::cell(condition_ast_path.to_vec()),
));
}
};
}
macro_rules! active {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -55,9 +55,7 @@ $k = (...) => ((bj(a) ? 1 : 0) | 11 | 14 | 2)
| undefined
| ???*0*
| b
| null
| pj(a, b, c)
| b["child"]
| cj(a, b, b["type"], b["pendingProps"], c)
| yj(a, b, c)
| ej(a, b, c)
Expand Down Expand Up @@ -117,7 +115,10 @@ $k = (...) => ((bj(a) ? 1 : 0) | 11 | 14 | 2)

*anonymous function 131212* = (...) => sl(null, a, b, !(1), c)

*anonymous function 131315* = (...) => (a["_reactRootContainer"] ? !(0) : !(1))
*anonymous function 131315* = (...) => (a["_reactRootContainer"] ? ???*0* : !(1))
- *0* !(0)
⚠️ sequence with side effects
⚠️ This value might have side effects

*anonymous function 131389* = (...) => undefined

Expand Down Expand Up @@ -577,14 +578,17 @@ Fh = Uf(Dh)

Fi = (...) => di()["memoizedState"]

Fj = (...) => (undefined | null | (???*0* ? b : null) | ???*1* | b["child"])
- *0* unsupported expression
⚠️ This value might have side effects
- *1* b
Fj = (...) => (undefined | ???*0* | null | (???*1* ? b : null) | b["child"])
- *0* null
⚠️ sequence with side effects
⚠️ This value might have side effects
- *1* unsupported expression
⚠️ This value might have side effects

Fk = (...) => null
Fk = (...) => (???*0* | null)
- *0* null
⚠️ sequence with side effects
⚠️ This value might have side effects

G = (...) => undefined

Expand Down Expand Up @@ -722,7 +726,7 @@ Jh = (...) => undefined

Ji = (...) => undefined

Jj = (...) => (undefined | ???*0* | null | (???*3* ? ???*4* : null))
Jj = (...) => (undefined | ???*0* | (???*3* ? ???*4* : null) | null)
- *0* (???*1* ? ???*2* : null)
⚠️ sequence with side effects
⚠️ This value might have side effects
Expand Down Expand Up @@ -1289,7 +1293,10 @@ Vk = (...) => undefined

W = (...) => undefined

Wa = (...) => (!(1) | !(0) | ((a !== c) ? !(0) : !(1)))
Wa = (...) => (!(1) | !(0) | ((a !== c) ? ???*0* : !(1)))
- *0* !(0)
⚠️ sequence with side effects
⚠️ This value might have side effects

Wb = (...) => (b["dehydrated"] | null)

Expand Down Expand Up @@ -1373,7 +1380,10 @@ Ya = (...) => A(
- *2* unsupported expression
⚠️ This value might have side effects

Yb = (...) => (((b !== a) ? null : a) | a | b | ((c["stateNode"]["current"] === c) ? a : b))
Yb = (...) => (((b !== a) ? null : a) | ???*0* | ((c["stateNode"]["current"] === c) ? a : b))
- *0* a
⚠️ sequence with side effects
⚠️ This value might have side effects

Yc = (...) => (
| a
Expand Down Expand Up @@ -5763,7 +5773,10 @@ kh = (...) => undefined

ki = (...) => c(*anonymous function 67764*)

kj = (...) => ($i(a, b, f) | b["child"])
kj = (...) => (???*0* | b["child"])
- *0* $i(a, b, f)
⚠️ sequence with side effects
⚠️ This value might have side effects

kk = (...) => undefined

Expand Down Expand Up @@ -5990,7 +6003,10 @@ n#292 = (

n#404 = (a | t["payload"])

n#431 = (...) => l
n#431 = (...) => (???*0* | l)
- *0* l
⚠️ sequence with side effects
⚠️ This value might have side effects

n#449 = r(e, u, h[w], k)

Expand Down Expand Up @@ -6356,7 +6372,10 @@ t#292 = ((0 !== ???*0*) | [] | Bd | Td | new t(x, `${w}enter`, n, c, e) | k | vf

t#404 = h

t#431 = (...) => l
t#431 = (...) => (???*0* | l)
- *0* l
⚠️ sequence with side effects
⚠️ This value might have side effects

t#454 = r(e, m, n["value"], k)

Expand Down
Loading
Loading