google / closure-compiler

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

Array type check not performed #628

Open pstjvn opened 9 years ago

pstjvn commented 9 years ago

IIt seems that type chekcing is not catching type mismatch when inserting in a typed array. I have the example run in web service. I thought the compiler should complain that I am trying to insert property into the myArray_ that is not part of the enumeration, but it does not complain. Is think this might be a bug.

// ==ClosureCompiler==
// @compilation_level ADVANCED_OPTIMIZATIONS
// @output_file_name default.js
// @formatting pretty_print
// @use_closure_library true
// @warning_level VERBOSE
// ==/ClosureCompiler==

goog.provide('myns.A');
goog.require('goog.array');

/**
 * @constructor
 */
myns.A = function() {
/**
* @type {Array.<myns.A.Type>}
* @private
*/
this.myArray_ = [];
};

myns.A.prototype.mymethod=function() {
goog.array.insert(this.myArray_, 17);
};

/**
* @enum {number}
*/
myns.A.Type = {
A: 1,
B: 2
}

var a = new myns.A();
a.mymethod();

I had similar problem in real code and the compiler did not catch it (so I have lost half a day trying to figure out what is going on).

concavelenz commented 9 years ago

This is a property of how the templated types work in the current type inferrence. This function is currently typed as:

/**
 * Pushes an item into an array, if it's not already in the array.
 * @param {Array.<T>} arr Array into which to insert the item.
 * @param {T} obj Value to add.
 * @template T
 */
goog.array.insert = function(arr, obj) {

What the current type inferencer does at the call site is visit the uses of T in the parameters and build a union of possible types. In your example, the type of T is (myns.A.Type|number) which doesn't result in a type warning (the types are compatible).

If this were "insert" method of an Array, the type of T could be declared instead of inferred and you would get the result you are expecting.

I believe the new type inference is going to try to tweak this algorithm.

pstjvn commented 9 years ago

Thank you, is there a way to tell the compiler that only items from the enum are permited? How come the type is myns.A.Type or a number, shouldn't it be only myns.A.Type ?

concavelenz commented 9 years ago

Both number and myns.A.Type are used as T, and as I said, the type inference currently builds a union of those types. For most types, the union would create a type warning, but we check arrays more loosely.