Incorrect/overly strong this inference for POJOs

[chancancode]

TypeScript Version:

nightly (1.9.0-dev.20160419)

Code

Ember.Object.extend({
  init() {
    this._super();
//  ^~~~ error TS2339: Property '_super' does not exist on type '{ init(): void; }'.
    ...
  }
});

Expected behavior:

this should be inferred as any

Actual behavior:

this is inferred to be { init(): void; }

---

This changed somewhere between [email protected] and [email protected]. I believe it is unsafe to assume anything about the this on the "methods" on a POJO, as there are several common patterns in JavaScript that breaks this assumption (using POJOs as an "options" or "extensions" dictionary).

[wycats]

I'd like to add that TC39 had the opportunity to make the desugaring from init() {} something more involved than init: function() {}. In particular, we were considering making "methods" on POJOs non-enumerable. I argued strongly against that course and we chose to stick with a simple desugaring.

The problem (in that context) that I was worried about, is that existing code expects to take a POJO, enumerate over its properties, and do something with that enumeration. The class pattern is the most common example, but options bags that can contain arbitrary functions is common as well.

I was worried that if refactoring from foo: function() {} to foo() {} in an object literal quite often didn't behave as expected, people would become resistant to the refactoring. In contrast, the complete refactoring for most POJOs containing "real methods" is refactoring to classes, which are a sufficiently different construct to justify the change in behavior (that refactoring is already sufficiently non-mechanical).

A similar situation is happening here in my opinion. Because there are some use-cases where inferring this from a POJO is convenient, we are creating an upwards refactoring problem from foo: function() {} to foo() {}. In practice, this will cause resistance to moving to the more concise form, which is something we expressly chose to avoid in TC39 (for good reason).

The meaning of "method in a POJO" is simply too ambiguous to try to guess that the user probably meant "this object is a singleton with methods", and that approach creates too much friction to using concise methods in the large to justify the cognitive overhead, in my opinion.

[RyanCavanaugh]

On the other hand, we've gotten the request to type this inside object literal functions about eleventy million times already. 😕

[wycats]

@RyanCavanaugh Are you sure that the eleventy million times are all cases where the object literal is the correct this, vs. cases like .extend(POJO) or defineClass(POJO) which wants a different behavior?

In my JavaScript experience, the set of cases where this inside an object literal is actually referring to the object itself (as opposed to a transplant object or with a this provided by an API like jQuery.ajax) is relatively small.

[mhegazy]

extend could define a contextual type for this to be Ember.CoreObject for instance, would that be desired in your scenario?

[wycats]

@mhegazy what we want is extend<T>(extension: T): this & T, where the resulting value is a valid superclass (we're willing to be explicit about that if necessary).

[wycats]

Actually that isn't quite right: this is the superclass and T augments the instance side, and we're returning a typeof that augmented type.

[wycats]

But more generally, I don't think this change in behavior is what people have in mind much of the time when they ask for this to be typed. It really does need to be specified by the code that takes the POJO, and not assumed to be the singleton POJO itself.

Or even more generally, it should be the job of jQuery, Ember.Object, Backbone.Model, React.createClass, etc to provide the this binding, not TypeScript.

[mhegazy]

the counter example is:

var x = {
    foo() { .. },
    bar() {  this.|  /* why no foo here? */ }
};

[wycats]

That may be a theoretical counterexample but how often does it happen in practice relative to the React.createClass and jQuery.ajax patterns?

At least for me, cases where I'm writing a POJO with methods that I intend to use as-is is in the minority relative to cases where this is provided by the callee.

This change means that people writing Backbone.Model.extend, React.createClass or Ember.Object.extend (as well as jQuery.ajax, new Benchmark(options), etc.) will not be able to use concise syntax, or they will have to say this as any a lot. I don't think that tradeoff is worth it.

It will also produce this WAT that is similar to yours:

let options = {
  url: "microsoft.com"
};
Object.assign(options, {
  invoke() { /* why doesn't this.url work? */ }
});

[RyanCavanaugh]

Right on cue? http://stackoverflow.com/questions/36730275/create-object-with-part-of-interface-typescript

[DanielRosenwasser]

or they will have to say this as any a lot

Well you'll be able to specify the type of this in each method, so you'll only have to do it once at the top of your method. Not necessarily the best if it wasn't your intention to get an automatic this type, but certainly not the worst either.

[wycats]

@DanielRosenwasser I don't know what criteria or rationale you are using to discuss this topic, but it would be helpful for me to understand:

1. Of the complaints that you have received, what concrete cases involving POJOs and this would be satisfied by this change? How many of them involve partial interfaces, where people would expect this.| to complete with fields and methods from the super-interface?
2. Of the complaints that you have received, how many cases look like partial interfaces, and how many look like options bags whose this is supplied by the caller?

It may well be that I'm in a bubble, and (2) is much less common in the real world than (1), but I'd really appreciate some insight here.

[DanielRosenwasser]

My feeling is (1) tends to be more common but that people using (2) haven't complained because this being any has just stayed out of their way. @RyanCavanaugh might know better.

@mhegazy mentioned above that we could accommodate both scenarios accurately (with contextual types). I think so too, but I think that the signature could get kind of hairy (#8252 (comment) ?). I'll have to look at it when I get back.

[sandersn]

@mhegazy and I hacked in the intersection of the contextual type and the object literal type. This works for the small examples here and doesn't seem to break any other tests (except 1, see below).

Basically, you get the intersection of the contextual type and the object literal's type as the this type. However, if the contextual type of the object literal is any, then this just becomes any.

It's in the branch smarter-object-literal-this-contextual-type-WIP. It causes a Salsa (Javascript) test to fail when assigning a function to prototype. I'll figure out why that happens and clean it up for a PR.

[sandersn]

#8356 fixes this issue, although it does so in a weird way, by intersecting the contextual type with the object literal's type. I'm not sure this is the right approach even though it works for the examples discussed here.

[kitsonk]

Sorry to butt in on all the fun...

Most of the real world examples I have run across fall into @wycats (1) category as well.

The (2) category does seem rare, mostly because of the any doesn't get in the way, but more importantly, it is uncommon to see this referencing methods in an object.

I don't think intersecting the contextual type with the object literal's type is weird, I think in most cases that is exactly what is being expressed. If/when it is merged, I will want to see if it makes some of the composition problems I have a bit easier (or if the inferred this typing actually is a step backwards).

[sandersn]

For now #8356 is a more conservative fix that says

1. If the contextual type of a function is any, the type of that function's this is now any.
2. If the contextual type of a function doesn't define a this parameter (which is nearly always the case right now), then the type of that function's this is now any.
3. If there is no contextual type for a function in an object literal, then use the object literal's type.

Previously (1) and (2) wouldn't set the this to any -- the compiler would keep looking for a type for this instead, and it would find the object literal type.

This doesn't make the compiler smarter about extends-like functions, but it does make it get out of the way better:

interface Arguments {
  init?: () => void;
  willDestroy?: () => void;
  [propName: string]: any;
}
declare function extends(arguments: Arguments);
extends({
  init() {
    // this: any here -- Arguments.init doesn't declare a `this` parameter.
  }
  foo() {
    // this: any here -- the string indexer contextually types 'foo': any
    // and this means that foo's this: any.
  }
}

And the context-free object literal typing stills works. We'll continue thinking about whether intersecting the object literal type with the contextual type is a good idea in general.

var o = {
  prop = 12
  bar() {
    this./*should see 'prop' and 'bar' here*/
  }
}

[mhegazy]

@wycats, and @chancancode can you give typescript@next a try tomorrow and let us know if this fix addresses the Ember scenarios?

[mhegazy]

To follow on @wycats comment about the difference in behavior between methods and properties in object literals, that was was not intentional, and should be fixed in #8382

[chancancode]

@mhegazy I can confirm that it's been fixed! Thank you!

[ahejlsberg]

See #14141.