Always fully compute variance structurally #48080
Conversation
typescript-bot
added
the
For Milestone Bug
|
@typescript-bot test this |
|
Heya @ahejlsberg, I've started to run the extended test suite on this PR at de4a166. You can monitor the build here. |
|
Heya @ahejlsberg, I've started to run the abridged perf test suite on this PR at de4a166. You can monitor the build here. Update: The results are in! |
|
Heya @ahejlsberg, I've started to run the diff-based community code test suite on this PR at de4a166. You can monitor the build here. Update: The results are in! |
|
Heya @ahejlsberg, I've started to run the parallelized Definitely Typed test suite on this PR at de4a166. You can monitor the build here. |
|
@ahejlsberg |
|
@ahejlsberg Here they are:Comparison Report - main..48080
System
Hosts
Scenarios
Developer Information: |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
@typescript-bot pack this |
|
Heya @RyanCavanaugh, I've started to run the tarball bundle task on this PR at de4a166. You can monitor the build here. |
|
@typescript-bot perf test faster |
|
Heya @ahejlsberg, I've started to run the abridged perf test suite on this PR at d059791. You can monitor the build here. Update: The results are in! |
|
@ahejlsberg Here they are:Comparison Report - main..48080
System
Hosts
Scenarios
Developer Information: |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Hey @RyanCavanaugh, I've packed this into an installable tgz. You can install it for testing by referencing it in your and then running |
|
Tests are clean. Performance is slightly impacted in |
That shouldn't be necessary because we don't cache relations that observed incomplete variances (they result in |
|
Sadly there are still issues with the fix in this PR as well as the fix in (the now closed) #45628. The core issue looks to be mutually recursive types in which variance is flipped by one of the types. For example: type Foo<T> = {
x: T;
f: FooFn<T>;
}
type FooFn<T> = (foo: Foo<T>) => void;
declare let foo1: Foo<string>;
declare let foo2: Foo<unknown>;
declare let fn1: FooFn<string>;
declare let fn2: FooFn<unknown>;
foo1 = foo2; // Error
foo2 = foo1; // Error
fn1 = fn2; // Error
fn2 = fn1; // ErrorAbove, the type FooFn<T> = (foo: Foo<T[]) => void;Now, because foo1 = foo2; // Error
foo2 = foo1;
fn1 = fn2;
fn2 = fn1;meaning co-variance for foo1 = foo2; // Error
foo2 = foo1;
fn1 = fn2;
fn2 = fn1; // Errori.e. contra-variance for The tricky aspect of this example (and others like it) is that invariance isn't revealed unless we examine multiple circular type references structurally and in-depth. Which of course is exactly what we're trying to avoid with the variance computation in the first place. |
Why is this making a difference? Is it purely an optimization for the most common case or something like that? I don't quite understand why naked and not-naked type parameters go through a different code path here 🤔 |
|
Heya @ahejlsberg, I've started to run the diff-based community code test suite on this PR at a323037. You can monitor the build here. Update: The results are in! |
|
Heya @ahejlsberg, I've started to run the parallelized Definitely Typed test suite on this PR at a323037. You can monitor the build here. |
|
@ahejlsberg Here they are:Comparison Report - main..48080
System
Hosts
Scenarios
Developer Information: |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
@ahejlsberg |
|
As mentioned - I'm very open to revisiting XState types to see if we can improve the situation anyhow there. I know that this is just a single library whereas this PR potentially impacts a lot more than that though. However, as a dev, I would always take a slower compilation over random errors - so this PR is a big improvement from my point of view. Is there any rough description for the overall algorithm of how the variance is computed? If I understand correctly then:
I wonder if there is any type construct that makes it impossible for you to take the associated "slot" into consideration? Like - I see how "makes" type params are taken into consideration, even when a type is "boxed" using some other type I can see how the relation gets transitive etc But when I think about crazy conditional & mapped types... I'm getting lost. In somewhat other words - I wonder if there is anything that we possibly could do to make TS not descend into a particular branch of the structure? |
|
@typescript-bot test this |
|
Heya @ahejlsberg, I've started to run the diff-based community code test suite on this PR at 08c0fa9. You can monitor the build here. Update: The results are in! |
|
Heya @ahejlsberg, I've started to run the parallelized Definitely Typed test suite on this PR at 08c0fa9. You can monitor the build here. |
|
Heya @ahejlsberg, I've started to run the abridged perf test suite on this PR at 08c0fa9. You can monitor the build here. Update: The results are in! |
|
Heya @ahejlsberg, I've started to run the extended test suite on this PR at 08c0fa9. You can monitor the build here. |
|
@ahejlsberg Here they are:Comparison Report - main..48080
System
Hosts
Scenarios
Developer Information: |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
@ahejlsberg |
|
@Andarist Here's a quick overview of the variance computation logic. For a generic type
To determine the variance of As we're relating these marker type instantiations, we may come upon other type instantiations for which we need to know the variance. If so, we recursively start a variance computation and use the results of that to relate the type arguments. We may also come upon a circular reference to an instantiation of a type for which we are already computing variance information. If so, we again structurally relate the instantiations, but only if some of the type arguments contain marker types. Ultimately, we stop relating after three levels of recursion. In other to understand where time is spent in the variance computation logic you can use the This is what the recursive invocation tree looks like for StateSchema [
Partial [
]
]
StatesConfig [
StateNodeConfig [
InvokeConfig [
InvokeCreator [
InvokeCallback [
]
PromiseLike [
]
StateMachine [
StateNodesConfig [
StateNode [
ActionObject [
ActionFunction [
ActionMeta [
State [
Mapper [
]
PropertyMapper [
]
MachineOptions [
Record [
]
ConditionPredicate [
GuardMeta [
GuardPredicate [
ActivityDefinition [
InvokeDefinition [
ActionFunctionMap [
ActivityConfig [
DelayFunctionMap [
DelayExpr [
SCXMLEventMeta [
Event [
]
]
]
]
MachineSchema [
]
Map [
IterableIterator [
IteratorYieldResult [
]
]
]
TransitionDefinition [
IteratorResult [
IteratorReturnResult [
]
]
Omit [
Exclude [
]
]
TransitionDefinitionMap [
TransitionConfig [
DelayedTransitionDefinition [
SendAction [
ExprWithMeta [
]
SendExpr [
]
AssignAction [
Assigner [
AssignMeta [
PropertyAssigner [
PartialAssigner [
LogAction [
LogExpr [
]
StopAction [
Expr [
]
ChooseAction [
ChooseConditon [
Readonly [
]
StateNodeDefinition [
StatesDefinition [
StateTransition [
]
]
]
]
]
]
]
]
]
]
]
]
]
]
]
]
]
]
]
]
]
]
]
]
]
]
]
]
]
]
]
]
]
]
SimpleActionsFrom [
]
]
]
Model [
MachineConfig [
]
ExtractEvent [
]
Prop [
]
]
Typestate [
]
RaiseAction [
]
Guard [
]
RaiseActionObject [
]
Action [
]
Interpreter [
Set [
]
StateListener [
]
ContextListener [
]
Observer [
]
SendActionObject [
]
Subscribable [
]
]
BaseActorRef [
]
ActorRef [
Sender [
]
]
Extract [
]
Event [
]
EventListener [
]
Promise [
]
Behavior [
ActorContext [
]
]
Iterable [
Iterator [
]
]
AdjList [
]
StateConfig [
]
ServiceConfig [
]
DelayConfig [
]
InvokeActionObject [
]
SingleOrArray [
]
AtomicStateNodeConfig [
] |
|
@typescript-bot pack this |
|
Heya @ahejlsberg, I've started to run the tarball bundle task on this PR at 367ef6d. You can monitor the build here. |
|
Hey @ahejlsberg, 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 |
|
I can confirm that both of my issues related to this are being fixed with the latest build of this PR. However, I don't see an explicit test case for this being introduced here. The minimal repro created by me can be found here. From what I understand this has been fixed within this diff but as we can see there - no new tests have been added as part of this. Perhaps this is actually tested by the changed baselines - but assessing that is over my head. |
|
Sadly we won't be able to merge this PR. While it improves accuracy of variance measurement, the adverse effects on type checking performance are simply too great. For example, the check time for the fairly popular |
Fixes #44572.