Skip to content
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

Polymorphic 'this' type #4910

Merged
merged 21 commits into from
Sep 30, 2015
Merged

Polymorphic 'this' type #4910

merged 21 commits into from
Sep 30, 2015

Conversation

ahejlsberg
Copy link
Member

This PR implements polymorphic typing of this for classes and interfaces as inspired by #3694. The new features are:

  • The type of this in an expression within a non-static class or interface member is considered to be an instance of some class that derives from the containing class as opposed to simply an instance of the containing class.
  • The this keyword can be used in a type position within a non-static class or interface member to reference the type of this.
  • When a class or interface is referenced as a type, all occurrences of the this type within the class (including those inherited from base classes) are replaced with the type itself.

This feature makes patterns such as fluent interfaces and covariant return types much easier to express and implement. Languages such as C++, Java, and C# use a somewhat involved generic pattern to emulate this feature, as described here.

An example:

class A {
    foo() {
        return this;
    }
}

class B extends A {
    bar() {
        return this;
    }
}

var b: B;
var x = b.foo().bar();  // Fluent pattern works, type of x is B

In a non-static member of a class or interface, this in a type position refers to the type of this. For example:

class Entity {
    clone(): this {
        // Code to clone Entity
    }
    equals(other: this): boolean {
        // Code to compare Entity with another Entity
    }
}

Each subclass of the above Entity will have a clone method that returns an instance of the subclass, and an equals method that takes another instance of the subclass.

The this type is a subtype of and assignable to the instance type of the containing class or interface, but not vice-versa (because this might actually be a subclass). That is a breaking change, and certain code patterns that previously compiled may now need an extra type annotation:

class A {
    getInstance() {  // Should be getInstance(): A
        return this;
    }
}

class B extends A {
    getInstance() {
        return new B();
    }
}

The example above now errors because the inferred return type of getInstance in A is this and the inferred type of getInstance in B is B, which is not assignable to this. The fix is to add a return type annotation for getInstance in A.

The polymorphic this type is implemented by providing every class and interface with an implied type parameter that is constrained to the containing type itself (except when the compiler can can tell that this is never referenced within the class or interface). That in particular turns a lot of previously non-generic classes into generic equivalents, causing more symbols and types to be created due to generic instantiation. The observed cost in batch compile time ranges from 0% in code that uses no classes to 6-7% in very class-heavy code.

Note that this PR specifically doesn't aim to implement other parts of #3694 such as this type annotations for functions. Those will be covered by other PRs if we choose to implement them.

@ahejlsberg
Copy link
Member Author

@jbondc Agreed, it might be good to show which class a particular this type belongs to in error messages and information produced by the language service.

@ghost
Copy link

ghost commented Sep 22, 2015

👏

Conflicts:
	src/compiler/diagnosticInformationMap.generated.ts
for (let node of baseTypeNodes) {
if (isSupportedExpressionWithTypeArguments(node)) {
let baseSymbol = resolveEntityName(node.expression, SymbolFlags.Type, /*ignoreErrors*/ true);
if (!baseSymbol || !(baseSymbol.flags & SymbolFlags.Interface) || getDeclaredTypeOfClassOrInterface(baseSymbol).thisType) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How does this handle the case where an interface extends a class that uses this ?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If one of the base types is a class (i.e. not an interface) we simply return true (because every class has a this type). We only return false from this function if the interface itself doesn't reference this, if every base type is an interface, and if no base interface has a this type.

@RyanCavanaugh
Copy link
Member

Need to un-comment the fourslash tests that were edited

@ahejlsberg
Copy link
Member Author

@RyanCavanaugh Yes, @mhegazy is looking at fixes for the failing fourslash tests.

@@ -119,6 +145,8 @@ tests/cases/conformance/expressions/thisKeyword/typeOfThis.ts(130,16): error TS1
!!! error TS1056: Accessors are only available when targeting ECMAScript 5 and higher.
var p = this;
var p: MyGenericTestClass<T, U>;
~
!!! error TS2403: Subsequent variable declarations must have the same type. Variable 'p' must be of type 'this', but here has type 'MyGenericTestClass<T, U>'.
p = v;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ideally these should not fail right? But only on instantiation.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, they should fail. The first declaration infers type this for v, the second declaration specifies type MyGenericTestClass<T, U> for v. Those are not identical types, so error.

@DanielRosenwasser
Copy link
Member

@ahejlsberg I spoke with @RyanCavanaugh, @weswigham, and @danquirk about this, and I think there are some issues with the meaning of this in a type literal. Take the following:

interface Thing {
    getResource(): {
        methodA(): this;
        methodB(): this;
    }
}

In the current implementation, this refers to the type assignable to Thing; however, this doesn't accurately reflect the type of this when methodA is invoked off of its returned object. This could be very confusing for users because the intuition is that the method literally returns the expression this.

For either one semantics of the this type, a workaround exists to get the alternative semantics by extracting the resource type into its own interface.

However, if this refers to the enclosing type literal, then an easier fix exists by adding a type parameter to the method:

interface Thing {
    getResource<OwnerT extends Thing>(): {
        a(): OwnerT;
        b(): this;
    }
}

I think that we should go with the semantics that most-closely matches the runtime: in this case, the this type should refer to the type returned by getResource().

@ahejlsberg
Copy link
Member Author

@DanielRosenwasser You're incorrect about the current behavior, your example is actually an error:

interface Thing {
    getResource(): {
        methodA(): this;  // Error
        methodB(): this;  // Error
    }
}

The error reported is 'this' type is available only in a non-static member of a class or interface and I think that is the behavior we want. If at some point we decide to make it possible to reference this of a function or method (perhaps because the function declared a this type parameter as speculated about in #3694) the above would become valid, but for now it should be an error.

In order to reference an outer this in an object type literal you have to introduce a generic type and capture this as an argument:

interface Resource<T> {
    methodA(): T;
    methodB(): T;
}

interface Thing {
    getResource(): Resource<this>;
}

Note, however, that you can capture the outer this in an object literal:

class A {
    foo() {
        return { x: this, f: () => this };
    }
}

In order to write down the type inferred for the above, you'd have to resort to a temporary type:

interface Foo<T> { x: T; f(): T; }

class A {
    foo(): Foo<this> {
        return { x: this, f: () => this };
    }
}

@ahejlsberg
Copy link
Member Author

@jbondc I think the this type and mixins are largely orthogonal issues. I'd take the discussion up on one of the many mixin suggestion threads we already have going on and keep this one focused to the semantics and implementation of the this type.

@DanielRosenwasser
Copy link
Member

your example is actually an error

I actually was asking others about the implementation and it was our understanding that it was allowed; sorry about that. I didn't get the chance to try the branch out yesterday.

Note, however, that you can capture the outer this in an object literal:

class A {
    foo() {
        return { x: this, f: () => this };
    }
}

But the .d.ts emit for that is currently not valid:

declare class A {
    foo(): {
        x: this;
        f: () => this;
    };
}

If I try to reuse that .d.ts file, I get:

foo.d.ts(3,12): error TS2526: 'this' type is available only in a non-static member of a class or interface.
foo.d.ts(4,18): error TS2526: 'this' type is available only in a non-static member of a class or interface.

@ahejlsberg
Copy link
Member Author

@DanielRosenwasser @mhegazy The .d.ts issue is tricky. Personally I think this is one of those situations where we should issue an error if a .d.ts is requested with guidance that a type annotation is required.

return true;
}

// A type is considered independent if it is a built-in type keyword, an array with an element type that is
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe clarify that this is not a built-in type keyword.

@Artazor
Copy link
Contributor

Artazor commented Sep 25, 2015

As I understand, polymorphic this type will greatly simplify the following code:

// interface Cloneable<T extends Cloneable<T>> {
interface Cloneable<T extends Cloneable<any>> {
      clone(): T;
}

turns it into

interface Cloneable {
       clone(): this;
}

It's cool!

However It looks like that it solves only a half of a problem (ok, let it be 90% in common practices)

Am I right, that the system

// interface Vertex<V extends Vertex<V,E>, E extends Edge<V,E>> {
interface Vertex<V extends Vertex<any,any>, E extends Edge<any,any>> {
      incoming: E[];
      outgoing: E[];
}

//interface Edge<V extends Vertex<V,E>, E extends Edge<V,E>> {
interface Edge<V extends Vertex<any,any>, E extends Edge<any,any>> {
     from: V;
     to: V;
}

class City extends Vertex<City, Road> { 
     constructor(public name: string){
          this.incoming = [];
          this.outgoing = [];
     }
}
class Road extends Edge<City, Road> { 
    constructor(public distance: number, public from: City, public to: City) {
           this.from.outgoing.push(this);
           this.to.incoming.push(this);
    }
}

var LA = new City('Los Angeles');
var SF = new City('San Francisco');
var hyperloop = [new Road(558.68, LA, SF), new Road(558.68, SF, LA)]

console.log(hyperloop[0].from.outgoing[0].to.name) // stronly typed

still could not benefit from this type without introducing high order types into TypeScript ?

I could imagine the following implementation

// interface Vertex<E<T> extends Edge<T>>  -or-
interface Vertex<E extends Edge> {
      incomming: E<this>[];
      outgoing: E<this>;
}

// interface Edge<V<T> extends Vertex<T>> -or-
interface Edge<V extends Vertex> {
     from: V<this>;
     to: V<this>;
}

class City extends Vertex<Road> { ... }
class Road extends Edge<City> { ... }

where for high order types the constraint
<G1 extends G2>
could be formulated as

G1<T> extends G2<T> recursively for every T

Am I right? Or there is a simpler solution that I've missed?

@Artazor
Copy link
Contributor

Artazor commented Sep 25, 2015

I mean that this type alone solves only direct type recursion problem. Mutually recursive constraints are still not expressible.
@ahejlsberg ?

@ahejlsberg
Copy link
Member Author

@Artazor Correct, the polymorphic this helps in the simple (and common) case, but the general solution likely requires higher-order types of some form since the dependent types can't otherwise refer to each other's this.

@vilicvane
Copy link
Contributor

🎉 👍

@mitjap
Copy link

mitjap commented Oct 26, 2015

should this also support this syntax?

interface A<T> { }
interface B<T extends A<T>> { }

@mhegazy
Copy link
Contributor

mhegazy commented Oct 26, 2015

@mitjap: see #2225

@mjohnsonengr
Copy link

This seems to cause the following issue. In the example below in the getProperty() method, style is assigned to this. This used to be able to infer the type Style, but the latest code including this pull request does not, and the assignment to style at the end of the while loop throws an error, "Type 'Style' is not assignable to type 'this'.

Is this expected?

class Style {
    public property: number;
    public basedOn: string;

    /** if property is null, get from basedOn */
    public getProperty(): number {
        var style = this; // used to infer type of Style
        while (style !== null) {
            if (style.property)
                return style.property;
            // get style referenced by basedOn
            style = style.basedOn === null ? null : getStyle(style.basedOn);
        }
        return null;
    }
}

@RyanCavanaugh
Copy link
Member

@mjohnsonengr this is the intended behavior. If there were a subclass of Style, the result of getStyle would be unassignable to a variable of type this (i.e. attempting to assign a Style to a SuperCoolStyle during a polymorphic invocation of getProperty).

I'd recommend writing var style: Style = this; - that will solve the issue

@mjohnsonengr
Copy link

Nevermind, saw the referenced issue, #5056

@mjohnsonengr
Copy link

Ahh, but thank your for your reply @RyanCavanaugh! :)

@aalpgiray
Copy link

aalpgiray commented Sep 7, 2016

so what should I do to use this instead of state below

function NavigatableRecordCreator(defaultValues: { [key: string]: any; }, name?: string) {
    abstract class NavigatableRecord<P extends NavigatableRecord<P>> extends Record(defaultValues, name) {
        SetValue<T>(fn: (x: NavigableObject<P>) => NavigableObject<T>, value: T, state: P) {
            return this.setIn(fn(new NavigableObject<P>(state)).getPath(), value) // can I use this instead of state here
        }
    }
    return NavigatableRecord;
}

image

@shelby3
Copy link

shelby3 commented Sep 10, 2016

@ahejlsberg wrote:

Each subclass of the above Entity will have a clone method that returns an instance of the subclass, and an equals method that takes another instance of the subclass.

The this type is a subtype of and assignable to the instance type of the containing class or interface, but not vice-versa (because this might actually be a subclass).

As I wrote in #3694, how can polymorphic clone(): this be implemented if it is not legal to assign an instance of the subclass to this??? It seems instead the compiler will need to deduce that inheritance from Entity types clone() as an abstract method even though the base has an implementation, so as to insure that each subclass provides a unique implementation.

@microsoft microsoft locked and limited conversation to collaborators Jun 19, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Breaking Change Would introduce errors in existing code
Projects
None yet
Development

Successfully merging this pull request may close these issues.