4.4.1 Regression: wrong dead code elimination => broken outcome (works OK in 4.4.0)

[WebReflection]

Bug report or Feature request?

Definitively a bug to me, introduced with this "new feature": 0ae57bd

Version (complete output of terser -V or specific git commit)

terser 4.4.1

Note with terser 4.4.0 this issue doesn't exists, meaning you have used a patch that introduced something breaking.

Complete CLI command or minify() options used

terser dist/index.js -m -c -o dist/min.js

---

terser input

Update stand alone code that reproduces the issue here

---

This is a tricky one, as the issue appeared after a huge refactoring of various libraries involved in neverland js-framework-benchmark test.

The REPL doesn't seem to show the issue, once isolating that specific page, but the culprit of the issue is this part of the code:

const Main = $(() => {
  const [{data, selected}, dispatch] = useReducer(listReducer, state);
  const jumbotron = useMemo(() => Jumbotron({dispatch}), []);

  return html`
    <div class="container">
      ${jumbotron}
      <table class="table table-hover table-striped test-data">
        <tbody>
        ${data.map(item => Row({data, item, dispatch, selected: item.id === selected}))}
        </tbody>
      </table>
      <span class="preloadicon glyphicon glyphicon-remove" aria-hidden="true" />
    </div>
  `;
});

terser output or error

The produced output completely drops Jumbotron, keeping the Row though, as you can see in here:

// terser
dt = Qe(() => {
  const [{
    data: e,
    selected: t
  }, n] = v(ft, ut);
  return Ue `<div class=container>${w(()=>({dispatch:n}),[])}<table class="table table-hover table-striped test-data"><tbody>${e.map(r=>it({data:e,item:r,dispatch:n,selected:r.id===t}))}</tbody></table><span class="preloadicon glyphicon glyphicon-remove" aria-hidden=true /></div>`
});

The it is the Row, while the w is the useMemo, and it looks good once inspected, but the Jumbotron is completely gone, so that it now returns always the object {dispatch: n} instead, which makes the test unusable, as all Buttons are gone too, so nothing is shown on the page.

Expected result

Using exactly same command via uglifyjs, installed via uglify-es 3.3.9, the outcome is the following one (which is also exactly the same with terser 4.4.0):

// uglify-es
it = Ve(() => {
  const [{
    data: e,
    selected: t
  }, n] = g(lt, ct);
  return Pe `<div class=container>${b(()=>(({dispatch:e})=>Pe` < div class = jumbotron > < div class = row > < div class = col - md - 6 > < h1 > neverland keyed < /h1></div > < div class = col - md - 6 > < div class = row > $ {
    st({
      id: "run",
      title: "Create 1,000 rows",
      cb: () => e({
        type: "run"
      })
    })
  }
  $ {
    st({
      id: "runlots",
      title: "Create 10,000 rows",
      cb: () => e({
        type: "runLots"
      })
    })
  }
  $ {
    st({
      id: "add",
      title: "Append 1,000 rows",
      cb: () => e({
        type: "add"
      })
    })
  }
  $ {
    st({
      id: "update",
      title: "Update every 10th row",
      cb: () => e({
        type: "update"
      })
    })
  }
  $ {
    st({
      id: "clear",
      title: "Clear",
      cb: () => e({
        type: "clear"
      })
    })
  }
  $ {
    st({
      id: "swaprows",
      title: "Swap Rows",
      cb: () => e({
        type: "swapRows"
      })
    })
  } < /div></div > < /div></div > `)({dispatch:n}),[])}<table class="table table-hover table-striped test-data"><tbody>${e.map(r=>ot({data:e,item:r,dispatch:n,selected:r.id===t}))}</tbody></table><span class="preloadicon glyphicon glyphicon-remove" aria-hidden=true /></div>`
});

The Jumbotron is rightly inlined, carrying all Button with it, and everything works as expected.

Using --verbose, both minifiers show the sequence:

WARN: Collapsing jumbotron [dist/index.js:2113,41]
WARN: Dropping unused variable jumbotron [dist/index.js:2111,10]

Testing Hints

# enter any testing folder
git clone https://github.com/WebReflection/js-framework-benchmark.git
cd js-framework-benchmark/frameworks/keyed/neverland
npm i && npm run build-prod

At this point, if you npx http-server . from the js-framework-benchmark folder, you can reach http://127.0.0.1:8080/frameworks/keyed/neverland/ and see everything is OK.

However, if you now edit js-framework-benchmark/frameworks/keyed/neverland/package.json, and you use terser instead of uglifyjs within the build-prod script, and you install terser via npm i -D terser and you npm run build-prod again, from that folder, you'll see an empty page, with no error whatsoever, and the issue would be at the end of the dist/index.js file (I've used Code formatter to post above outcome).

Note I haven't found a way to reproduce the issue in isolation, but while the augmentor library, that has been recently refactored, looks exactly the same once uglify or terser compress and mungle it, so that useMemo is identical and it does exactly the same thing once minified, terser doesn't seem to understand hooks logic behind the scene, which is a bummer.

reproducible non third parts / builds related issue here:
#527 (comment)

[WebReflection]

FYI dropping this part from the minified code, the outcome is the expected one.

The regression has been introduced in this commit, and simply using if(false) instead of if(1===i.argnames.length&&e.args.length<2&&"name"===n.start.type&&n.name===i.argnames[0].name) within the terser/dist/bundle.min.js produces the expected outcome.

Personal concern

This was a feature request, not an issue, neither a bug.

However, accordingly with semver features should land in a minor version update, not in a patch.

That commit wasn't technically fixing anything, and while it's OK (as in: it happens) that a new feature might land (not on a Sunday though) with some bug, so that patching at that points makes sense, I believe that specific extra optimization shouldn't have landed as patch, which is also the reson it took me so long to figure out when/why/how the code was breaking, 'cause I couldn't think of a patch changing the outcome so much.

Thanks for eventually considering following semver more strictly in the future, without landing new features as patches.

Best Regards

[midudev]

I can confirm the same problem is happening to us and breaking our codebase when using webpack as webpack-terser-plugin is using ^4.3.9 as package version.

We're fixing the version to 4.4.0 in our package.json until this is fixed: SUI-Components/sui#708

[WebReflection]

Update - Stand alone reproducible issue

Using similar code, mocking all the things, I can reproduce the issue without any third parts dependency / compiler / transpiler / bundler.

const html = template => template.join('');

const useMemo = fn => fn();

const useReducer = (fn, obj) => [obj, action => {
  obj = fn(obj, action);
}];

const Jumbotron = ({dispatch}) => html`
  <div class="jumbotron" onclick="${dispatch}"></div>
`;

const listReducer = state => ({...state});

const Main = state => {
  const [{data}, dispatch] = useReducer(listReducer, state);
  const jumbotron = useMemo(() => Jumbotron({dispatch}), [data]);
  return jumbotron;
};

console.log(Main({data: []}));

Passing this code as terser source.js -m -c -o min.js and running node min.js will produce the following outcome:

{ dispatch: [Function (anonymous)] }

The expected outcome is instead the following one:

<div class=jumbotron onclick=></div>

You can see the Jumbotron function gets removed so that useMemo always returns { dispatch: [Function (anonymous)] } instead of the result of executing Jumbotron({dispatch: fn}).

[andreieftimie]

Have the same issue. terser is used as a dependency of terser-webpack-plugin https://github.com/webpack-contrib/terser-webpack-plugin/blob/master/package.json#L46

This is used by default by webpack in production mode.

This broke our builds over the weekend.
Pinned terser to 4.4.0 in the main project and the minified builds work again.

[WebReflection]

To whom it might concern, since emails don't receive updates, the stand alone test case has been reduced to the minimum, and it doesn't require a browser to be tested.

[fabiosantoscode]

Awesome, thanks for bringing this up, and thanks to @WebReflection for the reproducible minimal code. I'm on it.

[fabiosantoscode]

WRT respecting semver, I take care to do that, except I didn't notice the inline function commit when I released (that's why it's not in the changelog). It might happen from time to time that I don't follow semver, but I never do it on purpose.

I do like to ship during the weekend to try and avoid major breakage during the week, and also because I spend most time on Terser during the weekend.

[WebReflection]

I didn't notice the inline function commit when I released (that's why it's not in the changelog)

My comment wasn't about blaming you or whoever merged that inline function commit, rather about finding better ways to ensure unrelated commits don't land as patch.

Between 4.4.0 and 4.4.1 there were very few commits, so I wonder if there's any review in place before releasing, and if not, if it makes sense to keep non-related optimizations, or minor related versions into a separate branch, instead of merging everything in master, 'cause I do see an issue with the current approach. With the same principle, a breaking major change could land as patch too, which is why I've raised my concern regarding this incident.

I do like to ship during the weekend to try and avoid major breakage during the week

It might be a matter of personal taste, but during the week, everyone using this tool, which is easily capable of breaking the Web, thanks to its huge adoption, can react quickly, while people being paged over the weekend because suddenly everything breaks, won't be too happy about your approach.

I understand your time is yours, and you spend it when and how you prefer, that's perfectly fine, but great adoption means great responsibilities, so that as well as many companies have the "no release Friday" policy, releasing on a Saturday night, or on a Sunday, has even more potential side-effects.

I personally do work on most of my personal stuff on the weekend too, but if I'm introducing something new, either I bump the minor version, or I ensure myself to be around whenever my release eventually broke developers trusting my code, so that I can react quickly if something broke.

In this bug report case, I've spent half of my Sunday trying to figure out what I did wrong, after a huge refactoring, to realize the issue was elsewhere, and from filing a bug for a patch to a solution, might take a while, so that if you're not around, or anyone else maintaining the project is, that's a problem.

This was me after few hours of investigation, 2 blocked merge requests due unexpected and sudden issues, and an ugly work around to avoid blocking those MRs.

In few words, many of us code on the weekend, so that tools suddenly failing due a patch that went too far, have consequences.

I hope I've explained my concerns in details now, and thanks regardless for taking care of this bug as soon as you found it.

[WebReflection]

Now, that was quick! Happy to re-test my whole stack that failed and re-confirm that fixed the issue.

Edit everything works as expected 🎉

Thank you!

[fabiosantoscode]

Thanks for expressing your concerns @WebReflection. I hadn't considered the people who would be on call during the weekend when I decided to ship during the weekend. I consider my weekend and holidays to be sacred and I try to respect others' as well. I will try and ship during the middle of the week so people are prepared to deal with any issues that might come from Terser breakage.

Happy to re-test my whole stack that failed and re-confirmed that fixed the issue

Cool! If you get issues, @ me.

Also, if your stack is open source and a large codebase, please post a link. I've been looking for large open codebases to test Terser on (by minifying the code and then running unit+functional tests on it). I have done this to the React codebase so far, Angular is up next and yesterday I figured out a neat way to test eslint is to lint a huge codebase with a strict and rich ruleset compare the output of the minified eslint between runs.

[WebReflection]

if your stack is open source and a large codebase, please post a link

this is the biggest mix-up of libraries and tools I know, and it's used to also automate performance regressions, so it's the biggest babel-tower I know, and most builds are based either on rollup or webpack, but use terser behind the scene.

If you read my initial way to reproduce the issue, you basically know already what to look for when a release is coming: those instructions would work for most of the other libraries in that repository, which includes a multitude of different code style and patterns, so that once you have vanilla, hooks, custom elements, redux, and other most common patterns covered, ensuring these use terser to create their builds, you'll have somehow the certainty most stuff shouldn't break ^(tm).

[fabiosantoscode]

Hey, that's a pretty nice link!

My usage is to test the frameworks, but I might do a simple script and minify every main in every package.json in the node_modules, since main should never have JSX, TS, flow and other extensions, and it's usually the whole framework, possibly already minified by something else. This already tells me if the commit caused a nice crash :)

It would be neat if there was also a "functional test", which made sure the frameworks provided the correct HTML after a benchmark is done. I'll bug them for that.