Report unmeasurable variance when measurements are circular #45578
Conversation
typescript-bot
added
Author: Team
For Milestone Bug
|
Instead of the refactoring to return |
andrewbranch
force-pushed
the
bug/44572
branch
from
47f26f7 to
8194a58
Compare
| Type 'string' is not assignable to type '"a"'. | ||
| Types of property 'x' are incompatible. | ||
| Type 'string' is not assignable to type '"a"'. | ||
| tests/cases/compiler/varianceMeasurement.ts(11,7): error TS2322: Type 'Foo1<string>' is not assignable to type 'Foo1<unknown>'. |
There was a problem hiding this comment.
The new error comes after a comment that says
// The type below should be invariant in T but is measured as covariant because
// we don't analyze recursive references.
interface Foo1<T> {
x: T;
y: Foo1<(arg: T) => void>;
}
declare const f10: Foo1<string>;
const f11: Foo1<'a'> = f10;
const f12: Foo1<unknown> = f10;👀
andrewbranch
changed the title
|
It’s OOMing? |
| // effectively means we measure variance only from type parameter occurrences that aren't nested in | ||
| // recursive instantiations of the generic type. | ||
| if (variances === emptyArray) { | ||
| outofbandVarianceMarkerHandler!(/*onlyUnreliable*/ false); |
There was a problem hiding this comment.
So we know that outofbandVarianceMarkerHandler will always be defined when variances is emptyArray? Kinda sketchy, but I guess so is the current mechanism.
|
? @typescript-bot perf test this |
|
Heya @DanielRosenwasser, I've started to run the perf test suite on this PR at 8194a58. You can monitor the build here. Update: The results are in! |
|
I was experimenting with effectively the same change as you now have in the PR, and did indeed see the tests stall out towards the end of the test run. I didn't let it run for long enough to get an OOM, but I'm sure that would have eventually happened. I'm guessing the issue is that being more aggressive about marking variances as unmeasurable causes us to resort to structural comparisons in more cases, and apparently that goes badly. Would be interesting to see which test is to blame and whether there's something specific we can do about it. |
|
@ahejlsberg one thing I’ve been having trouble conceptualizing is what it means if only one of the two the assignability checks with marker types (i.e., either the covariant or contravariant check) is circular, while the other comes back as definitely assignable or definitely unassignable. In my first iteration of this PR, I only set the variance to unmeasurable if both checks were circular, and tests passed, but it didn’t fix the result in varianceMeasurement.ts. |
|
@andrewbranch Variance measurement requires us to successfully compare two type instantiations in both directions, so if we fail in either direction, we have to consider the their variance to be unmeasurable. |
|
@andrewbranch One way to think of it is that variance is a measurement of whether two types |
|
The OOMing test is |
|
Hmm, yeah, I can see how those types are a disaster in a structural comparison. For example, to structurally compare two |
But for practical purposes, if we can determine that |
Just to check, you're saying that in the case where |
|
That’s almost, but not quite, what I’m saying (and what I’m saying is a question).
In the case where |
|
This question is apparently relevant to the OOMing test, because apparently variance probing in one direction was working, because there were no baseline changes and no OOMing when I restricted my change to both directions being unmeasurable. But, it feels like that must mean our analysis of the assignability in that test is either wrong, or only coincidentally right. Because what the current state of my PR is showing us is that at least one direction of variance probing on these types is inconclusive, but we’ve previously believed we know what the variance is, and were using that instead of structural comparisons to decide the assignability of |
|
@DanielRosenwasser Here they are:Comparison Report - main..45578
System
Hosts
Scenarios
Developer Information: |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Took a step back and dug a little deeper to understand why the repro posted by Ryan here works when the I now understand what the real problem is. We have logic in Changing our logic to skip variance checking for any type instantiation where at least one type argument is a marker type fixes the issue. It causes us to do a bit more structural comparing when computing variances, but obviously it was wrong not to do this extra work. I think this is the solution we want to go with. I will put up a new PR and we'll see what the performance impact is, if any. |
|
Alternative PR is up at #45628. |
Fixes #44572 - see my comment on the bug for a detailed explanation of what was happening.