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

Narrow set has if true #125

Closed
wants to merge 1 commit into from
Closed

Narrow set has if true #125

wants to merge 1 commit into from

Conversation

CarsonF
Copy link

@CarsonF CarsonF commented Mar 25, 2023

If the set includes the item then it's safe to narrow it to the type of the set.

const colors = new Set(['red', 'blue'] as const);
const inputColor = 'red' as string;
if (colors.has(inputColor)) {
  inputColor // => 'red' | 'blue'
}

@funtaps
Copy link

funtaps commented Apr 6, 2023

It is not really safe:

const colors = new Set(['red', 'blue'] as const);
const f = (s: Set<string>) => {
  s.add("asd");
}
f(colors);
console.log(colors.entries());

@CarsonF
Copy link
Author

CarsonF commented Apr 6, 2023

Mmm see your problem is you are mutating 😛

Jokes aside, that's not relevant to this.
TS thinks entries() returns

IterableIterator<["red" | "blue", "red" | "blue"]>

Which it's obviously now wrong about.
The fact that you can skate around some of the type safety is well known with TS.
TS tries to be pragmatically safe not absolutely safe.

If you declare a Set of X though and then check to see if Y is within it, you can confirm that Y is in X.
I believe this narrowing is safe.

@russelldavis
Copy link

russelldavis commented Feb 5, 2024

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

@DeepDoge
Copy link

DeepDoge commented Feb 5, 2024

@russelldavis

const colors = new Set(['red', 'blue'] as const);
const inputColor = 'red' as 'red' | 'blue' | 'green'
if (!colors.has(inputColor)) {
  inputColor // unsafe: type is 'green', but value is 'red'
}

The example you showed is expected and correct behavior. Issue shown in the link you provided is different.

In your example:
If inputColor can be "red", "blue" or "green" and if its not "red" or "blue", then it's "green".
In that case based on the runtime logic, code never reaches inside of the if statement.
If it can reach there then inputColor clearly not "red" or "blue".
So "green" is the correct type.

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:
When array/set T is not a literal, value is can remove the whole string type from value.

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 boolean, you can do this with overloads.

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 unknown as has() argument too.
(Which was the main point of my issue #180)

Playground

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 has() can also be copy/pasted for delete() too, because delete also returns true if it can find the value it's trying to delete.

@russelldavis
Copy link

russelldavis commented Feb 5, 2024

@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 new Set(['red', 'blue'] as const) to new Set<"red" | "blue">(). With that change, the set has the same type as before, but is now empty.

If you run that updated code, you'll see that inputColor has the type green but a value of red, and the line does get reached. The one case where this problem doesn't occur is if the set contains every value in the union (which happened to be true in the original example, but won't always be true). This is also talked about in the linked comment in the typescript repo.

@DeepDoge
Copy link

DeepDoge commented Feb 5, 2024

@russelldavis Oh alright, this makes sense now. Set accepts "red" | "blue" but empty, which makes the type to be wrong, true.

@mattpocock
Copy link
Owner

Closing for reasons described above. Thanks for the discussion, that was really useful.

@mattpocock mattpocock closed this May 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants