Skip to content

feat(minifier): convert switch with one case to if statement#24611

Open
armano2 wants to merge 15 commits into
oxc-project:mainfrom
armano2:feat/minifier-switch-with-one-case
Open

feat(minifier): convert switch with one case to if statement#24611
armano2 wants to merge 15 commits into
oxc-project:mainfrom
armano2:feat/minifier-switch-with-one-case

Conversation

@armano2

@armano2 armano2 commented Jul 17, 2026

Copy link
Copy Markdown
Contributor

Inline switch with 1 case to if statement or in case of default to statement expressions + block statement

https://github.com/oxc-project/oxc/actions/runs/29759622291#summary-88410679091

change extracted from #17523
ref #17544

@codspeed-hq

codspeed-hq Bot commented Jul 17, 2026

Copy link
Copy Markdown

Merging this PR will not alter performance

✅ 57 untouched benchmarks
⏩ 19 skipped benchmarks1


Comparing armano2:feat/minifier-switch-with-one-case (aea932d) with main (47ddedf)

Open in CodSpeed

Footnotes

  1. 19 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports.

Comment thread crates/oxc_minifier/src/peephole/minimize_statements.rs Outdated
@armano2
armano2 marked this pull request as ready for review July 20, 2026 16:28
found_unlabelled_break: bool,
}

impl<'a> VisitJs<'a> for FindNestedBreak {

@armano2 armano2 Jul 21, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

similar check is going to be required for loops, as currently we do not inline/remove them, tho in that case we also have to check for continue

eg. while(true) break;

I'm also not sure where to put this visitor

@Dunqing

Dunqing commented Jul 21, 2026

Copy link
Copy Markdown
Member

@codex review

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: aea932d191

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

SPAN,
switch_stmt.discriminant.take_in(ctx),
BinaryOperator::StrictEquality,
test,

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Preserve the switch case lexical scope for case tests

When a single-case switch contains a lexical declaration that shadows the case expression, switch evaluates the case test inside the switch's case-block scope, but this if condition is built outside the new block_stmt scope. For let x = 1; switch (0) { case x: let x = 2; }, the original reads the switch-scoped x and throws a TDZ ReferenceError, while the transformed if (0 === x) { let x = 2; } reads the outer x and continues, so this inlining changes runtime behavior.

Useful? React with 👍 / 👎.

@armano2 armano2 Jul 21, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minifier assumes that the input has not TDZ violations so this isn't supported at this point.

### No TDZ Violation
Code doesn't violate Temporal Dead Zones.
```javascript
// The minifier assumes this never happens:
console.log(x); // TDZ violation
let x = 1;
```

#24589

vec.pop().unwrap()
} else {
Statement::new_block_statement_with_scope_id(
SPAN,

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe we should take switch case span in here

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.

2 participants