ADVANCED compilation mode incorrectly removes non-dead code

[codedhead]

I am using webpack with closure compiler, I ran into an issue that some function calls from certain modules were removed by the compiler.

This is the minimal example for reproducing the issue, it's basically the webpack code for requiring a module

function __webpack_require__(moduleId) {
  var module = {
    id: moduleId,
    exports: {},
  };
  __webpack_modules__[moduleId].call(
    module.exports,
    module,
    module.exports,
    __webpack_require__
  );
  return module.exports;
}

__webpack_require__.o = (obj, prop) => {
  return Object.prototype.hasOwnProperty.call(obj, prop);
};

__webpack_require__.d = (exports, definition) => {
  var key;
  for (key in definition)
    if (
      __webpack_require__.o(definition, key) &&
      !__webpack_require__.o(exports, key)
    )
      Object.defineProperty(exports, key, {
        enumerable: true,
        get: definition[key],
      });
};

var __webpack_modules__ = {
  123: (
    __unused_webpack_module,
    __webpack_exports__,
    __webpack_require__
  ) => {
    function myFunction() {
      console.log('hello');
      // do more stuff
    }
    __webpack_require__.d(__webpack_exports__, {
      myFunction: () => {
        return myFunction;
      },
    });
  },
}

var myModule = __webpack_require__(123);

window['test'] = () => {
  myModule.myFunction();
};

Calling window.test() should print "hello" in the console. However the ADVANCED compilation mode turns window['test'] into an empty function

function c(b) {
  var a = { id: b, exports: {} };
  e[b].call(a.exports, a, a.exports, c);
  return a.exports;
}
c.g = (b, a) => Object.prototype.hasOwnProperty.call(b, a);
c.d = (b, a) => {
  for (var d in a)
    c.g(a, d) &&
      !c.g(b, d) &&
      Object.defineProperty(b, d, { enumerable: !0, get: a[d] });
};
var e = {
  123: (b, a, d) => {
    function f() {
      console.log("hello");
    }
    d.d(a, { h: () => f });
  },
};
c(123);
window.test = () => {};

Please help take a look, thanks!

[blickly]

I was able to reproduce this, and minimize the example a bit more:

function __webpack_modules__123() {
    function myFunction() {
      console.log("hello");
    }
    return {
        myFunction: function() { return myFunction; }
    };
}
var myModule = __webpack_modules__123();
window["test"] = function() {
  myModule.myFunction();
};

It looks like something in the peephole passes are causing this breakages, although I haven't identified exactly which yet.

[codedhead]

Hi @blickly thanks for taking a look! IIUC the example you provided seemed to work as expected, as myModule.myFunction() returns the inner myFunction function itself rather than calling the inner myFunction, so it has no side effect and is optimized away.

I was able to simplify my previous example further:

// version 1
function __webpack_require_module__() {
  const exports = {};
  function myFunction() {
    console.log('hello');
  }
  const definition = {
    myFunction: () => {
      return myFunction;
    },
  };

  for (const key in definition)
    Object.defineProperty(exports, key, {
      enumerable: true,
      get: definition[key],
    });
  return exports;

}

var myModule = __webpack_require_module__();
window['test'] = () => {
  myModule.myFunction();
};

Even without using Object.defineProperty and setting properties directly, the issue still exists:

// version 2
function __webpack_require_module__() {
  const exports = {};
  function myFunction() {
    console.log('hello');
  }
  const definition = {
    myFunction: () => {
      return myFunction;
    },
  };

  for (const key in definition)
    exports[key] = definition[key]();
  return exports;
}

var myModule = __webpack_require_module__();
window['test'] = () => {
  myModule.myFunction();
};

[codedhead]

I played around with example more, closure compiler can generate correct code for the following version that moves arrow function from the properties of definition into Object.defineProperty's getter:

// version 3
function __webpack_require_module__() {
  const exports = {};
  function myFunction() {
    console.log('hello');
  }
  const definition = {
    myFunction: myFunction,
  };

  for (const key in definition)
    Object.defineProperty(exports, key, {
      enumerable: true,
      get: () => definition[key],
    });
  return exports;
}

var myModule = __webpack_require_module__();
window['test'] = () => {
  myModule.myFunction();
};

[blickly]

Oops, thanks for catching that.

It still does look like one of the peephole passes is breaking the code.

I think that the issue is that when determining whether myModule.myFuntion() has side effects, it looks globally throughout the program at the definition of all functions named {$something}.myFunction. The only function it finds matching that description is the one defined in:

  const definition = {
    myFunction: () => {
      return myFunction;
    },
  };

and that one actually has no side effects. It can't see the function defined on exports, since that function property is created dynamically with [].

I'm honestly not sure if there's a way to fix this in the compiler without causing optimization backoff for other cases that folks care about.

[bworline]

This is a common pattern for Closure compiler consumers that use webpack, and it's due to the import/export glue that webpack generates--meaning these consumers don't have control over it.

Would you consider creating a command-line flag to disable this particular optimization?

[concavelenz]

Are you using https://github.com/webpack-contrib/closure-webpack-plugin?

In the abstract, without a plugin, this code is incompatible with Closure Compiler ADVANCED optimizations because from the compiler's perspective the definitions of "myFunction" are hidden by the reflective for loop. All the property optimizations will misbehave when faced with this code and you will be limited to the "SIMPLE" optimizations.

The goal of the compiler would to unwind this code and flatten it so that the definitions and calls were clearly visible.

[bworline]

I'm using a fork of it that was rewritten to work with webpack 5. Fundamentally all the plugin does is hook the optimization stage of webpack and create the correct input arguments for the Closure compiler.

Generally, webpack is generating valid JS constructs. These constructs are needed for the implementation of import/export across modules and across chunks. The issue is that the Closure compiler optimizer is not able to correctly reason over these constructs. Looking for {$something}.myFunction as blickly mentions is not a valid assumption for catching all of the uses of myFunction.

While I understand the hesitancy to lose an optimization, I also believe that correctness has to come before optimization. I'd love to see the optimizer correctly handle this case, but in the absence of that a flag can make everyone happy.