swc-project / swc

Rust-based platform for the Web
https://swc.rs
Apache License 2.0
31.21k stars 1.23k forks source link

minifier: cannot detect variable's value changed without using assignment operator #9263

Closed tmdghks closed 3 months ago

tmdghks commented 3 months ago

Describe the bug

I suspect that a minifier cannot detect changes to a variable's value without using an assignment operator (e.g., =, *=) when declaring variables with the var keyword.

For example, a minifier can properly minify ECMAScript code like the following:

Input Code Example (Well-Minified)

"use strict";
const k = (function () {
    var x = 42;
    for (var x = 4242;;) break;
    return x;
})();

However, a minifier cannot correctly minify ECMAScript code using for ... in ..., for ... of ..., and similar constructs. The example code is below.

Input Code Example (Incorrectly Minified)

"use strict";
const k = (function () {
    var x = 42;
    for (var x in [4242]) break;
    return x;
})();

Input code

"use strict";
const k = (function () {
    var x = 42;
    for (var x of [4242]) break;
    return x;
})();

Config

{
  "jsc": {
    "parser": {
      "syntax": "ecmascript",
      "jsx": false
    },
    "target": "es2022",
    "loose": false,
    "minify": {
      "compress": {
        "arguments": false,
        "arrows": true,
        "booleans": true,
        "booleans_as_integers": false,
        "collapse_vars": true,
        "comparisons": true,
        "computed_props": true,
        "conditionals": true,
        "dead_code": true,
        "directives": true,
        "drop_console": false,
        "drop_debugger": true,
        "evaluate": true,
        "expression": false,
        "hoist_funs": false,
        "hoist_props": true,
        "hoist_vars": false,
        "if_return": true,
        "join_vars": true,
        "keep_classnames": false,
        "keep_fargs": true,
        "keep_fnames": false,
        "keep_infinity": false,
        "loops": true,
        "negate_iife": true,
        "properties": true,
        "reduce_funcs": false,
        "reduce_vars": false,
        "side_effects": true,
        "switches": true,
        "typeofs": true,
        "unsafe": false,
        "unsafe_arrows": false,
        "unsafe_comps": false,
        "unsafe_Function": false,
        "unsafe_math": false,
        "unsafe_symbols": false,
        "unsafe_methods": false,
        "unsafe_proto": false,
        "unsafe_regexp": false,
        "unsafe_undefined": false,
        "unused": true,
        "const_to_let": true,
        "pristine_globals": true
      },
      "mangle": false
    }
  },
  "module": {
    "type": "es6"
  },
  "minify": false,
  "isModule": false
}

Playground link (or link to the minimal reproduction)

https://play.swc.rs/?version=1.7.0-nightly-20240715.2&code=H4sIAAAAAAAAAyWKMQrAIBAEe1%2BxpNJW7CQvCSmMKIigcJ5BCPl7JE63s7P1FtCYkufNCl9LY2TskLEXz6kWSIVHYHI7wpiX0fbfsRLkkjXiMNroU%2BGi4PIKKHCngmHFq6Sa7gMW516hbQAAAA%3D%3D&config=H4sIAAAAAAAAA32UO3LjMAyG%2B5zCozrFjostcoDtcgYOLYIyvSShIUDHmozvvtDDj40hdRI%2B%2FAAJgPh%2B2%2B2aE7XNx%2B5bPuWnt4Wg3P%2FFQkNmexFLA22y1JbQc%2FN%2BoycakbeRYDJdZ9KwLR3wpKL9r%2F1%2BUTQRkeCmWGwp5OCH55wtpr4A0ZNNrBKyJshM%2F%2BsXVvBrBFzqs%2F2AGMHmDWIsmZAZOiha4BZjtD2BOduiRBlPaksg1FKMsDI40xfsVZ5d4IBZcr5SB9aZFh0oKBRoOZxBk0kukWWS6yn3mbCDQ%2B26qc8%2F1HC2sVpWcsJlaomcVol6xEBsfM1aCWe4UoMZLsX9qQzeFOBa8qvuhCGv9OQvgFQgWqJsE2hxJw8v87Sm9pvKkL2MLA8Kl%2FnWbpmhk6KaELxS2bEyUDho3SzgagtjZVvtOAteKR8FBwa8l1lRQtNX4PaoJeWhB%2FQKkP5ar03VDMz9Fa7w8UFs4D9yS9YHbPFIlo%2FrlIZ0wLiRIAEf0W04SCsY13GRLXHp13nNDmQ0wKkulSbwugTkATCaOO3Ll9mQ5yERTRfx8FgTi8P1voeTzd3jvc%2Br%2BG1xaBK6OsFlyY%2F9nVfz7%2BbhdNvC94M3gT5vyjns9R%2F7ntJqMQYAAA%3D%3D

SWC Info output

No response

Expected behavior

"use strict";
let k = function() {
    for (var x of [
        4242
    ])break;
    return x;
}();

Actual behavior

"use strict";
let k = function() {
    for (var x of [
        4242
    ])break;
    return 42;
}();

Version

1.7.0-nightly-20240715.2

Additional context

Furthermore, this bug persists in version 1.6.7 as well.

Fnll commented 3 months ago

@tmdghks I suppose you mean the expected output should be 4242 right?

tmdghks commented 3 months ago

@tmdghks I suppose you mean the expected output should be 4242 right?

In the case of the following code (for ... in ...), x will be updated to the first key of object {0: 4242}. Therefore, output should be 0.

"use strict";
const k = (function () {
    var x = 42;
    for (var x in [4242]) break;
    return x;
})();

On the other hand, in the case of the following code (for ... of ...), x will be updated to the first value of the array {0: 4242}. The value of output should be 4242.

"use strict";
const k = (function () {
    var x = 42;
    for (var x of [4242]) break;
    return x;
})();

I'm sorry for the insufficient information.

hyp3rflow commented 3 months ago

@kdy1 I have a question after digging into this issue.

I noticed that reporting assignment is missing on VarDecl case in For in/of statement visitor (ForHead::VarDecl), so I added below codes in here, then it seems resolving this issue.

if let ForHead::VarDecl(decl) = &n.left {
    let VarDeclarator { name, .. } = decl.decls.last().unwrap();
    child.report_assign_pat(name, true);
}

Here is the question: but I'm not sure what should I have to give to the second parameter of report_assign_pat (which is named is_op) because reassignment can happen dynamically depending on the rhs value. (e.g. reassignment happens in for(var x of [1]) but not in for(var x of []).) If you don't mind, could you explain about the purpose of is_op parameter in report_assign? https://github.com/swc-project/swc/blob/38c8453103b89facd7698070a112288f420e1128/crates/swc_ecma_minifier/src/program_data.rs#L355 Thanks 😊

kdy1 commented 3 months ago

@hyp3rflow https://github.com/swc-project/swc/blob/aea4f45c7355f3a27ea22bd43f27682214cdc7c1/crates/swc_ecma_usage_analyzer/src/analyzer/mod.rs#L253

It might be renamed as is_read_write. Assignments with an assignment operator other than = is read-write, so it should be handled a bit differently.

swc-bot commented 2 months ago

This closed issue has been automatically locked because it had no new activity for a month. If you are running into a similar issue, please create a new issue with the steps to reproduce. Thank you.