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.

Sign up for GitHub

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

Jump to bottom

Make simplifyUnusedExpr non-recursive #12355

Closed
wants to merge 6 commits into from
Closed

Make simplifyUnusedExpr non-recursive #12355

wants to merge 6 commits into from

Conversation

paperdave
Copy link
Collaborator

What does this PR do?

Fixes #5398
Fixes #8021

simplifyUnusedExpr is called on any expression which is not dead code, but has a value that is unused. For example

a; // a is unused
a = (b, c); // b is unused
a = [d] ? e : f; // [d] is unused because it truthy. f is dead code

It is not safe to make this a naive recursive function because some minifiers will build very large comma expressions, with thousands of parts. Bun represents this as a nested binary expression, and when it attempts to simplify the parts, it stack overflows.

TODO:

  • Explain the method used here (maybe draw a diagram), it is kind of confusing and I will forget how this works in the future.
  • Fix E.If handling, it has a subtle flaw.
  • Performance testing. This is hot code as it is run on every expression statement. I have a feeling ArrayList is faster for this use-case. But I wanted to try SegmentedList because the idea of not re-allocating old items was cool.
  • Tests!

@paperdave
Copy link
Collaborator Author

not ready to merge btw. i also see i edited launch.json and webkit module (aaaargh)

Jarred-Sumner
Jarred-Sumner reviewed Jul 5, 2024
View reviewed changes
src/js_printer.zig Outdated Show resolved Hide resolved
@paperdave
Copy link
Collaborator Author

not ready.

image

@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Jul 5, 2024 •
edited
Loading

❌ @paperdave, your commit has failing tests :(

💪 18 failing tests Darwin AARCH64

  • test/bundler/bundler_browser.test.ts 1 failing
  • test/bundler/bundler_compile.test.ts 3 failing
  • test/bundler/bundler_decorator_metadata.test.ts 2 failing
  • test/bundler/bundler_regressions.test.ts 1 failing
  • test/bundler/esbuild/packagejson.test.ts 1 failing
  • test/bundler/esbuild/ts.test.ts 2 failing
  • test/cli/install/bunx.test.ts 1 failing
  • test/cli/install/registry/bun-install-registry.test.ts SIGKILL
  • test/cli/run/commonjs-no-export.test.ts 1 failing
  • test/integration/next-pages/test/dev-server-ssr-100.test.ts SIGKILL
  • test/integration/next-pages/test/dev-server.test.ts SIGKILL
  • test/integration/next-pages/test/next-build.test.ts 1 failing
  • test/js/bun/resolve/import-meta.test.js 1 failing
  • test/js/bun/util/inspect-error.test.js 2 failing
  • test/js/third_party/azure-service-bus.test.ts 1 failing
  • test/js/third_party/msw.test.ts 1 failing
  • test/js/third_party/postgres/postgres.test.ts 1 failing
  • test/js/third_party/yargs/yargs-cjs.test.js 1 failing

💻 18 failing tests Darwin x64 baseline

  • test/bundler/bundler_browser.test.ts 1 failing
  • test/bundler/bundler_compile.test.ts 3 failing
  • test/bundler/bundler_decorator_metadata.test.ts 2 failing
  • test/bundler/bundler_regressions.test.ts 1 failing
  • test/bundler/esbuild/packagejson.test.ts 1 failing
  • test/bundler/esbuild/ts.test.ts 2 failing
  • test/cli/install/bunx.test.ts 1 failing
  • test/cli/install/registry/bun-install-registry.test.ts SIGKILL
  • test/cli/run/commonjs-no-export.test.ts 1 failing
  • test/integration/next-pages/test/dev-server-ssr-100.test.ts SIGKILL
  • test/integration/next-pages/test/dev-server.test.ts SIGKILL
  • test/integration/next-pages/test/next-build.test.ts 1 failing
  • test/js/bun/resolve/import-meta.test.js 1 failing
  • test/js/bun/util/inspect-error.test.js 2 failing
  • test/js/third_party/azure-service-bus.test.ts 1 failing
  • test/js/third_party/msw.test.ts 1 failing
  • test/js/third_party/postgres/postgres.test.ts 1 failing
  • test/js/third_party/yargs/yargs-cjs.test.js 1 failing

💻 18 failing tests Darwin x64

  • test/bundler/bundler_browser.test.ts 1 failing
  • test/bundler/bundler_compile.test.ts 3 failing
  • test/bundler/bundler_decorator_metadata.test.ts 2 failing
  • test/bundler/bundler_regressions.test.ts 1 failing
  • test/bundler/esbuild/packagejson.test.ts 1 failing
  • test/bundler/esbuild/ts.test.ts 2 failing
  • test/cli/install/bunx.test.ts 1 failing
  • test/cli/install/registry/bun-install-registry.test.ts SIGKILL
  • test/cli/run/commonjs-no-export.test.ts 1 failing
  • test/integration/next-pages/test/dev-server-ssr-100.test.ts SIGKILL
  • test/integration/next-pages/test/dev-server.test.ts SIGKILL
  • test/integration/next-pages/test/next-build.test.ts 1 failing
  • test/js/bun/resolve/import-meta.test.js 1 failing
  • test/js/bun/util/inspect-error.test.js 2 failing
  • test/js/third_party/azure-service-bus.test.ts 1 failing
  • test/js/third_party/msw.test.ts 1 failing
  • test/js/third_party/postgres/postgres.test.ts 1 failing
  • test/js/third_party/yargs/yargs-cjs.test.js 1 failing

🐧💪 18 failing tests Linux AARCH64

  • test/bundler/bundler_browser.test.ts 1 failing
  • test/bundler/bundler_compile.test.ts 3 failing
  • test/bundler/bundler_decorator_metadata.test.ts 2 failing
  • test/bundler/bundler_regressions.test.ts 1 failing
  • test/bundler/esbuild/packagejson.test.ts 1 failing
  • test/bundler/esbuild/ts.test.ts 2 failing
  • test/cli/install/bunx.test.ts 1 failing
  • test/cli/install/registry/bun-install-registry.test.ts SIGKILL
  • test/cli/run/commonjs-no-export.test.ts 1 failing
  • test/integration/next-pages/test/dev-server-ssr-100.test.ts SIGKILL
  • test/integration/next-pages/test/dev-server.test.ts SIGKILL
  • test/integration/next-pages/test/next-build.test.ts 1 failing
  • test/js/bun/resolve/import-meta.test.js 1 failing
  • test/js/bun/util/inspect-error.test.js 2 failing
  • test/js/third_party/azure-service-bus.test.ts 1 failing
  • test/js/third_party/msw.test.ts 1 failing
  • test/js/third_party/postgres/postgres.test.ts 1 failing
  • test/js/third_party/yargs/yargs-cjs.test.js 1 failing

🐧🖥 18 failing tests Linux x64 baseline

  • test/bundler/bundler_browser.test.ts 1 failing
  • test/bundler/bundler_compile.test.ts 3 failing
  • test/bundler/bundler_decorator_metadata.test.ts 2 failing
  • test/bundler/bundler_regressions.test.ts 1 failing
  • test/bundler/esbuild/packagejson.test.ts 1 failing
  • test/bundler/esbuild/ts.test.ts 2 failing
  • test/cli/install/bunx.test.ts 1 failing
  • test/cli/install/registry/bun-install-registry.test.ts SIGKILL
  • test/cli/run/commonjs-no-export.test.ts 1 failing
  • test/integration/next-pages/test/dev-server-ssr-100.test.ts SIGKILL
  • test/integration/next-pages/test/dev-server.test.ts SIGKILL
  • test/integration/next-pages/test/next-build.test.ts 1 failing
  • test/js/bun/resolve/import-meta.test.js 1 failing
  • test/js/bun/util/inspect-error.test.js 2 failing
  • test/js/third_party/azure-service-bus.test.ts 1 failing
  • test/js/third_party/msw.test.ts 1 failing
  • test/js/third_party/postgres/postgres.test.ts 1 failing
  • test/js/third_party/yargs/yargs-cjs.test.js 1 failing

🐧🖥 18 failing tests Linux x64

  • test/bundler/bundler_browser.test.ts 1 failing
  • test/bundler/bundler_compile.test.ts 3 failing
  • test/bundler/bundler_decorator_metadata.test.ts 2 failing
  • test/bundler/bundler_regressions.test.ts 1 failing
  • test/bundler/esbuild/packagejson.test.ts 1 failing
  • test/bundler/esbuild/ts.test.ts 2 failing
  • test/cli/install/bunx.test.ts 1 failing
  • test/cli/install/registry/bun-install-registry.test.ts SIGKILL
  • test/cli/run/commonjs-no-export.test.ts 1 failing
  • test/integration/next-pages/test/dev-server-ssr-100.test.ts SIGKILL
  • test/integration/next-pages/test/dev-server.test.ts SIGKILL
  • test/integration/next-pages/test/next-build.test.ts 1 failing
  • test/js/bun/resolve/import-meta.test.js 1 failing
  • test/js/bun/util/inspect-error.test.js 2 failing
  • test/js/third_party/azure-service-bus.test.ts 1 failing
  • test/js/third_party/msw.test.ts 1 failing
  • test/js/third_party/postgres/postgres.test.ts 1 failing
  • test/js/third_party/yargs/yargs-cjs.test.js 1 failing

🪟💻 21 failing tests Windows x64 baseline

  • test/bundler/bundler_browser.test.ts 1 failing
  • test/bundler/bundler_compile.test.ts 3 failing
  • test/bundler/bundler_decorator_metadata.test.ts 2 failing
  • test/bundler/bundler_regressions.test.ts 1 failing
  • test/bundler/esbuild/packagejson.test.ts 1 failing
  • test/bundler/esbuild/ts.test.ts 2 failing
  • test/cli/install/bunx.test.ts 1 failing
  • test/cli/install/registry/bun-install-registry.test.ts SIGKILL
  • test/cli/install/registry/bun-install-windowsshim.test.ts code 1
  • test/cli/run/commonjs-no-export.test.ts 1 failing
  • test/integration/next-pages/test/dev-server-ssr-100.test.ts SIGKILL
  • test/integration/next-pages/test/dev-server.test.ts 1 failing
  • test/integration/next-pages/test/next-build.test.ts 1 failing
  • test/js/bun/resolve/import-meta.test.js 1 failing
  • test/js/bun/util/inspect-error.test.js 2 failing
  • test/js/node/child_process/child_process.test.ts 1 failing
  • test/js/node/tls/node-tls-connect.test.ts 1 failing
  • test/js/third_party/azure-service-bus.test.ts 1 failing
  • test/js/third_party/msw.test.ts 1 failing
  • test/js/third_party/postgres/postgres.test.ts 1 failing
  • test/js/third_party/yargs/yargs-cjs.test.js 1 failing

🪟💻 21 failing tests Windows x64

  • test/bundler/bundler_browser.test.ts 1 failing
  • test/bundler/bundler_compile.test.ts 3 failing
  • test/bundler/bundler_decorator_metadata.test.ts 2 failing
  • test/bundler/bundler_regressions.test.ts 1 failing
  • test/bundler/esbuild/packagejson.test.ts 1 failing
  • test/bundler/esbuild/ts.test.ts 2 failing
  • test/cli/install/bunx.test.ts 1 failing
  • test/cli/install/registry/bun-install-registry.test.ts SIGKILL
  • test/cli/install/registry/bun-install-windowsshim.test.ts code 1
  • test/cli/run/commonjs-no-export.test.ts 1 failing
  • test/integration/next-pages/test/dev-server-ssr-100.test.ts SIGKILL
  • test/integration/next-pages/test/dev-server.test.ts SIGKILL
  • test/integration/next-pages/test/next-build.test.ts 1 failing
  • test/js/bun/resolve/import-meta.test.js 1 failing
  • test/js/bun/util/inspect-error.test.js 2 failing
  • test/js/node/child_process/child_process.test.ts 1 failing
  • test/js/node/tls/node-tls-connect.test.ts 1 failing
  • test/js/third_party/azure-service-bus.test.ts 1 failing
  • test/js/third_party/msw.test.ts 1 failing
  • test/js/third_party/postgres/postgres.test.ts 1 failing
  • test/js/third_party/yargs/yargs-cjs.test.js 1 failing

View logs

@paperdave
Copy link
Collaborator Author

i will likely abandon this implementation for a more simple / narrow version of this. and then my misc refactors i'll just re-implement later.

@paperdave
Copy link
Collaborator Author

i close PR but intentionally am keeping the branch alive

@paperdave paperdave closed this Jul 6, 2024
@paperdave paperdave mentioned this pull request Jul 6, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Jarred-Sumner Jarred-Sumner Jarred-Sumner left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
2 participants
@paperdave @Jarred-Sumner