Add checker speculation helper, use in overload resolution #57421
Conversation
typescript-bot
added
Author: Team
For Milestone Bug
src/compiler/checker.ts
Outdated
| @@ -34848,63 +35045,67 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker { | |||
| if (!hasCorrectTypeArgumentArity(candidate, typeArguments) || !hasCorrectArity(node, args, candidate, signatureHelpTrailingComma)) { | |||
| continue; | |||
| } | |||
| const result = speculate(() => { | |||
There was a problem hiding this comment.
This is easier to read with whitespace diffs turned off.
| spreadOfParamsFromGeneratorMakesRequiredParams.ts(6,1): error TS2554: Expected 2 arguments, but got 1. | ||
|
|
||
|
|
||
| !!! error TS2318: Cannot find global type 'IterableIterator'. |
There was a problem hiding this comment.
This error only crops up while resolving one of the overloads (the iterator one, ofc), which isn't the last one that we "accept" (though it still has an error), so it gets tossed.
| @@ -37,7 +37,7 @@ foo(function(x) { x }); | |||
| >["hello"] : string[] | |||
| >"hello" : "hello" | |||
| >every : { <S extends string>(predicate: (value: string, index: number, array: string[]) => value is S, thisArg?: any): this is S[]; (predicate: (value: string, index: number, array: string[]) => unknown, thisArg?: any): boolean; } | |||
| >function(v,i,a) {return true;} : (v: string, i: number, a: string[]) => true | |||
| >function(v,i,a) {return true;} : (v: string, i: number, a: string[]) => boolean | |||
There was a problem hiding this comment.
This is one of those funky places where parameter type fixing would make the return type's type contextually type itself in overload passes after the first one. That no longer occurs.
tests/baselines/reference/thisConditionalOnMethodReturnOfGenericInstance.errors.txt
Outdated
Show resolved
Hide resolved
| @@ -66,7 +66,7 @@ arr.map((a: number | string, index: number) => { | |||
|
|
|||
| // This case still doesn't work because `reduce` has multiple overloads :( | |||
| arr.reduce((acc: Array<string>, a: number | string, index: number) => { | |||
| >arr.reduce((acc: Array<string>, a: number | string, index: number) => { return []}, []) : never[] | |||
| >arr.reduce((acc: Array<string>, a: number | string, index: number) => { return []}, []) : string[] | |||
There was a problem hiding this comment.
Yes, this does fix the // This case still doesn't work because reduce has multiple overloads :( comment 😀
Now, since every overload is done in its own speculative context, the [] expression no longer has its type locked in by the first overload, and it can be interpreted as desired by the later overload instead.
… have an instance (the node state may have been rewound)
|
I have confirmed locally that this PR would fix #46312 too A copy-pasteable test case can be found here: test case// @strict: true
// @noEmit: true
interface TypegenDisabled {
"@@xstate/typegen": false;
}
interface TypegenEnabled {
"@@xstate/typegen": true;
}
interface EventObject {
type: string;
}
interface ActionObject<TEvent extends EventObject> {
_TE: TEvent;
}
interface StateMachine<
TEvent extends EventObject,
TTypesMeta = TypegenDisabled,
> {
_TE: TEvent;
_TRTM: TTypesMeta;
}
interface MachineOptions<TEvent extends EventObject> {
action?: ActionObject<TEvent>;
}
type MaybeTypegenMachineOptions<
TEvent extends EventObject,
TTypesMeta = TypegenDisabled,
> = TTypesMeta extends TypegenEnabled
? {
action?: ActionObject<{ type: "WITH_TYPEGEN" }>;
}
: MachineOptions<TEvent>;
declare function assign<TEvent extends EventObject>(
assignment: (ev: TEvent) => void,
): ActionObject<TEvent>;
declare function useMachine<
TEvent extends EventObject,
TTypesMeta extends TypegenEnabled,
>(
getMachine: StateMachine<TEvent, TTypesMeta>,
options: MaybeTypegenMachineOptions<TEvent, TTypesMeta>,
): void;
declare function useMachine<TEvent extends EventObject>(
getMachine: StateMachine<TEvent>,
options?: MachineOptions<TEvent>,
): void;
const machine = {} as StateMachine<{ type: "WITHOUT_TYPEGEN" }>;
const ret = useMachine(machine, {
action: assign((_ev) => {
((_type: "WITHOUT_TYPEGEN") => null)(_ev.type);
}),
}); |
|
I'm curious to see how badly this is going to impact performance. It definitely sounds like a potential perf trap, at least for codebases with large quantities of overloads... |
…s, fixes timeout locally at least
… overload we check
|
@typescript-bot run dt |
|
@weswigham Here are the results of running the user test suite comparing There were infrastructure failures potentially unrelated to your change:
Otherwise... Something interesting changed - please have a look. Details
|
|
Hey @weswigham, the results of running the DT tests are ready. |
|
@weswigham Here are the results of running the top-repos suite comparing Something interesting changed - please have a look. Details
|
|
https://typescript.visualstudio.com/TypeScript/_build/results?buildId=160185&view=results made mui time out; I'm going to up the max time limit of the benchmarker as apparently 60 minutes was too slow, oops. |
|
@typescript-bot run dt |
|
Hey @weswigham, the results of running the DT tests are ready. |
|
@typescript-bot pack this |
|
Starting jobs; this comment will be updated as builds start and complete.
|
|
Hey @weswigham, 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 |
weswigham
force-pushed
the
speculation-helper-wip
branch
from
8aed762 to
fb3cac4
Compare
|
@typescript-bot run dt |
|
Hey @weswigham, the results of running the DT tests are ready. |
|
@weswigham Here are the results of running the user test suite comparing There were infrastructure failures potentially unrelated to your change:
Otherwise... Something interesting changed - please have a look. Details
|
|
@weswigham Here they are:
tscComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
Developer Information: |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Mmmm, vscode OOMing probably shouldn't report as "no difference" in the perf results with high probability. 😆 Definitely needs an indicator that the builds failed for some reason. |
It's low probability, p=1 which is the most bad. Happens when the perf numbers are zero (looks exactly like Go's benchstat). I agree that the table needs to say something about when the compilation fails (it just doesn't yet, only in logs). |
|
@weswigham Here are the results of running the top-repos suite comparing Something interesting changed - please have a look. Details
|
|
@typescript-bot pack this |
|
Hey @RyanCavanaugh, 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 |
|
Mmmm, this works, generally speaking (and there are big improvements in behavior!), but the perf cost is too high to do it this way over today's behavior (doubled checking times, much larger memory usage, ew), which feels quite bad. I'm not gonna keep this updated over time, and a future iteration is going to have to try an entirely different approach to try to get it to perform well, so I'm going to close this. |
weswigham
added
the
Experiment
|
Note for future readers: AFAIK, it's the downlevel-decorator access-intermediated caches that are slow to touch, so maybe something closer to this becomes more viable once we have native decorators, OR, if you feel like rewriting every cache access in the codebase, that might work, too. Maybe. |
This both prevents inference data from failed overloads from leaking into follow-on overloads, and allows contextual parameters to become unfixed after a failed overload and adopt a new type. This fixes a lot of longstanding design limitations.
For authors in the checker, the important changes in this PR are twofold:
There is now a function called
speculate- you pass it a callback, and it executes that callback within a speculative context. if the callback returns a truthy value, the context is kept when the callback completes (as you may want to do if a speculative operation is successful), otherwise it is rewound to the state it was when the callback was first invoked. Right now, it's just used in overload resolution, but you can imagine the use of it in a handful of other places.To support this,
Node- andSymbol-links have been refactored into classes with decorators on the properties we should speculatively cache. The decorator itself implements all the speculative caching. It basically works by recording timestamps writes occur at, and lazily discarding writes at timestamps marked as discarded upon read.I've narrowed the set of caches we consider "speculative" down about as much as I think I safely can. Only a handful of fields on the symbol and node
linksstructures (that retain context-sensitive calculated data or diagnostic data), plus diagnostics and relationship caches (since those retain diagnostic emit data!) are checkpointed and potentially rewound on speculation boundaries. Links speculation caches are lazy, but some other global structures are still eagerly copied - in particular, relationship caches (because they contain error reporting state!) are currently eagerly copied - this is probably a location ripe for improvement in real world scenarios.For users, since this does use the speculation helper in one big location already, this:
Fixes #49820 (Previously A2 would be fixed as a parameter type during checking of the first overload, now that is undone, and B2 can be correctly assigned on the second overload check)
Fixes #13430 (Previously,
xwould get an implicitanyassigned during checking of the first overload - that is now discarded (the function argument arity doesn't match, so the overload ultimately fails) and the second overload is selected)Fixes #12733 (Same as above)
Fixes #21525 (Previously the first overload would project incorrect contextual types for the second overload and fail, making the second overload fail as well)
And potentially many more - many of the issues in this vein have been classified "Design Limitation", or closed as duplicate/won't fix over the years.