Nightly - `typeof x == "function"` does not always work as typeguard to remove a Record type from a union

[phryneas]

Bug Report

🔎 Search Terms

• typeof function type guard

🕗 Version & Regression Information

• This changed between versions 4.7 and 4.8 dev

⏯ Playground Link

Minimal reproduction

💻 Code

export function configureStore<S extends object>(
  reducer: (() => void) | Record<keyof S, () => void>
) {
  let rootReducer: () => void

  if (typeof reducer === 'function') {
    rootReducer = reducer
/*
Type '(() => void) | (Record<keyof S, () => void> & Function)' is not assignable to type '() => void'.
  Type 'Record<keyof S, () => void> & Function' is not assignable to type '() => void'.
    Type 'Record<keyof S, () => void> & Function' provides no match for the signature '(): void'.(2322)
*/
  }
}

🙁 Actual behavior

Here the reducer is a union between a function and a Record with a yet-unresolved key generic. In previous versions, the if (typeof reducer === 'function') { would have narrowed the type down to the function type, but now it ends up with the function type OR Record<keyof S, () => void> & Function and cannot assign the result to the variable.

🙂 Expected behavior

No error. Our record is not a function.

[Andarist]

This is a fallout from https://github.com/microsoft/TypeScript/pull/49119

If I understand correctly... while annoying, this might be considered a bug fix in TS since functions might be assignable to Records sometimes:

declare function compatCheck<S extends object>(reducer: Record<keyof S, () => void>): void
compatCheck(() => {}) // it's ok in 4.7 (and prior versions)

And if we assume that this is OK (since it "always" has been) then in fact the typeof reducer === 'function' shouldn't discard the Record-based member of the union.

This doesn't exactly match the concrete variant of the same thing but with a concrete variant we can, somewhat, see why a function is not assignable to a Record:

declare function compatCheck2(reducer: Record<string, () => void>): void
// Argument of type '() => void' is not assignable to parameter of type 'Record<string, () => void>'.
//   Index signature for type 'string' is missing in type '() => void'.
compatCheck2(() => {})

The error here isn't really about a different "type" (function vs object) but rather about the lack of the appropriate index signature. So we can get back to our generic context example and check out what got inferred there and why a function was accepted there... 🥁

declare function compatCheck<S extends object>(reducer: Record<keyof S, () => void>): void

// inferred: function compatCheck<object>(reducer: Record<never, () => void>): void
compatCheck(() => {})

I personally know how annoying this can be - the last time I checked there was no actual way to restrict a generic to be of a POJO-like type 😢

[Andarist]

Ok, so I dug a little bit deeper and the change originates in the changed return statement of the narrowTypeByTypeof: https://github.dev/microsoft/TypeScript/blob/51b346d65a7d5e474a40113f72dc66106715a47a/src/compiler/checker.ts#L25102

-return getTypeWithFacts(assumeTrue && impliedType ? mapType(type, narrowUnionMemberByTypeof(impliedType)) : type, facts);
+return getTypeWithFacts(assumeTrue && impliedType ? narrowTypeByImpliedType(type, impliedType) : type, facts);

As we can see a different function is used here to narrow based on the impliedType.

As it turns out, previously narrowUnionMemberByTypeof was simply leaving the input type (Record<keyof S, () => void>) as-is because no if statement was satisfied here: https://github.dev/microsoft/TypeScript/blob/ba4525202e8e647cd71b87c7711ad0c4de8348ee/src/compiler/checker.ts#L25119-L25133 However, right now an intersection is created at the end of the narrowTypeByImpliedType: https://github.dev/microsoft/TypeScript/blob/51b346d65a7d5e474a40113f72dc66106715a47a/src/compiler/checker.ts#L25188

So we end up with a type that looks like this: Record<keyof S, () => void> & Function.

And later on getTypeWithFacts is not able to filter out this particular element of the union because it skips over the code block that was filtering it before: https://github.dev/microsoft/TypeScript/blob/51b346d65a7d5e474a40113f72dc66106715a47a/src/compiler/checker.ts#L23573-L23582

It skips over it because this element is no longer an object type - it's an intersection, and thus it gets to the branch responsible for computing type facts for intersections: https://github.dev/microsoft/TypeScript/blob/51b346d65a7d5e474a40113f72dc66106715a47a/src/compiler/checker.ts#L23605-L23610

And this intersection is not filtered out because an intersection with Function will always include a TypeofEQFunction fact (as Function includes that): https://github.dev/microsoft/TypeScript/blob/51b346d65a7d5e474a40113f72dc66106715a47a/src/compiler/checker.ts#L72

And in fact, such a weirdo is allowed in nature:

declare const test: Record<string, number> & ((a: number) => {})

test(42)
// @ts-expect-error
test('foo')

test.someString = 100
// @ts-expect-error
test.anotherString = 'bar'

So... while this is quite annoying because of how subtypes and intersections work. It feels "correct" to me. It's not super pragmatic and intuitive for users... but well 🤷‍♂️

[phryneas]

I get that the weirdo is theoretically possible, but then you'd have problems with situations like

declare const x: { a: string } | { b: number }
if ('a' in x) {
  // narrowed down to `{ a: string }`
}

just as well - even there could be a { a: string, b: number }.

The point here is that this only happens when S is unresolved.

It would happen with

export function configureStore<K extends string>(
  reducer: (() => void) | Record<K, () => void>
) {
  let rootReducer: () => void

  if (typeof reducer === 'function') {
    // error!
    rootReducer = reducer
  }
}

but not with

export function configureStore(
  reducer: (() => void) | Record<string, () => void>
) {
  let rootReducer: () => void

  if (typeof reducer === 'function') {
    // no error!
    rootReducer = reducer
  }
}

I assume that this is because due to the "unresolved nature", K in the first example could also be never which would mean that Record<never, any> & Function would fall back to Function but I'm not sure?

[Andarist]

Ye, I was also testing this exact case with concrete parameters yesterday and noticed the diff. It seems that I didn't end up posting it though :sweat_smile:

I'm also not saying that this behavior should be preserved - just saying that it's not completely incorrect here. I don't have a good sense of where correctness and pragmatism intersect when it comes to TS because it's very much an undefined line.

The concrete case is filtered out here: https://github.dev/microsoft/TypeScript/blob/51b346d65a7d5e474a40113f72dc66106715a47a/src/compiler/checker.ts#L25179-L25181 It seems that concrete Record<string, () => void> and Function are just not comparable and thus never gets returned, so this element gets removed from the union.

It's worth noting that this happens before the generic case was filtered in TS@<=4.7. The core of the difference is that previously the generic record was left "as is" for further processing/filtering whereas the new version creates an intersection with Function.

I myself would probably be in favor of changing the logic around this at the expense of lost "correctness" here.

[phryneas]

In the end I also don't know the right call (although I would like current behaviour to be preserved). I just noticed a not-obviously-correct change in type checking and reported it :)

Let's see what the team says about it ^^

[fatcerberus]

And in fact, such a weirdo is allowed in nature

JS in a nutshell. 🤣

[RyanCavanaugh]

This is definitely correct behavior, but I can see how it's annoying in this case. That said, class constructors are a really big problem here since they are also typeof "function". Simplified

export function configureStore(reducer: (() => void) | { n?: string }) {
  let rootReducer: () => void

  if (typeof reducer === 'function') {
    rootReducer = reducer
    rootReducer()
  }
}

class M {
  static n = "hello";
  constructor(s: string) {
    s.toLowerCase();
  }
}

// Throws
configureStore(M);

A common suggestion is to explicitly ban functions from appearing in the non-function branch of the union:

export function configureStore<S extends object>(
  reducer: (() => void) | (Record<keyof S, () => void> & { bind?: never })
) {

This makes the narrowing work as expected, and prevents common function-object hybrids like classes from appearing (in non-aliasing cases)

[phryneas]

That hack will probably not help in our case because I'm sure someone has a slice called bind in their Redux store, which would make it breaking from our side, but we'll get around it with a type assertion to restore past behaviour (the error is just in internal code), so no harm done on our side :)

[weswigham]

call or any other method on Function also works. :)

[phryneas]

@weswigham I'm pretty sure every method on Function is already taken by a property someone's Redux store, so like you can't just add any new keyword we can't just forbid a property name - but as I said, in our case it's just a nuisance :)

[weswigham]

so like you can't just add any new keyword we can't just forbid a property name

Sure you can! There are infinite unique inaccessible property names at your disposal:

declare const __reduxFunctionBrand: unique symbol;

declare global {
  interface Function {
    [__reduxFunctionBrand]: {};
  }
}

export function configureStore<S extends object>(
  reducer: (() => void) | (Record<keyof S, () => void> & { [__reduxFunctionBrand]?: never })
) {
  let rootReducer: () => void

  if (typeof reducer === 'function') {
    rootReducer = reducer
    rootReducer()
  }
}

class M {
  static n = "hello";
  constructor(s: string) {
    s.toLowerCase();
  }
}

// Throws
configureStore(M);

[phryneas]

That would become part of our external interfaces at some point... wouldn't that leak and cause all kinds of problems?

Or, if not, the other way round: wouldn't it make more sense if TS itself shipped a property that allowed identifying functions like that directly before every other library adds another property to Function?

[markerikson]

FWIW, while I think I sorta-kinda follow the "well sure, any function could also be a random Bag O' Record Fields" train of thought... I have to say that as a TS user and a lib maintainer who's trying to write TS code, having this break feels very unintuitive and annoying :(

I'm not particularly bothered by the fact that there was a breakage. That happens, this is a nightly build, that's why we have tests against next, etc. No harm no foul there.

But given that this has worked up until now, and that for the general JS user population a function and an object are generally two different things, having if (typeof maybeSomeFunctionOrObject === 'function') no longer narrow properly is disappointing.

So, it would be nice if we could get that working again as it was, even if in the theoretical correctness case it's not quite as accurate to the ways you can abuse a function.

[Andarist]

I think I would more appreciate the answer like "we've fixed an issue, look at those unsound things" than proposing those workarounds. I mean... it's kinda neat that you can do it but at the same time I can't really see those as viable solutions for problems like this. It's overly verbose and requires a deeper understanding of TS than most probably have. I also don't know how one is supposed to even learn about patterns like this as they are only "hidden" in the GitHub comments, SO answers etc. If this is the official answer then I think that such a recommendation should be put somewhere in the docs.

I know that you are put in a somewhat very unfortunate position - trying to balance the dynamic nature of JavaScript with correctness. It's an almost impossible job to provide both the correct and easy-to-use language here and I appreciate all the hard work that you are doing. It feels though that such recommendations are coming from an academic perspective and they are not really pragmatic for the day-to-day use.

I just find this case to be especially surprising because it works differently from the concrete case and we can just list a lot of unsound examples that are OK for the concrete case. Like the one that you have mentioned, or like this one:

export function configureStore2(reducer: ((a: string) => void) | Record<string, string>) {
  let rootReducer: (a: string) => void

  if (typeof reducer === 'function') {
    rootReducer = reducer // OK
    rootReducer('test')
  }
}

declare interface Fn {
  (a: number): void // incompatible signature
  [i: string]: string
}

declare const fn: Fn

// it has a matching index signature so it gets accepted
configureStore2(fn);

So my main question is - why is this one OK and why the reported case should error if they are both unsound? Is maintaining correctness for the reported case worth it if the 100% soundness was never TypeScript's goal?

[fatcerberus]

for the general JS user population a function and an object are generally two different things

I'll note that I often use classes containing only static members as singletons (probably more often than I should), in which case typeof StaticClass === 'function' is true, despite that calling StaticClass() is a runtime error and not something I'd ever want to happen. To be fair, I'm probably not going to use such an object in place of a Record, but I remain suspicious of any code that tries to distinguish between objects and functions due to the existence of this pattern.

[typescript-bot]

This issue has been marked 'Working as Intended' and has seen no recent activity. It has been automatically closed for house-keeping purposes.

[ahejlsberg]

I'm reopening this as a bug with a fix in #49422. We've seen similar breaking changes in other code bases and there's really no particularly good rationale for changing behavior here.