Skip to content

Commit

Permalink
add possible unknown mutation to objects and arrays (vercel/turborepo…
Browse files Browse the repository at this point in the history 
…#3882)

Before the static evaluation didn't consider the fact that objects and arrays can be mutated. So we can't just assume for sure that they have certain properties/items. Instead we add an unknown mutation alternative to handle that.

This avoids `if(obj.prop)` to be replaced with `if(true)` when prop is initialized with true. It might be modified in future.
  • Loading branch information
@sokra
sokra authored Feb 21, 2023
1 parent 937440c commit a52081c
Show file tree
Hide file tree
Showing 38 changed files with 2,021 additions and 1,378 deletions.
2 changes: 1 addition & 1 deletion crates/next-core/src/util.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -194,7 +194,7 @@ fn parse_config_from_js_value(module_asset: AssetVc, value: &JsValue) -> NextSou
.as_issue()
.emit()
};
if let JsValue::Object(_, parts) = value {
if let JsValue::Object { parts, .. } = value {
for part in parts {
match part {
ObjectPart::Spread(_) => invalid_config(
Expand Down
21 changes: 21 additions & 0 deletions crates/next-dev-tests/tests/integration/turbopack/basic/comptime/input/index.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,27 @@ it("should not follow conditional references", () => {
expect(func.toString()).not.toContain("import(");
});

it("should allow to mutate objects", () => {
const obj = { a: true, b: false };
if (!obj.a) {
throw new Error("should not be executed");
}
if (obj.b) {
throw new Error("should not be executed");
}
function changeIt(o) {
o.a = false;
o.b = true;
}
changeIt(obj);
if (obj.a) {
throw new Error("should not be executed");
}
if (!obj.b) {
throw new Error("should not be executed");
}
});

it("should allow replacements in IIFEs", () => {
(function func() {
if (false) {
Expand Down
54 changes: 41 additions & 13 deletions crates/turbopack-ecmascript/src/analyzer/builtin.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,8 +20,8 @@ pub fn early_replace_builtin(value: &mut JsValue) -> bool {
JsValue::Constant(_)
| JsValue::Url(_)
| JsValue::WellKnownObject(_)
| JsValue::Array(_, _)
| JsValue::Object(_, _)
| JsValue::Array { .. }
| JsValue::Object { .. }
| JsValue::Alternatives(_, _)
| JsValue::Concat(_, _)
| JsValue::Add(_, _)
Expand Down Expand Up @@ -79,7 +79,11 @@ pub fn replace_builtin(value: &mut JsValue) -> bool {
true
}
// matching property access on an array like `[1,2,3].prop` or `[1,2,3][1]`
JsValue::Array(_, array) => {
&mut JsValue::Array {
ref mut items,
mutable,
..
} => {
fn items_to_alternatives(items: &mut Vec<JsValue>, prop: &mut JsValue) -> JsValue {
items.push(JsValue::Unknown(
Some(Arc::new(JsValue::member(
Expand All @@ -95,8 +99,11 @@ pub fn replace_builtin(value: &mut JsValue) -> bool {
// We can replace this with the value at the index
JsValue::Constant(ConstantValue::Num(num @ ConstantNumber(_))) => {
if let Some(index) = num.as_u32_index() {
if index < array.len() {
*value = array.swap_remove(index);
if index < items.len() {
*value = items.swap_remove(index);
if mutable {
value.add_unknown_mutations();
}
true
} else {
*value = JsValue::Unknown(
Expand Down Expand Up @@ -130,13 +137,17 @@ pub fn replace_builtin(value: &mut JsValue) -> bool {
// otherwise we can say that this might gives an item of the array
// but we also add an unknown value to the alternatives for other properties
_ => {
*value = items_to_alternatives(array, prop);
*value = items_to_alternatives(items, prop);
true
}
}
}
// matching property access on an object like `{a: 1, b: 2}.a`
JsValue::Object(_, parts) => {
&mut JsValue::Object {
ref mut parts,
mutable,
..
} => {
fn parts_to_alternatives(
parts: &mut Vec<ObjectPart>,
prop: &mut Box<JsValue>,
Expand Down Expand Up @@ -220,6 +231,9 @@ pub fn replace_builtin(value: &mut JsValue) -> bool {
false,
);
}
if mutable {
value.add_unknown_mutations();
}
return true;
}
} else {
Expand All @@ -242,6 +256,9 @@ pub fn replace_builtin(value: &mut JsValue) -> bool {
true,
);
}
if mutable {
value.add_unknown_mutations();
}
true
}
// matching mutliple alternative properties on an object like `{a: 1, b: 2}[(a |
Expand All @@ -267,7 +284,7 @@ pub fn replace_builtin(value: &mut JsValue) -> bool {
JsValue::MemberCall(_, box ref mut obj, box ref mut prop, ref mut args) => {
match obj {
// matching calls on an array like `[1,2,3].concat([4,5,6])`
JsValue::Array(_, items) => {
JsValue::Array { items, mutable, .. } => {
// matching cases where the property is a const string
if let Some(str) = prop.as_str() {
match str {
Expand All @@ -276,7 +293,7 @@ pub fn replace_builtin(value: &mut JsValue) -> bool {
if args.iter().all(|arg| {
matches!(
arg,
JsValue::Array(..)
JsValue::Array { .. }
| JsValue::Constant(_)
| JsValue::Url(_)
| JsValue::Concat(..)
Expand All @@ -288,8 +305,13 @@ pub fn replace_builtin(value: &mut JsValue) -> bool {
}) {
for arg in args {
match arg {
JsValue::Array(_, inner) => {
JsValue::Array {
items: inner,
mutable: inner_mutable,
..
} => {
items.extend(take(inner));
*mutable |= *inner_mutable;
}
JsValue::Constant(_)
| JsValue::Url(_)
Expand Down Expand Up @@ -372,16 +394,22 @@ pub fn replace_builtin(value: &mut JsValue) -> bool {
true
}
// match object literals
JsValue::Object(_, parts) => {
JsValue::Object { parts, mutable, .. } => {
// If the object contains any spread, we might be able to flatten that
if parts
.iter()
.any(|part| matches!(part, ObjectPart::Spread(JsValue::Object(..))))
.any(|part| matches!(part, ObjectPart::Spread(JsValue::Object { .. })))
{
let old_parts = take(parts);
for part in old_parts {
if let ObjectPart::Spread(JsValue::Object(_, inner_parts)) = part {
if let ObjectPart::Spread(JsValue::Object {
parts: inner_parts,
mutable: inner_mutable,
..
}) = part
{
parts.extend(inner_parts);
*mutable |= inner_mutable;
} else {
parts.push(part);
}
Expand Down
4 changes: 2 additions & 2 deletions crates/turbopack-ecmascript/src/analyzer/graph.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1321,10 +1321,10 @@ impl VisitAstPath for Analyzer<'_> {

Pat::Array(arr) => {
match &value {
Some(JsValue::Array(_, value)) => {
Some(JsValue::Array { items, .. }) => {
ast_path.with(AstParentNodeRef::Pat(pat, PatField::Array), |ast_path| {
for (idx, elem) in arr.elems.iter().enumerate() {
self.current_value = value.get(idx).cloned();
self.current_value = items.get(idx).cloned();
ast_path.with(
AstParentNodeRef::ArrayPat(arr, ArrayPatField::Elems(idx)),
|ast_path| {
Expand Down
Loading

0 comments on commit a52081c

Please sign in to comment.