Reuse single errored signature candidate as resolved signature #61787
+258
−98
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
There was a problem hiding this comment.
Pull Request Overview
This PR enhances overload resolution by reusing a single errored signature candidate as the resolved signature and updates tests and baselines accordingly.
- Adjust overload failure logic in
checker.tsto prefer a lone errored candidate - Add new fourslash tests for failed inference in context-sensitive arguments
- Update numerous baseline reference files to reflect the new signature resolution behavior
Reviewed Changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/compiler/checker.ts | Adjust candidate selection to reuse a single errored signature |
| tests/cases/fourslash/quickInfoFailedInferenceWithContextSensitiveArg2.ts | Add quickInfo test for second context-sensitive failure scenario |
| tests/cases/fourslash/quickInfoFailedInferenceWithContextSensitiveArg1.ts | Add quickInfo test for first context-sensitive failure scenario |
| tests/cases/fourslash/completionsFailedInferenceWithContextSensitiveArg1.ts | Add completion test for context-sensitive inference failure |
| tests/cases/conformance/types/typeRelationships/typeInference/contextualSignatureInstantiation.ts | Update test to use baz signature reuse with explicit b2 |
| tests/baselines/reference/promiseTry.types | Update expected return type for Promise.try |
| tests/baselines/reference/promisePermutations3.types | Update expected IPromise chaining result |
| tests/baselines/reference/partiallyAnnotatedFunctionInferenceError.types | Revise expected inference error outputs |
| tests/baselines/reference/genericCallWithFunctionTypedArguments5.types | Correct expected return type from function-typed argument calls |
| tests/baselines/reference/generatorTypeCheck62.types | Update expected strategy generator signature |
| tests/baselines/reference/contextualSignatureInstantiation.types | Reflect new baz return types and renamed b2 |
| tests/baselines/reference/contextualSignatureInstantiation.symbols | Update symbol declarations for b2 and d vars |
| tests/baselines/reference/contextualSignatureInstantiation.js | Adjust JS baseline for b2 and d variable initializations |
| tests/baselines/reference/contextualSignatureInstantiation.errors.txt | Update error messages and variable names from b to b2 |
Andarist
commented
May 31, 2025
| @@ -33,8 +33,8 @@ declare function testError<T extends C>(a: (t: T, t1: T) => void): T | |||
|
|
|||
| // more args | |||
| testError((t1: D, t2, t3) => {}) | |||
| >testError((t1: D, t2, t3) => {}) : C | |||
| > : ^ | |||
| >testError((t1: D, t2, t3) => {}) : any | |||
There was a problem hiding this comment.
This looks bad but this call errors so I think it's a fair game that the returned type changes - as it doesn't quite matter anyway.
The reason it got changed is simple - this got inferred from the implicit any assigned to t2 and t3. The same was always inferred here internally. It's just the final assigned signature was the one computed by inferSignatureInstantiationForOverloadFailure and that ignores context--sensitive arguments.
All of the changes in baselines can be attributed to the same thing. It's just any here is, of course, extra suspicious.
Maybe this could be "improved" but given this callback has more parameters than the contextual signature - the contextual signature is not selected so it can't assign "better" types to t2 and t3. This gets ignored in getContextualCallSignature based on the isAritySmaller check.
Andarist
commented
May 31, 2025
Comment on lines
+23
to
+24
| var b2: number | string; | ||
| var b2 = baz(b2, b2, g); // Should be number | string |
There was a problem hiding this comment.
why the change here? it's because now the previous calls are able to change the assigned type to b (by not ignoring generic functions we infer string/number instead of unknown). So the contextual return type here changed from string | number to string and that introduced the error. But that never was the intention of this test case anyway - so I just separated this to decouple this from CFA type changes
|
would it make sense to emit a warning when any is inferred in these partially annotated functions, to help catch unintended inference? |
|
@typescript-bot test it |
|
Hey @jakebailey, the results of running the DT tests are ready. Everything looks the same! |
|
@jakebailey Here are the results of running the user tests with tsc comparing Everything looks good! |
|
@jakebailey Here they are:
tscComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
Developer Information: |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Andarist
commented
Jun 6, 2025
| > : ^^^^^^^^^^^^^^^^^^^^ | ||
| >_.map<number, string, Date>(c2, (x, y) => { return x.toFixed() }) : Collection<any, any> | ||
| > : ^^^^^^^^^^^^^^^^^^^^ | ||
| >r5a : Collection<number, Date> |
There was a problem hiding this comment.
the previous result was already somewhat bizzare here as it came from the overloaded signature with 2 type parameters but 3 are applied here - taking that into consideration, the new one makes much more sense to the user (it comes from the signature with 3 type parameters)
Andarist
commented
Jun 6, 2025
Comment on lines
+36707
to
+36708
| // for the time being only do this when there is a single candidate as the origin index of the `candidatesForArgumentError` is not known | ||
| // and it could be different from the selected `bestIndex` here |
There was a problem hiding this comment.
I'd consider revisiting this in Corsa where CallState is already inroduced and improving upon this would likely be slightly easier thanks to that
|
@jakebailey Here are the results of running the top 400 repos with tsc comparing Everything looks good! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
fixes #61500