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

Annotated parameters are not used for inferring rest parameters #58746

Open
hreinhardt opened this issue Jun 3, 2024 · 2 comments
Open

Annotated parameters are not used for inferring rest parameters #58746

hreinhardt opened this issue Jun 3, 2024 · 2 comments
Labels
Help Wanted You can do this Possible Improvement The current behavior isn't wrong, but it's possible to see that it might be better in some cases
Milestone

Comments

@hreinhardt
Copy link

πŸ”Ž Search Terms

annotated, "rest parameter"

πŸ•— Version & Regression Information

  • This is the behavior in every version I tried

⏯ Playground Link

https://www.typescriptlang.org/play/#code/GYVwdgxgLglg9mABAWwNYEFrzAHgJJgAOIUiApgB5RlgAmAzogIZgCeA2gLoB8AFE1gQAuRAG8AUIimJgARiG8AdMor0hBYlACUiALzdEANzgxaAGknTgAJgXLFq9URI79Rk+fEBfHaK-jxNExYBF4JaRlZXgoheigAJxgwAHMzRFYhMBBkACMyeN8vC0spG2i01kKLCIB6GsQKZjp05niyRCTgfLbaZkYAIhZWfurpOvJDGkQoAAs4EGSZmWsOxk7usl6mAeiROMSUipEs3PzXA2NTfu8tAG5xIA

πŸ’» Code

function mkAction<Input extends any[]>(action: {
    f1:(...xs:Input) => void,
    f2:(...xs:Input) => void,
}) {}

mkAction({
    f1(x:string, y:number) {},

    f2(x, y) {},
    // x and y are inferred as "any",
    // even though f2 is inferred as "(x: string, y: number) => void"
});

πŸ™ Actual behavior

Arguments "x" and "y" are inferred as "any", even though "f2" has the correct type (x: string, y: number) => void

πŸ™‚ Expected behavior

I would expect inference of x: string and y: number for the arguments of "f2".

Additional information about the issue

No response

@jcalz
Copy link
Contributor

jcalz commented Jun 3, 2024

related to #47599

@Andarist
Copy link
Contributor

Andarist commented Jun 4, 2024

This is an interesting one. inferFromAnnotatedParameters indeed doesn't infer from rest parameters and it doesn't try to infer into contextual rest parameters. The first thing manifests in another issue:

function mkAction2<Input extends any[]>(action: {
  f1: (...xs: Input) => void;
  f2: (...xs: Input) => void;
}) {}

mkAction2({
  f1(...rest: string[]) {},
  f2(...rest) {
    rest;
    // ^? (parameter) rest: any[]
  },
});

This one feels like a somewhat easy fix and I don't see any extra considerations here.

The second one (the one related to the OP's issue) also wouldn't be particularly hard to implement here. However, this one has some extra considerations. The problem with rest parameters is that we don't know how many items it might actually have. From what I know there is no unification algorithm for such inferred tuples.

function mkAction3<Input extends any[]>(action: {
  f1: (...xs: Input) => void;
  f2?: (...xs: Input) => void;
  f3?: (...xs: Input) => void;
}) {}

mkAction3({
  f1(x: string, y: number) {},
  f2(x: string, y: number, z: string) {},
  f3(x, y, z) {},
});

It would be great if this could infer [x: string, y: number, z: string] but it first infers [x: string, y: number], then infers the second [x: string, y: number, z: string] candidate. When it has two candidates like this it picks up the first one. And with that, it fails the signature applicability check as after instantiation it turns out that f2 should be (x: string, y: number) => void but the provided one expects one extra argument.

There is an extra bug here... as hovering over mkAction3 call shows us a wrong signature:

function mkAction3<any[]>(action: {
    f1: (...xs: any[]) => void;
    f2?: ((...xs: any[]) => void) | undefined;
    f3?: ((...xs: any[]) => void) | undefined;
}): void

This type argument isn't inferred as any[] - it's [x: string, y: number]. If we'd supply any[] manually this call would succeed.

On the other hand... this thing with tuples of different shapes is how it already works today, so I'm not sure if worrying about this is a valid concern here.

One extra interesting thing is that in the original issue the contextual signature has a rest parameter. In such a scenario, the contextual signature gets instantiated using a non-fixing inference mapper. This is different from the case when the contextual signature has no rest parameter since then a fixing inference mapper is used. There is an important distinction between them both, the fixing one is capable of using inferFromIntraExpressionSites while the non-fixing one isn't.

The preceding method in this object is such an intra-expression inference site and if that would get used (by the non-fixing mapper or if we'd switch to the fixing mapper when instantiating that contextual signature) then this would infer correctly too. Inferring from those intra-expression sites is definitely meant to fix inferences so including that in the non-fixing mapper would be incorrect. I'm not sure what would break if this would just switch to using the fixing mapper but the distinction has to matter here, the code is structured in such a way that this is definitely not by mistake.

Either way, I'd probably do changes to inferFromAnnotatedParameters and then try to explore ways to unify contravariant inferences made from rest parameters.

@RyanCavanaugh RyanCavanaugh added Help Wanted You can do this Possible Improvement The current behavior isn't wrong, but it's possible to see that it might be better in some cases labels Jun 14, 2024
@RyanCavanaugh RyanCavanaugh added this to the Backlog milestone Jun 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Help Wanted You can do this Possible Improvement The current behavior isn't wrong, but it's possible to see that it might be better in some cases
Projects
None yet
Development

No branches or pull requests

4 participants