[Feature] Minify away function calls to empty functions

[judofyr]

Example:

function assertPositive(num) {
  if (__DEV__ && num <= 0) throw new Error("number must be positive");
}

export function pos(num) {
  assertPositive(num);
  return num;
}

Current behavior:

$ esbuild ex.js --define:__DEV__=false --minify
function b(a){}export function pos(a){return b(a),a}

Wanted behavior:

$ esbuild ex.js --define:__DEV__=false --minify
export function pos(a){return a}

I understand that there can be complications here (e.g. if there are more method calls inside the call), but I think it shouldn't be too difficult to handle the main cases.

[evanw]

This currently isn't straightforward to do with esbuild's current architecture. Right now there are only three full-AST passes for speed:

1. Lexing + parsing + scope setup + symbol declaration
2. Symbol binding + constant folding + syntax lowering + syntax mangling
3. Printing + source map generation

This optimization would have to happen after symbol binding but before printing. The most straightforward way to do this would be to add a fourth full-AST pass but that would likely slow things down noticeably. It could potentially be done by folding it into the printing pass but that starts to get pretty messy.

[leeoniya]

if the only penalty here is speed rather than implementation complexity, why not make this an opt-in additional pass, similar to how terser has a passes option which improves compression (for a price).

i'm certainly in the audience that would prefer perfect dead code elim (on par with rollup+terser) and have esbuild be "only" 50x faster instead of 100x. i currently have several projects [1][2] that allow build-time feature opt-outs, and rely heavily on rollup & terser's excellent DCE. it would be a shame if this was an impossible task with esbuild.

/$0.02

[1] https://github.com/leeoniya/uPlot/blob/master/rollup.config.js#L51-L58
[2] https://github.com/domvm/domvm/blob/master/build.js#L10-L39

[evanw]

if the only penalty here is speed rather than implementation complexity

There actually is additional implementation complexity with the dead code elimination in Rollup+Terser. From what I understand they have a much more extensive partial evaluator for JavaScript that attempts to infer object shapes except without regard for execution order (at least if the information in rollup/rollup#1667 is still accurate).

And in addition to adding implementation complexity, it also introduces correctness bugs because it makes assumptions that aren't always true (e.g. rollup/rollup#2219). That makes me question adding these optimizations to esbuild because I want esbuild to be a tool you can trust to not cause correctness issues with your code.

i'm certainly in the audience that would prefer perfect dead code elim (on par with rollup+terser)

I think the optimization you suggested here should always be safe, and I think it could be reasonable to add it to esbuild. But other optimizations that Rollup+Terser do are not completely safe so I don't think esbuild should have the goal of being "on par" with Rollup+Terser's specific flavor of code compression.

[leeoniya]

attempts to infer object shapes

FWIW i don't think i've ever had code that would have benefitted from this. however, rollup/rollup#2201 is still open and i'd certainly like to see that land.

as for this issue, it seems safe to eliminate calls to empty functions and/or side-effect-free functions whose return values are entirely unused (even if called).

[ZhangJian-3ti]

I think to modern JavaScript runtime engine this shouldn't be a big deal. It's smart enough to optimize that empty function call.

[intrnl]

I think that people want to optimize for size as well, but couldn't the __DEV__ check be moved instead?

export function pos (num) {
  if (__DEV__) assertPositive(num);
}

[judofyr]

It's a bit more complicated in my case: I'm using TypeScript and I have a variable which is typed Foo | null. By having a separate function I'm able to do this:

function assertFoo(val: Foo | null): asserts val is Foo {
  if (__DEV__ && val === null) throw new Error("foo must be set");
}

function main() {
  const val = getFoo();
  assertFoo(val);
  console.log(val.bar);
}

And then TypeScript is able to know that val.bar is "safe" because the assertion verifies it.

If I move the __DEV__-check inside then I'll get an error on val.bar since it know it's not guaranteed:

function main() {
  const val = getFoo();
  if (__DEV__) assertFoo(val);
  console.log(val.bar);
}

In this case I can workaround it by changing it to val!.bar, but the assertions are not always simple null checks. Sometimes they're asserting more complicated things.

There's also the fact that I'd like to be apply these assertions consistently and by forcing the caller to add __DEV__ it's enforce this. (It's easy to forget to add if (__DEV__) and suddenly an expensive debug-check is happening in production.)