Skip to content

Commit

Permalink
fix(ecmascript): eval assignop to the ident (vercel#4609)
Browse files Browse the repository at this point in the history 
### Description

- closes WEB-889

When there is a variable with `+=` assignop only to the given ident

```
var css = '';
for (var i = 0; i < length; i++) {
        var partRule = this.rules[i];

        if (typeof partRule === 'string') {
          css += partRule;
        } else if (partRule) {
         css += partString;
        }
      }

      if (css) {
       ....
      }
```

evaluated result becomes

```
var css = '';
for(var i = 0; i < length; i++){
    var partRule = this.rules[i];
    if (typeof partRule === 'string') {
        css += partRule;
    } else if (partRule) {
        css += partString;
    }
}
if ("TURBOPACK compile-time falsy", 0) {
    "TURBOPACK unreachable";
}
```

so `if (css)` condition never executed even though it can be executed
depends on the runtime assignop condition.

PR attempts to evaluate if assignop is for the ident to prevent this.

---------

Co-authored-by: Tobias Koppers <tobias.koppers@googlemail.com>
  • Loading branch information
@kwonoj @sokra @NicholasLYang
2 people authored and NicholasLYang committed Apr 21, 2023
1 parent 26d3a3c commit fcc351a
Show file tree
Hide file tree
Showing 23 changed files with 3,365 additions and 2,427 deletions.
68 changes: 57 additions & 11 deletions crates/turbopack-ecmascript/src/analyzer/graph.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -320,21 +320,23 @@ impl EvalContext {
JsValue::concat(values)
}

fn eval_ident(&self, i: &Ident) -> JsValue {
let id = i.to_id();
if let Some(imported) = self.imports.get_import(&id) {
return imported;
}
if is_unresolved(i, self.unresolved_mark) {
JsValue::FreeVar(i.sym.clone())
} else {
JsValue::Variable(id)
}
}

pub fn eval(&self, e: &Expr) -> JsValue {
match e {
Expr::Paren(e) => self.eval(&e.expr),
Expr::Lit(e) => JsValue::Constant(e.clone().into()),
Expr::Ident(i) => {
let id = i.to_id();
if let Some(imported) = self.imports.get_import(&id) {
return imported;
}
if is_unresolved(i, self.unresolved_mark) {
JsValue::FreeVar(i.sym.clone())
} else {
JsValue::Variable(id)
}
}
Expr::Ident(i) => self.eval_ident(i),

Expr::Unary(UnaryExpr {
op: op!("!"), arg, ..
Expand Down Expand Up @@ -943,6 +945,29 @@ impl VisitAstPath for Analyzer<'_> {
AstParentNodeRef::AssignExpr(n, AssignExprField::Left),
|ast_path| match &n.left {
PatOrExpr::Expr(expr) => {
if let Some(key) = expr.as_ident() {
let value = match n.op {
AssignOp::Assign => unreachable!(
"AssignOp::Assign will never have an expression in n.left"
),
AssignOp::AndAssign | AssignOp::OrAssign | AssignOp::NullishAssign => {
let right = self.eval_context.eval(&n.right);
// We can handle the right value as alternative to the existing
// value
Some(right)
}
AssignOp::AddAssign => {
let left = self.eval_context.eval(expr);
let right = self.eval_context.eval(&n.right);
Some(JsValue::add(vec![left, right]))
}
_ => Some(JsValue::unknown_empty("unsupported assign operation")),
};
if let Some(value) = value {
self.add_value(key.to_id(), value);
}
}

ast_path.with(
AstParentNodeRef::PatOrExpr(&n.left, PatOrExprField::Expr),
|ast_path| {
Expand All @@ -951,6 +976,7 @@ impl VisitAstPath for Analyzer<'_> {
);
}
PatOrExpr::Pat(pat) => {
debug_assert!(n.op == AssignOp::Assign);
ast_path.with(
AstParentNodeRef::PatOrExpr(&n.left, PatOrExprField::Pat),
|ast_path| {
Expand All @@ -970,6 +996,26 @@ impl VisitAstPath for Analyzer<'_> {
);
}

fn visit_update_expr<'ast: 'r, 'r>(
&mut self,
n: &'ast UpdateExpr,
ast_path: &mut AstNodePath<AstParentNodeRef<'r>>,
) {
if let Some(key) = n.arg.as_ident() {
self.add_value(
key.to_id(),
JsValue::unknown_empty("updated with update expression"),
);
}

ast_path.with(
AstParentNodeRef::UpdateExpr(n, UpdateExprField::Arg),
|ast_path| {
self.visit_expr(&n.arg, ast_path);
},
);
}

fn visit_call_expr<'ast: 'r, 'r>(
&mut self,
n: &'ast CallExpr,
Expand Down
1 change: 1 addition & 0 deletions crates/turbopack-ecmascript/tests/analyzer/graph/assign/graph-effects.snapshot
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
[]
13 changes: 13 additions & 0 deletions crates/turbopack-ecmascript/tests/analyzer/graph/assign/graph-explained.snapshot
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
a = ("" | `${a}x`)

b = (0 | ???*0*)
- *0* unsupported assign operation

c = (0 | ???*0*)
- *0* updated with update expression

d = (0 | 1)

e = (1 | 2)

f = (1 | 2)
139 changes: 139 additions & 0 deletions crates/turbopack-ecmascript/tests/analyzer/graph/assign/graph.snapshot
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,139 @@
[
(
"a",
Alternatives(
5,
[
Constant(
Str(
Word(
Atom('' type=static),
),
),
),
Concat(
3,
[
Variable(
(
Atom('a' type=static),
#2,
),
),
Constant(
Str(
Word(
Atom('x' type=static),
),
),
),
],
),
],
),
),
(
"b",
Alternatives(
3,
[
Constant(
Num(
ConstantNumber(
0.0,
),
),
),
Unknown(
None,
"unsupported assign operation",
),
],
),
),
(
"c",
Alternatives(
3,
[
Constant(
Num(
ConstantNumber(
0.0,
),
),
),
Unknown(
None,
"updated with update expression",
),
],
),
),
(
"d",
Alternatives(
3,
[
Constant(
Num(
ConstantNumber(
0.0,
),
),
),
Constant(
Num(
ConstantNumber(
1.0,
),
),
),
],
),
),
(
"e",
Alternatives(
3,
[
Constant(
Num(
ConstantNumber(
1.0,
),
),
),
Constant(
Num(
ConstantNumber(
2.0,
),
),
),
],
),
),
(
"f",
Alternatives(
3,
[
Constant(
Num(
ConstantNumber(
1.0,
),
),
),
Constant(
Num(
ConstantNumber(
2.0,
),
),
),
],
),
),
]
12 changes: 12 additions & 0 deletions crates/turbopack-ecmascript/tests/analyzer/graph/assign/input.js
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
let a = "";
a += "x";
let b = 0;
b -= 1;
let c = 0;
c++;
let d = 0;
d ||= 1;
let e = 1;
e &&= 2;
let f = 1;
f ??= 2;
Empty file added crates/turbopack-ecmascript/tests/analyzer/graph/assign/resolved-effects.snapshot
View file
Empty file.
17 changes: 17 additions & 0 deletions crates/turbopack-ecmascript/tests/analyzer/graph/assign/resolved-explained.snapshot
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
a = ("" | `${("" | ???*0*)}x`)
- *0* `${???*1*}x`
⚠️ nested operation
- *1* a
⚠️ circular variable reference

b = (0 | ???*0*)
- *0* unsupported assign operation

c = (0 | ???*0*)
- *0* updated with update expression

d = (0 | 1)

e = (1 | 2)

f = (1 | 2)
2 changes: 1 addition & 1 deletion crates/turbopack-ecmascript/tests/analyzer/graph/md5-reduced/graph-explained.snapshot
View file
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ d#3 = (

d#6 = arguments[3]

i = (???*0* | 0)
i = (???*0* | 0 | (i + 16))
- *0* i
⚠️ pattern without value

Expand Down
20 changes: 19 additions & 1 deletion crates/turbopack-ecmascript/tests/analyzer/graph/md5-reduced/graph.snapshot
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1269,7 +1269,7 @@
(
"i",
Alternatives(
3,
6,
[
Unknown(
Some(
Expand All @@ -1289,6 +1289,24 @@
),
),
),
Add(
3,
[
Variable(
(
Atom('i' type=static),
#3,
),
),
Constant(
Num(
ConstantNumber(
16.0,
),
),
),
],
),
],
),
),
Expand Down
8 changes: 7 additions & 1 deletion crates/turbopack-ecmascript/tests/analyzer/graph/md5-reduced/resolved-explained.snapshot
View file
Original file line number Diff line number Diff line change
Expand Up @@ -34,9 +34,15 @@ d#6 = ???*0*
- *0* arguments[3]
⚠️ function calls are not analysed yet

i = (???*0* | 0)
i = (???*0* | 0 | ((???*1* | 0 | ???*2*) + 16))
- *0* i
⚠️ pattern without value
- *1* i
⚠️ pattern without value
- *2* (???*3* + 16)
⚠️ nested operation
- *3* i
⚠️ circular variable reference

len = ???*0*
- *0* arguments[1]
Expand Down
9 changes: 5 additions & 4 deletions crates/turbopack-ecmascript/tests/analyzer/graph/md5/graph-explained.snapshot
View file
Original file line number Diff line number Diff line change
Expand Up @@ -188,17 +188,18 @@ hex = (

hexTab = "0123456789abcdef"

i#3 = 0
i#3 = (0 | ???*0*)
- *0* updated with update expression

i#5 = (???*0* | 0)
i#5 = (???*0* | 0 | (i + 8))
- *0* i
⚠️ pattern without value

i#7 = (???*0* | 0)
i#7 = (???*0* | 0 | (i + 16))
- *0* i
⚠️ pattern without value

i#9 = (???*0* | 0)
i#9 = (???*0* | 0 | (i + 1) | (i + 8))
- *0* i
⚠️ pattern without value

Expand Down
3 changes: 2 additions & 1 deletion crates/turbopack-ecmascript/tests/analyzer/graph/md5/resolved-effects.snapshot
View file
Original file line number Diff line number Diff line change
Expand Up @@ -22,11 +22,12 @@

1 -> 6 free var = FreeVar(Array)

1 -> 11 member call = ???*0*["charCodeAt"](0)
1 -> 11 member call = ???*0*["charCodeAt"]((0 | ???*2*))
- *0* ???*1*(FreeVar(encodeURIComponent)(bytes))
⚠️ unknown callee
- *1* FreeVar(unescape)
⚠️ unknown global
- *2* updated with update expression

0 -> 12 call = (...) => (undefined | output)((???*0* | ???*1*))
- *0* arguments[0]
Expand Down

0 comments on commit fcc351a

Please sign in to comment.