Skip to content

Type narrowing bug in empty interfaces #18196

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

Closed
chancancode opened this issue Sep 1, 2017 · 13 comments · Fixed by #27157
Closed

Type narrowing bug in empty interfaces #18196

chancancode opened this issue Sep 1, 2017 · 13 comments · Fixed by #27157
Assignees
Labels
Bug A bug in TypeScript Fixed A PR has been merged for this issue

Comments

@chancancode
Copy link
Contributor

chancancode commented Sep 1, 2017

Reproduction (turn on strictNullChecks to see the red line)

let x: {} = "";

if (!x) {
  // x is `never` O_O
}
@DanielRosenwasser
Copy link
Member

Is the proposal to always revert back to the declared type in a false branch when narrowing on truthiness?

@DanielRosenwasser
Copy link
Member

Or perhaps to just not narrow in the false branch?

@wycats
Copy link

wycats commented Sep 1, 2017

Maybe there's something we're misunderstanding about {}, but it doesn't seem like simply removing the falsy values from {} should result in never, right?

@chancancode
Copy link
Contributor Author

Is the proposal to always revert back to the declared type in a false branch when narrowing on truthiness?

I think the bug is that {} is incorrectly (IMO) considered "always truthy" which seems wrong to me as you can assign 0 or "" to it just fine.

So today:

let x: string = ...;

if (!x) {
  // x is still `string`
}

let y: number = ...;

if (!y) {
  // y is still `number`
}

To the extent that ^^^ is the current behavior, I think {} should be treated the same, so, yes, I think {} should not be narrowed out in the false branches.

Ideally, since we have literally types now, I think all false branches should be something like originalType & (0 | "" | undefined | null | false), so x in above would become "" and y would become 0, but that probably requires some machinery that is not in the system today.

@mhegazy
Copy link
Contributor

mhegazy commented Sep 5, 2017

Technically you are correct in the case of {}.

Nevertheless, I think a better approach here is to use object | string | number | boolean instead of {} (I am assuming here that is what you originally intended)

@mhegazy mhegazy added the Bug A bug in TypeScript label Sep 5, 2017
@wycats
Copy link

wycats commented Sep 5, 2017

technically correct

@mhegazy at minimum, your union is missing symbol.

const x: object | string | number | boolean = Symbol(); // does not type check
const x: {} = Symbol(); // type checks

It seems problematic for us to need to maintain a union of all of the possible primitive types ourselves.

From my perspective, there are a few different things we might be trying to express with the core types:

  • "indexable" (you are allowed to use [] or .x at runtime)
    • anything other than null or undefined
    • hard to reliably express in TypeScript without enumerating a list of the primitive types
  • "nullish"
    • null | undefined
    • | void?
  • "falsy"
    • null | undefined | false | 0 | ""
  • "truthy"
    • anything not falsy
    • afaict impossible to express in TypeScript

We typically try to wrangle these types when we start new projects in TypeScript, and we started to formalize our own usage in ts-std. Our core.ts file has the types we use most often that aren't already in TypeScript: ts-std/src/core.ts.

We are happy to use whatever unions work to express the most common concepts, and got the idea for ts-std's unknown type from a very helpful discussion with @mhegazy early on in our TypeScript work.

We're happy to use {} to mean "not nullish", or happy to use a sanctioned union to mean that, but we really would like a canonical answer or implementation from the TypeScript team 😄

@mhegazy
Copy link
Contributor

mhegazy commented Sep 5, 2017

symbol is another primitive type. i would say {} is overly general in many cases. if i were starting over i would be more explicit, i.e. type primitive = number | string | boolean | symbol | undefined | null | void | object to mean {} | null | undefined.

I would like to fix this issue too, but will need to see what breaks. i am sure some one somewhere depends on this behavior inadvertently.

@DanielRosenwasser
Copy link
Member

Side note: the list of falsy types needs to include: NaN.

@chancancode
Copy link
Contributor Author

@DanielRosenwasser that is correct. However, as far as I can tell, NaN is not a literal type in TypeScript. Is there a trick?

@mhegazy
Copy link
Contributor

mhegazy commented Sep 7, 2017

NaN is not a type. the value NaN is of type number. and NaN is funny from a typing perspective, so let's not go there right now..

@wycats
Copy link

wycats commented Sep 7, 2017

Sure, let's leave NaN out for the time being.

To recap:

  1. We agree that there is a bug with narrowing in {}. Specifically, just as (! operation) & string = string and (! operation) & number = number, (! operation) & {} should = {}.

  2. @mhegazy proposed that we should just try to avoid using {}, and instead enumerate the types that {} is expressing. However, the list of those types is not permanently bounded. For example, symbol was recently introduced by JavaScript, which now must be included in such a list.

It seems that there are two options:

  1. Fix the bug in {} and say that it means "the union of all not-null, not-undefined types"
  2. Introduce a new built-in type that means "the union of all not-null, not-undefined types" that doesn't have the bug.

It is worth noting that JavaScript is adding a new primitive type (BigInt) that has typeof bigint === 'bigint' and has ToBoolean(0n) === false, so there will be additional types that are subject to this problem over time.

@DanielRosenwasser
Copy link
Member

Hey @chancancode and @wycats, now that unknown exists, I'd like to know whether you still end up running into this scenario.

@ljharb
Copy link
Contributor

ljharb commented Sep 14, 2018

unknown still includes null and undefined, though; so it'd be nice if there was a simple way to refer to object | string | boolean | number | symbol | bigint etc.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug A bug in TypeScript Fixed A PR has been merged for this issue
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants