Track source and target relationship stack depth seperately, only increase on change in value#41821
Uh oh!
There was an error while loading. Please reload this page.
Conversation
…rease on change in value
typescript-bot added Author: Team For Milestone Bug 
| 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!).
weswigham commented Dec 4, 2020
@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
andrewbranch commented Apr 2, 2021
@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 |
amcasey commented Apr 20, 2021
@weswigham What's happening with this? |
weswigham commented May 14, 2021
There's another issue this fixes - functionf1<T,K1extendskeyofT,K2extendskeyofT[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 parametersfunctionf2<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 ❤️ |
ahejlsberg commented May 19, 2021
@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! |
typescript-bot commented May 19, 2021
@ahejlsberg Here they are:Comparison Report - master..41821
System
Hosts
Scenarios
Developer Information: | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
ahejlsberg commented May 19, 2021
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? |
weswigham commented May 19, 2021
I think it is - a generative type should probably hit the issues-an-error limiter and not the quietly-pass-assignability limiter. |
ahejlsberg commented May 19, 2021
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 |
ahejlsberg commented May 19, 2021
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 |
ahejlsberg commented May 19, 2021
Let's run the tests to get some more real world data on what the effect is. |
ahejlsberg commented May 19, 2021
@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. |
Heya @ahejlsberg, I've started to run the parallelized community code test suite on this PR at b81c98b. You can monitor the build here. |
Heya @ahejlsberg, I've started to run the parallelized Definitely Typed test suite on this PR at b81c98b. You can monitor the build here. |
typescript-bot commented Aug 20, 2021
@weswigham Here they are:Comparison Report - main..41821
System
Hosts
Scenarios
Developer Information: | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
weswigham commented Aug 20, 2021
@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 |
weswigham commented Aug 20, 2021
All extended test suites (RWC, user, DT) look good. Let me try to remove the extra |
weswigham commented Aug 20, 2021
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? |
fabb commented Aug 23, 2021
This version still does not fix the issue for my case: #44404 (comment) |
| 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
andrewbranch commented Sep 30, 2021
Apparently I didn’t pull recently enough to resolve all the conflicts 🤦 |
6 participants
Fixes#41617
Fixes#43485
Fixes#44404