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

Narrowing types by typeof inside switch (true) does not work #37178

Closed
joscha opened this issue Mar 3, 2020 · 20 comments · Fixed by #53681
Closed

Narrowing types by typeof inside switch (true) does not work #37178

joscha opened this issue Mar 3, 2020 · 20 comments · Fixed by #53681
Labels
Awaiting More Feedback This means we'd like to hear from more people who would be helped by this feature Suggestion An idea for TypeScript

Comments

@joscha
Copy link
Contributor

joscha commented Mar 3, 2020

TypeScript Version: 3.8.3 Nightly

Search Terms:

  • switch statement
  • switch (true)

Code

let x: string | undefined;

if (typeof x !== 'undefined') {
  x.trim(); // works as expected
}

switch (true) {
  case typeof x !== 'undefined':
    x.trim(); // thinks x is potentially undefined
}

Expected behavior:

Narrowing types should work inside the case.

Actual behavior:

Any assertion in case seems to be disregarded

Playground Link: https://www.typescriptlang.org/play/index.html?ts=Nightly#code/DYUwLgBAHgXBDOYBOBLAdgcwgHwgVzQBMQAzdEQgbgChqUSIAKMATwAcQB7BqCAQgC8AiAHICxMmgoiAlBADe1CNAB0yFAFtGMyhAD0eiAHdOSANbwIAQ0sgoHAMZgK1AL614RlGAcALJsh4IHKKyg42IBCsHNzQ-EKi4qTkhCIwSsqq6lo6+oZgvugWcSiWbJzOaGAoVsDALPhEyVKEbkA

Related Issues:

@nmain
Copy link

nmain commented Mar 3, 2020

switch(true)

Never seen code like that before 😄. If you rewrite the if statement to be roughly equivalent to the switch statement, you get the same failure:

let x: string | undefined;

if (true === (typeof x !== 'undefined')) {
  x.trim(); // fails
}

Which makes this a duplicate of... well, I couldn't find an existing ticket. But I'm reasonably sure narrowing === true and === false patterns was rejected before because it adds perf costs for an uncommonly used idiom.

@joscha
Copy link
Contributor Author

joscha commented Mar 3, 2020

Not arguing at all that the shape is a bit odd, but if there are many variants for what you're switching over then it is more readable than a bunch of if/else if.

@DanielRosenwasser DanielRosenwasser changed the title Narrowing types inside switch statement does not work Narrowing types by typeof inside switch (true) does not work Mar 3, 2020
@DanielRosenwasser
Copy link
Member

DanielRosenwasser commented Mar 3, 2020

The crux of this was filed at #8934 discussed a bit at #2214, but I think the overall guidance is to write something along the lines of

switch (typeof foo) {
  case "number":
    // ...
    break;
  case "string":
    // ...
    break;
  default:
    // not undefined
}

In your case it would be the following:

switch (typeof foo) {
  case "undefined":
    break;
  default:
    // not undefined
}

@joscha
Copy link
Contributor Author

joscha commented Mar 4, 2020

Thanks @DanielRosenwasser - maybe I shouldn't have simplified the example so much :)

Switching over the type is not equivalent in case you have multiple possible matches that are staggered based on the value, e.g.:

switch(true) {
  case typeof process.env.X !== 'undefined':
    // we favor env var X over Y if it is defined
    break;
  case typeof process.env.Y !== 'undefined':
   // we fall back to Y if X is not defined
   break;
}

This is not possible by switching over the type only, it really needs the actual value.

Shall I close this issue as something that will never be fixed/implemented due to performance issues for an outlier case?

@RyanCavanaugh RyanCavanaugh added Awaiting More Feedback This means we'd like to hear from more people who would be helped by this feature Suggestion An idea for TypeScript labels Mar 4, 2020
@RyanCavanaugh
Copy link
Member

What's the advantage of

switch(true) {
  case typeof process.env.X !== 'undefined':
    // we favor env var X over Y if it is defined
    break;
  case typeof process.env.Y !== 'undefined':
   // we fall back to Y if X is not defined
   break;
}

over

if (typeof process.env.X !== 'undefined') {
    // we favor env var X over Y if it is defined
} else if (typeof process.env.Y !== 'undefined') {
   // we fall back to Y if X is not defined
}

?

@joscha
Copy link
Contributor Author

joscha commented Mar 4, 2020

Two come to mind:
• readability for big amounts of cases
• being able to use fall-throughs for overlapping cases

Having said that I understand it is an edge case. I came across this pattern in a bigger codebase and wanted to make a change. That's when I noticed that the types are not narrowed in each case.

@anatoliyarkhipov
Copy link

The third advantage is that in some cases it tastes as poor man's pattern matching.

@KilianKilmister
Copy link

Has there been any opdate or in progress work on this?

Fourth adwantage is when having to narrow an input which can be a primitive or a specific object type/class instance:

function processInput (input: string | RegExp | SomeType) {
  switch(true) {
    case typeof input === 'string':
      // <code for string....>
      break
    case input instanceof RegExp:
      // <code for regex....>
     break
    case isSomeType(input):
      //  <code for SomeType....>
     break
  }
  // <some more code....>
}

@wavded
Copy link

wavded commented Sep 21, 2020

I also ran into this. Typescript says geocode and zips are possibly undefined even though the case statement rules that out as a possibility.

export default function SearchArea() {
  let geocode = useSelector((s: T.State) => s.geocode)
  let zips = useSelector((s: T.State) => s.zips)
  let crs = useSelector((s: T.State) => s.crs)

  let area = 'NONE'
  switch (true) {
    case Boolean(crs):
      area = 'CUSTOM'
      break
    case Boolean(geocode):
      area = geocode.address //  TS2532: Object is possibly 'undefined'.
      break
    case Boolean(zips):
      area = zips.features  //  TS2532: Object is possibly 'undefined'.
        .slice(0, 5)
        .map((z) => z.properties.zip)
        .join(', ')
      area += zips.features.length > 5 ? ', ...' : ''  //  TS2532: Object is possibly 'undefined'.
  }

  return (
    <>
      <Typography variant="overline">Current search area</Typography>
      <Typography variant="body1" gutterBottom>
        {area}
      </Typography>
    </>
  )
}

@ExE-Boss
Copy link
Contributor

@wavded Your case won’t be fixed by this because of #16655.

@KilianKilmister
Copy link

KilianKilmister commented Sep 22, 2020

@wavded use !! (doublebang) instead. You'll still have the issue of this thread but TS2532: Object is possibly 'undefined'. should be gone

The doublebang is just 2 consecutive NOT operations, the first converts the value to truthy/falsy and inverts it, the second undoes the inversion. eg:

!!'' === false // => true
!!{} === true // => true
!!0 === true // => false

So your switch would look something like this:

  switch (true) {
    case !!crs:
      area = 'CUSTOM'
      break
    case !!geocode:
      area = geocode.address
      break
    case !!zips:
      area = zips.features
        .slice(0, 5)
        .map((z) => z.properties.zip)
        .join(', ')
      area += zips.features.length > 5 ? ', ...' : ''
  }

@wavded
Copy link

wavded commented Sep 22, 2020

@KilianKilmister I get the same error unfortunately.

@KilianKilmister
Copy link

@wavded Oh, my apologies, with the link @ExE-Boss shared i was just thinking about type inference of the Boolean() constructor, and didn't realise that the error would be the same.

So yes, you are having the same problem as mentioned in this issue.

The easiest fix got you would probably be to tell typescript that these values are non-nullable. eg. with the Non-null assertion operator.

  let geocode = useSelector((s: T.State) => s.geocode)! // <- note the added `!`
  let zips = useSelector((s: T.State) => s.zips)!
  let crs = useSelector((s: T.State) => s.crs)!

Check out the docs, they explain it.

I'd still suggest generally using the !! instead of Boolean() though. It's considered the more favourable practice

@polomsky
Copy link

I have similar usecase:

switch (true) {
    case error instanceof CustomError1:
        return error.foo;
    case error instanceof CustomError2:
        return error.bar;
    case error instanceof Error:
        return String(error);
}

And I am using this syntax in javascript because it looks better than chain of if else. Sadly can not use in typescript

@ScreamZ
Copy link

ScreamZ commented Jan 26, 2022

I have similar usecase:

switch (true) {
    case error instanceof CustomError1:
        return error.foo;
    case error instanceof CustomError2:
        return error.bar;
    case error instanceof Error:
        return String(error);
}

And I am using this syntax in javascript because it looks better than chain of if else. Sadly can not use in typescript

Exact same issue, maybe someone has some workaround for this ? @ahejlsberg @sheetalkamat @sandersn

@darcyrush
Copy link

I try not to pollute open issues with fluff like "me too!", but in the related closed thread I was surprised to see this commented as a 'wontfix' as it is not a 'common pattern'. Those comments were in 2016. I find this pattern a very clean and elegant approach that can be reused across an entire codebase to consistently handle very different scenarios.

Anecdotally, I see this pattern more and more in the wild.

@sandersn @RyanCavanaugh Would you still consider this an uncommon pattern today?

@jukibom
Copy link

jukibom commented Dec 29, 2022

Anecdotally, I see this pattern more and more in the wild.

Best argument I've heard for switch(true) is that sometimes you need to execute only one of lots of possible things and it most succinctly conveys that. While you can just else if a bunch of times, you have to read all the way through to make sure there's no missing else etc.

It's not often I need it but when I do then edge cases like this are a real shame.

@Thijs-van-Drongelen
Copy link

We have a connection broker with lots of different protocols, so our connection class has alot of different configuration types.

When a new connection is submitted by a user, the backend needs to know what connection handler needs to configure the connection. Here is an example:

image

Please tell me if there is a better way, But rn I'm thinking I'd like this feature

@epicmet
Copy link

epicmet commented Aug 19, 2023

Any updates on this issue?

@zeroarst
Copy link

zeroarst commented Sep 13, 2023

In Kotlin, when expression is much better in terms of intuition and usability.
https://kotlinlang.org/docs/control-flow.html#when-expression

when (x) {
    1 -> print("x == 1")
    2 -> print("x == 2")
    else -> {
        print("x is neither 1 nor 2")
    }
}

enum class Color {
    RED, GREEN, BLUE
}

when (getColor()) {
    Color.RED -> println("red")
    Color.GREEN -> println("green")
    Color.BLUE -> println("blue")
    // 'else' is not required because all cases are covered
}

when (getColor()) {
    Color.RED -> println("red") // no branches for GREEN and BLUE
    else -> println("not red") // 'else' is required
}

when {
    x.isOdd() -> print("x is odd")
    y.isEven() -> print("y is even")
    else -> print("x+y is odd")
}

Hoping TS can at least supports what OP mentioned.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Awaiting More Feedback This means we'd like to hear from more people who would be helped by this feature Suggestion An idea for TypeScript
Projects
None yet
Development

Successfully merging a pull request may close this issue.