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

Intra expression inference doesn't work within reverse mapped types #53018

Open
Andarist opened this issue Feb 28, 2023 · 6 comments Β· May be fixed by #54029
Open

Intra expression inference doesn't work within reverse mapped types #53018

Andarist opened this issue Feb 28, 2023 · 6 comments Β· May be fixed by #54029
Labels
Experience Enhancement Noncontroversial enhancements Suggestion An idea for TypeScript
Milestone

Comments

@Andarist
Copy link
Contributor

Andarist commented Feb 28, 2023

Bug Report

πŸ”Ž Search Terms

intra expression inference reverse mapped type

πŸ•— Version & Regression Information

  • This is the behavior in every version I tried

⏯ Playground Link

Playground link with relevant code (object variant)
Playground link with relevant code (tuple variant)

πŸ’» Code

declare function f<T>(
  arg: {
    [K in keyof T]: {
      produce: (n: string) => T[K];
      consume: (x: T[K]) => void;
    };
  }
): void;
// Works
f({
  a: {
    produce: () => "hello",
    consume: (x) => x.toLowerCase(),
  },
});
// Works
f({
  a: {
    produce: (n: string) => n,
    consume: (x) => x.toLowerCase(),
  },
});
// still doesn't work, even with "Improved Function Inference in Objects and Methods" aka intra-expression inference sites
f({
  a: {
    produce: (n) => n,
    consume: (x) => x.toLowerCase(),
  },
});
// still doesn't work, even with "Improved Function Inference in Objects and Methods" aka intra-expression inference sites
f({
  a: {
    produce: function () {
      return "hello";
    },
    consume: (x) => x.toLowerCase(),
  },
});
// still doesn't work, even with "Improved Function Inference in Objects and Methods" aka intra-expression inference sites
f({
  a: {
    produce() {
      return "hello";
    },
    consume: (x) => x.toLowerCase(),
  },
});

πŸ™ Actual behavior

This kind of inference that was improved in #48538 doesn't work within a reverse mapped type.

πŸ™‚ Expected behavior

It should work in the same way as it does outside of the reverse mapped type. The code in the playground is almost literally taken from the 4.7 announcement blog post about this improvement (here). The only difference is that the produce/consume "pair" is within a property of reverse mapped type


Part of the problem is likely in that inferReverseMappedType calls inferTypes with separate inferences here:
https://github.dev/microsoft/TypeScript/blob/e2283e99b47942b863d016c65a3e430dca1549b9/src/compiler/checker.ts#L23938-L23939
and that is not linked anyhow to the current inference context and inferFromIntraExpressionSites tries to infer this kind of stuff from context.intraExpressionInferenceSites+context.inferences and the latter won't be present for the computed reverse mapped type inference:
https://github.dev/microsoft/TypeScript/blob/e2283e99b47942b863d016c65a3e430dca1549b9/src/compiler/checker.ts#L23756-L23763

However, I also noticed that inferReverseMappedType is not called at all in this case so those inferences for it aren't even gathered in the first place.

@fatcerberus
Copy link

What is a β€œreverse mapped type”? I looked at the repro code but that just looks like a regular mapped type to me.

@Andarist
Copy link
Contributor Author

Andarist commented Feb 28, 2023

A reverse mapped type is a type parameter that has to be inferred from usage when the usage is a mapped type - at least that's my mental model around this, this definition doesn't have to be 100% precise. So, here - the T is the reversed mapped type (not the mapped type at the argument position). It's called like that because TS has "reverse" the mapped type at the argument position to construct "what this T could look like to satisfy this mapped type?". It's how it's called internally in the codebase - take a look at the inferReverseMappedType.

@RyanCavanaugh RyanCavanaugh added Suggestion An idea for TypeScript Experience Enhancement Noncontroversial enhancements labels Feb 28, 2023
@RyanCavanaugh RyanCavanaugh added this to the Backlog milestone Feb 28, 2023
@Andarist
Copy link
Contributor Author

Andarist commented Mar 1, 2023

So I guess this is related to the umbrella issue here as that is still open. I think though that this one here is more of a bug/unhandled case within the original fix than a new feature request. This umbrella issue stays open because there are still other situations for which the original fix didn't do much but this issue here is very similar to the ones fixed by that PR. I think that those other situations could be put into a "different kind of an expression" category - whereas here we deal with the same types of expressions, they are just contained in a mapped type. Inferring reverse mapped types is implemented sufficiently differently that this still manifests for them.

@bautistaaa
Copy link

throwing in a bug recently discovered that appears to be related: playground
looks like an explicit type annotation is needed to keep inference. further, not using an arrow function appears to break it as well

cc @nickmccurdy

@Andarist
Copy link
Contributor Author

Andarist commented Apr 17, 2023

further, not using an arrow function appears to break it as well

If you mean that it also doesn't work when you use a regular function expression (async function (request: Request) {}) then it's because such a function is also context-sensitive (just like the unannotated arrow). A regular function expression always has an implicit this parameter. That's also why it starts to work again when you annotate that parameter: async function (this: undefined, request: Request) {}

@Nick-Lucas
Copy link

Nick-Lucas commented Apr 22, 2023

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Experience Enhancement Noncontroversial enhancements Suggestion An idea for TypeScript
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants