feat(es/minifier): Handle expressions when indexing String, Array and Object types

[leafypout]

Description:

This PR is a WIP that fixes all known issues in #8747.

Related issue:

• Closes #8747

[changeset-bot[bot]]

⚠️ No Changeset found

Latest commit: cc2dcabece6a48dd36a412d8705e251286423c6d

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

[leafypout]

Not totally sure if the changed code for array literals is correct -- the code for this is a little more complicated than strings.

[][0] works correctly for now, but not [][[]]. Will look into this later.

[leafypout]

Another broken test case:

var _ = {0.5: 'test'}[0.5];

Currently simplifies to:

var _ = {
    0.5: 'test'
}[0.5];

Should simplify to:

var _ = 'test';

Not entirely sure if this is correct though because removing the ObjectLit could have side effects, ie if a value is a CallExpr

[leafypout]

Only one potential issue so far, that KnownOp no longer implements Eq because of Index now taking f64 instead of i64, but seems OK so far to me.

[leafypout]

@kdy1 The only remaining issue in terms of code changes is this line. Currently is_literal(props) does not work with the unit test below because the 0.5 is not considered a literal. I'm not sure why this fract() check exists and I'm not sure if changing this would break something.

https://github.com/levi-nz/swc/blob/454378d7eaac82bae40c5a770888e84580a1381b/crates/swc_ecma_utils/src/lib.rs#L1883

fold("({0.5: 'a'})['0.5']", "'a';");

[leafypout]

I'm pretty sure Index(f64) can be changed back to Index(i64) as well, as IndexStr is always used for objects.

[leafypout]

I have ran UPDATE=1 cargo test --test projects --test tsc, but not sure if all of these files should be changed, especially crates/swc/tests/tsc-references/templateStringInIndexExpressionES6.2.minified.js, which is now empty.

[leafypout]

This is getting a little complicated with property names, since we need to either correctly handle stuff like [].push not being replaced, or simply not replace invalid properties at all.

The solution I've implemented works and I don't think there's any issues, besides the fact that code like this will break:

Object.prototype.foobar = 5;
var _ = ({}).foobar; // returns 5, but will return undefined from the minifier

If there's no way to account for this correctly, we should ideally make this a configurable option where the user can make the minifier replace stuff like [].invalid with void 0 or just make it not replace anything at all.

If we continue with this approach, should we include deprecated properties like Object.watch to avoid any confusion (currently replaced with void 0), even though these no longer exist? This exists in vue.js. Ref: https://stackoverflow.com/questions/1029241/object-watch-for-all-browsers

In my opinion, replacing invalid properties needs to be a configurable option. In my use case, I want to replace invalid properties and ignore potential side effects, but this will introduce breaking changes for larger scripts and bundled scripts where other scripts are potentially changing the prototype of String, Object or Array. I assume there's no way to detect if the prototype of these are being changed, so it should just be an option that can be enabled/disabled. The whole reason this feature is implemented is so expressions like [][[]] can be minified, which is what I need for my use case.

[leafypout]

Babel's minify doesn't modify expressions like [][[]], but Babel's path.evaluate() API lets you transform these expressions. Should we perhaps make this some sort of API instead? Not sure where to go from here, but this should definitely be an option to make the simplifier as powerful as Babel's path.evaluate() option.

[leafypout]

Please move logics to https://github.com/swc-project/swc/blob/8eac0bec49f4f260a261d87f174ed2913c4bee63/crates/swc_ecma_minifier/src/compress/pure/evaluate.rs

swc_ecma_utils is not a good place for this kind of optimizations

Do we not care about modified prototypes? For example, in the current release version: https://github.com/swc-project/swc/blob/56e03a19602b36a7ebac9ccc899065101bb385af/crates/swc_ecma_transforms_optimization/src/simplify/expr/mod.rs#L165

We replace x[-1] with undefined, but that would break this code if minified:

Array.prototype[-1] = "foo";
[][-1] // outputs "foo", but swc replaces with void 0

Shouldn't this logic be moved to compress as well?

[kdy1]

There's an option named pristine_globals to mark globals as pristine, but I didn't bother to follow it in all places. Some rules uses it, but not all.

[leafypout]

Got it, I will update the minifier accordingly to try use it -- I think that option should be checked here in the aforementioned case

[leafypout]

Not sure why this exists, perhaps a bug with side effects? https://github.com/swc-project/swc/blob/56e03a19602b36a7ebac9ccc899065101bb385af/crates/swc_ecma_transforms_optimization/src/simplify/expr/mod.rs#L258

Current test case fold("[1,2,3,4,5,6,7,8,9,10][4 + []]", "5;"); fails, resulting in 0, 5 instead of just 5, removing that if block works but I think this may break things. Looking into why this is happening

[leafypout]

I seem to have fixed this but not sure if it will cause side effects anywhere, will try take a look. It appears as ArrayLiterals being replaced were always being replaced with a SeqExpr and if no side effects existed then there was a dummy 0 being added to the front. It just gets replaced with the single Expr now instead.

[leafypout]

Also, CI currently fails because of this. Is this normal? These unit tests don't seem to work on Windows, will try this on Mac later.

---- fixture_tests__terser__compress__evaluate__unsafe_string_bad_index__input_js stdout ----
Input: /home/runner/work/swc/swc/crates/swc_ecma_minifier/tests/terser/compress/evaluate/unsafe_string_bad_index/input.js
---- Config -----
{
    "evaluate": true,
    "unsafe": true
}

---- Input -----
console.log("1234".a + 1, "1234"["a"] + 1, "1234"[3.14] + 1);

thread 'fixture_tests__terser__compress__evaluate__unsafe_string_bad_index__input_js' panicked at crates/swc_ecma_minifier/src/compress/mod.rs:170:17:
Infinite loop detected (current pass = 202)
Code    0:









console.log("1234".a + 1, "1234"["a"] + 1, "1234"[3.14] + 1);


note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace


failures:
    fixture_tests__terser__compress__evaluate__unsafe_string_bad_index__input_js

test result: FAILED. 2674 passed; 1 failed; 27 ignored; 0 measured; 0 filtered out; finished in 96.08s

error: test failed, to rerun pass `-p swc_ecma_minifier --test compress`
Error: Process completed with exit code 101.

[leafypout]

Not sure if I got the span stuff correct. There is some copy pasta going on here from simplify::expr, should probably move that stuff into their own util functions.

Where should I create unit tests for the new compress functionality? I assume in crates/swc_ecma_minifier/tests/compress.rs but there's no unit tests in there currently, so I'm unsure.

[kdy1]

You can create a directory and place an input.js and config.json in https://github.com/swc-project/swc/tree/56e03a19602b36a7ebac9ccc899065101bb385af/crates/swc_ecma_minifier/tests/fixture

Note that you need to touch (or save again) tests/compress.rs once after adding a test file

[leafypout]

Will add watch and unwatch as exclusions for Object, even though they don't exist anymore. This way users can still use pristine_globals=true when using scripts like this to polyfill: https://gist.github.com/eligrey/384583

Not sure if there's any other exclusions we should add for property/method polyfills that don't exist anymore

[leafypout]

To do:

• Move stuff to ctx
• Handle known object properties
• Possibly add more unit tests

Will comment here again when everything is finalised

[leafypout]

Is there a reason why stuff like [1][0] was replaced with (0, 1) before instead of just 1? The tests seem to pass fine but I want to be sure. I don't see why it should be replaced with a SeqExpr instead of just a Literal if there are no side effects.

[leafypout]

cargo test -p swc_ecma_minifier terser_exec_tests__terser__compress__evaluate__unsafe_string_bad_index__input_js --features concurrent currently fails with a weird infinite loop bug, doesn't seem to be related to optimize_member_expr, need to investigate further

[kdy1]

(0, foo) is about making this undefined

[leafypout]

(0, foo) is about making this undefined

Does this apply to objects too? Like should ({a: 5}).a be replaced with (0, 5) or just 5? Currently it's replaced with the latter but I want to be sure

[leafypout]

Need to handle known object properties in simplify's optimize_member_expr. Will require a function signature change to accept obj and prop instead of expr. This PR should be ready for review tomorrow.

[kdy1]

(0, foo) is about making this undefined

Does this apply to objects too? Like should ({a: 5}).a be replaced with (0, 5) or just 5? Currently it's replaced with the latter but I want to be sure

I don't think so. this only matters for member exprs or direct eval calls

[leafypout]

Running cargo test run_fixture_test_tests__exec__issues_6xxx__6878__1__exec_js results in this error on both main and issue-8747 branch. Yarn installed with npm i -g yarn and corepack enable. Not sure how to fix?

levi@levi-kde-neon:~/RustroverProjects/levi-nz/swc$ cargo test run_fixture_test_tests__exec__issues_6xxx__6878__1__exec_js
    Finished test [unoptimized + debuginfo] target(s) in 0.39s
     Running unittests src/lib.rs (target/debug/deps/ast_node-e9d29c534642bab4)

running 0 tests

test result: ok. 0 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.00s

     Running unittests src/lib.rs (target/debug/deps/better_scoped_tls-2dc2c5ea5a8d762c)

running 0 tests

test result: ok. 0 passed; 0 failed; 0 ignored; 0 measured; 2 filtered out; finished in 0.00s

     Running unittests src/lib.rs (target/debug/deps/binding_macros-c052f01c3c2f3382)

running 0 tests

test result: ok. 0 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.00s

     Running unittests src/main.rs (target/debug/deps/dbg_swc-05912a335a982368)

running 0 tests

test result: ok. 0 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.00s

     Running unittests src/lib.rs (target/debug/deps/from_variant-86e39519e21950c5)

running 0 tests

test result: ok. 0 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.00s

     Running unittests src/lib.rs (target/debug/deps/jsdoc-f39585929aa7c829)

running 0 tests

test result: ok. 0 passed; 0 failed; 0 ignored; 0 measured; 9 filtered out; finished in 0.00s

     Running tests/fixture.rs (target/debug/deps/fixture-a6c989228dc4b47b)

running 0 tests

test result: ok. 0 passed; 0 failed; 0 ignored; 0 measured; 235 filtered out; finished in 0.00s

     Running unittests src/lib.rs (target/debug/deps/preset_env_base-e8b904940a0c4c1c)

running 0 tests

test result: ok. 0 passed; 0 failed; 0 ignored; 0 measured; 2 filtered out; finished in 0.00s

     Running unittests src/lib.rs (target/debug/deps/string_enum-f4111496889125a4)

running 0 tests

test result: ok. 0 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.00s

     Running tests/simple.rs (target/debug/deps/simple-07ac1a7eda7c6644)

running 0 tests

test result: ok. 0 passed; 0 failed; 0 ignored; 0 measured; 2 filtered out; finished in 0.00s

     Running unittests src/lib.rs (target/debug/deps/swc-b072fffd3fe75c6a)

running 0 tests

test result: ok. 0 passed; 0 failed; 0 ignored; 0 measured; 5 filtered out; finished in 0.00s

     Running tests/error_msg.rs (target/debug/deps/error_msg-423e285177753c6d)

running 0 tests

test result: ok. 0 passed; 0 failed; 0 ignored; 0 measured; 104 filtered out; finished in 0.00s

     Running tests/exec.rs (target/debug/deps/exec-ce806a5ca5bc9fa2)

running 1 test
➤ YN0000: · Yarn 4.0.2
➤ YN0000: ┌ Resolution step
➤ YN0000: └ Completed
➤ YN0000: ┌ Fetch step
➤ YN0000: └ Completed
➤ YN0000: ┌ Link step
➤ YN0000: └ Completed
➤ YN0000: · Done in 0s 194ms
$ dprint fmt
Formatted 193 files.
$ dprint fmt "scripts/*.js" -c scripts/.dprint.json
test run_fixture_test_tests__exec__issues_6xxx__6878__1__exec_js has been running for over 60 seconds.4.2 want: ^5.3.0
npm WARN deprecated @types/[email protected]: This is a stub types definition. terser provides its own type definitions, so you do not need this installed.
npm WARN deprecated [email protected]: This module is not supported, and leaks memory. Do not use it. Check out lru-cache if you want a good and tested way to coalesce async requests by a key value, which is much more comprehensive and powerful.
npm WARN deprecated @babel/[email protected]: This proposal has been merged to the ECMAScript standard and thus this plugin is no longer maintained. Please use @babel/plugin-transform-class-properties instead.
npm WARN deprecated @babel/[email protected]: This proposal has been merged to the ECMAScript standard and thus this plugin is no longer maintained. Please use @babel/plugin-transform-object-rest-spread instead.
npm WARN deprecated [email protected]: core-js@<3.23.3 is no longer maintained and not recommended for usage due to the number of issues. Because of the V8 engine whims, feature detection in old core-js versions could cause a slowdown up to 100x even if nothing is polyfilled. Some versions have web compatibility issues. Please, upgrade your dependencies to the actual version of core-js.
npm ERR! code 1
npm ERR! path /home/levi/RustroverProjects/levi-nz/swc/node_modules/wasm-sjlj
npm ERR! command failed
npm ERR! command sh -c node-gyp rebuild
npm ERR! gyp info it worked if it ends with ok
npm ERR! gyp info using [email protected]
npm ERR! gyp info using [email protected] | linux | x64
npm ERR! gyp info find Python using Python version 3.10.12 found at "/usr/bin/python3"
npm ERR! gyp info spawn /usr/bin/python3
npm ERR! gyp info spawn args [
npm ERR! gyp info spawn args '/home/levi/.nvm/versions/node/v20.12.2/lib/node_modules/npm/node_modules/node-gyp/gyp/gyp_main.py',
npm ERR! gyp info spawn args 'binding.gyp',
npm ERR! gyp info spawn args '-f',
npm ERR! gyp info spawn args 'make',
npm ERR! gyp info spawn args '-I',
npm ERR! gyp info spawn args '/home/levi/RustroverProjects/levi-nz/swc/node_modules/wasm-sjlj/build/config.gypi',
npm ERR! gyp info spawn args '-I',
npm ERR! gyp info spawn args '/home/levi/.nvm/versions/node/v20.12.2/lib/node_modules/npm/node_modules/node-gyp/addon.gypi',
npm ERR! gyp info spawn args '-I',
npm ERR! gyp info spawn args '/home/levi/.cache/node-gyp/20.12.2/include/node/common.gypi',
npm ERR! gyp info spawn args '-Dlibrary=shared_library',
npm ERR! gyp info spawn args '-Dvisibility=default',
npm ERR! gyp info spawn args '-Dnode_root_dir=/home/levi/.cache/node-gyp/20.12.2',
npm ERR! gyp info spawn args '-Dnode_gyp_dir=/home/levi/.nvm/versions/node/v20.12.2/lib/node_modules/npm/node_modules/node-gyp',
npm ERR! gyp info spawn args '-Dnode_lib_file=/home/levi/.cache/node-gyp/20.12.2/<(target_arch)/node.lib',
npm ERR! gyp info spawn args '-Dmodule_root_dir=/home/levi/RustroverProjects/levi-nz/swc/node_modules/wasm-sjlj',
npm ERR! gyp info spawn args '-Dnode_engine=v8',
npm ERR! gyp info spawn args '--depth=.',
npm ERR! gyp info spawn args '--no-parallel',
npm ERR! gyp info spawn args '--generator-output',
npm ERR! gyp info spawn args 'build',
npm ERR! gyp info spawn args '-Goutput_dir=.'
npm ERR! gyp info spawn args ]
npm ERR! gyp: binding.gyp not found (cwd: /home/levi/RustroverProjects/levi-nz/swc/node_modules/wasm-sjlj) while trying to load binding.gyp
npm ERR! gyp ERR! configure error 
npm ERR! gyp ERR! stack Error: `gyp` failed with exit code: 1
npm ERR! gyp ERR! stack at ChildProcess.<anonymous> (/home/levi/.nvm/versions/node/v20.12.2/lib/node_modules/npm/node_modules/node-gyp/lib/configure.js:271:18)
npm ERR! gyp ERR! stack at ChildProcess.emit (node:events:518:28)
npm ERR! gyp ERR! stack at ChildProcess._handle.onexit (node:internal/child_process:294:12)
npm ERR! gyp ERR! System Linux 6.5.0-28-generic
npm ERR! gyp ERR! command "/home/levi/.nvm/versions/node/v20.12.2/bin/node" "/home/levi/.nvm/versions/node/v20.12.2/lib/node_modules/npm/node_modules/node-gyp/bin/node-gyp.js" "rebuild"
npm ERR! gyp ERR! cwd /home/levi/RustroverProjects/levi-nz/swc/node_modules/wasm-sjlj
npm ERR! gyp ERR! node -v v20.12.2
npm ERR! gyp ERR! node-gyp -v v10.0.1
npm ERR! gyp ERR! not ok

npm ERR! A complete log of this run can be found in: /home/levi/.npm/_logs/2024-05-24T21_56_02_426Z-debug-0.log
test run_fixture_test_tests__exec__issues_6xxx__6878__1__exec_js ... FAILED

failures:

---- run_fixture_test_tests__exec__issues_6xxx__6878__1__exec_js stdout ----
Input: /home/levi/RustroverProjects/levi-nz/swc/crates/swc/tests/exec/issues-6xxx/6878/1/exec.js
thread 'run_fixture_test_tests__exec__issues_6xxx__6878__1__exec_js' panicked at crates/swc/tests/exec.rs:109:13:
assertion failed: status.success()
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace


failures:
    run_fixture_test_tests__exec__issues_6xxx__6878__1__exec_js

test result: FAILED. 0 passed; 1 failed; 0 ignored; 0 measured; 546 filtered out; finished in 108.41s

error: test failed, to rerun pass `-p swc --test exec`

[kdy1]

What's wasm-sjlj? I don't think SWC uses it.

[leafypout]

foo?.bar.baz?.() should be optimized to (void 0).baz?.(), but this is ignored for now. For some reason the AST becomes this

OptChainExpr { span: 53..65#0, optional: false, base: Member(MemberExpr { span: 53..65#0, obj: Unary(UnaryExpr { span: 45..47#0, op: "void", arg: Lit(Num(Number { span: 45..47#0, value: 0.0, raw: None })) }), prop: Ident(Ident { span: 62..65#0, sym: "baz", optional: false }) }) }

instead of

OptChainExpr { span: 11..27#0, optional: true, base: Call(OptCall { span: 11..27#0, callee: Member(MemberExpr { span: 11..23#0, obj: Unary(UnaryExpr { span: 12..18#0, op: "void", arg: Lit(Num(Number { span: 17..18#0, value: 0.0, raw: Some("0") })) }), prop: Ident(Ident { span: 20..23#0, sym: "baz", optional: false }) }), args: [], type_args: None }) }

which results in it becoming (void 0)?.() (due to eval_opt_chain's OptChainBase::Member optimization that removes pure undefined or null). If I check self.ctx.is_opt_call in eval_opt_chain it works as intended but I'm not sure if this would be correct since the AST appears to be messed up. Might come back to this later, otherwise will just have this as an ignored case.

[kdy1]

Also, can you rebase? (And thank you so much for the PR!)

[leafypout]

Also, can you rebase? (And thank you so much for the PR!)

Just saw this, will do