closure-compiler

closure-compiler copied to clipboard

depending on the order of externs passed to closure-compiler, type definitions might be silently lost!

while i wouldn't disagree that i should be taking better care with externs, it seems like an easy trap to fall into: my tooling was using a glob.glob('*.js') to build up the externs, and the code was passing locally w/out any errors. but it would fail on some other systems seemingly at random, and it took quite a while to track down/realize it was due to the externs order being different due to dirent ordering in the underlying FS.

normally closure-compiler complains when it sees duplicate definitions (which is great), so it seems like it should have issued errors or diagnostics of some sort when it clobbered/lost some type information.

the example below is basically what i had: a externs-base.js file that defined the common APIs from a diff module, and externs-addons.js that included local extensions and built off of the common API. so logically, addons should be imported after base.

$ closure-compiler --version
Closure Compiler (http://github.com/google/closure-compiler)
Version: v20200406
Built on: 2020-04-09 19:44

$ closure-compiler --checks-only '--jscomp_error=*' \
    --externs=externs-addons.js --externs=externs-base.js code.js
< no errors !? >

$ ../libdot/bin/closure-compiler --checks-only '--jscomp_error=*' \
    --externs=externs-base.js --externs=externs-addons.js code.js
code.js:7: ERROR - [JSC_TYPE_MISMATCH] actual parameter 1 of APIEvent.prototype.addListener does not match formal parameter
found   : function(Object, function(string): ?): undefined
required: function({id: string}, function(number): ?): ?
API.onOpen.addListener(onOpen);
                       ^^^^^^

1 error(s), 0 warning(s), 85.7% typed

here are the sample inputs

• externs-base.js

/** @externs */

var API = {};

/**
 * @interface
 * @template T
 */
function APIEvent() {}

/** @param {T} callback */
APIEvent.prototype.addListener = function(callback) {};

• externs-addons.js

/**
 * @interface
 * @extends {APIEvent<function(
 *   {id: string},
 *   function(number)
 * )>}
 */
API.Event;

/** @type {!API.Event} */
API.onOpen;

• code.js

/**
 * @param {!Object} options
 * @param {function(string)} onSuccess
 */
const onOpen = function(options, onSuccess) {};

API.onOpen.addListener(onOpen);

I often find running closure-compiler with this argument helps highlight issues with type resolution.

--jscomp_warning=reportUnknownTypes

thanks for the tip. i thought --jscomp_error=* would have done that for me, but i guess not. i gave it a try on my codebase and i have almost 5k warnings, so i've got a long way to go before i can turn that on :(.

To clarify, I am suggesting to periodically use reportUnknownTypes and to only examine the warnings for the script or type you are currently focused on. I am not recommending you permanently enable reportUnknownTypes and fix all warnings.

Here's a simpler repro. If the externs are reversed we get the expected type error. Specifically, if the definition of Foo is above the extends {Foo<number>}. So it looks like the issue has something to do with trying to fill the template map before the template has been defined.

/** @externs */

/** @interface @extends {Foo<number>} */
function Bar() {}

/** @type {!Bar} */
var bar;

//////////////

/** @interface @template T */
function Foo() {}

/** @type {T} */
Foo.prototype.bar;

const /** string */ x = bar.bar;

This is a known issue. Type resolution is broken with respect to forward-referenced, templated, ancestor types. There's no place where it attempts to connect the template map from such an ancestor to the template map of the subtype.

There's some entanglement with a related issue where prototype types never have template type maps. We'd need to decide how to model that before we could fix the main bug.

i'm just interested in having closure-compiler tell me there is a problem, not try and combine all the types together. i'd be fine with requiring the externs be in the "correct" order relative to decls.

Adding a new diagnostic is sadly more work than just fixing it, at least in our experience