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

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

Closed
tmdghks opened this issue Jul 16, 2024 · 5 comments · Fixed by #9340
Closed

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

tmdghks opened this issue Jul 16, 2024 · 5 comments · Fixed by #9340
Assignees
Labels
Milestone

Comments

@tmdghks
Copy link

tmdghks commented Jul 16, 2024

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.

@tmdghks tmdghks added the C-bug label Jul 16, 2024
@kdy1 kdy1 added this to the Planned milestone Jul 16, 2024
@kdy1 kdy1 self-assigned this Jul 16, 2024
@Fnll
Copy link
Contributor

Fnll commented Jul 16, 2024

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

@tmdghks
Copy link
Author

tmdghks commented Jul 16, 2024

@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
Copy link
Sponsor Contributor

hyp3rflow commented Jul 17, 2024

@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?

fn report_assign(&mut self, ctx: Ctx, i: Id, is_op: bool) {

Thanks 😊

@kdy1
Copy link
Member

kdy1 commented Jul 18, 2024

@hyp3rflow

let is_op_assign = n.op != op!("=");

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
Copy link
Collaborator

swc-bot commented Aug 31, 2024

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.

@swc-project swc-project locked as resolved and limited conversation to collaborators Aug 31, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Development

Successfully merging a pull request may close this issue.

5 participants