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

Ensure default case folding only happens when default is last #1084

Merged
merged 2 commits into from Oct 28, 2021

Conversation

jridgewell
Copy link
Collaborator

I failed to consider if multiple branches could evaluate to the test condition. Eg,

switch (true) {
    case definitely_true:
    default:
      return 'definitely';
    case maybe_true:
      return 'maybe';
}

Here, both definitely_true and maybe_true can be === true. And we can't eliminate the case definitely_true:, because it prevents us from falling into the maybe_true branch (when it is true).

Fixes #1083

@elad-yosifon
Copy link
Contributor

elad-yosifon commented Oct 10, 2021

I think this code is valuable, we only need to limit it to a more specific use case.
whenever there is a switch with an expressions that does not need to be evaluated and are not true (false should be completely folded), and all the case expressions are side effects free.

switch(false){  // <--- should be removed entirely
  //...
}

switch(true){  // <--- optimization should bail
  //...
}

switch(var1++){ // <-- optimization should bail
  case (B + 1):
  // ...
}

switch(var1){
  case (B + 1): // <-- fine
  case (B++): // <-- optimization should bail
  // ...
}

@jridgewell plz LMK if you have other reservations...

@jridgewell
Copy link
Collaborator Author

(false should be completely folded),

I don't think that's correct. Any switch statement which has cases that aren't mutually exclusive can cause this issue, and the only way we could know otherwise is if every case expression was statically evaluable. I don't think there is a way to perform reordering, we can only fold equivalent cases that appear directly next to each other.

@elad-yosifon
Copy link
Contributor

@jridgewell plz ignore my last reply.. was probably out of focus or something.. total nonsense.

If everything can be statically evaluate, I think we should apply the reordering optimization. It is actually very beneficial for one of my company products., and will probably help other projects as well - especially with Webpack and DCE using switch statements.

@jridgewell
Copy link
Collaborator Author

It is actually very beneficial for one of my company products., and will probably help other projects as well

If you want to open a PR that only performs the reordering when each case expression is static, I'd be happy to integrate that instead of reverting the original.

@elad-yosifon
Copy link
Contributor

elad-yosifon commented Oct 12, 2021 via email

I failed to consider if multiple branches could evaluate to the test condition. Eg,

```js
switch (true) {
    case definitely_true:
    default:
      return 'definitely';
    case maybe_true:
      return 'maybe';
}
```

Here, both `definitely_true` and `maybe_true` can be `=== true`. And we can't eliminate the `case definitely_true:`, because it prevents us from falling into the `maybe_true` branch (when it is true).
@jridgewell
Copy link
Collaborator Author

Updated this PR.

@fabiosantoscode fabiosantoscode merged commit aadf03d into terser:master Oct 28, 2021
@fabiosantoscode
Copy link
Collaborator

Great stuff, thanks!

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.

switch reachable case was removed
3 participants