GeezerVison

Anders Hejlsberg (@ahejlsberg) if you could take a look at this when you get a chance, that'd be great, thanks!

TypeScript Bot (@typescript-bot) pack this

Heya Andrew Branch (@andrewbranch), I've started to run the tarball bundle task on this PR at b81c98b. You can monitor the build here.

Hey Andrew Branch (@andrewbranch), I've packed this into an installable tgz. You can install it for testing by referencing it in your package.json like so:

{
    "devDependencies": {
        "typescript": "https://typescript.visualstudio.com/cf7ac146-d525-443c-b23c-0d58337efebc/_apis/build/builds/99984/artifacts?artifactName=tgz&fileId=BA6155E5AC16E3841FC2590B94FF87B9E10A9C49C2E9014AAB9A952942AB22E702&fileName=/typescript-4.3.0-insiders.20210402.tgz"
    }
}

and then running npm install.

There is also a playground for this build and an npm module you can use via "typescript": "npm:@typescript-deploys/[email protected]".;

Wesley Wigham (@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 T[K1][K2] to its base constraint takes > 5 steps, so without this change, hits our limiter and throws up a Maybe, which makes the comparison succeed.

Anders Hejlsberg (@ahejlsberg) it'd be super nice if you could review this ❤️

TypeScript Bot (@typescript-bot) perf test this

Heya Anders Hejlsberg (@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!

Anders Hejlsberg (@ahejlsberg)
The results of the perf run you requested are in!

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 Ternary.Maybe).

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.

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 Ternary.Maybe).

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 Ternary.Maybe result (as a non-caching True, since we can assume a comparison in progress is a success while looking to disprove that) allows the comparison to continue on the other parts of the type structure.

if that side instead flip-flops between two types, then all of a sudden we're fine with it (because we produce a Ternary.Maybe).

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 T[A][B] function as a never on the source side and be assignable to anything just because it's deeply generic? Meanwhile if we compare T[A][B] to U[C][D] we're more within our rights, imo, to just give up and say "it's too deeply generic on both sides and we don't wanna exhaustively explore this - just let it pass (unless a less nested check we have available fails)").

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.

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 sourceStack/targetStack when that side of the relationship is recurred on, but that would require threading more information through every recursive call of isRelatedTo. Maybe the intersectionState parameter could be hijacked to traffic this, but it probably wouldn't be pretty. Only appending an element when it changes is a decent approximation since we generally try very hard to not recur on a type and immediately produce the same comparison - generally speaking, we like to have every decomposition we perform change the type we decompose in some way, or we don't attempt the decomposition at all (specifically, we don't let a generic be directly constrained to itself - that's an error - it has to go through some cycle of deferred decompositions instead, which, under this scheme, will always result in new entries into the stack, and thus for a cycle to be detected).

Let's run the tests to get some more real world data on what the effect is.

TypeScript Bot (@typescript-bot) run dt
TypeScript Bot (@typescript-bot) test this
TypeScript Bot (@typescript-bot) user test this

Heya Anders Hejlsberg (@ahejlsberg), I've started to run the extended test suite on this PR at b81c98b. You can monitor the build here.

Heya Anders Hejlsberg (@ahejlsberg), I've started to run the parallelized community code test suite on this PR at b81c98b. You can monitor the build here.

Heya Anders Hejlsberg (@ahejlsberg), I've started to run the parallelized Definitely Typed test suite on this PR at b81c98b. You can monitor the build here.

Wesley Wigham (@weswigham)
The results of the perf run you requested are in!

TypeScript Bot (@typescript-bot) pack this for posterity

Heya Wesley Wigham (@weswigham), I've started to run the tarball bundle task on this PR at f76d880. You can monitor the build here.

Hey Wesley Wigham (@weswigham), I've packed this into an installable tgz. You can install it for testing by referencing it in your package.json like so:

{
    "devDependencies": {
        "typescript": "https://typescript.visualstudio.com/cf7ac146-d525-443c-b23c-0d58337efebc/_apis/build/builds/109006/artifacts?artifactName=tgz&fileId=32F14BC1CCCC5B6DD6C83299C9148D3BA148907CCB93F5D7064C29EF2F37E2A902&fileName=/typescript-4.5.0-insiders.20210820.tgz"
    }
}

and then running npm install.

There is also a playground for this build and an npm module you can use via "typescript": "npm:@typescript-deploys/[email protected]".;

All extended test suites (RWC, user, DT) look good. Let me try to remove the extra isDeeplyNestedType calls and give em another go - if they stay green we'll go with that, otherwise I'll revert that and go with this~

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. Anders Hejlsberg (@ahejlsberg) anything else to add?

Hey Wesley Wigham (@weswigham), I've packed this into an installable tgz. You can install it for testing by referencing it in your package.json like so:

{
    "devDependencies": {
        "typescript": "https://typescript.visualstudio.com/cf7ac146-d525-443c-b23c-0d58337efebc/_apis/build/builds/109006/artifacts?artifactName=tgz&fileId=32F14BC1CCCC5B6DD6C83299C9148D3BA148907CCB93F5D7064C29EF2F37E2A902&fileName=/typescript-4.5.0-insiders.20210820.tgz"
    }
}

and then running npm install.

There is also a playground for this build and an npm module you can use via "typescript": "npm:@typescript-deploys/[email protected]".;

This version still does not fix the issue for my case: #44404 (comment)

Apparently I didn’t pull recently enough to resolve all the conflicts 🤦