google / closure-compiler

A JavaScript checker and optimizer.
https://developers.google.com/closure/compiler/
Apache License 2.0
7.42k stars 1.16k forks source link

Different behavior when inlining anonymous functions? #1349

Open vobruba-martin opened 8 years ago

vobruba-martin commented 8 years ago

I use ADVANCED optimization and I expect both scripts to raise a warning because of bad callFn argument.

This script raises a warning:

/**
 * @param {function(number)} fn
 */
function callFn(fn) {
    fn(1);
};

var fnToCall = function (/** !Function */ arg) {
    console.log(arg);
};

callFn(fnToCall);

This does not (fnToCall inlined):

/**
 * @param {function(number)} fn
 */
function callFn(fn) {
    fn(1);
};

callFn(function fnToCall(/** !Function */ arg) {
    console.log(arg);
});
dimvar commented 8 years ago

You're right, there is a bug in the old type checker; there should be a warning in the 2nd example. This works correctly in the new type checker.

http://closure-compiler-debugger.appspot.com/#input0%3D%252F**%250A%2520*%2520%2540param%2520%257Bfunction(number)%257D%2520fn%250A%2520*%252F%250Afunction%2520callFn(fn)%2520%257B%250A%2520%2520%2520%2520fn(1)%253B%250A%257D%253B%250A%250AcallFn(function%2520fnToCall(%252F**%2520!Function%2520*%252F%2520arg)%2520%257B%250A%2520%2520%2520%2520console.log(arg)%253B%250A%257D)%253B%26input1%26conformanceConfig%26externs%26refasterjs-template%26includeDefaultExterns%3D1%26CHECK_TYPES_NEW_INFERENCE%3D1%26MISSING_PROPERTIES%3D1%26PRESERVE_TYPE_ANNOTATIONS%3D1%26PRETTY_PRINT%3D1

I recommend trying out to the new one, with the flag --new_type_inf. It finds many more warnings than the old one.

vobruba-martin commented 8 years ago

Is this new type checker stable? I get many weird errors using it for my project but I couldn't reproduce them using simpler code. Is there any chance that old type checker will be fixed?

dimvar commented 8 years ago

It's mature enough for projects to use it, but it's still under development. Most work that goes in the old type checker is fixing crashes (and the recent @record work). Other bugs won't be fixed.

I can probably give you some guidance to get you started with the new type checker. If you can't reproduce warnings with a small independent code sample, start a new thread here: https://groups.google.com/forum/#!forum/closure-compiler-discuss And post the most common warnings that you see, with snippets from your actual project. It doesn't matter that I won't be able to reproduce, maybe the snippets will contain enough information for me to give you some help.

dimvar commented 8 years ago

Also, this page can help you get started: https://github.com/google/closure-compiler/wiki/Using-NTI-(new-type-inference)

concavelenz commented 8 years ago

I don't think it is a bug in the OTI per se. I think that when the new short syntax was introduced, the type info the OTI is using wasn't updated, as the traditional syntax works correctly:

http://closure-compiler-debugger.appspot.com/#input0%3D%252F**%250A%2520*%2520%2540param%2520%257Bfunction(number)%257D%2520fn%250A%2520*%252F%250Afunction%2520callFn(fn)%2520%257B%250A%2520%2520%2520%2520fn(1)%253B%250A%257D%253B%250A%250AcallFn(%252F**%2520%2540param%2520%257B!Function%257D%2520arg%2520*%252F%2520function%2520fnToCall(%2520arg)%2520%257B%250A%2520%2520%2520%2520console.log(arg)%253B%250A%257D)%253B%26input1%26conformanceConfig%26externs%26refasterjs-template%26includeDefaultExterns%3D1%26CHECK_TYPES%3D1%26MISSING_PROPERTIES%3D1%26PRESERVE_TYPE_ANNOTATIONS%3D1%26PRETTY_PRINT%3D1