Not filtering return type using generic in generic async function

[syi0808]

Please verify that:

• There is an associated issue in the Backlog milestone (required)
• Code is up-to-date with the main branch
• You've successfully run hereby runtests locally
• There are new or updated unit tests validating the change

This issue created by https://github.com/microsoft/TypeScript/pull/51196/files#diff-d9ab6589e714c71e657f601cf30ff51dfc607fc98419bf72e04f6b0fa92cc4b8R29780-R29792

Fixes #59281

[syi0808]

@microsoft-github-policy-service agree

[Andarist]

I'm not convinced this is a proper fix for the reported issue or that the reported issue is an actual issue. See my comment here on how the type could be rewritten to satisfy your presented constraints.

I'm worried that this is a fix fitting the presented test case but it's a bit coincidental that it works. There is a subtle interplay in how both the current signature and the function flag contribute to contextually typing this return expression.

On top of that, overload selection suffers from a lack of #57421 . The problem is that the inner call expression receives the contextual type computed based on the first overload (Promise<never>) and its inference result gets cached so it gets carried over to the attempt of matching the second overloads. We can even verify that this PR resolves your issue: TS playground

The exact same caching happened before #51196 , it's just that back then a different type (non-filtered) impacted the inner inference. The non-filtered type during the first overload resolution was Partial<Record<keyof M, any>> but since we can always return T | Promise<T> from async functions that was "wrapped" into Partial<Record<keyof M, any>> | Promise<Partial<Record<keyof M, any>>>. This in turn, had a positive effect on the inner inference and a better type managed to be cached by it.

Note that in both cases (before #51196 and after it) the first overload fails to be matched. It's not even meant to be matched. The only difference is in what is cached by the inner call and what gets carried over to when the second overload gets matched.

I think that there are 3 possible solutions here:

• do nothing, it doesn't have to be treated as an issue - the reported code can be rewritten to better match how TS thinks about overloads
• find a way to reject the first overload before it gets to inferring the inner call happening in its context. This would prevent it from caching the wrong type
• assume that it's fine to propagate something than nothing when this filter filters everything out. I'm not sold on if that's a right approach conceptually but it feels like it shouldn't introduce adverse effects and that it might help some cases in practice. To achieve that, a code like this should be used:

                if (returnType.flags & TypeFlags.Never) {
                    return returnType;
                }
                const filtered = filterType(returnType, t => {
                    return !!(t.flags & (TypeFlags.AnyOrUnknown | TypeFlags.Void | TypeFlags.InstantiableNonPrimitive)) || !!getAwaitedTypeOfPromise(t);
                return filtered.flags & TypeFlags.Never ? returnType : filtered;

[syi0808]

Thank you for your kind and detailed review. I agree with you. I'm new to TypeScript contribution and just got fixed by trying indiscriminately on where the problem occurred. I couldn't predict any side effects on this either, just relied on testing.

First of all, I tried "T | Promise" and it gave me the wrong type when it wasn't "async function".

Then, I think it's okay to change the order of override you suggested. I think it's a measure to temporarily solve the problem we're facing now.

But I'm still questioning the underlying issue. I'd like to try to fix this because it's a bug that originated from a specific version, not the intended behavior.

I'm a junior developer and wanted to experience a big open source ecosystem. Is it okay to try to solve this problem more based on the method I suggested?

[Andarist]

just got fixed by trying indiscriminately on where the problem occurred. I couldn't predict any side effects on this either, just relied on testing.

This is already a nice achievement on its own 👍 😃

First of all, I tried "T | Promise" and it gave me the wrong type when it wasn't "async function".

Could you showcase that situation? It's hard for me to comment on this if I don't see the exact code that reported the error.

But I'm still questioning the underlying issue. I'd like to try to fix this because it's a bug that originated from a specific version, not the intended behavior.

Note that sometimes good results are achieved by bugs/imprecise behaviors. Even when something changes in some specific version of TypeScript for a less favorable outcome it doesn't always mean that it's a bug in that new version.

I'm a junior developer and wanted to experience a big open source ecosystem.

Keep up digging ❤️ You are off to a great start if you are no afraid of a complex codebase like this 😉

Is it okay to try to solve this problem more based on the method I suggested?

I'm not sure which is the method you refer to here. Do you refer to the current patch?

[syi0808]

Could you showcase that situation? It's hard for me to comment on this if I don't see the exact code that reported the error.

Sure. Weird types are provided, as shown in the screenshot below. (ex. it's not async function but they provide autocomplete promise methods.)

Is it okay to try to solve this problem more based on the method I suggested?

I'm going to research more of the two ways (excepted first way) you told me.

[syi0808]

Note that sometimes good results are achieved by bugs/imprecise behaviors. Even when something changes in some specific version of TypeScript for a less favorable outcome it doesn't always mean that it's a bug in that new version.

Does this mean that maybe the action is okay? I'm curious about your opinion that you know more context than I do.

[Andarist]

Sure. Weird types are provided, as shown in the screenshot below. (ex. it's not async function but they provide autocomplete promise methods.)

I'd say that this is a different issue. I created a new issue about it here: #59298

I'm going to research more of the two ways (excepted first way) you told me.

Cool. That makes sense.

[syi0808]

I've been thinking about it, but I'm not sure about the second way.

The type of importOriginal that comes over to parameter in the current problem is correct. TS playground
However, the same type of error occurs only when used in the return syntax. That is why I want to narrow down the cause with the function 'getContextualReturnType'.

The second method seems to be already working on a new design, and the impact is large, and there are cases against it, so I'm going to revise it to the third method you suggested.

And if the direction for this issue is set, i will also research the issue of the new 'promise union type'.

Additional: However, it will be solved by changing the order of the override, so it must be related to the second way you told me.