The TypeScript team hasn't accepted the linked issue #55667. If you can get it accepted, this PR will have a better chance of being reviewed.

@typescript-bot test top200
@typescript-bot user test this
@typescript-bot run dt
@typescript-bot perf test this
@typescript-bot pack this

Heya @jakebailey, I've started to run the diff-based top-repos suite on this PR at 0a2d583. You can monitor the build here.

Update: The results are in!

Heya @jakebailey, I've started to run the tarball bundle task on this PR at 0a2d583. You can monitor the build here.

Heya @jakebailey, I've started to run the diff-based user code test suite on this PR at 0a2d583. You can monitor the build here.

Update: The results are in!

Heya @jakebailey, I've started to run the parallelized Definitely Typed test suite on this PR at 0a2d583. You can monitor the build here.

Update: The results are in!

Heya @jakebailey, I've started to run the regular perf test suite on this PR at 0a2d583. You can monitor the build here.

Update: The results are in!

Hey @jakebailey, I've packed this into an installable tgz. You can install it for testing by referencing it in your package.json like so:

{
    "devDependencies": {
        "typescript": "https://typescript.visualstudio.com/cf7ac146-d525-443c-b23c-0d58337efebc/_apis/build/builds/157631/artifacts?artifactName=tgz&fileId=FE94CA1F17108FBA50E4F137461CB62A7EF9321D897F7A94C861969DCFCB26C502&fileName=/typescript-5.3.0-insiders.20230911.tgz"
    }
}

and then running npm install.

There is also a playground for this build and an npm module you can use via "typescript": "npm:@typescript-deploys/[email protected]".;

@jakebailey Here are the results of running the user test suite comparing main and refs/pull/55714/merge:

There were infrastructure failures potentially unrelated to your change:

• 1 instance of "Unknown failure"
• 2 instances of "Package install failed"

Otherwise...

Everything looks good!

@jakebailey
The results of the perf run you requested are in!

Why not InstanceType?

@jakebailey Here are the results of running the top-repos suite comparing main and refs/pull/55714/merge:

Something interesting changed - please have a look.

I missed the InstanceType with my multi-cursor edit :v thanks for catching that.

Hey @jakebailey, it looks like the DT test run failed. Please check the log for more details.
You can check the log here.

@typescript-bot user test this
@typescript-bot test top200

Heya @andrewbranch, I've started to run the diff-based user code test suite on this PR at e08f2a3. You can monitor the build here.

Update: The results are in!

Heya @andrewbranch, I've started to run the diff-based top-repos suite on this PR at e08f2a3. You can monitor the build here.

Update: The results are in!

@andrewbranch Here are the results of running the user test suite comparing main and refs/pull/55714/merge:

There were infrastructure failures potentially unrelated to your change:

• 1 instance of "Unknown failure"
• 2 instances of "Package install failed"

Otherwise...

Everything looks good!

@andrewbranch Here are the results of running the top-repos suite comparing main and refs/pull/55714/merge:

Something interesting changed - please have a look.

Extracted (not exactly minimal yet) repro from vuetify: TS playground

That’s... weird

Here is a 17 loc repro.

I think this is exposing an inference/assignability bug, not a logical consequence of switching the parameter type in the conditional extends type. I notice that if you swap infer R in NewReturnType for any or unknown, T1 starts taking the true branch of the conditional. So it seems like infer R is making the input type fail the extends assignability check, not ...args: never. That will need to be fixed first or as part of this PR.

@andrewbranch I'll tell you what I found out after some debugging of the code in this playground.

Inside getConditionalType, when we are applying the two versions of the conditional to <D extends Record<string, unknown>>(t: D) => Foo<D> we have:

1. With the classic definition of ReturnType:

2. With the modified defition, the one that uses never as the argument's type:
   

In both cases R is correctly inferred as Foo<Record<string, unknown>>, but what is failing is the isTypeAssignableTo check between checkType and inferredExtendsType. What is that check for?

Return falseType for a definitely false extends check. We check an instantiations of the two types with type parameters mapped to the wildcard type, the most permissive instantiations possible (the wildcard type is assignable to and from all types). If those are not related, then no instantiations will be and we can just return the false branch type.

This is exactly what happens, because the never version always returns what is present in the false branch in this specific playground.

So when compareSignaturesRelated comes into play because of isTypeAssignableTo we have:

1. any version:
   1.1 source is <D extends Record<string, unknown>>(t: D): Foo<D>
   1.2 target is (...args: any): Foo<Record<string, unknown>>
   1.3 source becomes (t: any): Foo<any> because of instantiateSignatureInContextOf
   1.4 So it seems all fine and dandy :)

2. never version:
   2.1 source is <D extends Record<string, unknown>>(t: D): Foo<D>
   2.2 target is (...args: never): Foo<Record<string, unknown>>
   2.3 source becomes (t: never): Foo<never> because of instantiateSignatureInContextOf
   2.4 When params are checked sourceType = [t: never] and targetType = never are compared and found related. OTOH Foo<never> and Foo<Record<string, unknown>>, the two return types, are not considered related because D, the parameter of Foo, is an invariant type parameter (even if actually they are the same type considering how Foo is defined)

I suppose that the source of the problem is instantiateSignatureInContextOf that produces the Foo<never> as return type for source instead of what we would need, that is R i.e. Record<string, unknown>. Another supposition: this problem has never come up with the usual ReturnType definition because instantiateSignatureInContextOf in that case uses any, that has more chances to be considered related to other types.

I plan to get back to this soon-ish but I have a backlog of PR reviews to address right now so it will take some time 😉

@jakebailey @andrewbranch could you rerun test suite here? :)

@typescript-bot user test this
@typescript-bot test top200

Heya @andrewbranch, I've started to run the diff-based top-repos suite on this PR at f06aaaa. You can monitor the build here.

Update: The results are in!

Heya @andrewbranch, I've started to run the diff-based user code test suite on this PR at f06aaaa. You can monitor the build here.

Update: The results are in!

@andrewbranch Here are the results of running the user test suite comparing main and refs/pull/55714/merge:

There were infrastructure failures potentially unrelated to your change:

• 1 instance of "Package install failed"

Otherwise...

Something interesting changed - please have a look.

@andrewbranch Here are the results of running the top-repos suite comparing main and refs/pull/55714/merge:

Everything looks good!

Hi @andrewbranch , do you think this PR can now become part of TS 5.4? The assignability problem has been solved by changing the definitions again (arguments/return type are being inferred even when not needed, then discarded), and there are no more failed tests.

Why are the infer _ needed?

Why are the infer _ needed?

#56919 and this comment explain what happens if you don't use it.

@typescript-bot perf test

Heya @andrewbranch, I've started to run the regular perf test suite on this PR at f06aaaa. You can monitor the build here.

Update: The results are in!

@andrewbranch
The results of the perf run you requested are in!