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

Regression 5.4: the new conditional type constraint is not working with a type parameter representing union type #57864

Closed
Alexsey opened this issue Mar 20, 2024 · 5 comments
Labels
Working as Intended The behavior described is the intended behavior; this is not a bug

Comments

@Alexsey
Copy link

Alexsey commented Mar 20, 2024

🔎 Search Terms

regression, type parameter, 5.4

🕗 Version & Regression Information

  • This is a crash
  • This changed between versions 5.3.3 and 5.4.2

⏯ Playground Link

https://www.typescriptlang.org/play?ts=5.4.2#code/C4TwDgpgBAggznAlgcwHYCUJwK4BtgA8MATsVBAB7ASoAmcUA9gEYBWEAxsANoC6AfFAC8sUgCgxAeklRuqPLgA0UeQFtmEYsux0IAM0SoItZXGDFDyXsMFzs6zafOXeY0JCgAxRPk1FSygAqjADyqojAgiIkZJTUdAzchnqaUAASEACGJlAAdPnJqYGZPq5QUAD86Vm05FQ09FDBYRFi5eVV3r7EBMU+QaHhkW3tAFyyGdnK+bld1D19Sk2DEfxlUON8ANwS7tAA6ozEANZwAAoWRwCsACwEAMp18Y0s7FxQAD4qClGwCCgYLB4QhzPz3ZTyXC4fj8XbgA5HU7CP5INCYHD4AignqvTjAT7fKEQn78IA

💻 Code

This is the simplified code

type AssignResult<Arr extends object[]> = Arr

// [null, number, undefined, string] => [number, string]
type Filter<Arr, ToOmit> = Arr extends [infer Head, ...infer Tail]
  ? Head extends ToOmit
    ? Filter<Tail, ToOmit>
    : [Head, ...Filter<Tail, ToOmit>]
  : [];

type WorksPrior54<S extends object | null> = AssignResult<Filter<S, null>>

type Works = AssignResult<Filter<object | null, null>>
This is the full code (error message is slightly different, but probably they are the same):
type AssignResult<Arr extends object[]> = Arr extends [...infer Rest extends object[], infer Last]
  ? {
    [K in keyof Last | keyof AssignResult<Rest>]: K extends keyof Last
      ? Last[K]
      : K extends keyof AssignResult<Rest>
        ? AssignResult<Rest>[K]
        : never;
  }
  : object;

type Filter<Arr, ToOmit> = Arr extends [infer Head, ...infer Tail]
  ? Head extends ToOmit
    ? Filter<Tail, ToOmit>
    : [Head, ...Filter<Tail, ToOmit>]
  : [];

declare function assignByDescriptors<
  Target extends object,
  // replace the two lines below to make it work with the this line
  // Sources extends (object | null | undefined)[],
  Source extends object | null | undefined,
  Sources extends Source[],
  Result extends AssignResult<[Target, ...Filter<Sources, null | undefined>]> = AssignResult<[Target, ...Filter<Sources, null | undefined>]>
>(target: Target, ...sources: Sources): Result

🙁 Actual behavior

I would expect to have no error for 5.4, the same as there was no error before it. Especially considering that there were not breaking changes in 5.4 announcement.

As for the simplified code, there should be no difference between how WorksPrior54 and Works.

As for the full version of the code, there should be no difference if part of Sources type parameter is extracted into an intermediate Source type parameter, or not

🙂 Expected behavior

Error (in the simplified version):

TS2344: Type Filter<S, null> does not satisfy the constraint object[]
Type [] | [unknown] is not assignable to type object[]
Type [unknown] is not assignable to type object[]

Error (in the full version):

TS2344: Type
[Target, ...Filter<Sources, null | undefined>]
does not satisfy the constraint object[]
Type
Target | Filter<Sources, null | undefined>[number]
is not assignable to type object
Type Filter<Sources, null | undefined>[number] is not assignable to type object
Type
(Source extends null | undefined ? [] : [Source])[number] | (Source extends null | undefined ? [] : [Source])[number]
is not assignable to type object
Type
(Source extends null | undefined ? [] : [Source])[number]
is not assignable to type object

Additional information about the issue

No response

@DanielRosenwasser
Copy link
Member

DanielRosenwasser commented Mar 20, 2024

I think this is working as intended in TypeScript 5.4:

https://devblogs.microsoft.com/typescript/announcing-typescript-5-4/#more-accurate-conditional-type-constraints

In a conditional type like T extends Foo ? TrueBranch : FalseBranch, where T is generic, the type system would look at the constraint of T, substitute it in for T itself, and decide on either the true or false branch.

But this behavior was inaccurate because it was overly-eager.
Even if the constraint of T isn't assignable to Foo, that doesn't mean that it won't be instantiated with something that is.
And so the more correct behavior is to produce a union type for the constraint of the conditional type in cases where it can't be proven that T never or always extends Foo.

TypeScript 5.4 adopts this more accurate behavior.
What this means in practice is that you may begin to find that some conditional type instances are no longer compatible with their branches.

The way you can get around this is by either

  1. changing the constraint of Arr in AssignResult to unknown[] instead of object[] (playground link),
  2. adding an object constraint to Head and an object[] constraint to Tail (playground link).

@RyanCavanaugh RyanCavanaugh added the Working as Intended The behavior described is the intended behavior; this is not a bug label Mar 20, 2024
@Alexsey
Copy link
Author

Alexsey commented Mar 20, 2024

@DanielRosenwasser Thank you for taking a look, this is probably the change that cause the error!

But here the situation is right the opposite of how it says it should work:
"if the constraint of T isn't assignable to Foo, that doesn't mean that it won't be instantiated with something that is."
-> in the example:
"if the condition of Filter isn't assignable to AssignResult, that doesn't mean that it won't be instantiated with something that is.". And Filter<(object | null)[], null> will be instantiated with object[] - exactly with the type that AssignResult is expecting!
So here we are having a "cases where it can't be proven that T never or always extends Foo", but the compiler is considering that it can be proven

The difference between WorksPrior54 and Works is an additional step we are making the compiler take (in the "full code", the additional step is the intermediate Source type parameter - inlining it is fixing the error). This additional step seems to break the new improved functionality

@Alexsey Alexsey changed the title Regression 5.4: contraints on type parameter are getting lost Regression 5.4: the new conditional type constraint is not working with a type parameter representing union type Mar 20, 2024
@DanielRosenwasser
Copy link
Member

DanielRosenwasser commented Mar 20, 2024

So without having been deeply in the guts of conditional type instantiation, I'm going to do my best on this one!

From the #56004:

A type variable represents any possible type within its constraint. So, given a type parameter T with the constraint C, the constraint of T extends X ? A : B is

  • A when C is known to always extend X,
  • B when C is known to never extend X, or
  • A | B when C possibly extends X.

We previously didn't consider the third possibility.

So previously, Filter would end up saying "S extends object | null, eh? Could that extend [infer Head, ...infer Tail]? Definitely not!"

So we'd always jump into the negative case and end up with []. That satisfies basically every array type, but not every tuple type. Try it out in the 5.3.3 playground.

type Filter<Arr, ToOmit> = Arr extends [infer Head, ...infer Tail]
  ? Head extends ToOmit
    ? Filter<Tail, ToOmit>
    : [Head, ...Filter<Tail, ToOmit>]
  : [];

function f<S extends object | null>(x: Filter<S, null>) {
  const a: object[] = x;
  const b: number[] = x; // WHAT?
  const c: string[] = x; // WHAT?
  
  const d: [] = x; // Okay, that explains it.
  const e: [object] = x; // Yup, this one gets an error - makes sense now.
}

As you can see from the examples, that's not correct though! An object doesn't extend an array, but that doesn't necessarily mean S itself couldn't be an array. In 5.4+, these all error as expected.


inlining it is fixing the error

Not sure exactly what you mean, but it might be related to a tradeoff we consciously made. The new behavior only kicks in on higher-order types (i.e. type parameters). In other words, if you pass in object | null, we don't say "well maybe that object is actually an array type" and try all the branches.

@Alexsey
Copy link
Author

Alexsey commented Mar 20, 2024

@DanielRosenwasser, I'm terribly sorry. I made a typo when simplifying the initial code. Of course, object | null is never an array—it should have been (object | null)[], and this one is working just fine!

I will take another try to simplify the initial code to a short snippet tomorrow. Thank you for your time, I truly appreciate your effort in taking a look at the issue!

@typescript-bot
Copy link
Collaborator

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

@typescript-bot typescript-bot closed this as not planned Won't fix, can't repro, duplicate, stale Mar 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Working as Intended The behavior described is the intended behavior; this is not a bug
Projects
None yet
Development

No branches or pull requests

4 participants