RemoveUnusedCode regression in v20180204

[thheller]

There seems to be an issue with the v20180204 Closure Compiler release where some code seems to be altered that shouldn't be. The previous v20180101 (and others) work without issue. The reported warnings seem to be identical and can be ignored.

This was reported by a shadow-cljs user but I managed to trace this down to the recent Closure release and reproduced it with just the distributed .jar files.

Originially this was discovered as part of a ClojureScript build which imported the quill npm package via react-quill, which stopped working when I released a new shadow-cljs version using v20180204.

Steps To Reproduce

Download necessary things

curl -O https://unpkg.com/quill@1.3.5/dist/quill.js
curl -O http://repo1.maven.org/maven2/com/google/javascript/closure-compiler/v20180204/closure-compiler-v20180204.jar
curl -O http://repo1.maven.org/maven2/com/google/javascript/closure-compiler/v20180101/closure-compiler-v20180101.jar

Compile with bad version

java -jar closure-compiler-v20180204.jar --js quill.js -O SIMPLE --js_output_file compiled.js

Open Browser

<div id="app"></div>
<script src="compiled.js"></script>
<script>
    new Quill("#app", {});
</script>

open index.html

Fails with Error (See Browser Console)

Uncaught TypeError: Cannot call a class as a function
    at e.a (compiled.js:102)
    at new e (compiled.js:229)
    at a.value (compiled.js:218)
    at new c (compiled.js:76)
    at index.html:4

Compile with good version

java -jar closure-compiler-v20180101.jar --js quill.js -O SIMPLE --js_output_file compiled.js

Reload browser and Error is gone.

Running git bisect traced down this issue to this commit:

6b807c063d42a463e3c32e5911c695a0976e2b67 is the first bad commit
commit 6b807c063d42a463e3c32e5911c695a0976e2b67
Author: bradfordcsmith <bradfordcsmith@google.com>
Date:   Wed Jan 17 14:24:56 2018 -0800

    RemoveUnusedCode: remove `x instanceof UnusedName`

    -------------
    Created by MOE: https://github.com/google/moe
    MOE_MIGRATED_REVID=182269894

:040000 040000 d44ac3cdd98601b33b21192fada4fc0957a18efc ac0a44df4d8bee7350d32e3bbe4933a15f9d5151 M      src
:040000 040000 a6510a4aa77bde61508ea6c33fbdf6601f8b61e0 feb62fb9bd9b4cd4cea6f3b32ee552b293971c70 M      test

[concavelenz]

Were you able to determine what code was unexpectedly removed?

[thheller]

Compiling with source maps points me to line 1023 which is one of those babel generated class checks.

function _classCallCheck(instance, Constructor) { if (!(instance instanceof Constructor)) { throw new TypeError("Cannot call a class as a function"); } }

https://gist.github.com/thheller/7f43caf785f1846ef6d0cd05a7bcbabc#file-quill-js-L1023

which is called in the constructor a few lines later (1085).

  function Quill(container) {
    var _this2 = this;

    var options = arguments.length > 1 && arguments[1] !== undefined ? arguments[1] : {};

    _classCallCheck(this, Quill);

https://gist.github.com/thheller/7f43caf785f1846ef6d0cd05a7bcbabc#file-quill-js-L1085

Note that the gist is not original quill.js. It is the raw input generated by shadow-cljs when passing it to the closure compiler. Not that it matters since the error occurs without that.

I was not able to identify what code exactly was removed/changed.

[thheller]

My analysis from yesterday was incorrect. Looked at the wrong piece of code.

I recompiled with pseudo names and pretty printed to get a look at the actual generated code.

https://gist.github.com/thheller/7f43caf785f1846ef6d0cd05a7bcbabc#file-quill-js-L1943-L1969

/***/ (function(module, exports, __webpack_require__) {

"use strict";


Object.defineProperty(exports, "__esModule", {
  value: true
});

function _classCallCheck(instance, Constructor) { if (!(instance instanceof Constructor)) { throw new TypeError("Cannot call a class as a function"); } }

var Module = function Module(quill) {
  var options = arguments.length > 1 && arguments[1] !== undefined ? arguments[1] : {};

  _classCallCheck(this, Module);

  this.quill = quill;
  this.options = options;
};

Module.DEFAULTS = {};

exports.default = Module;

/***/ }),
/* 10 */
/***/ (function(module, exports, __webpack_require__) {

is turned into

https://gist.github.com/thheller/7f43caf785f1846ef6d0cd05a7bcbabc#file-quill-after-simple-js-L1152-L1159

    }, function($Module_module$$, $exports$$, $__webpack_require__$$) {
      Object.defineProperty($exports$$, "__esModule", {value:!0});
      $Module_module$$ = function($__webpack_require__$$) {
        throw new TypeError("Cannot call a class as a function");
      };
      $Module_module$$.DEFAULTS = {};
      $exports$$.default = $Module_module$$;
    }, function($module$$, $exports$$, $__webpack_require__$$) {

the _classCallCheck is completely removed and the constructor just immediately throws.

If I read this correctly it doesn't seem to differentiate between module and Module?

[concavelenz]

Ok, there is the "instanceof". I think I can guess the chain of events: "Constructor" is not used for anything but the "instanceof" check. The logic added in the problematic change is trying to remove definitions only used for instanceof checks. It incorrectly replaces the "instanceof" expression with "false". The function is that inlined, and the rest of the constructor body removed.

Thanks for the details, this should be easy enough to back off when the definition of the variable isn't a known object.

[brad4d]

Here's a simpler repro.

var C = function C() {
  if (!(this instanceof C)) {
    throw new Error('not an instance');
  }
}
use(new C());

[brad4d]

RemoveUnusedCode is failing to recognize that the inner name C is aliased with the outer name C, so instances can get created that way.

[brad4d]

fix sent for internal review