-
Notifications
You must be signed in to change notification settings - Fork 12.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Typed 'this' in object literal methods #14141
Conversation
Latest commit adds contextual |
The one issue that remains for discussion here is whether the strongly typed |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like this change.
- Still need tests
- @bradenhs points out that the contextual typing code only applies to functions and object literal methods. It should apply to object literal getters/setters as well.
- I ran RWC to see what breaks. I found several good breaks, a few breaks that require a d.ts update with
ThisType
(Object.defineProperty, knockout and react), and a couple of places that show thatthis
needs to be the intersection of the object literal type and the parameter type.
src/compiler/checker.ts
Outdated
} | ||
|
||
function getThisTypeFromContextualType(type: Type): Type { | ||
return applyToContextualType(type, t => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shouldn't applyToContextualType
just be named mapType
and replace mapType
? It doesn't do anything specific to contextual types that I could see, and it's better than the current mapType
because it skips return values of undefined
. #Resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that makes sense.
@@ -41,19 +41,51 @@ tests/cases/conformance/fixSignatureCaching.ts(639,38): error TS2304: Cannot fin | |||
tests/cases/conformance/fixSignatureCaching.ts(640,13): error TS2304: Cannot find name 'window'. | |||
tests/cases/conformance/fixSignatureCaching.ts(641,13): error TS2304: Cannot find name 'window'. | |||
tests/cases/conformance/fixSignatureCaching.ts(704,18): error TS2339: Property 'prepareDetectionCache' does not exist on type '{}'. | |||
tests/cases/conformance/fixSignatureCaching.ts(704,45): error TS2339: Property '_cache' does not exist on type '{ constructor: (userAgent: any, maxPhoneWidth: any) => void; mobile: () => any; phone: () => any; tablet: () => any; userAgent: () => any; userAgents: () => any; os: () => any; version: (key: any) => any; versionStr: (key: any) => any; is: (key: any) => any; match: (pattern: any) => any; isPhoneSized: (maxPhoneWidth: any) => any; mobileGrade: () => any; }'. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The purpose of this test isn't obvious without following the pointer back to issue #10697. Can you add one more comment at the top of the file like "this test originally failed by using 100% CPU and should now take less than 1 second to run". I'd like a faster way to know that these additional failures are not a problem.
this.mine | ||
this.willDestroy | ||
//this.willDestroy |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just delete this, I think. #Resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
Summary of RWC breaks [1]: two good breaks from contextual typing of function assignment. Two breaks that show that [1] Note that I haven't looked at all of RWC yet; it hit a stack overflow halfway through.
declare function f(elt: HTMLElement): void;
function outer() {
var self = this;
var args: Draggable;
args.helper = function () {
return f(this);
// error 'this: Draggable' is not assignable to 'HTMLElement'
// Property 'accessKey' is missing in 'Draggable'
};
} This catches code that is just wrong. I looked at (This happened in two different projects.)
// Override the prototype method on C
CBase.prototype.debug = function() {
var self: C = this;
// error type 'CBase' is not assignable to 'C'
// Property '_subclassProperty' is missing in 'CBase' I'm not sure what to make of this. The comment makes it sound like the intention is to override the method on C, not on CBase. So probably this is a good error too.
Object.defineProperties(Array.prototype, {
empty: {
value: function() {
return this.length === 0;
}
}
}) It looks like
function defineProperty(c: any, name: string, defaultValue: any) {
var backingFieldName = "_" + name;
Object.defineProperty(
c.prototype,
name,
{
get: function(): any {
if (typeof this[backingFieldName] === 'undefined') {
this[backingFieldName] = defaultValue;
}
return this[backingFieldName];
}, // code continues ...
});
}; This would also be fixed by adding
interface MockJaxOptions {
url?: string;
responseTime?: number;
status?: number;
responseText?: string;
responseHeaders: StringMap<string>;
}
$.mockjax({
type: "GET",
url: "blah/blah/blah",
status: 200,
contentType: "application/json",
response: function() {
this.responseText = JSON.stringify({ stuff: "things" });
// error: property 'responseText' does not exist on '{ type: string ... }'
}
}); This is bad because |
More breaks:
public getEmitOutput(resource: URI, absoluteWorkersResourcePath: string): WinJS.TPromise<Modes.IEmitOutput> { // TODO@Ben technical debt: worker cannot resolve paths absolute
let model = this.resourceService.get(resource);
let cssLinks: string[] = this.cssLinks || [];
// Custom Renderer to fix href in images
let renderer = new Marked.marked.Renderer();
let $this = this;
renderer.image = function(href: string, title: string, text: string): string {
let out = '<img src="' + $this.fixHref(resource, href) + '" alt="' + text + '"';
if (title) {
out += ' title="' + title + '"';
}
out += (this.options && this.options.xhtml) ? '/>' : '>';
~~~~~~~
!!! error TS2339: Property 'options' does not exist on type 'MarkedRenderer'.
~~~~~~~
!!! error TS2339: Property 'options' does not exist on type 'MarkedRenderer'. This seems like a good break. I can't find a property named 'options' in
ko.observable.fn['toInt'] = function() {
return parseInt(this());
~~~~~~
!!! error TS2349: Cannot invoke an expression whose type lacks a call signature. Type 'KnockoutObservableFunctions<any>' has no compatible call signatures.
} This seems like an easy fix to knockout's d.ts. It's just that nobody ever noticed that
private setupEvents(xhr: XMLHttpRequest, request: AjaxRequest) {
const progressSubscriber = request.progressSubscriber;
xhr.ontimeout = function xhrTimeout(e) {
const {subscriber, progressSubscriber, request } = (<any>xhrTimeout);
if (progressSubscriber) {
progressSubscriber.error(e);
}
subscriber.error(new AjaxTimeoutError(this, request)); //TODO: Make betterer.
~~~~
!!! error TS2345: Argument of type 'XMLHttpRequestEventTarget' is not assignable to parameter of type 'XMLHttpRequest'.
!!! error TS2345: Property 'onreadystatechange' is missing in type 'XMLHttpRequestEventTarget'.
}; This seems like a good break too. The comment even indicates that the author is suspicious that something is wrong.
export function main() {
describe('es5 decorators', () => {
it('should declare directive class', () => {
var MyDirective = Directive({}).Class({constructor: function() { this.works = true; }});
~~~~~
!!! error TS2339: Property 'works' does not exist on type '{ constructor: () => void; }'.
expect(new MyDirective().works).toEqual(true);
}); This is probably another case where the type of the object literal needs to be intersected with the contextual type from the call signature. But it's in a test. It could just be a completely dynamic use of an undeclared property.
// tree benchmark in React
import {getIntParameter, bindAction} from 'angular2/src/test_lib/benchmark_util';
import * as React from './react.min';
var TreeComponent = React.createClass({
displayName: 'TreeComponent',
render: function() {
var treeNode = this.props.treeNode;
~~~~~
!!! error TS2339: Property 'props' does not exist on type '{ displayName: string; render: () => any; }'. This is evidence that |
I just tried this out and thought this would work:
But it looks like the getter isn't aware of the type of |
@bradenhs Nope, that is not correct. But the code right now specifically works with the contextual type of |
With latest commits we now properly type |
@@ -22,9 +21,6 @@ tests/cases/conformance/types/thisType/thisTypeInAccessorsNegative.ts(16,22): er | |||
} | |||
const contextual: Foo = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you move this test case to thisTypeInAccessors.ts
so that we can see the types?
Latest commit puts all of the new functionality under the |
In the else if (type.flags & TypeFlags.TypeParameter && (type as TypeParameter).isThisType) {
if (inObjectTypeLiteral) {
writer.reportInaccessibleThisError();
}
writer.writeKeyword("this");
} |
@aozgaa No, that is an unrelated check (it reports an error if |
@mhegazy I think this one is good to do. Want to take a look? |
In this scenario, is it possible to infer
|
I haven’t tested it but it should be possible with generics. declare function wrap<T, F extends (this: T) => any>(f: F): F |
Unfortunately that doesn't seem to work without specifying |
With this PR we strongly type
this
in methods of object literals and provide a facility for controlling the type ofthis
through contextual typing. The new behavior is only enabled in--noImplicitThis
mode.The type of the expression
this
in a method of an object literal is now determined as follows:this
parameter,this
has the type of that parameter.this
parameter,this
has the type of that parameter.--noImplicitThis
is enabled and the containing object literal has a contextual type that includes aThisType<T>
,this
has typeT
.--noImplicitThis
is enabled and the containing object literal has a contextual type that doesn't include aThisType<T>
,this
has the contextual type.--noImplicitThis
is enabledthis
has the type of the containing object literal.this
has typeany
.Some examples:
In a similar manner, when
--noImplicitThis
is enabled and a function expression is assigned to a target of the formobj.xxx
orobj[xxx]
, the contextual type forthis
in the function expression isobj
:In cases where an API produces a
this
value by transforming its arguments, a newThisType<T>
marker interface can be used to contextually indicate the transformed type. Specifically, when the contextual type for an object literal isThisType<T>
or an intersection includingThisType<T>
, the type ofthis
within methods of the object literal isT
.In the example above, the
methods
object in the argument tomakeObject
has a contextual type that includesThisType<D & M>
and therefore the type ofthis
in methods within themethods
object is{ x: number, y: number } & { moveBy(dx: number, dy: number): number }
. Notice how the type of themethods
property simultaneously is an inference target and a source for thethis
type in methods.The
ThisType<T>
marker interface is simply an empty interface declared inlib.d.ts
. Beyond being recognized in the contextual type of an object literal, the interface acts like any empty interface.Patterns similar to the above are used in several frameworks, including for example Vue and Ember. Using
ThisType<T>
we can now more accurately describe those frameworks.Supercedes #8382. We revoked that PR because it always made the type of
this
in object literal methods be the type of the object literal. We now make that the default behavior, but allow the default to be overridden using aThisType<T>
contextual type.