Track source and target relationship stack depth seperately, only increase on change in value #41821
Conversation
…rease on change in value
typescript-bot
added
Author: Team
For Milestone Bug
| @@ -63,4 +64,6 @@ tests/cases/compiler/infiniteConstraints.ts(36,71): error TS2536: Type '"foo"' c | |||
|
|
|||
| type Conv<T, U = T> = | |||
| { 0: [T]; 1: Prepend<T, Conv<ExactExtract<U, T>>>;}[U extends T ? 0 : 1]; | |||
| ~~~~~~~~~~~~~~~~~~~~~~~~ | |||
| !!! error TS2321: Excessive stack depth comparing types 'Conv<ExactExtract<U, T>, ExactExtract<U, T>>' and 'unknown[]'. | |||
There was a problem hiding this comment.
This part of this test was originally added because it had bad perf, and the test had a stack out error once the perf was fixed (circa #32079), however the error disappeared in #40971 and we just kidna didn't care (the test is for perf, after all). Turns out, the error should still be here, since the relationship does still infinitely expand (on only one side - we were erroneously flagging both sides are infinitely expanding - unknown[] definitely doesn't infinitely expand!).
|
@ahejlsberg if you could take a look at this when you get a chance, that'd be great, thanks! |
…e source conditional is already known to be spooling out of control
|
@typescript-bot pack this |
|
Heya @andrewbranch, I've started to run the tarball bundle task on this PR at b81c98b. You can monitor the build here. |
|
Hey @andrewbranch, 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 What's happening with this? |
|
There's another issue this fixes - function f1<T, K1 extends keyof T, K2 extends keyof T[K1]>(
x: T[K1][K2], y: Extract<T[K1][K2], string>) {
x = y;
y = x; // missing error
}
// for comparison, the equivalent comparison with bare, unconstrained type parameters
function f2<T>(x: T, y: Extract<T, string>) {
x = y;
y = x; // correctly has error
}decomposing @ahejlsberg it'd be super nice if you could review this ❤️ |
|
@typescript-bot perf test this |
|
Heya @ahejlsberg, I've started to run the perf test suite on this PR at b81c98b. You can monitor the build here. Update: The results are in! |
|
@ahejlsberg Here they are:Comparison Report - master..41821
System
Hosts
Scenarios
Developer Information: |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
What if one of the sides has a constraint that keeps on generating deeper and deeper types? Won't we just keep on going and eventually hit the 100 level panic limiter and issue an error? Is that what we want? |
|
I think it is - a generative type should probably hit the issues-an-error limiter and not the quietly-pass-assignability limiter. |
|
Hmm. But it just seems really odd that we would issue an error if nothing changes one one side, but if that side instead flip-flops between two types, then all of a sudden we're fine with it (because we produce a |
|
In other words, our limiter is based on the notion that we count occurrences of cycles, and all of a sudden we're saying that a cycle of length one doesn't count at all. I'm not sure I can make that make sense. |
In such a case, we never get to check the one side that "didn't change" precisely because the other side decomposed forever. That's exactly why it makes sense to issue an error - we never even got to look at the rhs because the lhs was expanding forever. If both the lhs and rhs expand forever, we'll still issue an error, because the lhs expanding forever will prevent us from even checking that the rhs expands forever. "This type expands forever" is a very distinct problem from "this type depends on a comparison which occurred in its history already" - the first we don't really have a reasonable operation to do (blindly allowing the assignment to anything isn't good - we're not even looking at the target!), while for the second, a
Specifically, I think this is at least more justifiable, since then we've actually explored some structure on both sides, we just want to refrain from exploring too deeply. Whereas when we only ever explore how one side of the relationship expands, it's very hard to justify the assignment (why should a
Except it's not actually counting cycles because it has no knowledge of which side of the comparison (if any) was actually recurred on to produce the new pair of types, which is what it'd need to count that (specifically, it'd only check if the side we recurred on is deeply nested). What we're tracking right now is just occurrences in history, which doesn't correctly admit, say, the LHS looping forever while the RHS just sits there uninspected because we have yet to recur on it (which is turns out is very common, as pretty simple LHS constructs end up taking more than 5 steps to expand to a final constraint!) Ideally, we'd only append to the |
|
Let's run the tests to get some more real world data on what the effect is. |
|
@typescript-bot run dt |
|
Heya @ahejlsberg, I've started to run the extended test suite on this PR at b81c98b. You can monitor the build here. |
|
@typescript-bot pack this for posterity |
|
Heya @weswigham, I've started to run the tarball bundle task on this PR at f76d880. You can monitor the build here. |
|
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 |
|
All extended test suites (RWC, user, DT) look good. Let me try to remove the extra |
|
Nope, scratch that - local tests fail if we remove them - the depth check guards are needed on the comparison of conditional constraints, otherwise we issue stack depth warnings when comparing conditional types that operate over recursive type aliases (rather than only erroring when their actual execution exceeds the instantiation limit). We need 'em so we stop recurring further into the (infinitely expanding) type, and instead gracefully try a different constraint form (eg, the distributive constraint instead of the non-distributive one, or vice-versa). So this should be good to go as-is. @ahejlsberg anything else to add? |
This version still does not fix the issue for my case: #44404 (comment) |
| @@ -18868,7 +18891,7 @@ namespace ts { | |||
| else { | |||
| targetKeys = nameType || constraintType; | |||
| } | |||
| if (isRelatedTo(source, targetKeys, reportErrors) === Ternary.True) { | |||
| if (isRelatedTo(source, targetKeys, RecursionFlags.Target, reportErrors) === Ternary.True) { | |||
There was a problem hiding this comment.
I added this one while resolving merge conflicts
|
Apparently I didn’t pull recently enough to resolve all the conflicts 🤦 |
Fixes #41617
Fixes #43485
Fixes #44404