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

Union type in a generic method is not always properly validated #51587

Closed
kugacz opened this issue Nov 18, 2022 · 4 comments
Closed

Union type in a generic method is not always properly validated #51587

kugacz opened this issue Nov 18, 2022 · 4 comments
Labels
Working as Intended The behavior described is the intended behavior; this is not a bug

Comments

@kugacz
Copy link

kugacz commented Nov 18, 2022

Bug Report

πŸ”Ž Search Terms

union literal generic method

πŸ•— Version & Regression Information

All versions including night build 5.0.0-dev.20221118

⏯ Playground Link

Playground link with relevant code

πŸ’» Code

class Field<DataType> {
    static readonly literalField: Field<'A'|'B'> = new Field<'A'|'B'>();    
    static readonly numberField: Field<1|2> = new Field<1|2>();
    static readonly mixedField: Field<'A'|'B'|1> = new Field<'A'|'B'|1>();    
}

class WrongFieldFilter {    
    filter<DataType>(field: Field<DataType>, value: DataType): void {
        console.log(value);
    }
}

class GoodFieldFilter<DataType> {
    constructor(private field: Field<DataType>) {}

    filter(value: DataType): void {
        console.log(value);
    }
}

// WrongFieldFilter -> this class has wrong behavior

const fieldFilter = new WrongFieldFilter();
fieldFilter.filter(Field.literalField, 'A'); // OK: 'A' is in 'A'|'B' type
fieldFilter.filter(Field.literalField, 'Z'); // !wrong behavior, should raise error: Argument of type 'Z' is not assignable to parameter of type '"A" | "B"'.(2345)
fieldFilter.filter(Field.literalField, 'Z' as const); // !wrong behavior, should raise error: Argument of type 'Z' is not assignable to parameter of type '"A" | "B"'.(2345)
fieldFilter.filter(Field.literalField, 22); // OK, reaises error: Argument of type '22' is not assignable to parameter of type '"A" | "B"'.(2345)


fieldFilter.filter(Field.numberField, 1); // OK: 1 is in 1|2 type
fieldFilter.filter(Field.numberField, 3); // !wrong behavior, should raise error: Argument of type '3' is not assignable to parameter of type '1 | 2'.(2345)
fieldFilter.filter(Field.numberField, 3 as const); // !wrong behavior, should raise error: Argument of type '3' is not assignable to parameter of type '1 | 2'.(2345)
fieldFilter.filter(Field.numberField, 'Z'); // OK, reaises error: Argument of type '"Z"' is not assignable to parameter of type '1 | 2'.(2345)

// if we use mixed union of number and string literals the behavior is correct!

fieldFilter.filter(Field.mixedField, 1); // OK: 1 is in 1|2 type
fieldFilter.filter(Field.mixedField, 3); // OK, reaises error: Argument of type '3' is not assignable to parameter of type '"A" | "B" | 1'.(2345)
fieldFilter.filter(Field.mixedField, 'A'); // OK: 'A' is in 'A'|'B' type
fieldFilter.filter(Field.mixedField, 'Z'); // OK, reaises error: Argument of type '"Z"' is not assignable to parameter of type '"A" | "B" | 1'.(2345)

// GoodFieldFilter -> this slightly different class is an example of good behavior

const fieldFilterLiteral = new GoodFieldFilter(Field.literalField); // we are providing a field in the constructor instead of the method call
fieldFilterLiteral.filter('A'); // OK: 'A' is in 'A'|'B' type
fieldFilterLiteral.filter('Z'); // OK, reaises error: Argument of type '"Z"' is not assignable to parameter of type '"A" | "B"'.(2345)
fieldFilterLiteral.filter(22); // OK, reaises error: Argument of type '22' is not assignable to parameter of type '"A" | "B"'.(2345)

const fieldFilterNumber = new GoodFieldFilter(Field.numberField); // we are providing a field in the constructor instead of the method call
fieldFilterNumber.filter(1); // OK: 1 is in 1|2 type
fieldFilterNumber.filter(3); // OK, reaises error: Argument of type '3' is not assignable to parameter of type '1 | 2'.(2345)
fieldFilterNumber.filter('Z'); // OK, reaises error: Argument of type '"Z"' is not assignable to parameter of type '1 | 2'.(2345)

πŸ™ Actual behavior

When we provide as a generic method parameter a union of literals of the same type (eg. strings) TS doesn't validate parameter values against this union literals, but wider against the literal's type.

πŸ™‚ Expected behavior

When provided a generic method parameter is a union of literals of the same type TS should validate this parameter against union literals and raise 2345 error when the passed parameter doesn't belong to this union.

@MartinJohns
Copy link
Contributor

MartinJohns commented Nov 18, 2022

fieldFilter.filter(Field.literalField, 'Z'); // !wrong behavior, should raise error: Argument of type 'Z' is not assignable to parameter of type '"A" | "B"'.(2345)

It's perfectly valid for the type argument to expand to "A" | "B" | "Z", and Field<"A" | "B"> is compatible with Field<"A" | "B" | "C">. This generic inference only happens with related types, that's why the Field.literalField, 22 errors, but that can be made working as well: fieldFilter.filter<"A" | "B" | 22>(Field.literalField, 22)

This is working as intended.

@RyanCavanaugh RyanCavanaugh added the Working as Intended The behavior described is the intended behavior; this is not a bug label Nov 18, 2022
@RyanCavanaugh
Copy link
Member

There's a secondary issue of Field not using its type argument.

The definition that does what you want is:

class Field<DataType> {
    static readonly literalField: Field<'A'|'B'> = new Field<'A'|'B'>();    
    static readonly numberField: Field<1|2> = new Field<1|2>();
    static readonly mixedField: Field<'A'|'B'|1> = new Field<'A'|'B'|1>();    

    _instance!: DataType;
}

class WrongFieldFilter {    
    filter<T extends Field<unknown>>(field: T, value: T['_instance']): void {
        console.log(value);
    }
}

@kugacz
Copy link
Author

kugacz commented Nov 19, 2022

Thank you both for the clarification.

@RyanCavanaugh I've found your issue describing a similar problem #14829
Using one of the solutions from this topic I can achieve the validation I expect this way:

type NoInfer<T> = [T][T extends any ? 0 : never];

class FieldFilter {    
    filter<DataType>(field: Field<DataType>, value: NoInfer<DataType>): void {
        console.log(value);
    }
}

I've created another option based on the above :

type FieldExactDataType<T> = T extends Field<infer U> ? U : never;

class FieldFilter {    
    filter<T extends Field<any>>(field: T, value: FieldExactDataType<T>): void {
        console.log(value);
    }
}

@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.

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

4 participants