microsoft / TypeScript

TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
https://www.typescriptlang.org
Apache License 2.0
100.07k stars 12.37k forks source link

Annotated parameters are not used for inferring rest parameters #58746

Open hreinhardt opened 3 months ago

hreinhardt commented 3 months ago

πŸ”Ž Search Terms

annotated, "rest parameter"

πŸ•— Version & Regression Information

⏯ 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 commented 3 months ago

related to #47599

Andarist commented 3 months ago

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.