- Notifications
You must be signed in to change notification settings - Fork 13.2k
Always fully compute variance structurally#48080
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Uh oh!
There was an error while loading. Please reload this page.
Conversation
ahejlsberg commented Mar 1, 2022
@typescript-bot test this |
typescript-bot commented Mar 1, 2022 • edited
Loading Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
Heya @ahejlsberg, I've started to run the extended test suite on this PR at de4a166. You can monitor the build here. |
typescript-bot commented Mar 1, 2022 • edited
Loading Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
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! |
typescript-bot commented Mar 1, 2022 • edited
Loading Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
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! |
typescript-bot commented Mar 1, 2022 • edited
Loading Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
Heya @ahejlsberg, I've started to run the parallelized Definitely Typed test suite on this PR at de4a166. You can monitor the build here. |
typescript-bot commented Mar 1, 2022
@ahejlsberg |
typescript-bot commented Mar 1, 2022
@ahejlsberg Here they are:Comparison Report - main..48080
System
Hosts
Scenarios
Developer Information: | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
RyanCavanaugh commented Mar 2, 2022
@typescript-bot pack this |
typescript-bot commented Mar 2, 2022 • edited
Loading Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
Heya @RyanCavanaugh, I've started to run the tarball bundle task on this PR at de4a166. You can monitor the build here. |
ahejlsberg commented Mar 2, 2022
@typescript-bot perf test faster |
typescript-bot commented Mar 2, 2022 • edited
Loading Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
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! |
typescript-bot commented Mar 2, 2022
@ahejlsberg Here they are:Comparison Report - main..48080
System
Hosts
Scenarios
Developer Information: | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
typescript-bot commented Mar 2, 2022
Hey @RyanCavanaugh, I've packed this into an installable tgz. You can install it for testing by referencing it in your and then running |
ahejlsberg commented Mar 2, 2022
Tests are clean. Performance is slightly impacted in |
weswigham left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good, but I wonder if the inconpleteVariancesObserved flag needs to be saved and restored from the cached relation result similarly to variance markers themselves.
ahejlsberg commented Mar 2, 2022
That shouldn't be necessary because we don't cache relations that observed incomplete variances (they result in |
Andarist commented Mar 3, 2022 • edited
Loading Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
ahejlsberg commented Mar 4, 2022 • edited
Loading Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
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: typeFoo<T>={x: T;f: FooFn<T>;}typeFooFn<T>=(foo: Foo<T>)=>void;declareletfoo1: Foo<string>;declareletfoo2: Foo<unknown>;declareletfn1: FooFn<string>;declareletfn2: FooFn<unknown>;foo1=foo2;// Errorfoo2=foo1;// Errorfn1=fn2;// Errorfn2=fn1;// ErrorAbove, the typeFooFn<T>=(foo: Foo<T[])=>void;Now, because foo1=foo2;// Errorfoo2=foo1;fn1=fn2;fn2=fn1;meaning co-variance for foo1=foo2;// Errorfoo2=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. |
Andarist commented Mar 5, 2022
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 🤔 |
typescript-bot commented Mar 6, 2022 • edited
Loading Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
Heya @ahejlsberg, I've started to run the extended test suite on this PR at a323037. You can monitor the build here. |
typescript-bot commented Mar 6, 2022 • edited
Loading Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
Heya @ahejlsberg, I've started to run the abridged perf test suite on this PR at a323037. You can monitor the build here. Update: The results are in! |
typescript-bot commented Mar 6, 2022 • edited
Loading Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
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! |
typescript-bot commented Mar 6, 2022 • edited
Loading Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
Heya @ahejlsberg, I've started to run the parallelized Definitely Typed test suite on this PR at a323037. You can monitor the build here. |
typescript-bot commented Mar 6, 2022
@ahejlsberg Here they are:Comparison Report - main..48080
System
Hosts
Scenarios
Developer Information: | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
typescript-bot commented Mar 6, 2022
@ahejlsberg |
Andarist commented Mar 6, 2022 • edited
Loading Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
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? |
ahejlsberg commented Mar 7, 2022
@typescript-bot test this |
typescript-bot commented Mar 7, 2022 • edited
Loading Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
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! |
typescript-bot commented Mar 7, 2022 • edited
Loading Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
Heya @ahejlsberg, I've started to run the parallelized Definitely Typed test suite on this PR at 08c0fa9. You can monitor the build here. |
typescript-bot commented Mar 7, 2022 • edited
Loading Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
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! |
typescript-bot commented Mar 7, 2022 • edited
Loading Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
Heya @ahejlsberg, I've started to run the extended test suite on this PR at 08c0fa9. You can monitor the build here. |
typescript-bot commented Mar 7, 2022
@ahejlsberg Here they are:Comparison Report - main..48080
System
Hosts
Scenarios
Developer Information: | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
typescript-bot commented Mar 7, 2022
@ahejlsberg |
ahejlsberg commented Mar 7, 2022
@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[] |
ahejlsberg commented Mar 7, 2022
@typescript-bot pack this |
typescript-bot commented Mar 7, 2022 • edited
Loading Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
Heya @ahejlsberg, I've started to run the tarball bundle task on this PR at 367ef6d. You can monitor the build here. |
typescript-bot commented Mar 7, 2022 • edited
Loading Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
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 |
Andarist commented Mar 8, 2022 • edited
Loading Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
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. |
ahejlsberg commented Mar 13, 2022
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.