Revert substitution type simplification

[ahejlsberg]

In #31354 I included a late change to simplify substitution types along with other simplifiable types. That was not a good idea. It had an outsize impact on batch compilation of styled-components:

Files:            28
Lines:         21723
Nodes:         60563
Identifiers:   19996
Symbols:      135703
Types:         82071
Memory used: 166601K
I/O read:      0.01s
I/O write:     0.00s
Parse time:    0.40s
Bind time:     0.16s
Check time:    8.61s
Emit time:     0.00s
Total time:    9.17s

This PR reverts the change, leaving substitution type simplification as it was. This reduces check time by about 25-30% and gets us back to the expected levels:

Files:            28
Lines:         21723
Nodes:         60563
Identifiers:   19996
Symbols:      121398
Types:         80828
Memory used: 167618K
I/O read:      0.01s
I/O write:     0.00s
Parse time:    0.40s
Bind time:     0.18s
Check time:    6.42s
Emit time:     0.00s
Total time:    7.00s

Ideally we'll get this into 3.5 RC.

[ahejlsberg]

I should mention that this is solely a batch compilation issue. Statement completion performance is not affected and remains snappy in styled-components either way (which explains why we didn't catch it).

[weswigham]

Just decomposing a substitute had such an outsized perf diff? This implies that we're skipping work we should be doing during the styled components batch build because we get an unexpected substitute out of getSimplifiedType (which we then fail to relate) - can we add a Debug.fail on seeing a substitute in the non-identity branch of structuredTypeRelatedTo?

[ahejlsberg]

I don't think it implies that. Rather, I'm thinking it's because when simplification removes substitution types, we end up "skipping over" types that involve substitution types and thus miss results we've previously cached.

can we add a Debug.fail on seeing a substitute in the non-identity branch of structuredTypeRelatedTo?

Tried it. We never hit the Debug.fail when compling styled-components.

[weswigham]

Rather, I'm thinking it's because when simplification removes substitution types, we end up "skipping over" types that involve substitution types and thus miss results we've previously cached.

Ah, because of all the times when we call getSimplifiedType within getSimplifiedType just to form a part of a type (rather than a top-level type); I see. Shouldn't we still recursively simplify/unwrap substitutes at the top level, then, even if not via getSimplifiedType? I'm thinking of a scenario like this: we get a {a: (Q extends T ? Q : B), b: never}["a" | "b"] in - this is a substitute which, say, expands to {a: (Q extends T ? Q : B), b: never}["a" | "b"] & {a: (Q extends T ? Q : B), b: never}["c"] (difficult, but not impossible via other simplifying conditionals), then compacts and simplifies to Q (because, eg, the conditional simplifies and in the object the "b" case is always never, and the "c" case is unknown), and that Q is itself a substitute on Q & T.

[ahejlsberg]

Shouldn't we still recursively simplify/unwrap substitutes at the top level, then, even if not via getSimplifiedType?

Possibly. But if so, that would be a new PR. The purpose of this PR is simply to revert a change that we didn't want.

[weswigham]

True, although I thought the original intent of doing this was to recursively unwrap - it just turns out it has negative performance characteristics because of the other things it affects (ergo, to re-implement the original recursive-unwrapping intent after reverting this change, we'd need to do a bit more than a straight revert). We've not have a commit in master that had deferred conditional type simplification without recursive substitution unwrapping yet, so as-is, reverting this does have the potential to reveal new, possibly unforeseen undesirable behavior.

What I'm saying is this: Yes, right now it's a "revert", but IMO a better PR would be a refactor that removes the substitution unwrapping from simplification but still does recursive unwrapping/simplifying within to top of isRelatedTo, since then we don't risk shipping something without the recursive unwrap behavior, since we know that if a substitution comes out of getSimplifiedType, the rest of relationship checking will not handle it.

[ahejlsberg]

I'm simply removing a late change that had no basis in demonstrated real world issues and reverting back to a state that is closer to where we were before. We can discuss alternate solutions in another PR. I'm going to run our RWC and DT tests and if they come through clean I'm going to merge this.

[ahejlsberg]

@typescript-bot test this

[typescript-bot]

Heya @ahejlsberg, I've started to run the extended test suite on this PR at 92a1547. You can monitor the build here. It should now contribute to this PR's status checks.

[ahejlsberg]

@typescript-bot run dt

[typescript-bot]

Heya @ahejlsberg, I've started to run the parallelized Definitely Typed test suite on this PR at 92a1547. You can monitor the build here. It should now contribute to this PR's status checks.