Add type check support for Object.create and Object.defineProperties

blickly commented 10 years ago

This issue was imported from Closure Compiler's previous home at http://closure-compiler.googlecode.com

The original discussion is archived at: http://blickly.github.io/closure-compiler-issues/#455

vpavlenko commented 10 years ago

Does this open issue also means that Object.defineProperty isn't implemented yet? This doesn't work:

var a = {};

Object.defineProperty(a, 'b', {
  get: function() { return 3; }
});

alert(a.b);

Error:

JSC_INEXISTENT_PROPERTY: Property b never defined on a at line 7 character 6
alert(a.b);
      ^

And, just to make sure, we're going to compile the code which uses it like that:

/**
 * @constructor
 */
function A() {}

Object.defineProperty(A.prototype, 'b', {});

alert(new A().b);

ChadKillingsworth commented 10 years ago

'b' is a quoted property in this case. You need to reference it consistently a['b'].

danbeam commented 10 years ago

@ChadKillingsworth This breaks typechecking, no?

Object.defineProperty(window, 'hello', {
  get: /** @return {string} */ function() { return 'world'; }
});
/** @type {number} */ var msg = window['hello'];  // no error

results in no errors, whereas:

/** @type {string} */ window.hello = 'world';
/** @type {number} */ var msg = window.hello;

results in the proper error:

JSC_TYPE_MISMATCH: initializing variable
found   : string
required: number at line 2 character 32
/** @type {number} */ var msg = window.hello;
                                ^

Also, we're not renaming in our project (currently).

ChadKillingsworth commented 10 years ago

Yes - type checking doesn't look at quoted properties. As mentioned in the original issue, Object.defineProperty always creates an external (quoted) property. This is by design.

If you are never going to use renaming on a project, then there are several workarounds. A renaming safe version of this depends on the work referenced in #179.

dimvar commented 10 years ago

type checking doesn't look at quoted properties.

FYI, this will change in the new type inference. For the simple case where a string literal is used as the property name in a [ ] access, we type check as if the access was using dot, since we know the property name.

vpavlenko commented 10 years ago

In Chrome JS we use a lot of Object.defineProperty() and treat properties declared this way as unquoted. We also use our Chrome-specific cr.defineProperty() which also has type information, like this:

/** @type {boolean} */
cr.defineProperty(Command, 'disabled', cr.PropertyKind.BOOL_ATTR);

We don't want to rewrite all the accesses to fields declared these ways to quoted manner and we also want to have type-checking on them. So we're going to do code rewriting in our ChromePass at the beginning of compiler pass pipeline. Specifically, consider we start with this snippet:

Object.defineProperty(Visit.prototype, 'checkBox', {
  get: function() {
    return this.domNode_.querySelector('input[type=checkbox]');
  },
});

Firstly, we manually add type information in the source, like this:

/** @type {Element} */
Object.defineProperty(Visit.prototype, 'checkBox', {
  get: function() {
    return this.domNode_.querySelector('input[type=checkbox]');
  },
});

Or we add type information in more legal way to compiler parser. And then we're going to rewrite it this way:

Object.defineProperty(Visit.prototype, 'checkBox', {
  get: function() {
    return this.domNode_.querySelector('input[type=checkbox]');
  },
});
/** @type {Element} */
Visit.prototype.checkBox;

Do you see any drawbacks of our approach?

I'm sorry that this comment is a bit unrelated to the problem of this issue.

MatrixFrog commented 10 years ago

In this case, your pass has completely removed the body of the getter, so it won't be typechecked at all. This is probably okay for a one-line function like that but you might want to think about it if you have more complex getter functions.

MatrixFrog commented 10 years ago

Discussed this a little bit with @dimvar -- one thing you'll want to make sure of is, if you're writing a pass that rewrites the JS in any way, make sure you first write out the result of the pass (in this case, the /** @type {Element} */Visit.prototype.checkBox;) and feed it into the compiler to make sure it gives you the typechecking behavior you expect.

Anyway, other than that, it seems like this is a good approach for what you want to do. Closing this issue since we're not making any changes on the Closure Compiler side, but feel free to comment here again if you have questions.

concavelenz commented 10 years ago

Object.defineProperties and Object.create should be supported by the type system.

SunboX commented 9 years ago

+1 :smirk:

michaelhogg commented 9 years ago

Is there any news regarding type-checking for objects created using Object.create()?

I would find this extremely helpful, for situations like this:

var a = {};

/** @param {number} x */
a.myFunction = function (x) {
    window.console.log(x);
};

var b = Object.create(a);

b.myFunction('not a number');  // Should raise TypeValidator.TYPE_MISMATCH_WARNING