Improve overload and generic signature inference: Inference alternatives and linked inferences #52944
Conversation
This file contains 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
Bugs which need squashing: * When multiple overloads match, the _last_ match should be chosen, rather than the first, this way the change to inference isn't as noticable (genericCallWithFunctionTypedArguments4.types and genericCallWithFunctionTypedArguments3.types) * Const contexts are sometimes applying a bit too eagerly to overload lists (genericCallWithOverloadedConstructorTypedArguments.types) * Return type inferences aren't merging right across alternatives (inferTypes1.types and awaitedTypeJQuery.types and reactReduxLikeDeferredInferenceAllowsAssignment.types) But otherwise this looks pretty good already.
typescript-bot
added
Author: Team
For Uncommitted Bug
weswigham
force-pushed
the
inference-alternatives-and-linked-inferences
branch
from
509fe20
to
83987a7
Compare
|
@typescript-bot test this |
|
Heya @jakebailey, I've started to run the diff-based top-repos suite on this PR at 83987a7. 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 83987a7. You can monitor the build here. |
|
Heya @jakebailey, I've started to run the perf test suite on this PR at 83987a7. 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 83987a7. You can monitor the build here. |
|
Heya @jakebailey, I've started to run the diff-based user code test suite on this PR at 83987a7. You can monitor the build here. Update: The results are in! |
|
Heya @jakebailey, I've started to run the diff-based top-repos suite (tsserver) on this PR at 83987a7. You can monitor the build here. |
|
Heya @jakebailey, I've started to run the extended test suite on this PR at 83987a7. You can monitor the build here. |
|
Heya @jakebailey, I've started to run the diff-based user code test suite (tsserver) on this PR at 83987a7. 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 and then running |
|
@jakebailey Here are the results of running the user test suite comparing Everything looks good! |
|
@jakebailey Here are the results of running the user test suite comparing Something interesting changed - please have a look. Detailsfp-ts
|
|
Heya @jakebailey, I've run the RWC suite on this PR - assuming you're on the TS core team, you can view the resulting diff here. |
|
@jakebailey Here they are:
CompilerComparison Report - main..52944
System
Hosts
Scenarios
TSServerComparison Report - main..52944
System
Hosts
Scenarios
StartupComparison Report - main..52944
System
Hosts
Scenarios
Developer Information: |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
RWC largely looks good, lots of improvements in fp-ts and azure, just a jquery promise + promise compat error to look in to (and if I'm lucky it'll get fixed by how I fix the single self-check new error). User suites are clean, top100 is... I dunno, it's just reporting completion entry details crashes which look suspiciously unrelated to this. I'm sure DT will come back with interesting results once it's done (should we, maybe, shard DT runs onto more workers, assuming we have more workers available? Even with 4 workers it's stretching up towards the 2hr range). |
|
I'll probably have to see if I can do anything about the pretty major perf regression in material-ui, but generally speaking this isn't nearly as bad as I thought it could have been. Some strict limits to how many alternatives we'll try'll probably help bring that back down to reasonable. |
|
Feel free to ignore and not rerun tsserver; I didn't mean to include it here. It's complaining because these errors are finnicky and I've mostly gotten it to be quiet, but apparently not all. |
Note that it doesn't actually reply (unlike every other task...), so you'll have to manually go and check the results via logs, if any. |
Ah, misread this; there are two top100 tests, the one you actually need to look at is still pending but will reply any minute now: https://typescript.visualstudio.com/TypeScript/_build/results?buildId=147381&view=results |
|
@jakebailey Here are the results of running the top-repos suite comparing Something interesting changed - please have a look. Details
|
|
@jakebailey Here are the results of running the top-repos suite comparing Something interesting changed - please have a look. Detailsangular/angular
|
|
@jakebailey Here are some more interesting changes from running the top-repos suite Detailspuppeteer/puppeteer
|
|
I might be misunderstanding some of the constraints here but initially thought that it would also resolve this issue (but it doesn't): #38930 |
|
@typescript-bot pack this |
|
Heya @jakebailey, I've started to run the tarball bundle task on this PR at 4b8b0a5. You can monitor the build here. |
|
Hey @jakebailey, I've packed this into an installable tgz. You can install it for testing by referencing it in your and then running There is also a playground for this build and an npm module you can use via |
|
The example given in the description looks a lot like a (simplified) example I ran into recently, but this change doesn't seem to allow my example to compile: interface Wrapper<T> {
wrapped: T;
}
function unwrap<T>(value: Wrapper<T>): T;
function unwrap<T>(value: Wrapper<T> | undefined): T | undefined;
function unwrap<T>(value: Wrapper<T> | undefined): T | undefined {
return value?.wrapped;
}
const foo: string = await Promise.resolve<Wrapper<string>>({ wrapped: 'foo' }).then(unwrap);Playground link for this PR build. Which still complains that |
|
Here's a potential regression with this PR I found after testing in our codebase declare function promisify<TArgs extends readonly unknown[], TResult>(
fn: (...args: [...TArgs, (err: any, result: TResult | undefined) => void]) => void,
): (...args: TArgs) => Promise<TResult>;
type Callback = (err?: null | Error, output?: string) => void
declare function stringify(callback?: Callback): void
declare function stringify(input: readonly unknown[], callback?: Callback): void
declare const data: string[][];
export const result: string = await promisify(stringify)(data); |
|
@typescript-bot run DT |
|
Heya @gabritto, I've started to run the parallelized Definitely Typed test suite on this PR at 4b8b0a5. You can monitor the build here. Update: The results are in! |
|
Hey @gabritto, the results of running the DT tests are ready. Branch only errors:Package: express-paginate Package: ember__array Package: openpgp Package: ember__array/v3 Package: ramda Package: rdf-dataset-ext Package: bluebird Package: ember Package: react-redux/v6 Package: bluebird/v2 Package: ember/v3 Package: storefront-ui__vue |
Andarist
reviewed
Apr 13, 2023
| @@ -6794,6 +6792,9 @@ export interface InferenceContext { | |||
| returnMapper?: TypeMapper; // Type mapper for inferences from return types (if any) | |||
| inferredTypeParameters?: readonly TypeParameter[]; // Inferred type parameters for function result | |||
| intraExpressionInferenceSites?: IntraExpressionInferenceSite[]; | |||
| freeTypeVariables?: InferenceInfo[]; // Extra inferences made for type parameters found during inference | |||
| freeTypeVariableSourceSignatures?: Map<object, Map<Signature, Signature>>; // For each recusion identity on the source stack, a cached mapping of signature copies made | |||
There was a problem hiding this comment.
Suggested change
| freeTypeVariableSourceSignatures?: Map<object, Map<Signature, Signature>>; // For each recusion identity on the source stack, a cached mapping of signature copies made | |
| freeTypeVariableSourceSignatures?: Map<object, Map<Signature, Signature>>; // For each recursion identity on the source stack, a cached mapping of signature copies made |
Andarist
reviewed
Apr 13, 2023
| @@ -37271,14 +37492,25 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker { | |||
| return false; | |||
| } | |||
|
|
|||
| function mergeInferences(target: InferenceInfo[], source: InferenceInfo[]) { | |||
| function mergeInferences(target: InferenceInfo[], source: InferenceInfo[], clone = false) { | |||
There was a problem hiding this comment.
For consistency with mergeInferenceContexts I'd call this one cloneInferences
Suggested change
| function mergeInferences(target: InferenceInfo[], source: InferenceInfo[], clone = false) { | |
| function mergeInferences(target: InferenceInfo[], source: InferenceInfo[], cloneInferences = false) { |
Andarist
reviewed
Apr 13, 2023
| } | ||
|
|
||
| /** | ||
| * Resets the alternatives list for the active infernce context to the supplied group of inference contexts |
There was a problem hiding this comment.
Suggested change
| * Resets the alternatives list for the active infernce context to the supplied group of inference contexts | |
| * Resets the alternatives list for the active inference context to the supplied group of inference contexts |
Andarist
reviewed
Apr 13, 2023
| * and add the resulting context(s) to a new resultant context list. | ||
| * | ||
| * Call multiple times to create multiple independent forks of the existing inference engine state. | ||
| * Call once with fork `false` to simply to do and action for every existing branch of the inference |
There was a problem hiding this comment.
"to simply to do and action for every [...]" - I'm not sure how to read this properly, perhaps this part could be improved/rephrased?
Andarist
reviewed
Apr 13, 2023
| if (!finalAlternative) { | ||
| // The last alternative in the list most closely mirrors previous behaviors of only considering the final overload in a list | ||
| // during inference. If none pass the `accept` function, this is the one which'll be used (likely for error reporting! It's | ||
| // possible to improve upon this for better errors, much like we how we do union member selection for errors in relationship checking!) |
There was a problem hiding this comment.
Suggested change
| // possible to improve upon this for better errors, much like we how we do union member selection for errors in relationship checking!) | |
| // possible to improve upon this for better errors, much like how we do union member selection for errors in relationship checking!) | |
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.
With this PR, we explore multiple options within inference when comparing an overload list to a single target signature, and pick a single match out of the list of possible inferences this produces, rather than creating a combined inference that may not actually work with the source signature list. In addition, we no longer erase generics when performing inference, instead preserving them and mapping them into free type variable locations, which we also perform inference on, allowing us to link up inferences across multiple positions within a (potentially overloaded!) signature, and, eg, keep an inferred type consistent between an argument and return position.
These two changes largely serve to improve promise-like monadic inferences (as the changed baselines show), and serve as, IMO, a basis for being able to properly type the JSX constructor function without all of the internal machinery we currently have that only applies to JSX tags (without introducing any new syntax).
The simplest ask this PR fixes, which you can find in our test suite's updated baselines in this PR (which date back to the initial snapshot of the compiler), is requesting that this:
typecheck, rather than error.
Fixes #35501
Fixes #30294 (partially)