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

Using type alias for record prevents compiler from inferring generic function argument correctly #54000

Closed
yonigibbs opened this issue Apr 24, 2023 · 9 comments Β· May be fixed by #54006
Closed
Labels
Not a Defect This behavior is one of several equally-correct options

Comments

@yonigibbs
Copy link

yonigibbs commented Apr 24, 2023

Bug Report

πŸ”Ž Search Terms

Record, type alias, infer, parameter, generic

πŸ•— Version & Regression Information

  • This won't compile
  • This changed between versions 4.2.3 and 4.3.5

⏯ Playground Link

Playground link with relevant code

πŸ’» Code

function foo<T>(record: Record<string, T>, entity: T) { }

type StringArrayRecord = Record<string, string[]>;

function test() {
    const working: Record<string, string[]> = {};
    foo(working, []); // Compiles

    const broken: StringArrayRecord = {};
    foo(broken, []); // Does not compile
}

πŸ™ Actual behavior

When using a type alias (StringArrayRecord in the example above), I cannot pass [] into function foo as the compiler can't infer that this is meant to be an array of strings. But if instead of the type alias I use the full Record definition, it does seem to infer it correctly, so I can pass in [].

πŸ™‚ Expected behavior

I'd expect the same behaviour in both cases. i.e. whether I use the type alias or the Record declaration I'd expect it either to compile or not compile (ideally compile of course). The issue in my opinion is that the introduction of a type alias seems to stop the compiler being able to infer the type.

Asked originally on StackOverflow: https://stackoverflow.com/questions/76090127/typescript-fails-to-infer-type-correctly-when-type-alias-used. There's a bit more info there showing that this doesn't seem to be related to the fact that the generic type parameter is an array.

Thank you!

@Andarist
Copy link
Contributor

Andarist commented Apr 24, 2023

My educated guess is that:

  • with a matching alias you go through inferFromTypeArguments and it nicely infers from the first argument
  • when a different alias is used a "naked" T used for the second parameter is preferred when choosing the inference candidate and the empty's array type is never[]

This experiment PR seems a little bit related to this kind of a problem: #47898

@typescript-bot
Copy link
Collaborator

The change between origin/release-4.2 and origin/release-4.3 occurred at 89a171e.

@Andarist
Copy link
Contributor

Based on the bisected commit... this works 🀣:

function foo<T>(record: { [key: string]: T }, entity: T) {}

type StringArrayRecord = { [key: string]: string[] };

function test() {
  const working: { [key: string]: string[] } = {};
  foo(working, []); // compiles

  const broken: StringArrayRecord = {};
  foo(broken, []); // this also compiles
}

TS playground

@RyanCavanaugh
Copy link
Member

Annotated

function test() {
    const working: Record<string, number[]> = {};
    // Candidates:
    // - Same-alias-instantiation match Record<string, T> and Record<string, number[]>, recur ->
    //   - Bare match between Record<string, T> and Record<string, number[]>
    // - Bare match from T and never[]
    // ===
    // ---> Two equal bare matches, subtype reduce between never[] and number[], T = number[]
    foo(working, []); // Compiles

    const broken: StringArrayRecord = {};
    // Candidates:
    // - Mapped type inference from StringArrayRecord and Record<string, number[]>
    // - Bare match from T and never[]
    // ===
    // -----> Bare match wins on priority grounds, T = never[]
    foo(broken, []); // Does not compile
}

If I had to pick one to be "wrong" here, it would be the first one. An inference from T<X> to T<Y> that doesn't take into account anything except the variance of T's type parameter means we're not preserving the same priority information as we would in a structural expansion of both sides.

Given the choice of having these be different or equally annoying, I think the former is preferable absent more concrete use cases.

The other option available would be to be a bit smarter about how to deal with [], but I'm not really sure that's the intended crux of the example. Without a motivating use case besides "are different" it's hard to say.

@RyanCavanaugh RyanCavanaugh added the Not a Defect This behavior is one of several equally-correct options label Apr 24, 2023
@jcalz
Copy link
Contributor

jcalz commented Apr 24, 2023

Just to recap... when we match Record<string, T> to Record<string, string[]>, the compiler can shortcut the comparison and just use variance markers on Record, while when you match Record<string, T> to StringArrayRecord, the compiler can't do that. So that's the source of the difference in behavior, and the particulars after that are interesting but neither of them is "incorrect". Is that a fair summary?

@RyanCavanaugh
Copy link
Member

πŸ’―

@yonigibbs
Copy link
Author

Thanks for looking at this so promptly, folks! Really impressive! I agree that being smarter about [] isn't really the issue here. I picked [] just as a simple example. In fact how I first came across this was in a union type:

type StringOrNumberRecord = Record<string, string | number>;

function test() {
    const working: Record<string, string | number> = {};
    foo(working, "string"); // Compiles

    const broken: StringOrNumberRecord = {};
    foo(broken, "string"); // Does not compile
}

I worked around the issue in our codebase by simply replacing the use of a bunch of the type aliases with the actual Record declaration. It wasn't too painful to do that in our particular example. So if the decision here is just accept the difference (rather than be "equally annoying") I think that's totally fair.

Thanks again!

@Andarist
Copy link
Contributor

Andarist commented Apr 25, 2023

As an experiment, I've opened a PR that avoids assigning this lower inference priority for certain mapped types (with primitive type parameter constraints): #54006

@github-actions
Copy link

github-actions bot commented Jun 8, 2023

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

@github-actions github-actions bot closed this as not planned Won't fix, can't repro, duplicate, stale Jun 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Not a Defect This behavior is one of several equally-correct options
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants