microsoft / TypeScript

TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
https://www.typescriptlang.org
Apache License 2.0
100.84k stars 12.46k forks source link

Private field check narrows generic class too far #46668

Open molisani opened 2 years ago

molisani commented 2 years ago

Bug Report

Discovered a potential issue with https://github.com/microsoft/TypeScript/pull/44648 in regards to how classes with type parameters are handled. The presence of the brand indicates that the object is an instance of this class, but doesn't say anything about the type arguments. For instanceof the current behavior is to pass any for all type arguments (see https://github.com/microsoft/TypeScript/issues/17473 for that issue) but the default here should be that the type matches the constraint for that corresponding type parameter.

πŸ”Ž Search Terms

ts4.5-beta, private fields, brand checks, generic, type parameter, narrowing

πŸ•— Version & Regression Information

TypeScript Version: ts4.5-beta

⏯ Playground Link

Playground link with relevant code

πŸ’» Code

type Constraint = { foo: string } | { bar: number };

class MyClass<T extends Constraint> {
    #brand = true;
    value: T;
    constructor(value: T) {
        this.value = value;
    }

    copyValueFrom(x: object) {
        if (#brand in x) {
            this.value = x.value; 
        }
    }
}

const wrappedFoo = new MyClass({ foo: "foo" });
const wrappedBar = new MyClass({ bar: 123 });
wrappedFoo.copyValueFrom(wrappedBar);
console.log(wrappedFoo.value.foo); // type expects foo, value is bar

πŸ™ Actual behavior

Assignment in copyValueFrom proceeded without error, results in mismatch between type and value.

πŸ™‚ Expected behavior

In copyValueFrom, expected brand to narrow x to MyClass<Constraint> and report error Type 'Constraint' is not assignable to type 'T'. when assigning this.value = x.value.

acutmore commented 2 years ago

Is there anything we can do to also make writing in the other direction safer?

type Constraint = { foo: string } | { bar: number };

class MyClass<T extends Constraint> {
    #brand = true;
    value: T;
    constructor(value: T) {
        this.value = value;
    }

    writeValueTo(x: object) {
        if (#brand in x) {
            // if x.value is `Constraint`, so we can still do an unsafe write
            x.value = this.value; 
        }
    }
}

const wrappedFoo = new MyClass({ foo: "foo" });
const wrappedBar = new MyClass({ bar: 123 });
wrappedFoo.writeValueTo(wrappedBar);
console.log(wrappedBar.value); // type expects bar, value is foo

https://www.typescriptlang.org/play?ts=4.5.0-beta#code/C4TwDgpgBAwg9gOwM7AE4EMCWDhQLxQDeUAZnHAFxQqrYDmUAvlAD5FQBG6qVCArgFsOEVEwDcAWABQ0gCYQAxgBtu0AG7coALyp8EAawRwA7gkkypy9EiRQAsiBgqbAHgAqUCAA9gEBLNt4ZDQsHAA+ImkoaKgAYg4Mf3woND4IcxioDSU0qjcMmIVEGj4FYDhUAAps3Kg3AEpIqUzM4AALTCQAOhroAl6C6MZpKJjjWl8ANXQciDc4Sq8qBydrJBcgmlDgMMbCUZaoLx6ZtOT2zpPZsSgAeluoGaUTWz0kdBJoccxfA6YR5pjCYQaazeYAfT0hhMCEWy0cznWUKMpl27D+mWOvXOHW6AzuDxQmCUSkeJJMEFkf2GFky3ympzmcHBCAgahEcPsCLWLlZ7NQaP2gMO9ygxkQAHJcE8TCk2tAwHBfDhME8QFA3h8vsCADScPi4drqRlQTpQIy4VAQNWPc1skQYmJYk0EC54xk3akAmnSIrBMUYMCQWQAMXIyVZxi5qxslWIZEoUAARAmk0x6uY-SgA+gg5SAEKaAiR6OIuOcbhUACMACYAMzp8zjXPBsNwLr0kGM+aVZt52SF1AZ33FOBKCBdZ50XuB4ODq5pDMElLgaDeSBlWwSrioCV67FmiUJiVQIA