-
Notifications
You must be signed in to change notification settings - Fork 124
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
Narrow set has if true #125
Conversation
It is not really safe:
|
Mmm see your problem is you are mutating 😛 Jokes aside, that's not relevant to this. IterableIterator<["red" | "blue", "red" | "blue"]> Which it's obviously now wrong about. If you declare a |
@CarsonF I agree w/ that assessment. Unfortunately there's a different issue that makes this unsafe. Type predicates also narrow in the false case. Which means this can happen: Playground link const colors = new Set<"red" | "blue">()
const inputColor = 'red' as 'red' | 'blue' | 'green'
if (!colors.has(inputColor)) {
inputColor // unsafe: type is 'green', but value is 'red'
} To fix this, typescript would need to add new language features. Discussed here: microsoft/TypeScript#51678 (comment) |
The example you showed is expected and correct behavior. Issue shown in the link you provided is different. In your example: const colors = new Set(['red', 'blue'] as const); // Set<"red" | "blue">
const inputColor = 'red' as 'red' | 'blue' | 'green'
if (!colors.has(inputColor)) {
inputColor // type is 'green', but value is `never` because code never reaches here.
} The actual issue pointed in the link you shared is: const colors = new Set(["red", "blue"]); // Set<string>
const inputColor = "red" as "red" | "blue" | "green";
if (!colors.has(inputColor)) {
inputColor; // type is `never` while it shouldn't be
} But, this can currently be avoided by checking if the array/set T is a literal or not. And if its not literal, it can not narrow and should just return Like this: interface Set<T> {
has<V>(value: IfTrue<IsLiteral<T>, T | (WidenLiteral<T> & {}), never>): value is T;
has<V>(value: IfTrue<IsLiteral<T>, never, T | (WidenLiteral<T> & {})>): boolean;
} But I would also do further changes and allow interface Set<T> {
has<V>(
value: V & IfTrue<IsLiteral<T>, V extends T ? T | (WidenLiteral<T> & {}) : V, never>,
): value is T;
has<V>(
value: V & IfTrue<IsLiteral<T>, never, V extends T ? T | (WidenLiteral<T> & {}) : V>,
): boolean;
}
// When T is not literal.
{
const colors = new Set(["red", "blue"]);
const inputColor = "red" as "red" | "blue" | "green";
if (!colors.has(inputColor)) {
inputColor; // "red" | "blue" | "green"
// ^?
} else {
inputColor; // "red" | "blue" | "green"
// ^?
}
}
// When T is literal
{
const colors = new Set(["red", "blue"] as const);
const inputColor = "red" as "red" | "blue" | "green";
if (!colors.has(inputColor)) {
inputColor; // "green"
// ^?
} else {
inputColor; // "red" | "blue"
// ^?
}
}
{
const colors = new Set(["red", "blue"] as const);
const inputColor = "red" as unknown;
if (!colors.has(inputColor)) {
inputColor; // unknown
// ^?
} else {
inputColor; // "red" | "blue"
// ^?
}
}
{
const colors = new Set(["red", "blue"] as const);
const inputColor = "red" as "red" | 123;
if (!colors.has(inputColor)) {
inputColor; // 123
// ^?
} else {
inputColor; // "red"
// ^?
}
}
{
const colors = new Set(["red", "blue", 0 as number] as const); // Set<"red" | "blue" | number>
const inputColor = "red" as "red" | 123;
if (!colors.has(inputColor)) {
inputColor; // "red" | 123
// ^?
} else {
inputColor; // "red" | 123
// ^?
}
} Also, as a note, everything we did here for |
@DeepDoge Sorry about that -- the issue is real (and is the same issue I linked to), but my example code didn't properly illustrate it. I've updated my comment and playground link with a proper example. I just changed If you run that updated code, you'll see that |
@russelldavis Oh alright, this makes sense now. |
Closing for reasons described above. Thanks for the discussion, that was really useful. |
If the set includes the item then it's safe to narrow it to the type of the set.