Skip to content

Polymorphic this[K] inside a class #41495

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

Closed
falahati opened this issue Nov 11, 2020 · 4 comments
Closed

Polymorphic this[K] inside a class #41495

falahati opened this issue Nov 11, 2020 · 4 comments
Labels
Working as Intended The behavior described is the intended behavior; this is not a bug

Comments

@falahati
Copy link

falahati commented Nov 11, 2020

Essentially it seems that the typescript can not resolve the type of a property when it is trying to do a [K in keyof T]: T[K] operation. I don't know the name of this. lookup? in any case, T[K] for T = this is always this[K] and is never resolved further inside of the class. It works great when used from the outside of the class.

TypeScript Version: 4.1.0-dev.20201101

Search Terms: "Polymorphic this" "this generic"

Code
Consider the following code:

type NonFunctionPropertyNames<T> = {
    // tslint:disable-next-line:ban-types
    [K in keyof T]: T[K] extends Function ? never : K;
}[keyof T];

class B {
    public test: number = 0;

    public bar() {
        var isThis = this.clone();
        isThis.test = 2; // WORKS, since `this` is resolved to `B`

        this.change({ test: 10 }); // ERROR, can't assign number to type `this["test"]`
        this.onChange(["test"]); // ERROR, can't validate "test" as a valid property name
    }
    
    public change(map: Partial<this>) {
        // do something
    }

    public onChange(fields: Array<NonFunctionPropertyNames<this>>) {
        // do something
    }

    public clone(): this {
        return this;
    }
}

const b = new B();
b.change({ test: 10 }); // works!!
b.onChange(["test"]); // works!!

Expected behavior:
For it to work regardless of the code being inside the class or outside of it.

Actual behavior:
See code.

Playground Link: typescriptlang.org with inheritance, my actual problem

Related Issues: This issue might have something to do with #39204 or #22934.

@falahati falahati changed the title Polymorphic this[K] inside a child class Polymorphic this[K] inside a class Nov 11, 2020
@RyanCavanaugh RyanCavanaugh added the Working as Intended The behavior described is the intended behavior; this is not a bug label Nov 13, 2020
@RyanCavanaugh
Copy link
Member

If you write code inside the class in terms of this, it's assumed that you're writing it that way on purpose to provide correct behavior in the presence of subclasses (otherwise you would just write B instead of this). The "incorrect" errors are correct identificiations of places where you would break a subclass with subtyped properties:

class B extends A {    
    public test: number = 0;
    public obj: object = { };

    public bar() {
        var isThis = this.clone();
        isThis.test = 2; // WORKS, since `this` is resolved to `B`

        // Violates C's contract
        this.change({ test: 10 });
        // Violates C's contract
        this.onChange(["obj"]);
    }
}

// Legal extension of B
class C extends B {
    test: 5 = 5;
    obj: Function = () => { };
}

Once you're outside the class, you're allowed to assume you're not aliasing and get back types resolved against the type of the reference (since otherwise you'd never be able to do anything ever).

@falahati
Copy link
Author

falahati commented Nov 13, 2020

You argument has some merits; however I dont think it explain the baheier fully. At least it didn't to me. Consider this:

type NonFunctionPropertyNames<T> = {
    // tslint:disable-next-line:ban-types
    [K in keyof T]: T[K] extends Function ? never : K;
}[keyof T];

class A {
    public change(map: Partial<this>) {
        // do something
    }

    public onChange(fields: Array<NonFunctionPropertyNames<this>>) {
        // do something
    }

    public clone(): this {
        return this;
    }
}

class B extends A {    
    public test: number = 0;
    public obj: object = { };

    public bar() {
        var isThis = this.clone();
        isThis.test = 2; // WORKS, since `this` is resolved to `B`; but should it? since it's type might be changed later

        this.change({ test: 10 }); // ERROR, can't assign number to type `this["test"]`
        this.onChange(["test"]); // ERROR, can't validate "test" as a valid property name
    }
}

class C extends B {
    public test: 5 = 5;
    public obj: Function = () => { };    
}

const b = new B();
b.change({ test: 10 }); // works as it should!!
b.onChange(["test"]); // works as it should!!

const c: B = new C();
c.change({ test: 10 }); // works; but it shouldn't if we follow the same logic
c.onChange(["test"]); // works as it should!!

There are following problems if I accept the reason provided here for this behavior:

  1. isThis.test = 2; in B.bar() also shouldn't work. Since test might get overridden by a new subtype.
  2. this.onChange(["obj"]); should work anyway, since the name of the property will be valid regardless of its type. If this behavior arises from the fact that we are checking for function properties via T[K] extends Function ? never : K, why removing the check still results in the same error anyway?
  3. c.change({ test: 10 }); works for c: B = new C();; if what you describe is the reason for error, this should also fail, in theory!

Breaking a subclass should be a developer's choice, and they should accept the responsibility of doing so. If it was up to me, I would not even allow that, but this is JS at the end of the day, so it's understandable. But now it is affecting a very legal and common use of this. In the current form, the use of the this is greatly limited to return types as it seems.

So I understand if you can't fix 1 and I don't expect anything to change with 3 since we are explicitly casting the object to B expecting a valid implementation from C. But for number 2, this seems to be a bug.

@typescript-bot
Copy link
Collaborator

This issue has been marked 'Working as Intended' and has seen no recent activity. It has been automatically closed for house-keeping purposes.

@RyanCavanaugh
Copy link
Member

The current behavior arises from a set of consciously-chosen trade-offs. We definitely could have made other trade-offs, as you describe, but we didn't - that isn't a bug, just a difference in opinion.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Working as Intended The behavior described is the intended behavior; this is not a bug
Projects
None yet
Development

No branches or pull requests

3 participants