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

test: failing tests for constant folding and TDZ issues #3150

Closed

Conversation

jluxenberg
Copy link

@jluxenberg jluxenberg commented Jun 8, 2023

RE: #3125

Spent a bit of time trying to fix these in js_parser.go but couldn't solve it. This PR has a few failing tests which illustrate the problem.

There are two issues:

(1) Constant propagation not working properly when the constant is declared after it is used

Consider this program:

{ 
  const f = () => x; 
  const x = 0; 
  f();
}

The definition of x should be hoisted to the top of the enclosing block. Since it is constant, it is eligible for constant folding/propagation. The propagation should respect the Temporal Dead Zone; accessing x before it is initialized should raise a ReferenceError: Cannot access 'x' before initialization.

esbuild minifies this program to (() => x)(). I believe this is because x is correctly identified as a constant, but then isn't able to do the substitution into the function body because it is defined after the function that uses it.

(2) Dead statement removal doesn't respect the Temporal Dead Zone

Consider this program:

{
  number; 
  const number=10;
}

This should crash with an error ReferenceError: Cannot access 'number' before initialization.

However, esbuild minifies it to something like {const n=10}, omitting the number; statement.

@evanw
Copy link
Owner

evanw commented Jun 25, 2023

Regarding (1), the issue #3125 was fixed and the fix already includes a test.

Regarding (2), this is because bound identifiers are considered to be side-effect free to allow for important optimizations. It's true that this means esbuild can incorrectly remove TDZ checks such as in the case you presented. However, code that triggers TDZ checks is inherently broken anyway, and disabling common and important tree-shaking optimizations for a rare edge case that only appears in broken code doesn't seem like a good trade-off for a production-oriented bundler like esbuild. This is why I have deliberately made the decision to not respect the TDZ check here.

@jluxenberg
Copy link
Author

Thanks for the detailed explanation and fix for #3125!

@jluxenberg jluxenberg closed this Jun 29, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants