Skip to content

Not filtering return type using generic in generic async function #59290

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

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 7 additions & 1 deletion src/compiler/checker.ts
Original file line number Diff line number Diff line change
Expand Up @@ -31300,16 +31300,22 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker {
const signature = getContextualSignatureForFunctionLikeDeclaration(functionDecl as FunctionExpression);
if (signature && !isResolvingReturnTypeOfSignature(signature)) {
const returnType = getReturnTypeOfSignature(signature);

if (returnType.flags & TypeFlags.Never) {
return returnType;
}

const functionFlags = getFunctionFlags(functionDecl);
if (functionFlags & FunctionFlags.Generator) {
return filterType(returnType, t => {
return !!(t.flags & (TypeFlags.AnyOrUnknown | TypeFlags.Void | TypeFlags.InstantiableNonPrimitive)) || checkGeneratorInstantiationAssignabilityToReturnType(t, functionFlags, /*errorNode*/ undefined);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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;

Copy link
Contributor Author

@syi0808 syi0808 Jul 16, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.)
스크린샷 2024-07-16 오전 12 48 54

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@syi0808 syi0808 Jul 16, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

});
}
if (functionFlags & FunctionFlags.Async) {
return filterType(returnType, t => {
const filtered = filterType(returnType, t => {
return !!(t.flags & (TypeFlags.AnyOrUnknown | TypeFlags.Void | TypeFlags.InstantiableNonPrimitive)) || !!getAwaitedTypeOfPromise(t);
});
return filtered.flags & TypeFlags.Never ? returnType : filtered;
}
return returnType;
}
Expand Down
45 changes: 45 additions & 0 deletions tests/baselines/reference/returnTypeWithGenericAsyncFunction.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
//// [tests/cases/compiler/returnTypeWithGenericAsyncFunction.ts] ////

//// [index.ts]
type MockFactoryWithHelper<M = unknown> = (
importOriginal: <T extends M = M>() => Promise<T>
) => Partial<Record<keyof M, any>>;
type PromiseMockFactoryWithHelper<M = unknown> = (
importOriginal: <T extends M = M>() => Promise<T>
) => Promise<Partial<Record<keyof M, any>>>;

const util: {
mock<T>(module: Promise<T>, factory?: MockFactoryWithHelper<T>): void;
mock<T>(module: Promise<T>, factory?: PromiseMockFactoryWithHelper<T>): void;
} = {
mock: (() => {}) as any,
};

util.mock(import("pkg"), async (importOriginal) => ({
...(await importOriginal()),
}));

//// [import.d.ts]
export interface ImportInterface {}
//// [require.d.ts]
export interface RequireInterface {}
//// [package.json]
{
"name": "pkg",
"version": "0.0.1",
"exports": {
"import": "./import.js",
"require": "./require.js"
}
}


//// [index.js]
"use strict";
Object.defineProperty(exports, "__esModule", { value: true });
const util = {
mock: (() => { }),
};
util.mock(import("pkg"), async (importOriginal) => ({
...(await importOriginal()),
}));
Original file line number Diff line number Diff line change
@@ -0,0 +1,87 @@
//// [tests/cases/compiler/returnTypeWithGenericAsyncFunction.ts] ////

=== /index.ts ===
type MockFactoryWithHelper<M = unknown> = (
>MockFactoryWithHelper : Symbol(MockFactoryWithHelper, Decl(index.ts, 0, 0))
>M : Symbol(M, Decl(index.ts, 0, 27))

importOriginal: <T extends M = M>() => Promise<T>
>importOriginal : Symbol(importOriginal, Decl(index.ts, 0, 43))
>T : Symbol(T, Decl(index.ts, 1, 19))
>M : Symbol(M, Decl(index.ts, 0, 27))
>M : Symbol(M, Decl(index.ts, 0, 27))
>Promise : Symbol(Promise, Decl(lib.es5.d.ts, --, --), Decl(lib.es2015.iterable.d.ts, --, --), Decl(lib.es2015.promise.d.ts, --, --), Decl(lib.es2015.symbol.wellknown.d.ts, --, --), Decl(lib.es2018.promise.d.ts, --, --))
>T : Symbol(T, Decl(index.ts, 1, 19))

) => Partial<Record<keyof M, any>>;
>Partial : Symbol(Partial, Decl(lib.es5.d.ts, --, --))
>Record : Symbol(Record, Decl(lib.es5.d.ts, --, --))
>M : Symbol(M, Decl(index.ts, 0, 27))

type PromiseMockFactoryWithHelper<M = unknown> = (
>PromiseMockFactoryWithHelper : Symbol(PromiseMockFactoryWithHelper, Decl(index.ts, 2, 35))
>M : Symbol(M, Decl(index.ts, 3, 34))

importOriginal: <T extends M = M>() => Promise<T>
>importOriginal : Symbol(importOriginal, Decl(index.ts, 3, 50))
>T : Symbol(T, Decl(index.ts, 4, 19))
>M : Symbol(M, Decl(index.ts, 3, 34))
>M : Symbol(M, Decl(index.ts, 3, 34))
>Promise : Symbol(Promise, Decl(lib.es5.d.ts, --, --), Decl(lib.es2015.iterable.d.ts, --, --), Decl(lib.es2015.promise.d.ts, --, --), Decl(lib.es2015.symbol.wellknown.d.ts, --, --), Decl(lib.es2018.promise.d.ts, --, --))
>T : Symbol(T, Decl(index.ts, 4, 19))

) => Promise<Partial<Record<keyof M, any>>>;
>Promise : Symbol(Promise, Decl(lib.es5.d.ts, --, --), Decl(lib.es2015.iterable.d.ts, --, --), Decl(lib.es2015.promise.d.ts, --, --), Decl(lib.es2015.symbol.wellknown.d.ts, --, --), Decl(lib.es2018.promise.d.ts, --, --))
>Partial : Symbol(Partial, Decl(lib.es5.d.ts, --, --))
>Record : Symbol(Record, Decl(lib.es5.d.ts, --, --))
>M : Symbol(M, Decl(index.ts, 3, 34))

const util: {
>util : Symbol(util, Decl(index.ts, 7, 5))

mock<T>(module: Promise<T>, factory?: MockFactoryWithHelper<T>): void;
>mock : Symbol(mock, Decl(index.ts, 7, 13), Decl(index.ts, 8, 72))
>T : Symbol(T, Decl(index.ts, 8, 7))
>module : Symbol(module, Decl(index.ts, 8, 10))
>Promise : Symbol(Promise, Decl(lib.es5.d.ts, --, --), Decl(lib.es2015.iterable.d.ts, --, --), Decl(lib.es2015.promise.d.ts, --, --), Decl(lib.es2015.symbol.wellknown.d.ts, --, --), Decl(lib.es2018.promise.d.ts, --, --))
>T : Symbol(T, Decl(index.ts, 8, 7))
>factory : Symbol(factory, Decl(index.ts, 8, 29))
>MockFactoryWithHelper : Symbol(MockFactoryWithHelper, Decl(index.ts, 0, 0))
>T : Symbol(T, Decl(index.ts, 8, 7))

mock<T>(module: Promise<T>, factory?: PromiseMockFactoryWithHelper<T>): void;
>mock : Symbol(mock, Decl(index.ts, 7, 13), Decl(index.ts, 8, 72))
>T : Symbol(T, Decl(index.ts, 9, 7))
>module : Symbol(module, Decl(index.ts, 9, 10))
>Promise : Symbol(Promise, Decl(lib.es5.d.ts, --, --), Decl(lib.es2015.iterable.d.ts, --, --), Decl(lib.es2015.promise.d.ts, --, --), Decl(lib.es2015.symbol.wellknown.d.ts, --, --), Decl(lib.es2018.promise.d.ts, --, --))
>T : Symbol(T, Decl(index.ts, 9, 7))
>factory : Symbol(factory, Decl(index.ts, 9, 29))
>PromiseMockFactoryWithHelper : Symbol(PromiseMockFactoryWithHelper, Decl(index.ts, 2, 35))
>T : Symbol(T, Decl(index.ts, 9, 7))

} = {
mock: (() => {}) as any,
>mock : Symbol(mock, Decl(index.ts, 10, 5))

};

util.mock(import("pkg"), async (importOriginal) => ({
>util.mock : Symbol(mock, Decl(index.ts, 7, 13), Decl(index.ts, 8, 72))
>util : Symbol(util, Decl(index.ts, 7, 5))
>mock : Symbol(mock, Decl(index.ts, 7, 13), Decl(index.ts, 8, 72))
>"pkg" : Symbol("/node_modules/pkg/import", Decl(import.d.ts, 0, 0))
>importOriginal : Symbol(importOriginal, Decl(index.ts, 14, 32))

...(await importOriginal()),
>importOriginal : Symbol(importOriginal, Decl(index.ts, 14, 32))

}));

=== /node_modules/pkg/import.d.ts ===
export interface ImportInterface {}
>ImportInterface : Symbol(ImportInterface, Decl(import.d.ts, 0, 0))

=== /node_modules/pkg/require.d.ts ===
export interface RequireInterface {}
>RequireInterface : Symbol(RequireInterface, Decl(require.d.ts, 0, 0))

96 changes: 96 additions & 0 deletions tests/baselines/reference/returnTypeWithGenericAsyncFunction.types
Original file line number Diff line number Diff line change
@@ -0,0 +1,96 @@
//// [tests/cases/compiler/returnTypeWithGenericAsyncFunction.ts] ////

=== /index.ts ===
type MockFactoryWithHelper<M = unknown> = (
>MockFactoryWithHelper : MockFactoryWithHelper<M>
> : ^^^^^^^^^^^^^^^^^^^^^^^^

importOriginal: <T extends M = M>() => Promise<T>
>importOriginal : <T extends M = M>() => Promise<T>
> : ^ ^^^^^^^^^ ^^^^^^^^^^^

) => Partial<Record<keyof M, any>>;
type PromiseMockFactoryWithHelper<M = unknown> = (
>PromiseMockFactoryWithHelper : PromiseMockFactoryWithHelper<M>
> : ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

importOriginal: <T extends M = M>() => Promise<T>
>importOriginal : <T extends M = M>() => Promise<T>
> : ^ ^^^^^^^^^ ^^^^^^^^^^^

) => Promise<Partial<Record<keyof M, any>>>;

const util: {
>util : { mock<T>(module: Promise<T>, factory?: MockFactoryWithHelper<T>): void; mock<T>(module: Promise<T>, factory?: PromiseMockFactoryWithHelper<T>): void; }
> : ^^^^^^^ ^^ ^^ ^^ ^^^ ^^^ ^^^^^^^ ^^ ^^ ^^ ^^^ ^^^ ^^^

mock<T>(module: Promise<T>, factory?: MockFactoryWithHelper<T>): void;
>mock : { <T>(module: Promise<T>, factory?: MockFactoryWithHelper<T>): void; <T_1>(module: Promise<T_1>, factory?: PromiseMockFactoryWithHelper<T_1>): void; }
> : ^^^ ^^ ^^ ^^ ^^^ ^^^ ^^^^^^^^ ^^ ^^ ^^^ ^^^ ^^^
>module : Promise<T>
> : ^^^^^^^^^^
>factory : MockFactoryWithHelper<T>
> : ^^^^^^^^^^^^^^^^^^^^^^^^

mock<T>(module: Promise<T>, factory?: PromiseMockFactoryWithHelper<T>): void;
>mock : { <T_1>(module: Promise<T_1>, factory?: MockFactoryWithHelper<T_1>): void; <T>(module: Promise<T>, factory?: PromiseMockFactoryWithHelper<T>): void; }
> : ^^^^^^^^ ^^ ^^ ^^^ ^^^ ^^^ ^^ ^^ ^^ ^^^ ^^^ ^^^
>module : Promise<T>
> : ^^^^^^^^^^
>factory : PromiseMockFactoryWithHelper<T>
> : ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

} = {
>{ mock: (() => {}) as any,} : { mock: any; }
> : ^^^^^^^^ ^^^

mock: (() => {}) as any,
>mock : any
>(() => {}) as any : any
>(() => {}) : () => void
> : ^^^^^^^^^^
>() => {} : () => void
> : ^^^^^^^^^^

};

util.mock(import("pkg"), async (importOriginal) => ({
>util.mock(import("pkg"), async (importOriginal) => ({ ...(await importOriginal()),})) : void
> : ^^^^
>util.mock : { <T>(module: Promise<T>, factory?: MockFactoryWithHelper<T>): void; <T>(module: Promise<T>, factory?: PromiseMockFactoryWithHelper<T>): void; }
> : ^^^ ^^ ^^ ^^ ^^^ ^^^ ^^^ ^^ ^^ ^^ ^^^ ^^^ ^^^
>util : { mock<T>(module: Promise<T>, factory?: MockFactoryWithHelper<T>): void; mock<T>(module: Promise<T>, factory?: PromiseMockFactoryWithHelper<T>): void; }
> : ^^^^^^^ ^^ ^^ ^^ ^^^ ^^^ ^^^^^^^ ^^ ^^ ^^ ^^^ ^^^ ^^^
>mock : { <T>(module: Promise<T>, factory?: MockFactoryWithHelper<T>): void; <T>(module: Promise<T>, factory?: PromiseMockFactoryWithHelper<T>): void; }
> : ^^^ ^^ ^^ ^^ ^^^ ^^^ ^^^ ^^ ^^ ^^ ^^^ ^^^ ^^^
>import("pkg") : Promise<{ default: typeof import("/node_modules/pkg/import"); }>
> : ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>"pkg" : "pkg"
> : ^^^^^
>async (importOriginal) => ({ ...(await importOriginal()),}) : (importOriginal: <T extends { default: typeof import("/node_modules/pkg/import"); } = { default: typeof import("/node_modules/pkg/import"); }>() => Promise<T>) => Promise<{ default?: any; }>
> : ^ ^^^ ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>importOriginal : <T extends { default: typeof import("/node_modules/pkg/import"); } = { default: typeof import("/node_modules/pkg/import"); }>() => Promise<T>
> : ^ ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>({ ...(await importOriginal()),}) : { default?: any; }
> : ^^^^^^^^^^^^^^^^^^
>{ ...(await importOriginal()),} : { default?: any; }
> : ^^^^^^^^^^^^^^^^^^

...(await importOriginal()),
>(await importOriginal()) : Partial<Record<"default", any>>
> : ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>await importOriginal() : Partial<Record<"default", any>>
> : ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>importOriginal() : Promise<Partial<Record<"default", any>>>
> : ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>importOriginal : <T extends { default: typeof import("/node_modules/pkg/import"); } = { default: typeof import("/node_modules/pkg/import"); }>() => Promise<T>
> : ^ ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

}));

=== /node_modules/pkg/import.d.ts ===

export interface ImportInterface {}
=== /node_modules/pkg/require.d.ts ===

export interface RequireInterface {}
35 changes: 35 additions & 0 deletions tests/cases/compiler/returnTypeWithGenericAsyncFunction.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
// @module: nodenext
// @target: esnext
// @outDir: out
// @filename: /index.ts
type MockFactoryWithHelper<M = unknown> = (
importOriginal: <T extends M = M>() => Promise<T>
) => Partial<Record<keyof M, any>>;
type PromiseMockFactoryWithHelper<M = unknown> = (
importOriginal: <T extends M = M>() => Promise<T>
) => Promise<Partial<Record<keyof M, any>>>;

const util: {
mock<T>(module: Promise<T>, factory?: MockFactoryWithHelper<T>): void;
mock<T>(module: Promise<T>, factory?: PromiseMockFactoryWithHelper<T>): void;
} = {
mock: (() => {}) as any,
};

util.mock(import("pkg"), async (importOriginal) => ({
...(await importOriginal()),
}));

// @filename: /node_modules/pkg/import.d.ts
export interface ImportInterface {}
// @filename: /node_modules/pkg/require.d.ts
export interface RequireInterface {}
// @filename: /node_modules/pkg/package.json
{
"name": "pkg",
"version": "0.0.1",
"exports": {
"import": "./import.js",
"require": "./require.js"
}
}