Fix Callback argument type not inferred for union of interfaces #59309 by MichalMarsalek · Pull Request #59672 · microsoft/TypeScript

MichalMarsalek · 2024-08-18T07:58:48Z

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

src/compiler/checker.ts

Andarist · 2024-08-18T21:39:31Z

src/compiler/checker.ts

+                // This signature will contribute to contextual union signature
+                signatureList = [signature];
+            }
+            else if (!compareSignaturesIdentical(signatureList[0], signature, /*partialMatch*/ true, /*ignoreThisTypes*/ true, /*ignoreReturnTypes*/ true, compareTypesSubtypeOf)) {


If you are loosening here how parameter types are meant to be compared I'd probably expect here:

Suggested change

else if (!compareSignaturesIdentical(signatureList[0], signature, /*partialMatch*/ true, /*ignoreThisTypes*/ true, /*ignoreReturnTypes*/ true, compareTypesSubtypeOf)) {

else if (!compareSignaturesIdentical(signatureList[0], signature, /*partialMatch*/ true, /*ignoreThisTypes*/ true, /*ignoreReturnTypes*/ true, compareTypesAssignable)) {

Unless there is already a reason why the subtype relationship was used here.

I'm not really sure when each relation should be used. I used compareTypesSubtypeOf because that's what is used in a call to compareSignaturesIdentical with partialMatch === true in the function findMatchingSignatures.

findMatchingSignatures gets only called by getUnionSignatures so I agree that this probably makes sense

Andarist · 2024-08-18T21:40:35Z

src/compiler/checker.ts

+        const types = (type as UnionType).types
+            .map(type => ({ type, signature: getContextualCallSignature(type, node) }))
+            .filter(type => type.signature)
+            .sort((t1, t2) => t1.signature!.parameters.length - t2.signature!.parameters.length);


You need to account for rest parameters here so it would be more appropriate to use getParameterCount here over directly checking the .parameters.length. Please also consider cases like

((a: string, b?: string) => void) | ((a: string, ...rest: string[]) => void)

Both of those signatures have the same parameter count but perhaps you'd like to specifically sort one of them as the first one.

Thank you. I think that actually what I need is getMinArgumentCount.
For the case ((a: string, b?: string) => void) | ((a: string, ...rest: string[]) => void) I don't think it matters which one is first.

I'm not sure if it will make any difference but to exercise even more funky test cases you could add test cases with signatures of this shape:

((a: string, ...rest: [string, number?] | [number, boolean?]) => void) | ((a: string, ...rest: [string, number] | [number, boolean]) => void)

Note that I didn't think through what types you should use in those rest parameters, just that they can be unions of tuples and that one of the signatures might have some of the elements there required and some might have them optional etc.

jakebailey · 2024-08-19T06:09:15Z

@typescript-bot test it

typescript-bot · 2024-08-19T06:09:19Z

Starting jobs; this comment will be updated as builds start and complete.

Command	Status	Results
`test top400`	✅ Started	👀 Results
`user test this`	✅ Started	👀 Results
`run dt`	✅ Started	✅ Results
`perf test this faster`	✅ Started	👀 Results

typescript-bot · 2024-08-19T06:30:43Z

Hey @jakebailey, the results of running the DT tests are ready.

Everything looks the same!

You can check the log here.

typescript-bot · 2024-08-19T06:34:56Z

@jakebailey Here are the results of running the user tests with tsc comparing main and refs/pull/59672/merge:

Something interesting changed - please have a look.

Details

webpack

tsconfig.types.test.json

[NEW] error TS2554: Expected 3 arguments, but got 2.
- test/configCases/externals/resolve/webpack.config.js#L10

typescript-bot · 2024-08-19T06:46:03Z

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

Here they are:

tsc

Comparison Report - baseline..pr

_Metric	_baseline	_pr	_Delta	_Best	_Worst	_p-value
_{Compiler-Unions - node (v18.15.0, x64)}
_Errors	₃₀	₃₀	_~	_~	_~	_{p=1.000 n=6}
_Symbols	_62,153	_62,153	_~	_~	_~	_{p=1.000 n=6}
_Types	_50,242	_50,242	_~	_~	_~	_{p=1.000 n=6}
_{Memory used}	_{194,208k (± 0.97%)}	_{193,534k (± 0.94%)}	_~	_192,326k	_195,902k	_{p=0.298 n=6}
_{Parse Time}	_{1.31s (± 0.64%)}	_{1.31s (± 0.79%)}	_~	_1.29s	_1.32s	_{p=0.788 n=6}
_{Bind Time}	_0.71s	_{0.71s (± 0.57%)}	_~	_0.71s	_0.72s	_{p=0.405 n=6}
_{Check Time}	_{9.57s (± 0.39%)}	_{9.54s (± 0.29%)}	_~	_9.51s	_9.57s	_{p=0.295 n=6}
_{Emit Time}	_{2.73s (± 1.08%)}	_{2.74s (± 0.55%)}	_~	_2.72s	_2.76s	_{p=0.681 n=6}
_{Total Time}	_{14.31s (± 0.34%)}	_{14.29s (± 0.16%)}	_~	_14.27s	_14.33s	_{p=0.195 n=6}
_{angular-1 - node (v18.15.0, x64)}
_Errors	₇	₇	_~	_~	_~	_{p=1.000 n=6}
_Symbols	_945,757	_945,760	_{+3 (+ 0.00%)}	_~	_~	_{p=0.001 n=6}
_Types	_410,067	_410,072	_{+5 (+ 0.00%)}	_~	_~	_{p=0.001 n=6}
_{Memory used}	_{1,222,599k (± 0.00%)}	_{1,222,685k (± 0.00%)}	_{+86k (+ 0.01%)}	_1,222,663k	_1,222,733k	_{p=0.005 n=6}
_{Parse Time}	_{6.66s (± 0.44%)}	_{6.64s (± 0.11%)}	_~	_6.63s	_6.65s	_{p=0.280 n=6}
_{Bind Time}	_{1.86s (± 0.34%)}	_{1.86s (± 0.63%)}	_~	_1.85s	_1.88s	_{p=1.000 n=6}
_{Check Time}	_{31.04s (± 0.45%)}	_{31.24s (± 0.28%)}	_{+0.20s (+ 0.64%)}	_31.15s	_31.38s	_{p=0.006 n=6}
_{Emit Time}	_{14.90s (± 0.69%)}	_{14.96s (± 0.37%)}	_~	_14.87s	_15.01s	_{p=0.171 n=6}
_{Total Time}	_{54.47s (± 0.32%)}	_{54.71s (± 0.24%)}	_~	_54.54s	_54.88s	_{p=0.054 n=6}
_{mui-docs - node (v18.15.0, x64)}
_Errors	₀	₀	_~	_~	_~	_{p=1.000 n=6}
_Symbols	_2,505,734	_2,506,432	_{+698 (+ 0.03%)}	_~	_~	_{p=0.001 n=6}
_Types	_993,039	_993,701	_{+662 (+ 0.07%)}	_~	_~	_{p=0.001 n=6}
_{Memory used}	_{2,376,628k (± 0.00%)}	_{2,377,892k (± 0.00%)}	_{+1,264k (+ 0.05%)}	_2,377,797k	_2,378,050k	_{p=0.005 n=6}
_{Parse Time}	_{9.29s (± 0.19%)}	_{9.30s (± 0.32%)}	_~	_9.28s	_9.35s	_{p=0.506 n=6}
_{Bind Time}	_{2.19s (± 0.48%)}	_{2.19s (± 0.55%)}	_~	_2.18s	_2.21s	_{p=0.282 n=6}
_{Check Time}	_{73.77s (± 0.25%)}	_{73.92s (± 0.71%)}	_~	_73.26s	_74.86s	_{p=0.575 n=6}
_{Emit Time}	_{0.28s (± 2.70%)}	_{0.28s (± 2.88%)}	_~	_0.27s	_0.29s	_{p=0.306 n=6}
_{Total Time}	_{85.52s (± 0.21%)}	_{85.70s (± 0.64%)}	_~	_85.01s	_86.70s	_{p=0.378 n=6}
_{self-build-src - node (v18.15.0, x64)}
_Errors	₀	₀	_~	_~	_~	_{p=1.000 n=6}
_Symbols	_1,231,749	_1,231,762	_{+13 (+ 0.00%)}	_~	_~	_{p=0.001 n=6}
_Types	_264,525	_264,531	_{+6 (+ 0.00%)}	_~	_~	_{p=0.001 n=6}
_{Memory used}	_{2,353,013k (± 0.02%)}	_{2,353,054k (± 0.02%)}	_~	_2,352,453k	_2,353,510k	_{p=1.000 n=6}
_{Parse Time}	_{4.99s (± 0.42%)}	_{4.97s (± 0.70%)}	_~	_4.92s	_5.01s	_{p=0.261 n=6}
_{Bind Time}	_{1.88s (± 0.75%)}	_{1.88s (± 0.48%)}	_~	_1.87s	_1.89s	_{p=1.000 n=6}
_{Check Time}	_{34.69s (± 0.35%)}	_{34.83s (± 0.23%)}	_~	_34.68s	_34.91s	_{p=0.092 n=6}
_{Emit Time}	_{3.37s (± 1.00%)}	_{3.39s (± 2.87%)}	_~	_3.30s	_3.57s	_{p=1.000 n=6}
_{Total Time}	_{44.94s (± 0.32%)}	_{45.08s (± 0.40%)}	_~	_44.79s	_45.32s	_{p=0.128 n=6}
_{self-build-src-public-api - node (v18.15.0, x64)}
_Errors	₀	₀	_~	_~	_~	_{p=1.000 n=6}
_Symbols	_1,231,749	_1,231,762	_{+13 (+ 0.00%)}	_~	_~	_{p=0.001 n=6}
_Types	_264,525	_264,531	_{+6 (+ 0.00%)}	_~	_~	_{p=0.001 n=6}
_{Memory used}	_{2,427,537k (± 0.02%)}	_{2,427,445k (± 0.02%)}	_~	_2,426,785k	_2,428,164k	_{p=0.810 n=6}
_{Parse Time}	_{6.25s (± 0.46%)}	_{6.22s (± 0.74%)}	_~	_6.17s	_6.27s	_{p=0.810 n=6}
_{Bind Time}	_{2.03s (± 0.76%)}	_{2.04s (± 0.80%)}	_~	_2.02s	_2.06s	_{p=0.463 n=6}
_{Check Time}	_{41.67s (± 0.58%)}	_{41.63s (± 0.55%)}	_~	_41.23s	_41.90s	_{p=0.689 n=6}
_{Emit Time}	_{4.04s (± 0.40%)}	_{4.15s (± 4.13%)}	_~	_3.98s	_4.43s	_{p=0.148 n=6}
_{Total Time}	_{54.00s (± 0.48%)}	_{54.05s (± 0.39%)}	_~	_53.82s	_54.30s	_{p=0.936 n=6}
_{self-compiler - node (v18.15.0, x64)}
_Errors	₀	₀	_~	_~	_~	_{p=1.000 n=6}
_Symbols	_256,947	_256,960	_{+13 (+ 0.01%)}	_~	_~	_{p=0.001 n=6}
_Types	_105,643	_105,649	_{+6 (+ 0.01%)}	_~	_~	_{p=0.001 n=6}
_{Memory used}	_{429,391k (± 0.01%)}	_{429,444k (± 0.02%)}	_~	_429,297k	_429,528k	_{p=0.093 n=6}
_{Parse Time}	_{3.35s (± 0.56%)}	_{3.35s (± 0.56%)}	_~	_3.33s	_3.38s	_{p=1.000 n=6}
_{Bind Time}	_{1.30s (± 0.94%)}	_{1.29s (± 0.63%)}	_~	_1.28s	_1.30s	_{p=0.120 n=6}
_{Check Time}	_{18.08s (± 0.34%)}	_{18.12s (± 0.17%)}	_~	_18.08s	_18.16s	_{p=0.332 n=6}
_{Emit Time}	_{1.65s (± 0.86%)}	_{1.67s (± 1.28%)}	_~	_1.64s	_1.69s	_{p=0.168 n=6}
_{Total Time}	_{24.39s (± 0.21%)}	_{24.44s (± 0.13%)}	_~	_24.41s	_24.49s	_{p=0.293 n=6}
_{ts-pre-modules - node (v18.15.0, x64)}
_Errors	₃₅	₃₅	_~	_~	_~	_{p=1.000 n=6}
_Symbols	_225,018	_225,018	_~	_~	_~	_{p=1.000 n=6}
_Types	_94,245	_94,245	_~	_~	_~	_{p=1.000 n=6}
_{Memory used}	_{370,189k (± 0.02%)}	_{370,154k (± 0.04%)}	_~	_370,024k	_370,428k	_{p=0.230 n=6}
_{Parse Time}	_{2.76s (± 0.87%)}	_{2.75s (± 0.48%)}	_~	_2.73s	_2.76s	_{p=0.373 n=6}
_{Bind Time}	_{1.56s (± 0.52%)}	_{1.56s (± 0.75%)}	_~	_1.55s	_1.58s	_{p=0.738 n=6}
_{Check Time}	_{15.64s (± 0.19%)}	_{15.64s (± 0.21%)}	_~	_15.58s	_15.67s	_{p=0.809 n=6}
_{Emit Time}	_0.00s	_0.00s	_~	_~	_~	_{p=1.000 n=6}
_{Total Time}	_{19.97s (± 0.13%)}	_{19.95s (± 0.18%)}	_~	_19.88s	_19.98s	_{p=0.687 n=6}
_{vscode - node (v18.15.0, x64)}
_Errors	₀	₀	_~	_~	_~	_{p=1.000 n=6}
_Symbols	_3,006,672	_3,006,672	_~	_~	_~	_{p=1.000 n=6}
_Types	_1,035,540	_1,035,540	_~	_~	_~	_{p=1.000 n=6}
_{Memory used}	_{3,130,150k (± 0.00%)}	_{3,130,170k (± 0.00%)}	_~	_3,130,060k	_3,130,255k	_{p=0.575 n=6}
_{Parse Time}	_{13.98s (± 0.21%)}	_{13.95s (± 0.50%)}	_~	_13.85s	_14.04s	_{p=0.260 n=6}
_{Bind Time}	_{4.33s (± 2.16%)}	_{4.29s (± 0.36%)}	_~	_4.27s	_4.31s	_{p=0.413 n=6}
_{Check Time}	_{79.82s (± 0.28%)}	_{79.92s (± 0.25%)}	_~	_79.68s	_80.16s	_{p=0.630 n=6}
_{Emit Time}	_{20.48s (± 0.64%)}	_{20.52s (± 0.37%)}	_~	_20.41s	_20.61s	_{p=0.335 n=6}
_{Total Time}	_{118.62s (± 0.22%)}	_{118.68s (± 0.15%)}	_~	_118.48s	_118.88s	_{p=0.936 n=6}
_{webpack - node (v18.15.0, x64)}
_Errors	₀	₀	_~	_~	_~	_{p=1.000 n=6}
_Symbols	_275,352	_275,353	_{+1 (+ 0.00%)}	_~	_~	_{p=0.001 n=6}
_Types	_112,436	_112,436	_~	_~	_~	_{p=1.000 n=6}
_{Memory used}	_{424,135k (± 0.01%)}	_{424,239k (± 0.02%)}	_{+104k (+ 0.02%)}	_424,120k	_424,370k	_{p=0.045 n=6}
_{Parse Time}	_{3.98s (± 0.54%)}	_{3.97s (± 0.49%)}	_~	_3.94s	_3.99s	_{p=0.745 n=6}
_{Bind Time}	_{1.73s (± 0.60%)}	_{1.72s (± 0.85%)}	_~	_1.71s	_1.74s	_{p=0.190 n=6}
_{Check Time}	_{17.44s (± 0.46%)}	_{17.47s (± 0.37%)}	_~	_17.38s	_17.57s	_{p=0.471 n=6}
_{Emit Time}	_0.00s	_0.00s	_~	_~	_~	_{p=1.000 n=6}
_{Total Time}	_{23.16s (± 0.35%)}	_{23.16s (± 0.34%)}	_~	_23.07s	_23.28s	_{p=1.000 n=6}
_{xstate-main - node (v18.15.0, x64)}
_Errors	₀	₀	_~	_~	_~	_{p=1.000 n=6}
_Symbols	_534,239	_534,254	_{+15 (+ 0.00%)}	_~	_~	_{p=0.001 n=6}
_Types	_176,225	_176,228	_{+3 (+ 0.00%)}	_~	_~	_{p=0.001 n=6}
_{Memory used}	_{479,429k (± 0.01%)}	_{479,319k (± 0.01%)}	_{-110k (- 0.02%)}	_479,275k	_479,412k	_{p=0.013 n=6}
_{Parse Time}	_{4.26s (± 0.62%)}	_{4.25s (± 0.44%)}	_~	_4.22s	_4.27s	_{p=0.257 n=6}
_{Bind Time}	_{1.55s (± 1.00%)}	_{1.54s (± 0.86%)}	_~	_1.53s	_1.56s	_{p=0.323 n=6}
_{Check Time}	_{22.27s (± 0.43%)}	_{22.25s (± 0.21%)}	_~	_22.19s	_22.33s	_{p=1.000 n=6}
_{Emit Time}	_0.00s	_0.00s	_~	_~	_~	_{p=1.000 n=6}
_{Total Time}	_{28.08s (± 0.35%)}	_{28.05s (± 0.13%)}	_~	_28.01s	_28.10s	_{p=0.748 n=6}

System info unknown

Hosts

node (v18.15.0, x64)

Scenarios

Compiler-Unions - node (v18.15.0, x64)
angular-1 - node (v18.15.0, x64)
mui-docs - node (v18.15.0, x64)
self-build-src - node (v18.15.0, x64)
self-build-src-public-api - node (v18.15.0, x64)
self-compiler - node (v18.15.0, x64)
ts-pre-modules - node (v18.15.0, x64)
vscode - node (v18.15.0, x64)
webpack - node (v18.15.0, x64)
xstate-main - node (v18.15.0, x64)

_Benchmark	_Name	_Iterations
_Current	_pr	₆
_Baseline	_baseline	₆

Developer Information:

Download Benchmarks

Andarist · 2024-08-19T07:03:32Z

The reported error in webpack is correct. Right now it errors earlier (which is not mentioned by this comment): TS playground. It's enough to add an extra unused parameter to this function to get the same error as the one reported by the bot.

That said, maybe you could also filter out here signatures that can't be applicable in the first place. Like, if the function is async then only functions that return Promise<any> are valid as contextual signatures for it. Note that the contextual signature could still return a union containing a type assignable to Promise<any>. The same principle applies to generators (and async generators). I've done similar filtering on contextual return types here. That patch would still be relevant, I think, but some contextual signatures could even be discarded earlier

tests/cases/compiler/contextualSignatureWithExtraParameters.ts

typescript-bot · 2024-08-19T07:45:43Z

@jakebailey Here are the results of running the top 400 repos with tsc comparing main and refs/pull/59672/merge:

Something interesting changed - please have a look.

Details

vuejs/pinia

5 of 7 projects failed to build with the old tsc and were ignored

tsconfig.json

error TS2339: Property 'nonA' does not exist on type '{ a: boolean; nested: { foo: string; a: { b: string; }; }; } & _StoreWithGetters<{}> & PiniaCustomProperties<string, StateTree, _GettersTree<StateTree>, _ActionsTree>'.
- packages/pinia/__tests__/actions.spec.ts#L22
error TS7023: 'getNonA' implicitly has return type 'any' because it does not have a return type annotation and is referenced directly or indirectly in one of its return expressions.
- packages/pinia/__tests__/actions.spec.ts#L26
error TS2339: Property 'nonA' does not exist on type '{ getNonA(): any; simple(): "simple"; toggle(): boolean; setFoo(foo: string): void; combined(): void; throws(): never; rejects(): Promise<never>; } & { a: boolean; nested: { ...; }; } & _StoreWithState<...> & _StoreWithGetters<...> & PiniaCustomProperties<...>'.
- packages/pinia/__tests__/actions.spec.ts#L27

MichalMarsalek · 2024-08-19T09:49:00Z

Like, if the function is async then only functions that return Promise<any> are valid as contextual signatures for it.

That's smart, I added that.

src/compiler/checker.ts

tests/cases/compiler/contextualSignatureWithExtraParameters.ts

MichalMarsalek added 3 commits August 18, 2024 08:53

contextual type

25b5914

Merge branch 'main' into contextual-type

effe3a0

fix getContextualSignature

ed2bfa4

typescript-bot added the For Backlog Bug PRs that fix a backlog bug label Aug 18, 2024

format

2e530af

Andarist reviewed Aug 18, 2024

View reviewed changes

src/compiler/checker.ts Outdated Show resolved Hide resolved

Andarist reviewed Aug 18, 2024

View reviewed changes

MichalMarsalek added 2 commits August 19, 2024 07:11

simplify getContextualSignature

ac64f64

sort signatures by getMinArgumentCount

6eaf5e3