Using rest destructing on class objects seems to include getters in the resulting type

[divillysausages]

Bug Report

If you have a class along the lines of:

class User {
    constructor(
        public uid: number,
        public password: string // sensitive info
    ) { }
	
    get score(): number {
        return 10; // example code - imagine having to fetch something a little more complex
    }
}

If we then want to destructure a User object (e.g. returning a client-friendly basic object from a server), we first declare a type:

type UserClient = Omit<User, 'password' | 'score'>;

We pass score to the Omit here because, as we're using destructing, only basic properties will be returned, not any getters. We can then create our method:

toClient(): UserClient {
    const { password, ...client } = this;
    return client;
}

However, score is something that we want to return, so we change our UserClient type to:

type UserClient = Omit<User, 'password' | 'score'> | { score: number };

and our toClient() method now becomes:

toClient(): UserClient {
    const { password, ...client } = this;
    return {
        score: this.score,
        ...client
    }
}

This, however, gives the error 'score' is specified more than once, so this usage will be overwritten, even though when we destructure an object, it doesn't give us the getters - it seems like the inferred type from this line:

const { password, ...client } = this;

is wrong (specifically, it's Omit<this, 'password'> for client). In order to fix it, I need to change that line to

const { password, score, ...client } = this;

This doesn't really have any effect on the underlying JavaScript code, so it's a minor bug (I think)

🔎 Search Terms

• rest
• destructuring

🕗 Version & Regression Information

I'm currently on 4.4.3 though running in TypeScript Playground, 3.9.7 is when it first seems to show up.

In that version, the inferred type is Pick<this, Exclude<keyof this, "password">>

In 3.8.3 the inferred type is the same, though the error isn't present, so there must have been a change to the underlying Pick code (perhaps to include getters?)

• This changed between versions 3.8.3 and 3.9.7

⏯ Playground Link

https://www.typescriptlang.org/play?ts=4.4.4#code/MYGwhgzhAECqEFMBO0DeAoAkMA9gOwgBckBXYQnJACi0wAcSAjEAS2GhJYBMAuaPEgFtGyADS0GzNtDqQIAd0q9oRJCzwBzaAHptKhARaEWANwTR1AMxxYAlGmgBfLFg0JCK3EgRVbfAcLIaLTehCRIeNAAjAAMANw6eggAHmCCdCDmuFzmALQWgmAa6uYAFmAm6loU0JbuwKUqOILupVXQYNCshISZ0IKUWc0ZKVjO6NCT0BQAwqwGhL588MhzLAvBU1vQuAQeqDJyikhcotAAdJeg63gejtAAvNNtEHET25Oh4ZEYHx8QXgQfEIL3OAMG4j+20u52uC3eH2cW2czkIAE86OYVkg1hsngB5QRGAA82LOAHJZFBjlxydAAD7Qcng7zkgB8DIcLKB-CEIhQjjiQA

💻 Code

class User {
    constructor(
        public uid: number,
        public password: string // sensitive info
    ) { }
	
    get score(): number {
        return 10; // example code - imagine having to fetch something a little more complex
    }

    toClient(): UserClient {
        const { password, ...client } = this;
        return {
            score: this.score, // ERROR: 'score' is specified more than once, so this usage will be overwritten
            ...client
        }
    }
}
type UserClient = Omit<User, 'password' | 'score'> | { score: number };

🙁 Actual behavior

The inferred type on client seems to include getters

🙂 Expected behavior

The inferred type on client to only include basic properties as it's created through destructuring

[whzx5byb]

Workaround: Use the specified class type instead of the polymorphic this type

toClient() {
    const { password, ...client } = this as User 
    return {
        score: this.score, // OK
        ...client
    }
}

[divillysausages]

@whzx5byb Can you explain that a bit, please? Why is this not already considered a User inside it's own class method?

[fatcerberus]

Why is this not already considered a User inside it's own class method?

this is considered generic, i.e. this extends User, because your method may be called through a subclass.

[jakebailey]

Thanks for giving a version range where this stopped working; I was able to bisect this to #37192.

[jakebailey]

Hm, that PR is actually just what expanded out the error message; the core issue that rest destructuring this doesn't omit properties seems to have existed for much longer (potentially always).

Here's a modified example that shows this behavior:

class User {
	constructor(
		public uid: number,
		public password: string // sensitive info
	) { }
	
	get score(): number {
		return 10; // example code - imagine having to fetch something a little more complex
	}

    toClient(): { uid: number; score?: never } {
        const { password: _, ...client } = this;
        return client; // Type 'this["score"]' is not assignable to type 'undefined'.
    }

    toClient2(): { uid: number; score?: never } {
        const { password: _, ...client } = this as User;
        return client; // OK
    }
}

https://www.typescriptlang.org/play?ts=4.5.2#code/MYGwhgzhAECqEFMBO0DeAoAkMA9gOwgBckBXYQnJACi0wAcSAjEAS2GhJYBMAuaPEgFtGyADS0GzNtDqQIAd0q9oRJCzwBzaAHptKhARaEWANwTR1AMxxYAlGmgBfLFg0JCK3EgRVbfAcLIaLTehCRIeNAAjAAMANw6eggAHmCCdCDmuFzmALQWgmAa6uYAFmAm6loU0JbuwKUqOILupVXQYNCshISZ0IKUWc0ZKVjO6NCT0BQAwqwGhL58qBzc-kIiSAkQXggA-P4IZiiOwVPn0LgEHiuyUIpIygD6otAAdB+gLAtO0AC80zaEDiEwuk1C4UiXwWIPO43Os3meEIACYlg5OMoApttrsDvwjkFThgwZMrkQHHcFEo+C93p8kR5TgDCECOjB4MhYaSIRFLozuZNnM4gA

With the desired behavior being that both toClient and toClient2 have no errors, but as User is the only one that works consistently.

[jakebailey]

I was able to "fix" the original case by not using Omit on this when destructuring, however one can also destructure a value of type T extends SomeClassWithPropGetter and get the same thing. The extended test case is:

class A {
    constructor(public foo: string) {}

    get bar(): number {
        return 1;
    }

    func() {
        const {           ...rest1 } = this;
        const {           ...rest2 } = this as A;
        const { foo: _f1, ...rest3 } = this;
        const { foo: _f2, ...rest4 } = this as A;

        // Rest destructuring drops properties provided by getters.
        // "bar" should not be present in any of these.
        rest1.bar;
        rest2.bar;
        rest3.bar;
        rest4.bar;
    }
}

function destructure<T extends A>(x: T) {
    const {           ...rest1 } = x;
    const {           ...rest2 } = x as A;
    const { foo: _f1, ...rest3 } = x;
    const { foo: _f2, ...rest4 } = x as A;

    // Rest destructuring drops properties provided by getters.
    // "bar" should not be present in any of these.
    rest1.bar;
    rest2.bar;
    rest3.bar;
    rest4.bar;
}

Playground Link

Where all 8 accesses to bar should be errored.

[divillysausages]

Hi @jakebailey ,

Thanks for looking at this, and getting a fix so quickly.

To be clear, in your last Playground link, it'll error on all 8 access to bar in a future TypeScript version? In v4.5.2, it only errors out on the 4 as A lines for me

[jakebailey]

That's what my PR does, yes. You can try out my PR here: Playground Link

There's unfortunately a little more nuance that my PR needs to handle better, namely, it should probably be legal to destructure non-public properties when it's this of the current class being unpacked; mine disallows this but probably only because I'm reusing the code that previously only ran on the non-generic case, where you're only considering the public interface of a type.

I'll make sure we discuss it before merging.

[jakebailey]

Per #47078 (comment), dropping non-public seems correct, so the behavior I'm showing is the final one.

[divillysausages]

Thanks again, @jakebailey and @RyanCavanaugh for the quick turnaround! 👍

[bergus]

as we're using destructing, only basic properties will be returned, not any getters.

This seems like an unsafe assumption. Whether a property is copied into a rest pattern depends on whether it is an own property, not on whether a property is a getter or a method. As far as I understand, TypeScript doesn't distinguish own from inherited properties?