Compiler missing "deep" type errors - 4.5.5 regression

[mmkal]

Bug Report

When dealing with a fairly large type produced by a generic, differences in types can lead to what should be compile-time errors being ignored. Example (see last // @ts-expect-error).

🔎 Search Terms

A hard thing to search for, but I tried "deep" and "false negative" and didn't see anything.

🕗 Version & Regression Information

• This changed between versions 4.4.4 and 4.5.5
• Still broken on current nightly

⏯ Playground Link

Playground link with relevant code

💻 Code

type DeepBrand<T> = T extends string | number | boolean | symbol | bigint | null | undefined | void
  ? {
      t: 'primitive'
      value: T
    }
  : T extends new (...args: any[]) => any
  ? {
      t: 'constructor'
      params: ConstructorParameters<T>
      instance: DeepBrand<InstanceType<Extract<T, new (...args: any) => any>>>
    }
  : T extends (...args: infer P) => infer R // avoid functions with different params/return values matching
  ? {
      t: 'function'
      this: DeepBrand<ThisParameterType<T>>
      params: DeepBrand<P>
      return: DeepBrand<R>
    }
  : T extends any[]
  ? {
      t: 'array'
      items: {[K in keyof T]: DeepBrand<T[K]>}
    }
  : {
      t: 'object'
      properties: {[K in keyof T]: DeepBrand<T[K]>}
      stringKeys: Extract<keyof T, string>
      numberKeys: Extract<keyof T, number>
      symbolKeys: Extract<keyof T, symbol>
    }

// @ts-expect-error string vs void
const t1: DeepBrand<() => void> = {} as DeepBrand<() => string>

// @ts-expect-error string vs void
const t2: DeepBrand<() => () => void> = {} as DeepBrand<() => () => string>

// @ts-expect-error string vs void (should error, doesn't)
const t3: DeepBrand<() => () => () => void> = {} as DeepBrand<() => () => () => string>

🙁 Actual behavior

The compiler failed to noticed the difference between void and string, when it's sufficiently "deep" in the expanded object.

🙂 Expected behavior

The compiler should have an error on the last line of the snippet.

[andrewbranch]

@typescript-bot bisect good v4.4.4 bad v4.5.5

[typescript-bot]

The change between v4.4.4 and v4.5.5 occurred at bf6d164.

[andrewbranch]

That seems highly unlikely, @typescript-bot

[fatcerberus]

Pretty sure I’ve yet to see the bot’s bisect find the correct commit. How ironic that the thing meant to find bugs is buggy. 🤣

[andrewbranch]

The issue with the bisect was that the culprit commit was cherry-picked from main to release-4.5, and I only look at the timeline of commits going into main. I’m far from a git expert, but (warning: likely butchering terminology ahead) git bisect gets mad if you give it a pair like v4.4.4 and v4.5.5 because those tags exist on separate branches off of main that never reunite with main. To account for that, I actually run the bisect between the merge-base of the respective inputs and main, so I’m actually bisecting between the point where release-4.4 and release-4.5 branched off of main. But in hindsight I think I only need to take the merge-base of the old ref. There still might be a problem if the behavior is different between the old ref and its merge base with main, though. I think that should be less common.

The cause: #46974 / #46599

And FWIW, the bisect usually works, you just coincidentally only notice the ones that are particularly embarrassing 😛

[andrewbranch]

Reading the description on #46599, it seems like this is expected behavior.

[mmkal]

Reading the description on #46599, it seems like this is expected behavior.

@andrewbranch @RyanCavanaugh I get that the compiler might give up on extremely deep generated types (I'm not sure I followed that thread fully, or at least that I identified the part that said this is expected behaviour) but shouldn't it "give up" by erroring, rather than saying "everything's fine" which seems dangerous?

[RyanCavanaugh]

It's actually quite common to hit the depth limiter in sound code that can't be written any other way.

[papb]

@RyanCavanaugh but what about incorrect code that reaches the depth limit?

I would suggest changing the default behavior of depth-limiting to be an error (if it's necessary to resolve to something, it could resolve to never or to a throw-type once it's added). People that wrote sound code that reaches the depth limit should be able to suppress the problem with // @ts-expect-error for example (since, in this case, the fact that the code is sound is already dependent on human analysis anyway).

[RyanCavanaugh]

I would suggest changing the default behavior of depth-limiting to be an error

We tried that, of course. The places where it's practical (from a user perspective) to error already do error, such as Type_instantiation_is_excessively_deep_and_possibly_infinite.

These types of things are very common in unsuspicious code that doesn't appear to have any circularities, e.g.

     function f3<T>(x: Partial<T>[keyof T], y: NonNullable<Partial<T>[keyof T]>) {
         x = y;

If someone wants to send a PR for something that only errors in suspicious circumstances without incurring a big perf hit, we're welcome to it, but right now this is all one code path and we can't "fix" the OP without breaking the above snippet.

People that wrote sound code that reaches the depth limit should be able to suppress the problem

Issue an error where, though? The "starting point" for the instigating line of code for of a depth limiter hit that starts with a type comparison is effectively nondeterministic.