Unreachable code in jquery_event_deprecation

[Turbo87]

In Firefox with Ember v3.7.0 I'm seeing the following warning on the console in production:

unreachable code after return statement (jquery_event_deprecation.js:10)

It looks like the minifier is not doing its thing here. We should try to find a way to restructure that code so that terser is dropping the dead code correctly.

[simonihmig]

I'll try to fix this!

[simonihmig]

Here's a PR that fixes this: #17555

The first iteration of this did not work as expected, so I played around a bit, to see what's happening, and found something weird.

Given the following code excerpt:

  console.log(DEBUG);
  if (JQUERY_INTEGRATION && DEBUG && HAS_NATIVE_PROXY) {
    let boundFunctions = new Map();

ember.prod.js looks like this:

    console.log(false
    /* DEBUG */
    );

    if (_deprecatedFeatures.JQUERY_INTEGRATION && false
    /* DEBUG */
    && _utils.HAS_NATIVE_PROXY) {
      let boundFunctions = new Map();

which is fine, so DEBUG has been replaced with false, and thus the if expression will always evaluate to false as well. So you would expect terser to completely remove that if block. But this is what ember.min.js will look like instead:

e.default=function(e){console.log(!1)
if((i.JQUERY_INTEGRATION,0)&&n.HAS_NATIVE_PROXY){let t=new Map

Not only did it not remove the if block, it removed false (from DEBUG) from the expression. Which is semantically totally wrong! Would have been ok if it was true. 😱

Note that it does work when DEBUG is the first item in the if expression, as is the case in the PR!

Anyone can explain this, other than this being a bug in terser?

[rwjblue]

We should probably report over there I think...

[Turbo87]

you missed the ,0 part. (i.JQUERY_INTEGRATION,0) will evaluate the property and then return 0 because it's a comma expression.

[simonihmig]

you missed the ,0 part.

Ah, indeed! So it's semantically not wrong, but could have still figured out this to be always false, and thus removed the if block...

[rwjblue]

Right. It’s still a bug IMHO.

[simonihmig]

Hm, probably terser is not able to reduce the expression to a simple true or false, because a getter for i.JQUERY_INTEGRATION could have a side effect, so the safe bet would be to keep it. And if the whole expression cannot be reduced to a simple static boolean, it is maybe not smart enough to remove the whole if block, even if effectively the expression will always evaluate to the same value!?

Just speculating, but it would explain why changing the order makes a difference: when DEBUG/false is the first operand, it is safe to skip evaluating the other properties, as that's what would happen at runtime as well.

[Turbo87]

@simonihmig yep, that is what I would expect to be the reason too.

it could remove the &&n.HAS_NATIVE_PROXY part as the first part is already falsy, but apparently it's not quite clever enough for that 😉

[rwjblue]

I agree that the import could have side effects but in any case it knows that the body of the if could never be executed and it should therefore be stripped....